feat: replace global issue lock with keyed persistent store (Closes #443)
Store per remote/org/repo/issue locks under GITEA_ISSUE_LOCK_DIR with atomic writes and per-session binding. Integrate own-branch adoption for lock recovery, update worktree-start and cleanup reconcile, and add tests documenting the ban on manual global lock seeding. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
+186
-76
@@ -6,6 +6,7 @@ the MCP protocol) with mocked API responses.
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
import unittest
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
@@ -45,6 +46,7 @@ from gitea_auth import get_profile # noqa: E402
|
||||
import gitea_config # noqa: E402
|
||||
|
||||
import mcp_server
|
||||
import issue_lock_store
|
||||
|
||||
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
|
||||
|
||||
@@ -97,9 +99,6 @@ CREATE_PR_ENV = {
|
||||
),
|
||||
}
|
||||
|
||||
ISSUE_LOCK_FILE = "/tmp/gitea_issue_lock.json"
|
||||
|
||||
|
||||
def _sample_issue_lock(issue_number=123, branch_name="feat/x", **overrides):
|
||||
import issue_lock_provenance
|
||||
|
||||
@@ -116,6 +115,7 @@ def _sample_issue_lock(issue_number=123, branch_name="feat/x", **overrides):
|
||||
"remote": "dadeschools",
|
||||
"org": "Scaled-Tech-Consulting",
|
||||
"repo": "Gitea-Tools",
|
||||
"worktree_path": "/tmp/test-worktree",
|
||||
"work_lease": work_lease,
|
||||
"lock_provenance": issue_lock_provenance.build_sanctioned_lock_provenance(
|
||||
tool="gitea_lock_issue",
|
||||
@@ -131,6 +131,17 @@ def _clear_duplicate_context_fetcher(*_args, **_kwargs):
|
||||
return [], [], {"status": "not_claimed"}
|
||||
|
||||
|
||||
def _bind_test_lock(**overrides) -> str:
|
||||
remote = overrides.get("remote", "dadeschools")
|
||||
record = _sample_issue_lock(**overrides)
|
||||
if remote in mcp_server.REMOTES:
|
||||
profile = mcp_server.REMOTES[remote]
|
||||
record.setdefault("org", profile["org"])
|
||||
record.setdefault("repo", profile["repo"])
|
||||
record["remote"] = remote
|
||||
return issue_lock_store.bind_session_lock(record)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Create Issue
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -193,18 +204,21 @@ class TestCreatePR(unittest.TestCase):
|
||||
return_value=(True, []))
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("os.path.exists", return_value=True)
|
||||
@patch("builtins.open")
|
||||
def test_creates_pr(self, mock_open, mock_exists, _auth, mock_api, _role, _dup_fetcher):
|
||||
lock_json = json.dumps(_sample_issue_lock(issue_number=123, branch_name="feat/x"))
|
||||
mock_open.return_value.__enter__.return_value.read.return_value = lock_json
|
||||
def test_creates_pr(self, _auth, mock_api, _role, _dup_fetcher):
|
||||
worktree = os.path.realpath(os.getcwd())
|
||||
mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"}
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
result = gitea_create_pr(title="feat: X Closes #123", head="feat/x", base="main")
|
||||
with tempfile.TemporaryDirectory() as lock_dir:
|
||||
env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir}
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
_bind_test_lock(issue_number=123, branch_name="feat/x", worktree_path=worktree)
|
||||
result = gitea_create_pr(
|
||||
title="feat: X Closes #123",
|
||||
head="feat/x",
|
||||
base="main",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
self.assertEqual(result["number"], 3)
|
||||
self.assertNotIn("url", result)
|
||||
mock_exists.assert_called_with(ISSUE_LOCK_FILE)
|
||||
mock_open.assert_called_with(ISSUE_LOCK_FILE, "r", encoding="utf-8")
|
||||
payload = mock_api.call_args[0][3]
|
||||
self.assertEqual(payload["head"], "feat/x")
|
||||
self.assertEqual(payload["base"], "main")
|
||||
@@ -218,30 +232,42 @@ class TestCreatePR(unittest.TestCase):
|
||||
return_value=(True, []))
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("os.path.exists", return_value=True)
|
||||
@patch("builtins.open")
|
||||
def test_create_pr_reveal_opt_in_includes_url(self, mock_open, mock_exists, _auth, mock_api, _role, _dup_fetcher):
|
||||
lock_json = json.dumps(_sample_issue_lock(issue_number=123, branch_name="feat/x"))
|
||||
mock_open.return_value.__enter__.return_value.read.return_value = lock_json
|
||||
def test_create_pr_reveal_opt_in_includes_url(self, _auth, mock_api, _role, _dup_fetcher):
|
||||
worktree = os.path.realpath(os.getcwd())
|
||||
mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"}
|
||||
env = {**CREATE_PR_ENV, "GITEA_MCP_REVEAL_ENDPOINTS": "1"}
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
result = gitea_create_pr(title="feat: X Closes #123", head="feat/x", base="main")
|
||||
with tempfile.TemporaryDirectory() as lock_dir:
|
||||
env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir, "GITEA_MCP_REVEAL_ENDPOINTS": "1"}
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
_bind_test_lock(issue_number=123, branch_name="feat/x", worktree_path=worktree)
|
||||
result = gitea_create_pr(
|
||||
title="feat: X Closes #123",
|
||||
head="feat/x",
|
||||
base="main",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
self.assertIn("pulls/3", result["url"])
|
||||
|
||||
@patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop",
|
||||
return_value=(True, []))
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
@patch("os.path.exists", return_value=True)
|
||||
@patch("builtins.open")
|
||||
def test_create_pr_locked_issue_mismatch_fails(self, mock_open, mock_exists, _auth, _role):
|
||||
lock_json = json.dumps(_sample_issue_lock(issue_number=123, branch_name="feat/x"))
|
||||
mock_open.return_value.__enter__.return_value.read.return_value = lock_json
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
gitea_create_pr(title="feat: X Closes #999", head="feat/x", base="main")
|
||||
def test_create_pr_locked_issue_mismatch_fails(self, _auth, _role):
|
||||
worktree = os.path.realpath(os.getcwd())
|
||||
with tempfile.TemporaryDirectory() as lock_dir:
|
||||
env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir}
|
||||
with patch.dict(os.environ, env, clear=True):
|
||||
_bind_test_lock(
|
||||
issue_number=123,
|
||||
branch_name="feat/x",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
gitea_create_pr(
|
||||
title="feat: X Closes #999",
|
||||
head="feat/x",
|
||||
base="main",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
self.assertIn("Closes #123", str(ctx.exception))
|
||||
mock_open.assert_called_with(ISSUE_LOCK_FILE, "r", encoding="utf-8")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -3069,7 +3095,12 @@ class TestIssueLocking(unittest.TestCase):
|
||||
"""Test issue locking and PR gating constraints."""
|
||||
|
||||
def setUp(self):
|
||||
self._env_patcher = patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True)
|
||||
self._lock_dir = tempfile.TemporaryDirectory()
|
||||
env = {
|
||||
**ISSUE_WRITE_ENV,
|
||||
"GITEA_ISSUE_LOCK_DIR": self._lock_dir.name,
|
||||
}
|
||||
self._env_patcher = patch.dict(os.environ, env, clear=True)
|
||||
self._env_patcher.start()
|
||||
self._dup_fetcher_patcher = patch(
|
||||
"mcp_server.issue_duplicate_context_fetcher",
|
||||
@@ -3080,15 +3111,21 @@ class TestIssueLocking(unittest.TestCase):
|
||||
def tearDown(self):
|
||||
self._dup_fetcher_patcher.stop()
|
||||
self._env_patcher.stop()
|
||||
if os.path.exists(ISSUE_LOCK_FILE):
|
||||
os.remove(ISSUE_LOCK_FILE)
|
||||
self._lock_dir.cleanup()
|
||||
|
||||
def _create_pr_env(self) -> dict:
|
||||
return {
|
||||
**CREATE_PR_ENV,
|
||||
"GITEA_ISSUE_LOCK_DIR": self._lock_dir.name,
|
||||
}
|
||||
|
||||
@patch(
|
||||
"mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||
return_value=_clean_master_git_state_for_lock(),
|
||||
)
|
||||
@patch("mcp_server.api_get_all", return_value=[])
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_lock_issue_success(self, _auth, _git_state):
|
||||
def test_lock_issue_success(self, _auth, _api, _git_state):
|
||||
res = gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs")
|
||||
self.assertTrue(res["success"])
|
||||
self.assertEqual(res["work_lease"]["operation_type"], "author_issue_work")
|
||||
@@ -3098,9 +3135,8 @@ class TestIssueLocking(unittest.TestCase):
|
||||
self.assertIn("expires_at", res["work_lease"])
|
||||
self.assertIn("last_heartbeat_at", res["work_lease"])
|
||||
self.assertEqual(res["work_lease"]["claimant"]["profile"], "gitea-default")
|
||||
self.assertTrue(os.path.exists(ISSUE_LOCK_FILE))
|
||||
with open(ISSUE_LOCK_FILE, encoding="utf-8") as f:
|
||||
lock = json.load(f)
|
||||
self.assertIn("lock_file_path", res)
|
||||
lock = issue_lock_store.read_lock_file(res["lock_file_path"])
|
||||
self.assertIn("worktree_path", lock)
|
||||
self.assertIn("work_lease", lock)
|
||||
|
||||
@@ -3156,32 +3192,81 @@ class TestIssueLocking(unittest.TestCase):
|
||||
gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs")
|
||||
self.assertIn("remote branch(es) already match issue pattern", str(ctx.exception))
|
||||
|
||||
def test_lock_issue_blocks_active_same_operation_lease(self):
|
||||
with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f:
|
||||
json.dump({
|
||||
@patch(
|
||||
"mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||
return_value=_clean_master_git_state_for_lock(),
|
||||
)
|
||||
@patch("mcp_server.api_get_all")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_lock_issue_adopts_exact_own_branch(self, _auth, mock_api, _git_state):
|
||||
branch = "feat/issue-196-mutations"
|
||||
self.mock_dup_fetcher.return_value = ([], [branch], {"status": "not_claimed"})
|
||||
mock_api.return_value = [{"name": branch, "commit": {"id": "abc123"}}]
|
||||
res = gitea_lock_issue(issue_number=196, branch_name=branch, remote="prgs")
|
||||
self.assertTrue(res["success"])
|
||||
self.assertIn("adoption", res)
|
||||
self.assertEqual(res["adoption"]["branch_head_commit"], "abc123")
|
||||
|
||||
@patch(
|
||||
"mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||
return_value=_clean_master_git_state_for_lock(),
|
||||
)
|
||||
@patch("mcp_server.api_get_all", return_value=[])
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_lock_issue_blocks_active_same_operation_lease(self, _auth, _api, _git_state):
|
||||
prgs_repo = mcp_server.REMOTES["prgs"]["repo"]
|
||||
issue_lock_store.save_lock_file(
|
||||
issue_lock_store.lock_file_path(
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo=prgs_repo,
|
||||
issue_number=196,
|
||||
),
|
||||
{
|
||||
"issue_number": 196,
|
||||
"branch_name": "feat/issue-196-other-work",
|
||||
"remote": "prgs",
|
||||
"org": "Scaled-Tech-Consulting",
|
||||
"repo": prgs_repo,
|
||||
"worktree_path": "/tmp/other-worktree",
|
||||
"work_lease": {
|
||||
"operation_type": "author_issue_work",
|
||||
"expires_at": "2999-01-01T00:00:00Z",
|
||||
},
|
||||
}, f)
|
||||
},
|
||||
)
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs")
|
||||
self.assertIn("already has an active author_issue_work lease", str(ctx.exception))
|
||||
|
||||
def test_lock_issue_blocks_expired_same_operation_lease_for_recovery(self):
|
||||
with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f:
|
||||
json.dump({
|
||||
@patch(
|
||||
"mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||
return_value=_clean_master_git_state_for_lock(),
|
||||
)
|
||||
@patch("mcp_server.api_get_all", return_value=[])
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_lock_issue_blocks_expired_same_operation_lease_for_recovery(self, _auth, _api, _git_state):
|
||||
prgs_repo = mcp_server.REMOTES["prgs"]["repo"]
|
||||
issue_lock_store.save_lock_file(
|
||||
issue_lock_store.lock_file_path(
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo=prgs_repo,
|
||||
issue_number=196,
|
||||
),
|
||||
{
|
||||
"issue_number": 196,
|
||||
"branch_name": "feat/issue-196-other-work",
|
||||
"remote": "prgs",
|
||||
"org": "Scaled-Tech-Consulting",
|
||||
"repo": prgs_repo,
|
||||
"worktree_path": "/tmp/other-worktree",
|
||||
"work_lease": {
|
||||
"operation_type": "author_issue_work",
|
||||
"expires_at": "2000-01-01T00:00:00Z",
|
||||
},
|
||||
}, f)
|
||||
},
|
||||
)
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs")
|
||||
self.assertIn("Recovery review is required before takeover", str(ctx.exception))
|
||||
@@ -3248,9 +3333,7 @@ class TestIssueLocking(unittest.TestCase):
|
||||
return_value=(True, []))
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_create_pr_missing_lock_fails(self, _auth, _role):
|
||||
if os.path.exists(ISSUE_LOCK_FILE):
|
||||
os.remove(ISSUE_LOCK_FILE)
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
with patch.dict(os.environ, self._create_pr_env(), clear=True):
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
gitea_create_pr(title="feat: X Closes #196", head="feat/issue-196-mutations", remote="prgs")
|
||||
self.assertIn("Issue lock is missing", str(ctx.exception))
|
||||
@@ -3259,37 +3342,64 @@ class TestIssueLocking(unittest.TestCase):
|
||||
return_value=(True, []))
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_create_pr_branch_mismatch_fails(self, _auth, _role):
|
||||
with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f:
|
||||
json.dump(_sample_issue_lock(
|
||||
issue_number=196, branch_name="feat/issue-196-mutations"), f)
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
worktree = os.path.realpath(os.getcwd())
|
||||
_bind_test_lock(
|
||||
issue_number=196,
|
||||
branch_name="feat/issue-196-mutations",
|
||||
remote="prgs",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
with patch.dict(os.environ, self._create_pr_env(), clear=True):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
gitea_create_pr(title="feat: X Closes #196", head="feat/issue-196-different", remote="prgs")
|
||||
gitea_create_pr(
|
||||
title="feat: X Closes #196",
|
||||
head="feat/issue-196-different",
|
||||
remote="prgs",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
self.assertIn("does not match locked branch", str(ctx.exception))
|
||||
|
||||
@patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop",
|
||||
return_value=(True, []))
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_create_pr_forbidden_terms_fails(self, _auth, _role):
|
||||
with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f:
|
||||
json.dump(_sample_issue_lock(
|
||||
issue_number=196, branch_name="feat/issue-196-mutations"), f)
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
worktree = os.path.realpath(os.getcwd())
|
||||
_bind_test_lock(
|
||||
issue_number=196,
|
||||
branch_name="feat/issue-196-mutations",
|
||||
remote="prgs",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
with patch.dict(os.environ, self._create_pr_env(), clear=True):
|
||||
for term in ("equivalent to #196", "related to #196", "same as #196"):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
gitea_create_pr(title=f"feat: X {term}", head="feat/issue-196-mutations", remote="prgs")
|
||||
gitea_create_pr(
|
||||
title=f"feat: X {term}",
|
||||
head="feat/issue-196-mutations",
|
||||
remote="prgs",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
self.assertIn("contains forbidden term", str(ctx.exception))
|
||||
|
||||
@patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop",
|
||||
return_value=(True, []))
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_create_pr_missing_closes_ref_fails(self, _auth, _role):
|
||||
with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f:
|
||||
json.dump(_sample_issue_lock(
|
||||
issue_number=196, branch_name="feat/issue-196-mutations"), f)
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
worktree = os.path.realpath(os.getcwd())
|
||||
_bind_test_lock(
|
||||
issue_number=196,
|
||||
branch_name="feat/issue-196-mutations",
|
||||
remote="prgs",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
with patch.dict(os.environ, self._create_pr_env(), clear=True):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
gitea_create_pr(title="feat: X refs #196", head="feat/issue-196-mutations", remote="prgs")
|
||||
gitea_create_pr(
|
||||
title="feat: X refs #196",
|
||||
head="feat/issue-196-mutations",
|
||||
remote="prgs",
|
||||
worktree_path=worktree,
|
||||
)
|
||||
self.assertIn("must contain 'Closes #196' or 'Fixes #196' exactly", str(ctx.exception))
|
||||
|
||||
@patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop",
|
||||
@@ -3297,13 +3407,13 @@ class TestIssueLocking(unittest.TestCase):
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_create_pr_worktree_mismatch_fails(self, _auth, _role):
|
||||
scratch = os.path.realpath("/tmp/gitea-tools-author-scratch/issue-249-pr")
|
||||
with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f:
|
||||
json.dump(_sample_issue_lock(
|
||||
issue_number=249,
|
||||
branch_name="feat/issue-249-issue-lock-scratch-worktree",
|
||||
worktree_path=scratch,
|
||||
), f)
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
_bind_test_lock(
|
||||
issue_number=249,
|
||||
branch_name="feat/issue-249-issue-lock-scratch-worktree",
|
||||
worktree_path=scratch,
|
||||
remote="prgs",
|
||||
)
|
||||
with patch.dict(os.environ, self._create_pr_env(), clear=True):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
gitea_create_pr(
|
||||
title="feat: lock scratch worktree Closes #249",
|
||||
@@ -3342,13 +3452,13 @@ class TestIssueLocking(unittest.TestCase):
|
||||
def test_create_pr_honors_scratch_worktree_lock(self, _auth, _role, mock_api):
|
||||
scratch = os.path.realpath("/tmp/gitea-tools-author-scratch/issue-249-e2e")
|
||||
mock_api.return_value = {"number": 250, "html_url": "https://example/pr/250"}
|
||||
with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f:
|
||||
json.dump(_sample_issue_lock(
|
||||
issue_number=249,
|
||||
branch_name="feat/issue-249-issue-lock-scratch-worktree",
|
||||
worktree_path=scratch,
|
||||
), f)
|
||||
with patch.dict(os.environ, CREATE_PR_ENV, clear=True):
|
||||
_bind_test_lock(
|
||||
issue_number=249,
|
||||
branch_name="feat/issue-249-issue-lock-scratch-worktree",
|
||||
worktree_path=scratch,
|
||||
remote="prgs",
|
||||
)
|
||||
with patch.dict(os.environ, self._create_pr_env(), clear=True):
|
||||
res = gitea_create_pr(
|
||||
title="feat: issue-lock scratch worktree Closes #249",
|
||||
head="feat/issue-249-issue-lock-scratch-worktree",
|
||||
|
||||
Reference in New Issue
Block a user