mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(orchestrator): harden external-PR supersede close-on-land
Scope close_pull_request repo resolution by project_id and thread the umbrella's project into close-on-land, so a contributor PR is never resolved (or closed) against a same-numbered PR in another project's repo. Skip the comment + close PATCH when the PR is already closed, so a retried sweep never re-posts the 'superseded' comment. Require a non-cancelled descendant that actually landed a PR before retiring the contributor PR, so an umbrella force-completed over a cancelled code subtask leaves the contributor's still-valid PR open. Run close-on-land from the always-on sweeper rather than the default-off poll loop, so a supersede that lands after the feature is toggled off is still reconciled. Serialize concurrent supersede triggers under a lock so a double-click can't cut two branches / spawn two umbrellas. Anchor the supersede marker checks to the marker line so appended CEO notes can't be mistaken for the closed/dedup tokens. Make the fork-head branch cut idempotent (forced refspec) so a commit-fail retry converges. Also drop an importlib.reload(roboco.config) in a unit test that rebound the settings singleton and leaked into the PM decision-window test.
This commit is contained in:
Renn F
@@ -62,6 +62,8 @@ def test_pr_author_allowed(
|
||||
("", None),
|
||||
("no marker here", None),
|
||||
("external_pr_supersede pr=notanint review=abc", None),
|
||||
# A CEO note on a later line must not shadow the marker line's pr=.
|
||||
("external_pr_supersede pr=11 review=abc\nceo_approval_notes: pr=99 ok", 11),
|
||||
],
|
||||
)
|
||||
def test_parse_supersede_pr(quick_context: str, expected: int | None) -> None:
|
||||
|
||||
@@ -1,7 +1,5 @@
|
||||
"""Unit tests: commit-trailer links use ROBOCO_PUBLIC_BASE_URL."""
|
||||
|
||||
import importlib
|
||||
|
||||
import pytest
|
||||
import roboco.config as config_module
|
||||
from roboco.templates.git.commit import (
|
||||
@@ -41,17 +39,18 @@ def test_build_commit_message_strips_trailing_slash() -> None:
|
||||
|
||||
def test_links_use_public_base_url(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""settings.public_base_url drives the api_base passed to build_commit_message."""
|
||||
# Construct a fresh Settings() to read the patched env — do NOT
|
||||
# importlib.reload(roboco.config), which would rebind the module-level
|
||||
# ``settings`` singleton to a new object and leak that divergence into any
|
||||
# other test that captured the original reference via ``from roboco.config
|
||||
# import settings`` (e.g. the PM decision-window gate).
|
||||
monkeypatch.setenv("ROBOCO_PUBLIC_BASE_URL", "https://roboco.example.com")
|
||||
importlib.reload(config_module)
|
||||
try:
|
||||
settings = config_module.Settings()
|
||||
api_base = settings.public_base_url.rstrip("/") + "/api"
|
||||
ctx = _make_ctx()
|
||||
out = build_commit_message(ctx, api_base)
|
||||
assert "https://roboco.example.com" in out
|
||||
assert "127.0.0.1" not in out
|
||||
finally:
|
||||
importlib.reload(config_module)
|
||||
settings = config_module.Settings()
|
||||
api_base = settings.public_base_url.rstrip("/") + "/api"
|
||||
ctx = _make_ctx()
|
||||
out = build_commit_message(ctx, api_base)
|
||||
assert "https://roboco.example.com" in out
|
||||
assert "127.0.0.1" not in out
|
||||
|
||||
|
||||
def test_commit_context_invalid_type_raises() -> None:
|
||||
|
||||
@@ -161,6 +161,9 @@ async def test_close_pull_request_patches_state_closed(
|
||||
status_code = 200
|
||||
text = ""
|
||||
|
||||
def json(self) -> dict[str, str]:
|
||||
return {"state": "open"}
|
||||
|
||||
class _Client:
|
||||
async def __aenter__(self) -> _Client:
|
||||
return self
|
||||
@@ -168,6 +171,10 @@ async def test_close_pull_request_patches_state_closed(
|
||||
async def __aexit__(self, *_a: Any) -> None:
|
||||
return None
|
||||
|
||||
async def get(self, url: str, **_kw: Any) -> _Resp:
|
||||
calls.append(("GET", url))
|
||||
return _Resp()
|
||||
|
||||
async def post(self, url: str, **_kw: Any) -> _Resp:
|
||||
calls.append(("POST", url))
|
||||
return _Resp()
|
||||
@@ -185,3 +192,74 @@ async def test_close_pull_request_patches_state_closed(
|
||||
) in calls
|
||||
assert ("PATCH", "https://api.github.com/repos/owner/repo/pulls/159") in calls
|
||||
delete_branch.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_pull_request_idempotent_when_already_closed(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""An already-closed PR is a no-op: no duplicate comment, no PATCH.
|
||||
|
||||
Guards the close-on-land retry path — a transient failure between the
|
||||
comment POST and the close PATCH must not re-post the explanatory comment
|
||||
on the next sweep.
|
||||
"""
|
||||
svc = _git_service()
|
||||
task = type("T", (), {"id": "t", "assigned_to": None, "created_by": None})()
|
||||
session = AsyncMock()
|
||||
session.execute = AsyncMock(
|
||||
return_value=type("Res", (), {"scalar_one_or_none": lambda _self: task})()
|
||||
)
|
||||
monkeypatch.setattr(svc, "session", session, raising=False)
|
||||
monkeypatch.setattr(
|
||||
svc,
|
||||
"_project_for_task",
|
||||
AsyncMock(return_value=type("P", (), {"slug": "proj"})()),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
svc, "_resolve_workspace_agent_id", MagicMock(return_value=None)
|
||||
)
|
||||
monkeypatch.setattr(svc, "get_workspace", AsyncMock(return_value=Path("/tmp/ws")))
|
||||
monkeypatch.setattr(
|
||||
svc, "_get_project_token_or_raise", AsyncMock(return_value="tok")
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
svc, "_parse_github_remote", MagicMock(return_value=("owner", "repo"))
|
||||
)
|
||||
monkeypatch.setattr(svc, "_delete_pr_branch_best_effort", AsyncMock())
|
||||
|
||||
calls: list[tuple[str, str]] = []
|
||||
|
||||
class _Resp:
|
||||
is_success = True
|
||||
status_code = 200
|
||||
text = ""
|
||||
|
||||
def json(self) -> dict[str, str]:
|
||||
return {"state": "closed"}
|
||||
|
||||
class _Client:
|
||||
async def __aenter__(self) -> _Client:
|
||||
return self
|
||||
|
||||
async def __aexit__(self, *_a: Any) -> None:
|
||||
return None
|
||||
|
||||
async def get(self, url: str, **_kw: Any) -> _Resp:
|
||||
calls.append(("GET", url))
|
||||
return _Resp()
|
||||
|
||||
async def post(self, url: str, **_kw: Any) -> _Resp:
|
||||
calls.append(("POST", url))
|
||||
return _Resp()
|
||||
|
||||
async def patch(self, url: str, **_kw: Any) -> _Resp:
|
||||
calls.append(("PATCH", url))
|
||||
return _Resp()
|
||||
|
||||
with patch("roboco.services.git.httpx.AsyncClient", return_value=_Client()):
|
||||
await svc.close_pull_request(
|
||||
159, comment="superseded by #158", delete_branch=False
|
||||
)
|
||||
|
||||
assert [c[0] for c in calls] == ["GET"] # no POST comment, no PATCH
|
||||
|
||||
@@ -0,0 +1,171 @@
|
||||
"""Supersede umbrella close-on-land guards.
|
||||
|
||||
Covers the parts of the external-PR supersede flow that decide whether — and
|
||||
which — a landed supersede's contributor PR gets retired:
|
||||
|
||||
- ``supersede_marker_line`` anchors marker/state checks to the marker line, so
|
||||
free-form CEO escalation/approval notes appended to the same multi-writer
|
||||
``quick_context`` can't be mistaken for the marker.
|
||||
- ``supersede_umbrellas_pending_close`` only returns umbrellas whose
|
||||
replacement work actually landed (a non-cancelled descendant carrying a PR),
|
||||
not every COMPLETED umbrella — the CEO can force-complete over a cancelled
|
||||
code subtask.
|
||||
- ``mark_supersede_pr_closed`` writes the ``closed=1`` idempotency token onto
|
||||
the marker line, surviving appended notes.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.models.base import TaskStatus
|
||||
from roboco.services.task import TaskService, supersede_marker_line
|
||||
|
||||
_MARKER = "external_pr_supersede pr=5 review=abc"
|
||||
|
||||
|
||||
def _scalars_all(rows: list[object]) -> MagicMock:
|
||||
"""A session.execute return value whose .scalars().all() yields `rows`."""
|
||||
res = MagicMock()
|
||||
res.scalars.return_value.all.return_value = rows
|
||||
return res
|
||||
|
||||
|
||||
def _service(execute_returns: object) -> TaskService:
|
||||
session = MagicMock()
|
||||
session.execute = AsyncMock(return_value=execute_returns)
|
||||
session.flush = AsyncMock()
|
||||
return TaskService(session)
|
||||
|
||||
|
||||
def _bind(svc: TaskService, name: str, value: object) -> None:
|
||||
object.__setattr__(svc, name, value)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# supersede_marker_line — line anchoring
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_marker_line_returns_marker_ignoring_appended_notes() -> None:
|
||||
qc = f"{_MARKER}\nceo_approval_notes: shipped, looks good"
|
||||
assert supersede_marker_line(qc) == _MARKER
|
||||
|
||||
|
||||
def test_marker_line_not_fooled_by_closed_token_in_note() -> None:
|
||||
qc = f"{_MARKER}\nceo_approval_notes: marked closed=1 in jira"
|
||||
# The marker line itself carries no closed=1, so the PR is NOT yet closed.
|
||||
assert "closed=1" not in supersede_marker_line(qc).split()
|
||||
|
||||
|
||||
def test_marker_line_empty_when_absent() -> None:
|
||||
assert supersede_marker_line("no marker here\nescalation_notes: x") == ""
|
||||
assert supersede_marker_line(None) == ""
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# supersede_umbrellas_pending_close — closed-token + landed-replacement gates
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pending_close_excludes_umbrella_with_closed_marker() -> None:
|
||||
umbrella = MagicMock(id=uuid4(), quick_context=f"{_MARKER} closed=1")
|
||||
svc = _service(_scalars_all([umbrella]))
|
||||
_bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True))
|
||||
assert await svc.supersede_umbrellas_pending_close() == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pending_close_keeps_umbrella_with_closed_token_only_in_note() -> None:
|
||||
# A CEO note containing the literal "closed=1" must NOT retire the PR.
|
||||
umbrella = MagicMock(
|
||||
id=uuid4(), quick_context=f"{_MARKER}\nceo_approval_notes: closed=1 elsewhere"
|
||||
)
|
||||
svc = _service(_scalars_all([umbrella]))
|
||||
_bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=True))
|
||||
out = await svc.supersede_umbrellas_pending_close()
|
||||
assert out == [umbrella]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pending_close_requires_landed_replacement() -> None:
|
||||
# COMPLETED + no closed marker, but the replacement never landed (the code
|
||||
# subtask was cancelled) — close-on-land must skip it.
|
||||
umbrella = MagicMock(id=uuid4(), quick_context=_MARKER)
|
||||
svc = _service(_scalars_all([umbrella]))
|
||||
_bind(svc, "_supersede_replacement_landed", AsyncMock(return_value=False))
|
||||
assert await svc.supersede_umbrellas_pending_close() == []
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _supersede_replacement_landed — subtree walk
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_replacement_landed_true_for_completed_descendant_with_pr() -> None:
|
||||
child = MagicMock(id=uuid4(), status=TaskStatus.COMPLETED, pr_number=42)
|
||||
svc = _service(_scalars_all([child]))
|
||||
assert await svc._supersede_replacement_landed(uuid4()) is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_replacement_landed_false_when_descendant_cancelled() -> None:
|
||||
child = MagicMock(id=uuid4(), status=TaskStatus.CANCELLED, pr_number=42)
|
||||
svc = _service(_scalars_all([child]))
|
||||
# The `seen` guard terminates the walk even though the mock re-returns child.
|
||||
assert await svc._supersede_replacement_landed(uuid4()) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_replacement_landed_false_when_completed_without_pr() -> None:
|
||||
child = MagicMock(id=uuid4(), status=TaskStatus.COMPLETED, pr_number=None)
|
||||
svc = _service(_scalars_all([child]))
|
||||
assert await svc._supersede_replacement_landed(uuid4()) is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# find_supersede_umbrella — marker-line dedup
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_find_umbrella_matches_marker_not_note() -> None:
|
||||
match = MagicMock(id=uuid4(), quick_context=f"{_MARKER}\nescalation_notes: x")
|
||||
other = MagicMock(
|
||||
id=uuid4(),
|
||||
# marker for a different PR, but a note mentions "pr=5 review=" text
|
||||
quick_context="external_pr_supersede pr=9 review=z\nnote: see pr=5 review= ok",
|
||||
)
|
||||
svc = _service(_scalars_all([other, match]))
|
||||
found = await svc.find_supersede_umbrella(uuid4(), 5)
|
||||
assert found is match
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# mark_supersede_pr_closed — token written on the marker line
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mark_closed_appends_token_to_marker_line() -> None:
|
||||
task = MagicMock(quick_context=f"{_MARKER}\nceo_approval_notes: shipped")
|
||||
svc = _service(_scalars_all([]))
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
await svc.mark_supersede_pr_closed(uuid4())
|
||||
lines = task.quick_context.splitlines()
|
||||
assert lines[0] == f"{_MARKER} closed=1"
|
||||
assert lines[1] == "ceo_approval_notes: shipped" # note untouched
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_mark_closed_is_idempotent_on_marker_line() -> None:
|
||||
task = MagicMock(quick_context=f"{_MARKER} closed=1\nceo_approval_notes: x")
|
||||
svc = _service(_scalars_all([]))
|
||||
_bind(svc, "get", AsyncMock(return_value=task))
|
||||
await svc.mark_supersede_pr_closed(uuid4())
|
||||
# No second closed=1 token appended.
|
||||
assert task.quick_context.splitlines()[0] == f"{_MARKER} closed=1"
|
||||
Reference in New Issue
Block a user