Merge branch 'master' into fix/issue-850-native-mcp-bootstrap
This commit is contained in:
@@ -19,6 +19,7 @@ from pr_work_lease import ( # noqa: E402
|
||||
assess_reviewer_mutation_blocked,
|
||||
assess_reviewer_stale_head_final_report,
|
||||
format_conflict_fix_lease_body,
|
||||
find_active_conflict_fix_lease,
|
||||
parse_conflict_fix_lease_comment,
|
||||
parse_reviewer_lease_comment,
|
||||
)
|
||||
@@ -203,5 +204,157 @@ class TestFormatLease(unittest.TestCase):
|
||||
self.assertEqual(parsed["pr_number"], 376)
|
||||
|
||||
|
||||
class TestConflictFixLeaseLifecycle(unittest.TestCase):
|
||||
def test_claim_followed_by_matching_release(self):
|
||||
claim_body = _conflict_fix_body(phase="claimed", worktree="branches/fix-376")
|
||||
expires = (NOW + timedelta(minutes=60)).isoformat().replace("+00:00", "Z")
|
||||
release_body = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"branch: feat/fix-376",
|
||||
"worktree: branches/fix-376",
|
||||
"profile: prgs-author",
|
||||
"phase: released",
|
||||
f"head_before: {HEAD_A}",
|
||||
f"head_after: {HEAD_B}",
|
||||
f"expires_at: {expires}",
|
||||
])
|
||||
comments = [{"body": claim_body}, {"body": release_body}]
|
||||
lease = find_active_conflict_fix_lease(comments, pr_number=376, now=NOW)
|
||||
self.assertIsNone(lease)
|
||||
|
||||
def test_expired_claim_without_release(self):
|
||||
past_expires = (NOW - timedelta(minutes=10)).isoformat().replace("+00:00", "Z")
|
||||
claim_body = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"phase: claimed",
|
||||
f"head_before: {HEAD_A}",
|
||||
f"expires_at: {past_expires}",
|
||||
"profile: prgs-author",
|
||||
])
|
||||
comments = [{"body": claim_body}]
|
||||
lease = find_active_conflict_fix_lease(comments, pr_number=376, now=NOW)
|
||||
self.assertIsNone(lease)
|
||||
|
||||
def test_mismatched_release_different_head(self):
|
||||
claim_body = _conflict_fix_body(phase="claimed", worktree="branches/fix-376")
|
||||
expires = (NOW + timedelta(minutes=60)).isoformat().replace("+00:00", "Z")
|
||||
release_body = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"profile: prgs-author",
|
||||
"phase: released",
|
||||
f"head_before: {HEAD_B}",
|
||||
f"expires_at: {expires}",
|
||||
])
|
||||
comments = [{"body": claim_body}, {"body": release_body}]
|
||||
lease = find_active_conflict_fix_lease(comments, pr_number=376, now=NOW)
|
||||
self.assertIsNotNone(lease)
|
||||
self.assertEqual(lease["phase"], "claimed")
|
||||
|
||||
def test_mismatched_release_different_branch(self):
|
||||
claim_body = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"branch: feat/branch-A",
|
||||
"phase: claimed",
|
||||
f"head_before: {HEAD_A}",
|
||||
f"expires_at: {(NOW + timedelta(minutes=60)).isoformat().replace('+00:00', 'Z')}",
|
||||
"profile: prgs-author",
|
||||
])
|
||||
release_body = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"branch: feat/branch-B",
|
||||
"phase: released",
|
||||
f"head_before: {HEAD_A}",
|
||||
f"expires_at: {(NOW + timedelta(minutes=60)).isoformat().replace('+00:00', 'Z')}",
|
||||
"profile: prgs-author",
|
||||
])
|
||||
comments = [{"body": claim_body}, {"body": release_body}]
|
||||
lease = find_active_conflict_fix_lease(comments, pr_number=376, now=NOW)
|
||||
self.assertIsNotNone(lease)
|
||||
self.assertEqual(lease["phase"], "claimed")
|
||||
|
||||
def test_release_followed_by_newer_claim(self):
|
||||
claim_1 = _conflict_fix_body(phase="claimed", worktree="branches/fix-376")
|
||||
expires = (NOW + timedelta(minutes=60)).isoformat().replace("+00:00", "Z")
|
||||
release_1 = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"profile: prgs-author",
|
||||
"phase: released",
|
||||
f"head_before: {HEAD_A}",
|
||||
f"head_after: {HEAD_B}",
|
||||
f"expires_at: {expires}",
|
||||
])
|
||||
claim_2 = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"profile: prgs-author",
|
||||
"phase: claimed",
|
||||
f"head_before: {HEAD_B}",
|
||||
f"expires_at: {expires}",
|
||||
])
|
||||
comments = [{"body": claim_1}, {"body": release_1}, {"body": claim_2}]
|
||||
lease = find_active_conflict_fix_lease(comments, pr_number=376, now=NOW)
|
||||
self.assertIsNotNone(lease)
|
||||
self.assertEqual(lease["head_before"], HEAD_B)
|
||||
|
||||
def test_malformed_or_ambiguous_markers(self):
|
||||
malformed_release = "\n".join([
|
||||
CONFLICT_FIX_LEASE_MARKER,
|
||||
"pr: #376",
|
||||
"phase: released",
|
||||
# missing head_before and profile
|
||||
])
|
||||
claim_body = _conflict_fix_body(phase="claimed")
|
||||
comments = [{"body": claim_body}, {"body": malformed_release}]
|
||||
lease = find_active_conflict_fix_lease(comments, pr_number=376, now=NOW)
|
||||
self.assertIsNotNone(lease)
|
||||
|
||||
def test_pr818_historical_sequence(self):
|
||||
comment_14696 = "\n".join([
|
||||
"<!-- mcp-conflict-fix-lease:v1 -->",
|
||||
"pr: #818",
|
||||
"branch: feat/issue-638-webui-app-shell-phase1",
|
||||
"worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/issue-638-webui-app-shell-phase1",
|
||||
"profile: prgs-author",
|
||||
"session_id: unknown",
|
||||
"phase: claimed",
|
||||
"head_before: 08061b7b8aebdd099a37d1abf5dafcf38e4fd3fb",
|
||||
"expires_at: 2026-07-23T07:12:13Z",
|
||||
"reviewer_active: no",
|
||||
])
|
||||
comment_14730 = "\n".join([
|
||||
"<!-- mcp-conflict-fix-lease:v1 -->",
|
||||
"pr: #818",
|
||||
"branch: feat/issue-638-webui-app-shell-phase1",
|
||||
"worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/issue-638-webui-app-shell-phase1",
|
||||
"profile: prgs-author",
|
||||
"session_id: prgs-author-61241-e5129c60",
|
||||
"phase: released",
|
||||
"head_before: 08061b7b8aebdd099a37d1abf5dafcf38e4fd3fb",
|
||||
"head_after: 64b6eb5d5402663098de5ded3b0617cc3b3df98f",
|
||||
"expires_at: 2026-07-23T06:05:00Z",
|
||||
"reviewer_active: no",
|
||||
])
|
||||
comments = [{"body": comment_14696}, {"body": comment_14730}]
|
||||
check_now = datetime(2026, 7, 23, 6, 30, tzinfo=timezone.utc)
|
||||
lease = find_active_conflict_fix_lease(comments, pr_number=818, now=check_now)
|
||||
self.assertIsNone(lease)
|
||||
|
||||
reviewer_gate = assess_reviewer_mutation_blocked(
|
||||
pr_number=818,
|
||||
comments=comments,
|
||||
reviewed_head_sha="64b6eb5d5402663098de5ded3b0617cc3b3df98f",
|
||||
live_head_sha="64b6eb5d5402663098de5ded3b0617cc3b3df98f",
|
||||
mutation="approve",
|
||||
now=check_now,
|
||||
)
|
||||
self.assertTrue(reviewer_gate["mutation_allowed"])
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -0,0 +1,340 @@
|
||||
"""Tests for the MCP restart coordinator and impact analysis (#658).
|
||||
|
||||
Multi-session fixtures exercise every verdict branch: safe, unsafe (live work),
|
||||
override, and the fail-closed deny on incomplete inventory. Also covers the
|
||||
critical-section deny path and the new ``ControlPlaneDB.list_sessions``.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import tempfile
|
||||
import unittest
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
import restart_coordinator as rc
|
||||
from control_plane_db import ControlPlaneDB
|
||||
|
||||
|
||||
NOW = datetime(2026, 7, 24, 6, 0, 0, tzinfo=timezone.utc)
|
||||
|
||||
|
||||
def _ts(dt: datetime) -> str:
|
||||
return dt.isoformat()
|
||||
|
||||
|
||||
def _live_pid() -> int:
|
||||
return os.getpid()
|
||||
|
||||
|
||||
def _dead_pid() -> int:
|
||||
# A pid that is essentially never alive. os.kill(0) on it raises
|
||||
# ProcessLookupError → is_process_alive False.
|
||||
return 2_000_000_000
|
||||
|
||||
|
||||
def _session(session_id, *, pid, status="active", heartbeat=None, role="author"):
|
||||
return {
|
||||
"session_id": session_id,
|
||||
"role": role,
|
||||
"profile": "prgs-author",
|
||||
"pid": pid,
|
||||
"status": status,
|
||||
"last_heartbeat_at": _ts(heartbeat or NOW),
|
||||
}
|
||||
|
||||
|
||||
def _lease(
|
||||
lease_id,
|
||||
*,
|
||||
session_id,
|
||||
freshness,
|
||||
kind="issue",
|
||||
number=658,
|
||||
phase="allocated",
|
||||
worktree=None,
|
||||
role="author",
|
||||
):
|
||||
return {
|
||||
"lease_id": lease_id,
|
||||
"session_id": session_id,
|
||||
"role": role,
|
||||
"phase": phase,
|
||||
"work_kind": kind,
|
||||
"work_number": number,
|
||||
"worktree_path": worktree,
|
||||
"freshness": {"freshness": freshness},
|
||||
}
|
||||
|
||||
|
||||
class EvaluateRestartImpactTest(unittest.TestCase):
|
||||
def test_incomplete_inventory_denies_fail_closed(self) -> None:
|
||||
report = rc.evaluate_restart_impact(
|
||||
{"inventory_complete": False, "incomplete_reasons": ["db down"]},
|
||||
now=NOW,
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_UNSAFE)
|
||||
self.assertFalse(report.allow_restart)
|
||||
self.assertFalse(report.restart_performed)
|
||||
self.assertIn("db down", report.incomplete_reasons)
|
||||
self.assertTrue(
|
||||
any("fail closed" in reasoning for reasoning in report.reasons)
|
||||
)
|
||||
|
||||
def test_missing_completeness_flag_denies(self) -> None:
|
||||
# No inventory_complete key at all → treated as incomplete.
|
||||
report = rc.evaluate_restart_impact({}, now=NOW)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_UNSAFE)
|
||||
self.assertFalse(report.allow_restart)
|
||||
|
||||
def test_no_other_work_is_safe(self) -> None:
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [_session("requester", pid=_live_pid())],
|
||||
"leases": [],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_SAFE)
|
||||
self.assertTrue(report.allow_restart)
|
||||
self.assertEqual(report.blast_radius, rc.BLAST_NONE)
|
||||
self.assertEqual(report.affected_issues, [])
|
||||
|
||||
def test_dead_foreign_session_and_lease_are_not_disruptive(self) -> None:
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [
|
||||
_session("requester", pid=_live_pid()),
|
||||
_session("dead", pid=_dead_pid()),
|
||||
],
|
||||
"leases": [
|
||||
_lease("l-dead", session_id="dead", freshness="stale_dead_process")
|
||||
],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_SAFE)
|
||||
self.assertTrue(report.allow_restart)
|
||||
self.assertEqual(report.counts["leases_disruptive"], 0)
|
||||
self.assertEqual(report.counts["sessions_live_other"], 0)
|
||||
|
||||
def test_live_foreign_lease_denies_without_override(self) -> None:
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [
|
||||
_session("requester", pid=_live_pid()),
|
||||
_session("worker", pid=_live_pid()),
|
||||
],
|
||||
"leases": [
|
||||
_lease(
|
||||
"l1",
|
||||
session_id="worker",
|
||||
freshness="active",
|
||||
worktree="/tmp/wt-658",
|
||||
phase="implementing",
|
||||
)
|
||||
],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_UNSAFE)
|
||||
self.assertFalse(report.allow_restart)
|
||||
# Critical section detected: active lease with a live owner.
|
||||
self.assertEqual(len(report.critical_sections), 1)
|
||||
self.assertEqual(report.affected_issues, [658])
|
||||
self.assertEqual(report.counts["mutations"], 1)
|
||||
self.assertTrue(report.override_would_allow)
|
||||
self.assertEqual(report.blast_radius, rc.BLAST_HIGH)
|
||||
# Placeholder ack state for the affected session.
|
||||
self.assertEqual(report.ack_state.get("worker"), "pending")
|
||||
|
||||
def test_operator_override_allows_despite_live_work(self) -> None:
|
||||
inv = {
|
||||
"inventory_complete": True,
|
||||
"sessions": [
|
||||
_session("requester", pid=_live_pid()),
|
||||
_session("worker", pid=_live_pid()),
|
||||
],
|
||||
"leases": [_lease("l1", session_id="worker", freshness="active")],
|
||||
}
|
||||
report = rc.evaluate_restart_impact(
|
||||
inv,
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
operator_override=True,
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_OVERRIDE)
|
||||
self.assertTrue(report.allow_restart)
|
||||
self.assertFalse(report.restart_performed)
|
||||
|
||||
def test_deny_when_critical_section_open(self) -> None:
|
||||
# A single live author lease in a mutating phase is a critical section
|
||||
# that must deny an un-overridden restart.
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [_session("worker", pid=_live_pid())],
|
||||
"leases": [
|
||||
_lease(
|
||||
"l1",
|
||||
session_id="worker",
|
||||
freshness="active",
|
||||
phase="merging",
|
||||
kind="pr",
|
||||
number=900,
|
||||
)
|
||||
],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_UNSAFE)
|
||||
self.assertFalse(report.allow_restart)
|
||||
self.assertEqual(report.affected_prs, [900])
|
||||
self.assertEqual(len(report.critical_sections), 1)
|
||||
|
||||
def test_terminal_lock_makes_restart_unsafe(self) -> None:
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [_session("requester", pid=_live_pid())],
|
||||
"leases": [],
|
||||
"terminal_lock": {"terminal_pr": 812},
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_UNSAFE)
|
||||
self.assertFalse(report.allow_restart)
|
||||
self.assertIsNotNone(report.terminal_lock)
|
||||
self.assertTrue(
|
||||
any("terminal" in reasoning for reasoning in report.reasons)
|
||||
)
|
||||
|
||||
def test_other_live_session_without_lease_is_disruptive(self) -> None:
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [
|
||||
_session("requester", pid=_live_pid()),
|
||||
_session("idle-but-live", pid=_live_pid()),
|
||||
],
|
||||
"leases": [],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_UNSAFE)
|
||||
self.assertEqual(report.counts["sessions_live_other"], 1)
|
||||
|
||||
def test_stale_heartbeat_session_not_counted_live(self) -> None:
|
||||
stale = NOW - timedelta(hours=2)
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [
|
||||
_session("requester", pid=_live_pid()),
|
||||
_session("stale", pid=_live_pid(), heartbeat=stale),
|
||||
],
|
||||
"leases": [],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.verdict, rc.VERDICT_SAFE)
|
||||
self.assertEqual(report.counts["sessions_live_other"], 0)
|
||||
|
||||
def test_prior_recovery_attempts_echoed(self) -> None:
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [_session("requester", pid=_live_pid())],
|
||||
"leases": [],
|
||||
"prior_recovery_attempts": [
|
||||
{"kind": "client_reconnect", "at": _ts(NOW)}
|
||||
],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(len(report.prior_recovery_attempts), 1)
|
||||
self.assertEqual(report.counts["prior_recovery_attempts"], 1)
|
||||
|
||||
def test_bare_string_freshness_accepted(self) -> None:
|
||||
lease = _lease("l1", session_id="worker", freshness="active")
|
||||
lease["freshness"] = "active" # bare string, not a dict
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [_session("worker", pid=_live_pid())],
|
||||
"leases": [lease],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.counts["leases_disruptive"], 1)
|
||||
|
||||
def test_as_dict_is_serializable_dto(self) -> None:
|
||||
import json
|
||||
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": [_session("requester", pid=_live_pid())],
|
||||
"leases": [],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
payload = report.as_dict()
|
||||
# Round-trips through JSON — safe for the console DTO.
|
||||
encoded = json.dumps(payload)
|
||||
decoded = json.loads(encoded)
|
||||
self.assertEqual(decoded["verdict"], rc.VERDICT_SAFE)
|
||||
self.assertIn("audit_record", decoded)
|
||||
self.assertEqual(decoded["audit_record"]["event"], "restart_impact_evaluated")
|
||||
self.assertFalse(decoded["restart_performed"])
|
||||
self.assertIn("coordinator_version", decoded)
|
||||
|
||||
|
||||
class ListSessionsTest(unittest.TestCase):
|
||||
def setUp(self) -> None:
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.db = ControlPlaneDB(os.path.join(self._tmp.name, "cp.sqlite3"))
|
||||
|
||||
def tearDown(self) -> None:
|
||||
self._tmp.cleanup()
|
||||
|
||||
def test_list_sessions_filters_by_status(self) -> None:
|
||||
self.db.upsert_session(session_id="a", role="author", pid=1, status="active")
|
||||
self.db.upsert_session(session_id="b", role="author", pid=2, status="ended")
|
||||
active = self.db.list_sessions(statuses=("active",))
|
||||
ids = {row["session_id"] for row in active}
|
||||
self.assertEqual(ids, {"a"})
|
||||
every = self.db.list_sessions()
|
||||
self.assertEqual({row["session_id"] for row in every}, {"a", "b"})
|
||||
|
||||
def test_list_sessions_feeds_coordinator(self) -> None:
|
||||
self.db.upsert_session(
|
||||
session_id="requester", role="author", pid=os.getpid(), status="active"
|
||||
)
|
||||
report = rc.evaluate_restart_impact(
|
||||
{
|
||||
"inventory_complete": True,
|
||||
"sessions": self.db.list_sessions(statuses=("active",)),
|
||||
"leases": [],
|
||||
},
|
||||
now=NOW,
|
||||
requesting_session_id="requester",
|
||||
)
|
||||
self.assertEqual(report.counts["sessions_total"], 1)
|
||||
|
||||
|
||||
if __name__ == "__main__": # pragma: no cover
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user