fix: use exact issue-number boundary in own-branch adoption (#442)
Replace substring issue-marker matching with a numeric word-boundary regex so issue-42 adoption is not false-blocked by unrelated issue-420 branches. Add regression tests for the #42 vs #420 collision. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
+16
-2
@@ -14,6 +14,8 @@ this module additionally records whether they passed for proof purposes.
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import re
|
||||||
|
|
||||||
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"
|
||||||
@@ -33,6 +35,19 @@ def _branch_sha(entry) -> str | None:
|
|||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
def _branch_carries_issue_marker(branch_name: str, issue_number: int) -> bool:
|
||||||
|
"""Return True when *branch_name* references issue *issue_number* exactly.
|
||||||
|
|
||||||
|
Uses a numeric word-boundary so ``issue-42`` does not match inside
|
||||||
|
``issue-420`` (AC6 / #440).
|
||||||
|
"""
|
||||||
|
name = (branch_name or "").strip()
|
||||||
|
if not name:
|
||||||
|
return False
|
||||||
|
pattern = rf"(?:^|/)issue-{int(issue_number)}(?![0-9])"
|
||||||
|
return re.search(pattern, name) is not None
|
||||||
|
|
||||||
|
|
||||||
def assess_own_branch_adoption(
|
def assess_own_branch_adoption(
|
||||||
*,
|
*,
|
||||||
issue_number: int,
|
issue_number: int,
|
||||||
@@ -59,13 +74,12 @@ def assess_own_branch_adoption(
|
|||||||
BLOCK_COMPETING: at least one same-issue branch is not the requested branch.
|
BLOCK_COMPETING: at least one same-issue branch is not the requested branch.
|
||||||
NO_MATCH: no branch carries the issue marker — normal lock path applies.
|
NO_MATCH: no branch carries the issue marker — normal lock path applies.
|
||||||
"""
|
"""
|
||||||
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 _branch_carries_issue_marker(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})
|
||||||
|
|||||||
@@ -84,6 +84,7 @@ class TestIssueLockArtifactWarning(unittest.TestCase):
|
|||||||
"mcp_server.issue_duplicate_context_fetcher",
|
"mcp_server.issue_duplicate_context_fetcher",
|
||||||
return_value=([], [], {"status": "not_claimed"}),
|
return_value=([], [], {"status": "not_claimed"}),
|
||||||
)
|
)
|
||||||
|
@patch("mcp_server.api_get_all", return_value=[])
|
||||||
@patch("mcp_server._auth", return_value="token x")
|
@patch("mcp_server._auth", return_value="token x")
|
||||||
@patch("mcp_server._resolve", return_value=("h", "o", "r"))
|
@patch("mcp_server._resolve", return_value=("h", "o", "r"))
|
||||||
@patch("mcp_server.ISSUE_LOCK_FILE", new_callable=lambda: tempfile.mktemp())
|
@patch("mcp_server.ISSUE_LOCK_FILE", new_callable=lambda: tempfile.mktemp())
|
||||||
|
|||||||
@@ -75,17 +75,31 @@ class TestAssessOwnBranchAdoption(unittest.TestCase):
|
|||||||
)
|
)
|
||||||
self.assertEqual(result["outcome"], NO_MATCH)
|
self.assertEqual(result["outcome"], NO_MATCH)
|
||||||
|
|
||||||
def test_substring_issue_number_not_falsely_matched_as_exact(self):
|
def test_higher_issue_number_branch_does_not_block_lower_issue_adoption(self):
|
||||||
# A different issue's branch must not be adopted as ours.
|
# issue-420 must not be treated as competing work for issue #42.
|
||||||
|
own_branch = "feat/issue-42-widget"
|
||||||
|
result = assess_own_branch_adoption(
|
||||||
|
issue_number=42,
|
||||||
|
requested_branch=own_branch,
|
||||||
|
existing_branches=[
|
||||||
|
{"name": own_branch, "commit_sha": "abc1234"},
|
||||||
|
{"name": "feat/issue-420-server-code-parity"},
|
||||||
|
],
|
||||||
|
)
|
||||||
|
self.assertEqual(result["outcome"], ADOPT)
|
||||||
|
self.assertTrue(result["adopt"])
|
||||||
|
self.assertFalse(result["block"])
|
||||||
|
self.assertEqual(result["matched_branch"], own_branch)
|
||||||
|
|
||||||
|
def test_unrelated_higher_number_branch_is_ignored_without_own_branch(self):
|
||||||
result = assess_own_branch_adoption(
|
result = assess_own_branch_adoption(
|
||||||
issue_number=42,
|
issue_number=42,
|
||||||
requested_branch="feat/issue-42-thing",
|
requested_branch="feat/issue-42-thing",
|
||||||
existing_branches=[{"name": "feat/issue-420-server-code-parity"}],
|
existing_branches=[{"name": "feat/issue-420-server-code-parity"}],
|
||||||
)
|
)
|
||||||
# 'issue-42' is a substring of 'issue-420', so marker matches but the
|
self.assertEqual(result["outcome"], NO_MATCH)
|
||||||
# name differs -> competing block, never adoption of the wrong branch.
|
self.assertFalse(result["block"])
|
||||||
self.assertEqual(result["outcome"], BLOCK_COMPETING)
|
self.assertFalse(result["adopt"])
|
||||||
self.assertNotEqual(result["matched_branch"], "feat/issue-420-server-code-parity")
|
|
||||||
|
|
||||||
|
|
||||||
class TestBuildAdoptionProof(unittest.TestCase):
|
class TestBuildAdoptionProof(unittest.TestCase):
|
||||||
|
|||||||
@@ -123,8 +123,9 @@ class TestDuplicateReportOutcome(unittest.TestCase):
|
|||||||
|
|
||||||
|
|
||||||
class TestInjectableDuplicateFetcher(unittest.TestCase):
|
class TestInjectableDuplicateFetcher(unittest.TestCase):
|
||||||
|
@patch("mcp_server.api_get_all", return_value=[])
|
||||||
@patch("mcp_server.get_auth_header", return_value="token x")
|
@patch("mcp_server.get_auth_header", return_value="token x")
|
||||||
def test_lock_issue_uses_injected_fetcher(self, _auth):
|
def test_lock_issue_uses_injected_fetcher(self, _auth, _api):
|
||||||
seen = {}
|
seen = {}
|
||||||
|
|
||||||
def fetcher(h, o, r, auth, issue_number):
|
def fetcher(h, o, r, auth, issue_number):
|
||||||
|
|||||||
Reference in New Issue
Block a user