feat: non-destructive branch recovery after lost issue locks (Closes #440)
Implements the #420 recovery umbrella: keyed persistent issue locks with own-branch adoption, structured branch ownership parsing, create_pr lock resolution after MCP restart, and workflow docs forbidding destructive branch deletion as the default recovery path. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
@@ -291,7 +291,9 @@ recovery path.** That global slot is deprecated and can clobber unrelated live
|
|||||||
leases (#438). After an MCP restart, call `gitea_lock_issue` again — own-branch
|
leases (#438). After an MCP restart, call `gitea_lock_issue` again — own-branch
|
||||||
adoption rebinds the session when the issue's exact branch already exists (#442).
|
adoption rebinds the session when the issue's exact branch already exists (#442).
|
||||||
`gitea_create_pr` resolves the durable keyed lock by session pointer or by
|
`gitea_create_pr` resolves the durable keyed lock by session pointer or by
|
||||||
matching `head` branch without unsafe manual seeding.
|
matching `head` branch without unsafe manual seeding (#440). Branch ownership
|
||||||
|
uses structured ``(fix|feat|docs|chore)/issue-<n>-`` parsing — not broad
|
||||||
|
substring matches like ``issue-420`` inside ``issue-4200``.
|
||||||
|
|
||||||
Remote branches matching the issue number are also treated as active work unless
|
Remote branches matching the issue number are also treated as active work unless
|
||||||
the recovery review proves the branch is abandoned or superseded. Never delete
|
the recovery review proves the branch is abandoned or superseded. Never delete
|
||||||
|
|||||||
+2
-1
@@ -537,6 +537,7 @@ import role_namespace_gate # noqa: E402
|
|||||||
import task_capability_map # noqa: E402
|
import task_capability_map # noqa: E402
|
||||||
import review_proofs # noqa: E402
|
import review_proofs # noqa: E402
|
||||||
import agent_temp_artifacts
|
import agent_temp_artifacts
|
||||||
|
import issue_branch_ownership # noqa: E402
|
||||||
import issue_lock_worktree # noqa: E402
|
import issue_lock_worktree # noqa: E402
|
||||||
import issue_lock_store # noqa: E402
|
import issue_lock_store # noqa: E402
|
||||||
import issue_lock_adoption # noqa: E402
|
import issue_lock_adoption # noqa: E402
|
||||||
@@ -1226,7 +1227,7 @@ def gitea_lock_issue(
|
|||||||
pr_title = pr.get("title", "")
|
pr_title = pr.get("title", "")
|
||||||
pr_body = pr.get("body", "")
|
pr_body = pr.get("body", "")
|
||||||
|
|
||||||
if expected_pattern in pr_head:
|
if issue_branch_ownership.branch_tracks_issue(pr_head, issue_number):
|
||||||
raise ValueError(
|
raise ValueError(
|
||||||
f"Issue #{issue_number} is already tied to an open PR (PR #{pr.get('number')}, branch '{pr_head}') (fail closed)"
|
f"Issue #{issue_number} is already tied to an open PR (PR #{pr.get('number')}, branch '{pr_head}') (fail closed)"
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -0,0 +1,31 @@
|
|||||||
|
"""Structured issue/branch ownership evidence (#440).
|
||||||
|
|
||||||
|
Author branches must follow ``(fix|feat|docs|chore)/issue-<n>-<desc>``. Duplicate-work
|
||||||
|
and recovery gates use this parser instead of broad substring checks like
|
||||||
|
``issue-420`` inside unrelated names (for example ``issue-4200``).
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import re
|
||||||
|
|
||||||
|
IMPLEMENTATION_BRANCH_RE = re.compile(
|
||||||
|
r"^(?:fix|feat|docs|chore)/issue-(\d+)(?:-.+)?$"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def parse_tracked_issue_number(branch_name: str) -> int | None:
|
||||||
|
"""Return the issue number encoded in a canonical author branch name."""
|
||||||
|
text = (branch_name or "").strip()
|
||||||
|
if not text:
|
||||||
|
return None
|
||||||
|
match = IMPLEMENTATION_BRANCH_RE.match(text)
|
||||||
|
if not match:
|
||||||
|
return None
|
||||||
|
return int(match.group(1))
|
||||||
|
|
||||||
|
|
||||||
|
def branch_tracks_issue(branch_name: str, issue_number: int) -> bool:
|
||||||
|
"""True when ``branch_name`` structurally belongs to ``issue_number``."""
|
||||||
|
parsed = parse_tracked_issue_number(branch_name)
|
||||||
|
return parsed == issue_number if parsed is not None else False
|
||||||
@@ -14,6 +14,8 @@ this module additionally records whether they passed for proof purposes.
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import issue_branch_ownership
|
||||||
|
|
||||||
ADOPT = "adopt_existing_branch"
|
ADOPT = "adopt_existing_branch"
|
||||||
BLOCK_COMPETING = "block_competing_branch"
|
BLOCK_COMPETING = "block_competing_branch"
|
||||||
NO_MATCH = "no_matching_branch"
|
NO_MATCH = "no_matching_branch"
|
||||||
@@ -40,13 +42,12 @@ def assess_own_branch_adoption(
|
|||||||
existing_branches,
|
existing_branches,
|
||||||
) -> dict:
|
) -> dict:
|
||||||
"""Decide whether an existing matching branch is adoptable."""
|
"""Decide whether an existing matching branch is adoptable."""
|
||||||
marker = f"issue-{issue_number}"
|
|
||||||
requested = (requested_branch or "").strip()
|
requested = (requested_branch or "").strip()
|
||||||
|
|
||||||
matches: list[tuple[str, str | None]] = []
|
matches: list[tuple[str, str | None]] = []
|
||||||
for entry in existing_branches or []:
|
for entry in existing_branches or []:
|
||||||
name = _branch_name(entry).strip()
|
name = _branch_name(entry).strip()
|
||||||
if marker in name:
|
if issue_branch_ownership.branch_tracks_issue(name, issue_number):
|
||||||
matches.append((name, _branch_sha(entry)))
|
matches.append((name, _branch_sha(entry)))
|
||||||
|
|
||||||
competing = sorted({name for name, _ in matches if name != requested})
|
competing = sorted({name for name, _ in matches if name != requested})
|
||||||
|
|||||||
@@ -309,6 +309,15 @@ Do not implement unclaimed work.
|
|||||||
|
|
||||||
If the claim/lock gates are broken, produce a recovery handoff.
|
If the claim/lock gates are broken, produce a recovery handoff.
|
||||||
|
|
||||||
|
### Lost lock recovery after push (non-destructive)
|
||||||
|
|
||||||
|
If the MCP server restarts after the issue branch was pushed but before PR creation:
|
||||||
|
|
||||||
|
* **Do not** delete the remote branch as the normal recovery path.
|
||||||
|
* Call `gitea_lock_issue` again with the same issue number and exact branch name. Own-branch adoption rebinds the session when open-PR, competing-lock, and worktree safety checks pass.
|
||||||
|
* Call `gitea_create_pr` afterward. Durable keyed locks resolve from the session pointer or by matching the PR `head` branch without manual lock seeding.
|
||||||
|
* If adoption is blocked by an open PR, a competing live lease, or a different same-issue branch, stop and produce a recovery handoff.
|
||||||
|
|
||||||
Create a tooling issue only if this run is explicitly authorized to switch to issue-creation mode and exact `create_issue` capability is proven.
|
Create a tooling issue only if this run is explicitly authorized to switch to issue-creation mode and exact `create_issue` capability is proven.
|
||||||
|
|
||||||
Report:
|
Report:
|
||||||
|
|||||||
@@ -0,0 +1,35 @@
|
|||||||
|
"""Unit tests for structured issue/branch ownership parsing (#440)."""
|
||||||
|
import sys
|
||||||
|
import unittest
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||||
|
|
||||||
|
from issue_branch_ownership import ( # noqa: E402
|
||||||
|
branch_tracks_issue,
|
||||||
|
parse_tracked_issue_number,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestIssueBranchOwnership(unittest.TestCase):
|
||||||
|
def test_parses_canonical_author_branch(self):
|
||||||
|
self.assertEqual(
|
||||||
|
parse_tracked_issue_number("feat/issue-420-server-code-parity"),
|
||||||
|
420,
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_issue_4200_does_not_track_issue_420(self):
|
||||||
|
self.assertEqual(parse_tracked_issue_number("feat/issue-4200-unrelated"), 4200)
|
||||||
|
self.assertFalse(branch_tracks_issue("feat/issue-4200-unrelated", 420))
|
||||||
|
|
||||||
|
def test_branch_tracks_issue_exact_match(self):
|
||||||
|
self.assertTrue(
|
||||||
|
branch_tracks_issue("fix/issue-440-branch-recovery", 440)
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_unrelated_branch_does_not_track(self):
|
||||||
|
self.assertFalse(branch_tracks_issue("feat/issue-999-other", 440))
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
@@ -43,6 +43,14 @@ class TestAssessOwnBranchAdoption(unittest.TestCase):
|
|||||||
)
|
)
|
||||||
self.assertEqual(result["outcome"], NO_MATCH)
|
self.assertEqual(result["outcome"], NO_MATCH)
|
||||||
|
|
||||||
|
def test_issue_number_substring_collision_is_ignored(self):
|
||||||
|
result = assess_own_branch_adoption(
|
||||||
|
issue_number=420,
|
||||||
|
requested_branch=REQ,
|
||||||
|
existing_branches=[{"name": "feat/issue-4200-unrelated"}],
|
||||||
|
)
|
||||||
|
self.assertEqual(result["outcome"], NO_MATCH)
|
||||||
|
|
||||||
|
|
||||||
class TestBuildAdoptionProof(unittest.TestCase):
|
class TestBuildAdoptionProof(unittest.TestCase):
|
||||||
def test_proof_has_required_fields(self):
|
def test_proof_has_required_fields(self):
|
||||||
|
|||||||
@@ -0,0 +1,152 @@
|
|||||||
|
"""Integration tests for non-destructive lock/PR recovery (#440)."""
|
||||||
|
import os
|
||||||
|
import sys
|
||||||
|
import tempfile
|
||||||
|
import unittest
|
||||||
|
from pathlib import Path
|
||||||
|
from unittest import mock
|
||||||
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||||
|
|
||||||
|
import issue_lock_adoption # noqa: E402
|
||||||
|
import issue_lock_store as ils # noqa: E402
|
||||||
|
import mcp_server # noqa: E402
|
||||||
|
from mcp_server import gitea_create_pr, gitea_lock_issue # noqa: E402
|
||||||
|
from tests.test_mcp_server import CREATE_PR_ENV, ISSUE_WRITE_ENV # noqa: E402
|
||||||
|
|
||||||
|
FAKE_AUTH = "token fake"
|
||||||
|
BRANCH = "feat/issue-420-server-code-parity"
|
||||||
|
|
||||||
|
|
||||||
|
def _lock_record(**overrides):
|
||||||
|
record = {
|
||||||
|
"issue_number": 420,
|
||||||
|
"branch_name": BRANCH,
|
||||||
|
"remote": "prgs",
|
||||||
|
"org": "Scaled-Tech-Consulting",
|
||||||
|
"repo": mcp_server.REMOTES["prgs"]["repo"],
|
||||||
|
"worktree_path": "/tmp/wt-420",
|
||||||
|
"work_lease": {
|
||||||
|
"operation_type": "author_issue_work",
|
||||||
|
"expires_at": "2999-01-01T00:00:00Z",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
record.update(overrides)
|
||||||
|
return record
|
||||||
|
|
||||||
|
|
||||||
|
def _clean_git_state():
|
||||||
|
return {
|
||||||
|
"current_branch": BRANCH,
|
||||||
|
"porcelain_status": "",
|
||||||
|
"base_equivalent": True,
|
||||||
|
"inspected_git_root": os.getcwd(),
|
||||||
|
"base_branch": "master",
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
class TestIssueLockRecovery(unittest.TestCase):
|
||||||
|
def setUp(self):
|
||||||
|
self._tmpdir = tempfile.TemporaryDirectory()
|
||||||
|
self._lock_dir = self._tmpdir.name
|
||||||
|
self._env = {
|
||||||
|
**ISSUE_WRITE_ENV,
|
||||||
|
"GITEA_ISSUE_LOCK_DIR": self._lock_dir,
|
||||||
|
}
|
||||||
|
self._create_pr_env = {
|
||||||
|
**CREATE_PR_ENV,
|
||||||
|
"GITEA_ISSUE_LOCK_DIR": self._lock_dir,
|
||||||
|
}
|
||||||
|
|
||||||
|
def tearDown(self):
|
||||||
|
self._tmpdir.cleanup()
|
||||||
|
|
||||||
|
def test_adoption_after_restart_without_session_pointer(self):
|
||||||
|
with patch.dict(os.environ, self._env, clear=True):
|
||||||
|
with mock.patch("os.getpid", return_value=9999):
|
||||||
|
self.assertIsNone(ils.read_session_issue_lock())
|
||||||
|
|
||||||
|
with mock.patch(
|
||||||
|
"mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||||
|
return_value=_clean_git_state(),
|
||||||
|
), mock.patch("mcp_server.api_get_all") as mock_api, mock.patch(
|
||||||
|
"mcp_server.get_auth_header", return_value=FAKE_AUTH
|
||||||
|
):
|
||||||
|
mock_api.side_effect = [
|
||||||
|
[],
|
||||||
|
[{"name": BRANCH, "commit": {"id": "934688a"}}],
|
||||||
|
]
|
||||||
|
res = gitea_lock_issue(
|
||||||
|
issue_number=420,
|
||||||
|
branch_name=BRANCH,
|
||||||
|
remote="prgs",
|
||||||
|
worktree_path=os.path.realpath(os.getcwd()),
|
||||||
|
)
|
||||||
|
self.assertTrue(res["success"])
|
||||||
|
self.assertIn("adoption", res)
|
||||||
|
self.assertEqual(res["adoption"]["branch_head_commit"], "934688a")
|
||||||
|
|
||||||
|
@mock.patch(
|
||||||
|
"mcp_server.role_session_router.check_author_mutation_after_reviewer_stop",
|
||||||
|
return_value=(True, []),
|
||||||
|
)
|
||||||
|
@mock.patch("mcp_server.api_request")
|
||||||
|
@mock.patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||||
|
def test_create_pr_resolves_keyed_lock_after_restart(self, _auth, mock_api, _role):
|
||||||
|
mock_api.return_value = {"number": 501, "html_url": "https://example/pr/501"}
|
||||||
|
worktree = os.path.realpath(os.getcwd())
|
||||||
|
with patch.dict(os.environ, self._create_pr_env, clear=True):
|
||||||
|
path = ils.lock_file_path(
|
||||||
|
remote="prgs",
|
||||||
|
org=mcp_server.REMOTES["prgs"]["org"],
|
||||||
|
repo=mcp_server.REMOTES["prgs"]["repo"],
|
||||||
|
issue_number=420,
|
||||||
|
lock_dir=self._lock_dir,
|
||||||
|
)
|
||||||
|
ils.save_lock_file(path, _lock_record(worktree_path=worktree))
|
||||||
|
|
||||||
|
with mock.patch("os.getpid", return_value=4242):
|
||||||
|
self.assertIsNone(ils.read_session_issue_lock())
|
||||||
|
|
||||||
|
res = gitea_create_pr(
|
||||||
|
title="feat: recovery Closes #420",
|
||||||
|
head=BRANCH,
|
||||||
|
base="master",
|
||||||
|
remote="prgs",
|
||||||
|
worktree_path=worktree,
|
||||||
|
)
|
||||||
|
self.assertEqual(res["number"], 501)
|
||||||
|
|
||||||
|
def test_open_pr_blocks_adoption(self):
|
||||||
|
with patch.dict(os.environ, self._env, clear=True):
|
||||||
|
with mock.patch(
|
||||||
|
"mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||||
|
return_value=_clean_git_state(),
|
||||||
|
), mock.patch("mcp_server.api_get_all") as mock_api, mock.patch(
|
||||||
|
"mcp_server.get_auth_header", return_value=FAKE_AUTH
|
||||||
|
):
|
||||||
|
mock_api.side_effect = [
|
||||||
|
[{"number": 99, "head": {"ref": BRANCH}, "title": "", "body": ""}],
|
||||||
|
[{"name": BRANCH, "commit": {"id": "934688a"}}],
|
||||||
|
]
|
||||||
|
with self.assertRaises(ValueError) as ctx:
|
||||||
|
gitea_lock_issue(
|
||||||
|
issue_number=420,
|
||||||
|
branch_name=BRANCH,
|
||||||
|
remote="prgs",
|
||||||
|
worktree_path=os.path.realpath(os.getcwd()),
|
||||||
|
)
|
||||||
|
self.assertIn("already tied to an open PR", str(ctx.exception))
|
||||||
|
|
||||||
|
def test_structured_ownership_ignores_issue_4200_collision(self):
|
||||||
|
result = issue_lock_adoption.assess_own_branch_adoption(
|
||||||
|
issue_number=420,
|
||||||
|
requested_branch=BRANCH,
|
||||||
|
existing_branches=[{"name": "feat/issue-4200-unrelated"}],
|
||||||
|
)
|
||||||
|
self.assertEqual(result["outcome"], issue_lock_adoption.NO_MATCH)
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
Reference in New Issue
Block a user