Compare commits

..
Author SHA1 Message Date
sysadminandClaude Opus 4.8 80aa295635 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]>
2026-07-07 16:42:23 -04:00
9 changed files with 153 additions and 168 deletions
+3 -23
View File
@@ -291,29 +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
### Non-destructive lock recovery after push (#440) substring matches like ``issue-420`` inside ``issue-4200``.
When work is pushed but the in-memory MCP session lock is lost (server restart,
crashed process, or stale session pointer):
1. **Do not delete the remote branch.** Branch deletion is not a normal recovery
step and can destroy the only copy of unmerged work.
2. **Re-run `gitea_lock_issue`** with the same `issue_number`, exact
`branch_name`, and active `branches/` worktree. Own-branch adoption
reacquires the lease when the remote branch is the caller's exact branch and
no open PR or competing same-issue branch exists.
3. **Call `gitea_create_pr`** with the same `head` branch. The server resolves
the durable keyed lock file even when the session pointer was cleared at
restart.
4. **Stop fail-closed** when another actor owns a competing branch, an open PR
already exists, or a live foreign lease blocks takeover — never adopt their
branch.
Branch ownership uses structured evidence
`(fix|feat|docs|chore)/issue-<n>-<desc>` — not broad substring matches on
`issue-<n>`.
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
+6 -6
View File
@@ -537,10 +537,10 @@ 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
import issue_branch_ownership # noqa: E402
import already_landed_reconcile # noqa: E402 import already_landed_reconcile # noqa: E402
import author_mutation_worktree # noqa: E402 import author_mutation_worktree # noqa: E402
import issue_claim_heartbeat # noqa: E402 import issue_claim_heartbeat # noqa: E402
@@ -1172,11 +1172,11 @@ def gitea_lock_issue(
worktree_path: Author scratch-clone path to validate (defaults to worktree_path: Author scratch-clone path to validate (defaults to
GITEA_AUTHOR_WORKTREE or the MCP server project root). GITEA_AUTHOR_WORKTREE or the MCP server project root).
""" """
# 1. Enforce canonical issue branch ownership (#440) # 1. Enforce branch name includes issue number
if not issue_branch_ownership.branch_belongs_to_issue(branch_name, issue_number): expected_pattern = f"issue-{issue_number}"
if expected_pattern not in branch_name:
raise ValueError( raise ValueError(
f"Branch name '{branch_name}' must match " f"Branch name '{branch_name}' must contain locked issue pattern '{expected_pattern}' (fail closed)"
f"(fix|feat|docs|chore)/issue-{issue_number}-<desc> (fail closed)"
) )
blocked = _profile_permission_block( blocked = _profile_permission_block(
@@ -1227,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 issue_branch_ownership.branch_belongs_to_issue(pr_head, issue_number): 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)"
) )
+15 -12
View File
@@ -1,28 +1,31 @@
"""Structured issue/branch ownership evidence (#440). """Structured issue/branch ownership evidence (#440).
Author branches must follow ``(fix|feat|docs|chore)/issue-<n>-<desc>``. Duplicate-work Author branches must follow ``(fix|feat|docs|chore)/issue-<n>-<desc>``. Duplicate-work
and lock-recovery gates use this parser instead of broad substring matching on and recovery gates use this parser instead of broad substring checks like
``issue-<n>`` so unrelated branch names cannot false-positive. ``issue-420`` inside unrelated names (for example ``issue-4200``).
""" """
from __future__ import annotations from __future__ import annotations
import re import re
ISSUE_BRANCH_RE = re.compile( IMPLEMENTATION_BRANCH_RE = re.compile(
r"^(?P<prefix>fix|feat|docs|chore)/issue-(?P<num>\d+)(?:-|$)" r"^(?:fix|feat|docs|chore)/issue-(\d+)(?:-.+)?$"
) )
def parse_issue_branch(branch_name: str) -> int | None: def parse_tracked_issue_number(branch_name: str) -> int | None:
"""Return the issue number encoded in a canonical author branch, else None.""" """Return the issue number encoded in a canonical author branch name."""
match = ISSUE_BRANCH_RE.match((branch_name or "").strip()) text = (branch_name or "").strip()
if not text:
return None
match = IMPLEMENTATION_BRANCH_RE.match(text)
if not match: if not match:
return None return None
return int(match.group("num")) return int(match.group(1))
def branch_belongs_to_issue(branch_name: str, issue_number: int) -> bool: def branch_tracks_issue(branch_name: str, issue_number: int) -> bool:
"""True when ``branch_name`` is a canonical branch for ``issue_number``.""" """True when ``branch_name`` structurally belongs to ``issue_number``."""
parsed = parse_issue_branch(branch_name) parsed = parse_tracked_issue_number(branch_name)
return parsed is not None and parsed == issue_number return parsed == issue_number if parsed is not None else False
+2 -2
View File
@@ -1,4 +1,4 @@
"""Own-branch lock adoption / recovery for ``gitea_lock_issue`` (#440 / #442 / #443). """Own-branch lock adoption / recovery for ``gitea_lock_issue`` (#442 / #443).
When an issue's own already-pushed branch exists, lock reacquisition must be When an issue's own already-pushed branch exists, lock reacquisition must be
allowed (adoption) instead of being treated as #400 duplicate competing work. allowed (adoption) instead of being treated as #400 duplicate competing work.
@@ -47,7 +47,7 @@ def assess_own_branch_adoption(
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 issue_branch_ownership.branch_belongs_to_issue(name, issue_number): 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:
+15 -16
View File
@@ -1,35 +1,34 @@
"""Structured issue branch ownership (#440).""" """Unit tests for structured issue/branch ownership parsing (#440)."""
import sys import sys
import unittest import unittest
from pathlib import Path from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
import issue_branch_ownership as ibo # noqa: E402 from issue_branch_ownership import ( # noqa: E402
branch_tracks_issue,
parse_tracked_issue_number,
)
class TestIssueBranchOwnership(unittest.TestCase): class TestIssueBranchOwnership(unittest.TestCase):
def test_canonical_branch_parses_issue_number(self): def test_parses_canonical_author_branch(self):
self.assertEqual( self.assertEqual(
ibo.parse_issue_branch("feat/issue-420-server-code-parity"), parse_tracked_issue_number("feat/issue-420-server-code-parity"),
420, 420,
) )
def test_prefix_variants_supported(self): def test_issue_4200_does_not_track_issue_420(self):
for prefix in ("fix", "feat", "docs", "chore"): self.assertEqual(parse_tracked_issue_number("feat/issue-4200-unrelated"), 4200)
branch = f"{prefix}/issue-440-lock-recovery" self.assertFalse(branch_tracks_issue("feat/issue-4200-unrelated", 420))
self.assertTrue(ibo.branch_belongs_to_issue(branch, 440))
def test_substring_false_positive_rejected(self): def test_branch_tracks_issue_exact_match(self):
self.assertFalse( self.assertTrue(
ibo.branch_belongs_to_issue("feat/my-issue-420-backport", 420) branch_tracks_issue("fix/issue-440-branch-recovery", 440)
) )
self.assertFalse(ibo.branch_belongs_to_issue("release/issue-420-hotfix", 420))
def test_different_issue_number_rejected(self): def test_unrelated_branch_does_not_track(self):
self.assertFalse( self.assertFalse(branch_tracks_issue("feat/issue-999-other", 440))
ibo.branch_belongs_to_issue("feat/issue-421-server-code-parity", 420)
)
if __name__ == "__main__": if __name__ == "__main__":
+2 -3
View File
@@ -43,14 +43,13 @@ class TestAssessOwnBranchAdoption(unittest.TestCase):
) )
self.assertEqual(result["outcome"], NO_MATCH) self.assertEqual(result["outcome"], NO_MATCH)
def test_substring_only_branch_name_is_ignored(self): def test_issue_number_substring_collision_is_ignored(self):
result = assess_own_branch_adoption( result = assess_own_branch_adoption(
issue_number=420, issue_number=420,
requested_branch=REQ, requested_branch=REQ,
existing_branches=[{"name": "feat/my-issue-420-backport"}], existing_branches=[{"name": "feat/issue-4200-unrelated"}],
) )
self.assertEqual(result["outcome"], NO_MATCH) self.assertEqual(result["outcome"], NO_MATCH)
self.assertFalse(result["block"])
class TestBuildAdoptionProof(unittest.TestCase): class TestBuildAdoptionProof(unittest.TestCase):
+79 -84
View File
@@ -1,9 +1,10 @@
"""End-to-end lock recovery scenarios for issue #440.""" """Integration tests for non-destructive lock/PR recovery (#440)."""
import os import os
import sys import sys
import tempfile import tempfile
import unittest import unittest
from pathlib import Path from pathlib import Path
from unittest import mock
from unittest.mock import patch from unittest.mock import patch
sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
@@ -12,31 +13,10 @@ import issue_lock_adoption # noqa: E402
import issue_lock_store as ils # noqa: E402 import issue_lock_store as ils # noqa: E402
import mcp_server # noqa: E402 import mcp_server # noqa: E402
from mcp_server import gitea_create_pr, gitea_lock_issue # 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
PRGS_REPO = mcp_server.REMOTES["prgs"]["repo"] FAKE_AUTH = "token fake"
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
BRANCH = "feat/issue-420-server-code-parity" BRANCH = "feat/issue-420-server-code-parity"
ISSUE_WRITE_ENV = {
"GITEA_ALLOWED_OPERATIONS": (
"gitea.issue.create,gitea.issue.close,gitea.issue.comment"
),
}
CREATE_PR_ENV = {
"GITEA_PROFILE_NAME": "author-test",
"GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.pr.create,gitea.branch.push",
"GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.approve,gitea.pr.merge,gitea.pr.review",
}
def _clean_master_git_state_for_lock():
return {
"current_branch": "master",
"porcelain_status": "",
"base_equivalent": True,
"inspected_git_root": "/tmp/repo",
"base_branch": "master",
}
def _lock_record(**overrides): def _lock_record(**overrides):
@@ -45,8 +25,8 @@ def _lock_record(**overrides):
"branch_name": BRANCH, "branch_name": BRANCH,
"remote": "prgs", "remote": "prgs",
"org": "Scaled-Tech-Consulting", "org": "Scaled-Tech-Consulting",
"repo": PRGS_REPO, "repo": mcp_server.REMOTES["prgs"]["repo"],
"worktree_path": os.path.realpath(os.getcwd()), "worktree_path": "/tmp/wt-420",
"work_lease": { "work_lease": {
"operation_type": "author_issue_work", "operation_type": "author_issue_work",
"expires_at": "2999-01-01T00:00:00Z", "expires_at": "2999-01-01T00:00:00Z",
@@ -56,102 +36,117 @@ def _lock_record(**overrides):
return record 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): class TestIssueLockRecovery(unittest.TestCase):
def setUp(self): def setUp(self):
self._tmpdir = tempfile.TemporaryDirectory() self._tmpdir = tempfile.TemporaryDirectory()
self.addCleanup(self._tmpdir.cleanup) self._lock_dir = self._tmpdir.name
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 test_substring_branch_does_not_trigger_adoption(self): def tearDown(self):
result = issue_lock_adoption.assess_own_branch_adoption( self._tmpdir.cleanup()
issue_number=420,
requested_branch=BRANCH,
existing_branches=[{"name": "feat/my-issue-420-backport"}],
)
self.assertEqual(result["outcome"], issue_lock_adoption.NO_MATCH)
def test_competing_actor_branch_blocks_adoption(self): def test_adoption_after_restart_without_session_pointer(self):
result = issue_lock_adoption.assess_own_branch_adoption( with patch.dict(os.environ, self._env, clear=True):
issue_number=420, with mock.patch("os.getpid", return_value=9999):
requested_branch=BRANCH, self.assertIsNone(ils.read_session_issue_lock())
existing_branches=[{"name": "feat/issue-420-other-work"}],
)
self.assertTrue(result["block"])
@patch( with mock.patch(
"mcp_server.issue_lock_worktree.read_worktree_git_state", "mcp_server.issue_lock_worktree.read_worktree_git_state",
return_value=_clean_master_git_state_for_lock(), return_value=_clean_git_state(),
) ), mock.patch("mcp_server.api_get_all") as mock_api, mock.patch(
@patch("mcp_server.api_get_all") "mcp_server.get_auth_header", return_value=FAKE_AUTH
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) ):
def test_lock_recovery_after_restart_adopts_own_branch(self, _auth, mock_api, _git):
mock_api.side_effect = [ mock_api.side_effect = [
[], [],
[{"name": BRANCH, "commit": {"id": "934688a"}}], [{"name": BRANCH, "commit": {"id": "934688a"}}],
] ]
env = {**ISSUE_WRITE_ENV, "GITEA_ISSUE_LOCK_DIR": self.lock_dir}
with patch.dict(os.environ, env, clear=True):
with patch("os.getpid", return_value=9999):
res = gitea_lock_issue( res = gitea_lock_issue(
issue_number=420, issue_number=420,
branch_name=BRANCH, branch_name=BRANCH,
remote="prgs", remote="prgs",
worktree_path=os.path.realpath(os.getcwd()),
) )
self.assertTrue(res["success"]) self.assertTrue(res["success"])
self.assertIn("adoption", res) self.assertIn("adoption", res)
found = ils.find_lock_for_branch( self.assertEqual(res["adoption"]["branch_head_commit"], "934688a")
remote="prgs",
org="Scaled-Tech-Consulting",
repo=PRGS_REPO,
branch_name=BRANCH,
lock_dir=self.lock_dir,
)
self.assertEqual(found["branch_name"], BRANCH)
@patch("mcp_server.api_request") @mock.patch(
@patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) "mcp_server.role_session_router.check_author_mutation_after_reviewer_stop",
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) return_value=(True, []),
def test_create_pr_resolves_durable_lock_after_session_loss(self, _auth, _role, mock_api): )
mock_api.return_value = {"number": 421, "html_url": "https://example/pr/421"} @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()) worktree = os.path.realpath(os.getcwd())
with patch.dict(os.environ, self._create_pr_env, clear=True):
path = ils.lock_file_path( path = ils.lock_file_path(
remote="prgs", remote="prgs",
org="Scaled-Tech-Consulting", org=mcp_server.REMOTES["prgs"]["org"],
repo=PRGS_REPO, repo=mcp_server.REMOTES["prgs"]["repo"],
issue_number=420, issue_number=420,
lock_dir=self.lock_dir, lock_dir=self._lock_dir,
) )
ils.save_lock_file(path, _lock_record(worktree_path=worktree)) ils.save_lock_file(path, _lock_record(worktree_path=worktree))
env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": self.lock_dir} with mock.patch("os.getpid", return_value=4242):
with patch.dict(os.environ, env, clear=True): self.assertIsNone(ils.read_session_issue_lock())
with patch("os.getpid", return_value=8888):
self.assertIsNone(ils.read_session_issue_lock(lock_dir=self.lock_dir))
res = gitea_create_pr( res = gitea_create_pr(
title=f"feat: server parity Closes #420", title="feat: recovery Closes #420",
head=BRANCH, head=BRANCH,
base="master",
remote="prgs", remote="prgs",
worktree_path=worktree, worktree_path=worktree,
) )
self.assertEqual(res["number"], 421) self.assertEqual(res["number"], 501)
@patch( 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", "mcp_server.issue_lock_worktree.read_worktree_git_state",
return_value=_clean_master_git_state_for_lock(), return_value=_clean_git_state(),
) ), mock.patch("mcp_server.api_get_all") as mock_api, mock.patch(
@patch("mcp_server.api_get_all") "mcp_server.get_auth_header", return_value=FAKE_AUTH
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) ):
def test_open_pr_blocks_recovery_lock(self, _auth, mock_api, _git):
mock_api.side_effect = [ mock_api.side_effect = [
[{"number": 99, "head": {"ref": BRANCH}, "title": "WIP", "body": ""}], [{"number": 99, "head": {"ref": BRANCH}, "title": "", "body": ""}],
[], [{"name": BRANCH, "commit": {"id": "934688a"}}],
] ]
env = {**ISSUE_WRITE_ENV, "GITEA_ISSUE_LOCK_DIR": self.lock_dir}
with patch.dict(os.environ, env, clear=True):
with self.assertRaises(ValueError) as ctx: with self.assertRaises(ValueError) as ctx:
gitea_lock_issue(issue_number=420, branch_name=BRANCH, remote="prgs") 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)) 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__": if __name__ == "__main__":
unittest.main() unittest.main()
+1 -1
View File
@@ -3115,7 +3115,7 @@ class TestIssueLocking(unittest.TestCase):
def test_lock_issue_mismatch_branch_fails(self): def test_lock_issue_mismatch_branch_fails(self):
with self.assertRaises(ValueError) as ctx: with self.assertRaises(ValueError) as ctx:
gitea_lock_issue(issue_number=196, branch_name="feat/issue-195-mutations", remote="prgs") gitea_lock_issue(issue_number=196, branch_name="feat/issue-195-mutations", remote="prgs")
self.assertIn("must match", str(ctx.exception)) self.assertIn("must contain locked issue pattern", str(ctx.exception))
@patch( @patch(
"mcp_server.issue_lock_worktree.read_worktree_git_state", "mcp_server.issue_lock_worktree.read_worktree_git_state",