From e4adccd82a86c9f417c20fe8339ca4ef2d5edfe0 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 16:33:31 -0400 Subject: [PATCH] fix: inject duplicate-work context fetcher for testable lock/create_pr paths (#400) Expose issue_duplicate_context_fetcher on the MCP server so lock_issue, commit_files, and create_pr duplicate rechecks avoid live Gitea calls in unit tests. Update affected test suites to patch the fetcher without weakening duplicate-work gate assertions. Co-Authored-By: Claude Opus 4.8 (1M context) --- gitea_mcp_server.py | 20 +++++++- tests/test_agent_temp_artifacts.py | 5 +- tests/test_commit_files_capability.py | 7 +++ tests/test_commit_payloads.py | 7 +++ tests/test_issue_work_duplicate_gate.py | 31 +++++++++++++ tests/test_mcp_server.py | 61 +++++++++++++++---------- 6 files changed, 105 insertions(+), 26 deletions(-) diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index c4e4fdc..f1b1849 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -548,7 +548,7 @@ def _load_existing_issue_lock() -> dict | None: if not os.path.exists(ISSUE_LOCK_FILE): return None try: - with open(ISSUE_LOCK_FILE, encoding="utf-8") as f: + with open(ISSUE_LOCK_FILE, "r", encoding="utf-8") as f: data = json.load(f) return data if isinstance(data, dict) else None except Exception: @@ -642,7 +642,7 @@ def _branch_entry_name(branch: dict | str) -> str: return str(branch.get("name") or branch.get("ref") or "") -def _collect_issue_duplicate_context( +def _live_fetch_issue_duplicate_context( h: str, o: str, r: str, @@ -667,6 +667,22 @@ def _collect_issue_duplicate_context( return open_prs, branch_names, claim_entry +# Injectable duplicate-work context fetcher (#400). Production uses the live +# Gitea API path above; unit tests patch this symbol instead of hitting the +# network. +issue_duplicate_context_fetcher = _live_fetch_issue_duplicate_context + + +def _collect_issue_duplicate_context( + h: str, + o: str, + r: str, + auth: str, + issue_number: int, +) -> tuple[list[dict], list[str], dict]: + return issue_duplicate_context_fetcher(h, o, r, auth, issue_number) + + def _assess_issue_duplicate_gate( issue_number: int, *, diff --git a/tests/test_agent_temp_artifacts.py b/tests/test_agent_temp_artifacts.py index 903eab8..1532143 100644 --- a/tests/test_agent_temp_artifacts.py +++ b/tests/test_agent_temp_artifacts.py @@ -80,7 +80,10 @@ class TestIssueLockArtifactWarning(unittest.TestCase): def tearDown(self): self._env_patcher.stop() - @patch("mcp_server.api_get_all", return_value=[]) + @patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) @patch("mcp_server._auth", return_value="token x") @patch("mcp_server._resolve", return_value=("h", "o", "r")) @patch("mcp_server.ISSUE_LOCK_FILE", new_callable=lambda: tempfile.mktemp()) diff --git a/tests/test_commit_files_capability.py b/tests/test_commit_files_capability.py index d1203bd..c1474ab 100644 --- a/tests/test_commit_files_capability.py +++ b/tests/test_commit_files_capability.py @@ -76,7 +76,14 @@ class CommitFilesCapabilityBase(unittest.TestCase): with open(self.config_path, "w", encoding="utf-8") as fh: fh.write(json.dumps(CONFIG)) + self._dup_fetcher_patcher = patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) + self._dup_fetcher_patcher.start() + def tearDown(self): + self._dup_fetcher_patcher.stop() self._remotes.stop() mcp_server._IDENTITY_CACHE.clear() mcp_server._preflight_whoami_called, mcp_server._preflight_capability_called = ( diff --git a/tests/test_commit_payloads.py b/tests/test_commit_payloads.py index e8e4123..1e684e7 100644 --- a/tests/test_commit_payloads.py +++ b/tests/test_commit_payloads.py @@ -84,7 +84,14 @@ class TestCommitPayloads(unittest.TestCase): mcp_server._preflight_whoami_called = True mcp_server._preflight_capability_called = True + self._dup_fetcher_patcher = patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) + self._dup_fetcher_patcher.start() + def tearDown(self): + self._dup_fetcher_patcher.stop() self._remotes.stop() mcp_server._IDENTITY_CACHE.clear() diff --git a/tests/test_issue_work_duplicate_gate.py b/tests/test_issue_work_duplicate_gate.py index a041ef3..6e9ea5a 100644 --- a/tests/test_issue_work_duplicate_gate.py +++ b/tests/test_issue_work_duplicate_gate.py @@ -121,6 +121,37 @@ class TestDuplicateReportOutcome(unittest.TestCase): self.assertEqual(good["outcome"], OUTCOME_DUPLICATE_WORK_NOT_PREVENTED) +class TestInjectableDuplicateFetcher(unittest.TestCase): + @patch("mcp_server.get_auth_header", return_value="token x") + def test_lock_issue_uses_injected_fetcher(self, _auth): + seen = {} + + def fetcher(h, o, r, auth, issue_number): + seen["issue_number"] = issue_number + return [], [], {"status": "not_claimed"} + + with patch( + "mcp_server.issue_duplicate_context_fetcher", + side_effect=fetcher, + ), patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value={ + "current_branch": "master", + "porcelain_status": "", + "base_equivalent": True, + }, + ), patch.dict(os.environ, { + "GITEA_ALLOWED_OPERATIONS": "gitea.issue.comment", + }, clear=True): + with patch.object(mcp_server, "ISSUE_LOCK_FILE", tempfile.mktemp()): + mcp_server.gitea_lock_issue( + issue_number=400, + branch_name="feat/issue-400-duplicate-work-preflight", + remote="prgs", + ) + self.assertEqual(seen["issue_number"], 400) + + class TestMcpDuplicateRecheck(unittest.TestCase): def setUp(self): self._dir = tempfile.TemporaryDirectory() diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 4002b5e..588e6e8 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -112,6 +112,11 @@ def _sample_issue_lock(issue_number=123, branch_name="feat/x", **overrides): return record +def _clear_duplicate_context_fetcher(*_args, **_kwargs): + """Default injectable duplicate-work context for lock/create_pr tests.""" + return [], [], {"status": "not_claimed"} + + # --------------------------------------------------------------------------- # Create Issue # --------------------------------------------------------------------------- @@ -166,13 +171,17 @@ class TestCreateIssue(unittest.TestCase): # --------------------------------------------------------------------------- class TestCreatePR(unittest.TestCase): + @patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", 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): + 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 mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} @@ -187,13 +196,17 @@ class TestCreatePR(unittest.TestCase): self.assertEqual(payload["base"], "main") self.assertIn("Closes #123", payload["title"]) + @patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", 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): + 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 mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} @@ -3044,8 +3057,14 @@ class TestIssueLocking(unittest.TestCase): def setUp(self): self._env_patcher = patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True) self._env_patcher.start() + self._dup_fetcher_patcher = patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) + self.mock_dup_fetcher = self._dup_fetcher_patcher.start() def tearDown(self): + self._dup_fetcher_patcher.stop() self._env_patcher.stop() if os.path.exists(ISSUE_LOCK_FILE): os.remove(ISSUE_LOCK_FILE) @@ -3054,10 +3073,8 @@ class TestIssueLocking(unittest.TestCase): "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_success(self, _auth, mock_api, _git_state): - mock_api.return_value = [] # no open PRs + def test_lock_issue_success(self, _auth, _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") @@ -3082,50 +3099,48 @@ class TestIssueLocking(unittest.TestCase): "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_reused_by_open_pr_branch(self, _auth, mock_api, _git_state): - mock_api.return_value = [{ + def test_lock_issue_reused_by_open_pr_branch(self, _auth, _git_state): + self.mock_dup_fetcher.return_value = ([{ "number": 200, "head": {"ref": "feat/issue-196-boundary"}, "title": "Some PR", - "body": "No closes ref" - }] + "body": "No closes ref", + }], [], {"status": "not_claimed"}) with self.assertRaises(ValueError) as ctx: gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") - self.assertIn("already tied to an open PR", str(ctx.exception)) + self.assertIn("open PR #200 already covers issue", str(ctx.exception)) @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_reused_by_open_pr_closes_ref(self, _auth, mock_api, _git_state): - mock_api.return_value = [{ + def test_lock_issue_reused_by_open_pr_closes_ref(self, _auth, _git_state): + self.mock_dup_fetcher.return_value = ([{ "number": 200, "head": {"ref": "feat/other-branch"}, "title": "Some PR", - "body": "fixes #196" - }] + "body": "fixes #196", + }], [], {"status": "not_claimed"}) with self.assertRaises(ValueError) as ctx: gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") - self.assertIn("already tied to an open PR", str(ctx.exception)) + self.assertIn("open PR #200 already covers issue", str(ctx.exception)) @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_reused_by_remote_branch(self, _auth, mock_api, _git_state): - mock_api.side_effect = [ + def test_lock_issue_reused_by_remote_branch(self, _auth, _git_state): + self.mock_dup_fetcher.return_value = ( [], - [{"name": "feat/issue-196-existing-work"}], - ] + ["feat/issue-196-existing-work"], + {"status": "not_claimed"}, + ) with self.assertRaises(ValueError) as ctx: gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") - self.assertIn("already has matching branch", str(ctx.exception)) + 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: