From 80aa2956359c6314281ef96cc92e347553218b43 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 16:42:23 -0400 Subject: [PATCH] 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) --- docs/llm-workflow-runbooks.md | 4 +- gitea_mcp_server.py | 3 +- issue_branch_ownership.py | 31 ++++ issue_lock_adoption.py | 5 +- .../workflows/work-issue.md | 9 ++ tests/test_issue_branch_ownership.py | 35 ++++ tests/test_issue_lock_adoption.py | 8 + tests/test_issue_lock_recovery.py | 152 ++++++++++++++++++ 8 files changed, 243 insertions(+), 4 deletions(-) create mode 100644 issue_branch_ownership.py create mode 100644 tests/test_issue_branch_ownership.py create mode 100644 tests/test_issue_lock_recovery.py diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index f0165d8..1040415 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -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 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 -matching `head` branch without unsafe manual seeding. +matching `head` branch without unsafe manual seeding (#440). Branch ownership +uses structured ``(fix|feat|docs|chore)/issue--`` parsing — not broad +substring matches like ``issue-420`` inside ``issue-4200``. 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 diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 19abd4a..c56ba50 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -537,6 +537,7 @@ import role_namespace_gate # noqa: E402 import task_capability_map # noqa: E402 import review_proofs # noqa: E402 import agent_temp_artifacts +import issue_branch_ownership # noqa: E402 import issue_lock_worktree # noqa: E402 import issue_lock_store # noqa: E402 import issue_lock_adoption # noqa: E402 @@ -1226,7 +1227,7 @@ def gitea_lock_issue( pr_title = pr.get("title", "") pr_body = pr.get("body", "") - if expected_pattern in pr_head: + if issue_branch_ownership.branch_tracks_issue(pr_head, issue_number): raise ValueError( f"Issue #{issue_number} is already tied to an open PR (PR #{pr.get('number')}, branch '{pr_head}') (fail closed)" ) diff --git a/issue_branch_ownership.py b/issue_branch_ownership.py new file mode 100644 index 0000000..58fa04f --- /dev/null +++ b/issue_branch_ownership.py @@ -0,0 +1,31 @@ +"""Structured issue/branch ownership evidence (#440). + +Author branches must follow ``(fix|feat|docs|chore)/issue--``. 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 \ No newline at end of file diff --git a/issue_lock_adoption.py b/issue_lock_adoption.py index 7c8e5ef..db5071a 100644 --- a/issue_lock_adoption.py +++ b/issue_lock_adoption.py @@ -14,6 +14,8 @@ this module additionally records whether they passed for proof purposes. from __future__ import annotations +import issue_branch_ownership + ADOPT = "adopt_existing_branch" BLOCK_COMPETING = "block_competing_branch" NO_MATCH = "no_matching_branch" @@ -40,13 +42,12 @@ def assess_own_branch_adoption( existing_branches, ) -> dict: """Decide whether an existing matching branch is adoptable.""" - marker = f"issue-{issue_number}" requested = (requested_branch or "").strip() matches: list[tuple[str, str | None]] = [] for entry in existing_branches or []: 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))) competing = sorted({name for name, _ in matches if name != requested}) diff --git a/skills/llm-project-workflow/workflows/work-issue.md b/skills/llm-project-workflow/workflows/work-issue.md index f450cd7..e500963 100644 --- a/skills/llm-project-workflow/workflows/work-issue.md +++ b/skills/llm-project-workflow/workflows/work-issue.md @@ -309,6 +309,15 @@ Do not implement unclaimed work. 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. Report: diff --git a/tests/test_issue_branch_ownership.py b/tests/test_issue_branch_ownership.py new file mode 100644 index 0000000..a1f1b79 --- /dev/null +++ b/tests/test_issue_branch_ownership.py @@ -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() \ No newline at end of file diff --git a/tests/test_issue_lock_adoption.py b/tests/test_issue_lock_adoption.py index bf64ac1..e40f27a 100644 --- a/tests/test_issue_lock_adoption.py +++ b/tests/test_issue_lock_adoption.py @@ -43,6 +43,14 @@ class TestAssessOwnBranchAdoption(unittest.TestCase): ) 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): def test_proof_has_required_fields(self): diff --git a/tests/test_issue_lock_recovery.py b/tests/test_issue_lock_recovery.py new file mode 100644 index 0000000..df507ff --- /dev/null +++ b/tests/test_issue_lock_recovery.py @@ -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() \ No newline at end of file