+66
-18
@@ -302,6 +302,39 @@ def assess_author_issue_bootstrap(
|
||||
}
|
||||
|
||||
|
||||
import fcntl
|
||||
|
||||
|
||||
class BootstrapTransitionLock:
|
||||
"""Inter-process file lock scoped to the transition identity / idempotency key."""
|
||||
|
||||
def __init__(self, idempotency_key: str, journal_dir: str | None = None):
|
||||
safe_key = "".join(
|
||||
c if c.isalnum() or c in ("-", "_", ".") else "_"
|
||||
for c in idempotency_key
|
||||
)
|
||||
lock_dir = get_journal_dir(journal_dir)
|
||||
self.lock_path = os.path.join(lock_dir, f"{safe_key}.lock")
|
||||
self.fd = None
|
||||
|
||||
def __enter__(self):
|
||||
self.fd = open(self.lock_path, "a+")
|
||||
fcntl.flock(self.fd, fcntl.LOCK_EX)
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc_val, exc_tb):
|
||||
if self.fd:
|
||||
try:
|
||||
fcntl.flock(self.fd, fcntl.LOCK_UN)
|
||||
except Exception:
|
||||
pass
|
||||
try:
|
||||
self.fd.close()
|
||||
except Exception:
|
||||
pass
|
||||
self.fd = None
|
||||
|
||||
|
||||
def bootstrap_author_issue_worktree(
|
||||
*,
|
||||
issue_number: int,
|
||||
@@ -359,6 +392,8 @@ def bootstrap_author_issue_worktree(
|
||||
lease_id=lease_id,
|
||||
)
|
||||
|
||||
# Acquire cross-process file lock scoped to the idempotency key / transition identity
|
||||
with BootstrapTransitionLock(key):
|
||||
# Idempotency check
|
||||
existing = load_phase_journal(key)
|
||||
if existing and existing.get("completed"):
|
||||
@@ -401,8 +436,16 @@ def bootstrap_author_issue_worktree(
|
||||
),
|
||||
}
|
||||
|
||||
# Initialize Phase Journal
|
||||
journal = existing or {
|
||||
# Initialize or resume Phase Journal
|
||||
if existing:
|
||||
journal = existing
|
||||
artifacts = journal.setdefault("artifacts_created", {})
|
||||
artifacts.setdefault("branch_created", False)
|
||||
artifacts.setdefault("worktree_dir_created", False)
|
||||
artifacts.setdefault("worktree_registered", False)
|
||||
artifacts.setdefault("lock_created", False)
|
||||
else:
|
||||
journal = {
|
||||
"idempotency_key": key,
|
||||
"issue_number": issue_number,
|
||||
"assignment_id": assignment_id,
|
||||
@@ -490,6 +533,7 @@ def bootstrap_author_issue_worktree(
|
||||
}
|
||||
|
||||
# Phase 2: BRANCH_CONFIRMED
|
||||
was_branch_created_previously = journal["artifacts_created"].get("branch_created", False)
|
||||
branch_check = subprocess.run(
|
||||
["git", "-C", root, "rev-parse", "--verify", target_branch],
|
||||
capture_output=True,
|
||||
@@ -528,7 +572,8 @@ def bootstrap_author_issue_worktree(
|
||||
"Inspect or remove the incompatible branch before bootstrapping."
|
||||
),
|
||||
}
|
||||
journal["artifacts_created"]["branch_created"] = False
|
||||
# Preserve creation provenance monotonically across interruption and replay
|
||||
journal["artifacts_created"]["branch_created"] = was_branch_created_previously
|
||||
else:
|
||||
# Create branch
|
||||
create_res = subprocess.run(
|
||||
@@ -558,7 +603,7 @@ def bootstrap_author_issue_worktree(
|
||||
journal["current_phase"] = PHASE_3_PATH_RESERVED
|
||||
save_phase_journal(journal)
|
||||
|
||||
# Phase 3: PATH_RESERVED
|
||||
# Phase 3: PATH_RESERVED & Phase 4: WORKTREE_CONFIRMED
|
||||
if not author_mutation_worktree.is_path_under_branches(
|
||||
target_worktree, root
|
||||
):
|
||||
@@ -577,6 +622,9 @@ def bootstrap_author_issue_worktree(
|
||||
),
|
||||
}
|
||||
|
||||
was_dir_created_previously = journal["artifacts_created"].get("worktree_dir_created", False)
|
||||
was_registered_previously = journal["artifacts_created"].get("worktree_registered", False)
|
||||
|
||||
dir_exists = os.path.exists(target_worktree)
|
||||
if dir_exists:
|
||||
# Check porcelain directly
|
||||
@@ -625,20 +673,11 @@ def bootstrap_author_issue_worktree(
|
||||
"Inspect or remove the pre-existing worktree folder before bootstrapping."
|
||||
),
|
||||
}
|
||||
journal["artifacts_created"]["worktree_dir_created"] = False
|
||||
journal["artifacts_created"]["worktree_dir_created"] = was_dir_created_previously
|
||||
journal["artifacts_created"]["worktree_registered"] = (
|
||||
was_registered_previously or was_dir_created_previously
|
||||
)
|
||||
else:
|
||||
journal["artifacts_created"]["worktree_dir_created"] = True
|
||||
|
||||
journal["phases"][PHASE_3_PATH_RESERVED] = {
|
||||
"status": "completed",
|
||||
"worktree_path": target_worktree,
|
||||
"preexisting_dir": dir_exists,
|
||||
}
|
||||
journal["current_phase"] = PHASE_4_WORKTREE_CONFIRMED
|
||||
save_phase_journal(journal)
|
||||
|
||||
# Phase 4: WORKTREE_CONFIRMED & Phase 5: REGISTRATION_VERIFIED
|
||||
if journal["artifacts_created"]["worktree_dir_created"]:
|
||||
wt_add_res = subprocess.run(
|
||||
[
|
||||
"git",
|
||||
@@ -664,9 +703,18 @@ def bootstrap_author_issue_worktree(
|
||||
"message": f"Failed to execute git worktree add: {wt_add_res.stderr.strip()}",
|
||||
"exact_next_action": "Verify git worktree capabilities and retry.",
|
||||
}
|
||||
journal["artifacts_created"]["worktree_dir_created"] = True
|
||||
journal["artifacts_created"]["worktree_registered"] = True
|
||||
|
||||
# Verify registration in git worktree list
|
||||
journal["phases"][PHASE_3_PATH_RESERVED] = {
|
||||
"status": "completed",
|
||||
"worktree_path": target_worktree,
|
||||
"preexisting_dir": dir_exists,
|
||||
}
|
||||
journal["current_phase"] = PHASE_4_WORKTREE_CONFIRMED
|
||||
save_phase_journal(journal)
|
||||
|
||||
# Phase 5: REGISTRATION_VERIFIED
|
||||
wt_list_res = subprocess.run(
|
||||
["git", "-C", root, "worktree", "list", "--porcelain"],
|
||||
capture_output=True,
|
||||
|
||||
+34
-14
@@ -40,21 +40,41 @@ def _normalize_path(path: str) -> str:
|
||||
return (path or "").replace("\\", "/").rstrip("/")
|
||||
|
||||
|
||||
def is_path_under_branches(path: str, project_root: str | None = None) -> bool:
|
||||
"""True when *path* resolves inside ``<project_root>/branches/``."""
|
||||
normalized = _normalize_path(path)
|
||||
if not normalized:
|
||||
return False
|
||||
if "/branches/" in f"{normalized}/":
|
||||
return True
|
||||
if normalized.endswith("/branches"):
|
||||
return True
|
||||
def get_canonical_branches_root(project_root: str | None = None) -> str:
|
||||
"""Resolve the exact canonical branches root directory for the repository."""
|
||||
if project_root:
|
||||
root = _normalize_path(os.path.realpath(project_root))
|
||||
real = _normalize_path(os.path.realpath(path))
|
||||
if real.startswith(f"{root}/"):
|
||||
rel = real[len(root) + 1 :]
|
||||
return rel == "branches" or rel.startswith("branches/")
|
||||
root = os.path.realpath(project_root)
|
||||
else:
|
||||
root = os.path.realpath(os.getcwd())
|
||||
|
||||
norm = root.replace("\\", "/")
|
||||
if "/branches/" in norm:
|
||||
base_part = norm.split("/branches/")[0]
|
||||
return os.path.realpath(os.path.join(base_part, "branches"))
|
||||
elif norm.endswith("/branches"):
|
||||
return os.path.realpath(norm)
|
||||
|
||||
return os.path.realpath(os.path.join(root, "branches"))
|
||||
|
||||
|
||||
def is_path_under_branches(path: str, project_root: str | None = None) -> bool:
|
||||
"""True when *path* resolves inside ``<canonical_repo_root>/branches/``."""
|
||||
if not path or not str(path).strip():
|
||||
return False
|
||||
try:
|
||||
real_path = os.path.realpath(os.path.abspath(str(path).strip()))
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
branches_root = get_canonical_branches_root(project_root)
|
||||
|
||||
if real_path == branches_root:
|
||||
return True
|
||||
|
||||
prefix = branches_root + os.sep
|
||||
if real_path.startswith(prefix):
|
||||
return True
|
||||
|
||||
return False
|
||||
|
||||
|
||||
|
||||
@@ -13,6 +13,18 @@ import author_issue_bootstrap
|
||||
import task_capability_map
|
||||
|
||||
|
||||
def _concurrent_bootstrap_worker(args: tuple[str, int, str, str, str, str]) -> dict:
|
||||
repo_dir, issue_num, key, lock_dir, journal_dir, master_sha = args
|
||||
os.environ["GITEA_BOOTSTRAP_JOURNAL_DIR"] = journal_dir
|
||||
return author_issue_bootstrap.bootstrap_author_issue_worktree(
|
||||
issue_number=issue_num,
|
||||
canonical_repo_root=repo_dir,
|
||||
expected_base_sha=master_sha,
|
||||
idempotency_key=key,
|
||||
lock_dir=lock_dir,
|
||||
)
|
||||
|
||||
|
||||
class TestAuthorIssueBootstrap(unittest.TestCase):
|
||||
"""Test suite covering AC1-AC10 and comment #14959 specification."""
|
||||
|
||||
@@ -187,6 +199,149 @@ class TestAuthorIssueBootstrap(unittest.TestCase):
|
||||
branch_check = subprocess.run(["git", "-C", self.repo_dir, "rev-parse", "--verify", journal["branch_name"]], capture_output=True, text=True, check=False)
|
||||
self.assertNotEqual(branch_check.returncode, 0)
|
||||
|
||||
def test_cross_process_concurrency(self):
|
||||
"""Review #525 Finding 1: Genuine cross-process concurrency locking prevents corruption."""
|
||||
import concurrent.futures
|
||||
|
||||
key = "test_concurrent_key_850"
|
||||
args = (self.repo_dir, 850, key, self.lock_dir, self.journal_dir, self.master_sha)
|
||||
|
||||
with concurrent.futures.ProcessPoolExecutor(max_workers=2) as executor:
|
||||
fut1 = executor.submit(_concurrent_bootstrap_worker, args)
|
||||
fut2 = executor.submit(_concurrent_bootstrap_worker, args)
|
||||
res1 = fut1.result(timeout=10)
|
||||
res2 = fut2.result(timeout=10)
|
||||
|
||||
self.assertTrue(res1["success"], f"res1 failed: {res1}")
|
||||
self.assertTrue(res2["success"], f"res2 failed: {res2}")
|
||||
# One process performs creation, the other process receives idempotent replay
|
||||
replayed_count = sum(1 for r in (res1, res2) if r.get("replayed"))
|
||||
created_count = sum(1 for r in (res1, res2) if not r.get("replayed"))
|
||||
self.assertEqual(replayed_count, 1)
|
||||
self.assertEqual(created_count, 1)
|
||||
self.assertEqual(res1["worktree_path"], res2["worktree_path"])
|
||||
|
||||
def test_interrupted_replay_preserves_artifacts_created_provenance(self):
|
||||
"""Review #525 Finding 2: Replaying incomplete journal preserves creation provenance monotonically."""
|
||||
key = "test_key_interrupted_replay_1"
|
||||
branch = "fix/issue-850-interrupted-replay"
|
||||
wt_path = os.path.join(self.branches_dir, "fix-issue-850-interrupted-replay")
|
||||
|
||||
# Simulate Phase 2/3 completion where branch and worktree directory were created by this transition
|
||||
journal = {
|
||||
"idempotency_key": key,
|
||||
"issue_number": 850,
|
||||
"branch_name": branch,
|
||||
"worktree_path": wt_path,
|
||||
"active_identity": "jcwalker3",
|
||||
"active_profile": "prgs-author",
|
||||
"remote": "prgs",
|
||||
"org": "Scaled-Tech-Consulting",
|
||||
"repo": "Gitea-Tools",
|
||||
"phases": {
|
||||
author_issue_bootstrap.PHASE_1_REQUEST_ACCEPTED: {"status": "completed"},
|
||||
author_issue_bootstrap.PHASE_2_BRANCH_CONFIRMED: {"status": "completed", "created": True},
|
||||
},
|
||||
"artifacts_created": {
|
||||
"branch_created": True,
|
||||
"worktree_dir_created": True,
|
||||
"worktree_registered": True,
|
||||
"lock_created": False,
|
||||
},
|
||||
"current_phase": author_issue_bootstrap.PHASE_3_PATH_RESERVED,
|
||||
"completed": False,
|
||||
}
|
||||
# Pre-create the branch and worktree on disk to simulate partial state after crash
|
||||
subprocess.run(["git", "-C", self.repo_dir, "branch", branch, self.master_sha], check=True, capture_output=True)
|
||||
subprocess.run(["git", "-C", self.repo_dir, "worktree", "add", wt_path, branch], check=True, capture_output=True)
|
||||
author_issue_bootstrap.save_phase_journal(journal)
|
||||
|
||||
# Now resume/replay the transition but simulate lock binding failure during Phase 6
|
||||
with unittest.mock.patch("issue_lock_store.bind_session_lock", side_effect=RuntimeError("Lock failure test")):
|
||||
res = author_issue_bootstrap.bootstrap_author_issue_worktree(
|
||||
issue_number=850,
|
||||
canonical_repo_root=self.repo_dir,
|
||||
branch_name=branch,
|
||||
worktree_path=wt_path,
|
||||
idempotency_key=key,
|
||||
lock_dir=self.lock_dir,
|
||||
)
|
||||
|
||||
self.assertFalse(res["success"])
|
||||
self.assertEqual(res.get("reason_code"), "issue_lock_acquisition_failed")
|
||||
|
||||
# Verify that compensating recovery correctly deleted transition-created branch & worktree
|
||||
# because creation provenance was preserved across replay (NOT downgraded to False!)
|
||||
self.assertFalse(os.path.exists(wt_path))
|
||||
branch_check = subprocess.run(["git", "-C", self.repo_dir, "rev-parse", "--verify", branch], capture_output=True, text=True, check=False)
|
||||
self.assertNotEqual(branch_check.returncode, 0)
|
||||
|
||||
def test_transition_created_only_compensation(self):
|
||||
"""Review #525 Finding 4: Preexisting branch is NOT deleted by compensation when only worktree was transition-created."""
|
||||
key = "test_key_preexisting_branch_compensation"
|
||||
preexisting_branch = "fix/issue-850-preexisting"
|
||||
wt_path = os.path.join(self.branches_dir, "fix-issue-850-preexisting")
|
||||
|
||||
# Create branch BEFORE bootstrap (preexisting branch)
|
||||
subprocess.run(["git", "-C", self.repo_dir, "branch", preexisting_branch, self.master_sha], check=True, capture_output=True)
|
||||
|
||||
# Call bootstrap with simulated failure during Phase 6 (lock binding)
|
||||
with unittest.mock.patch("issue_lock_store.bind_session_lock", side_effect=RuntimeError("Simulated lock failure")):
|
||||
res = author_issue_bootstrap.bootstrap_author_issue_worktree(
|
||||
issue_number=850,
|
||||
canonical_repo_root=self.repo_dir,
|
||||
branch_name=preexisting_branch,
|
||||
worktree_path=wt_path,
|
||||
idempotency_key=key,
|
||||
lock_dir=self.lock_dir,
|
||||
)
|
||||
|
||||
self.assertFalse(res["success"])
|
||||
# Worktree dir was created by transition -> removed by compensation
|
||||
self.assertFalse(os.path.exists(wt_path))
|
||||
|
||||
# Preexisting branch was NOT created by transition -> MUST BE PRESERVED!
|
||||
branch_check = subprocess.run(["git", "-C", self.repo_dir, "rev-parse", "--verify", preexisting_branch], capture_output=True, text=True, check=False)
|
||||
self.assertEqual(branch_check.returncode, 0, "Preexisting branch was deleted by mistake!")
|
||||
|
||||
def test_incompatible_idempotency_replay_refusal(self):
|
||||
"""Review #525 Finding 4: Replaying key with incompatible parameters returns refusal."""
|
||||
key = "test_key_incompatible_replay"
|
||||
res1 = author_issue_bootstrap.bootstrap_author_issue_worktree(
|
||||
issue_number=850,
|
||||
canonical_repo_root=self.repo_dir,
|
||||
branch_name="fix/issue-850-param-a",
|
||||
idempotency_key=key,
|
||||
lock_dir=self.lock_dir,
|
||||
)
|
||||
self.assertTrue(res1["success"])
|
||||
|
||||
# Second call with different branch_name
|
||||
res2 = author_issue_bootstrap.bootstrap_author_issue_worktree(
|
||||
issue_number=850,
|
||||
canonical_repo_root=self.repo_dir,
|
||||
branch_name="fix/issue-850-param-b",
|
||||
idempotency_key=key,
|
||||
lock_dir=self.lock_dir,
|
||||
)
|
||||
self.assertFalse(res2["success"])
|
||||
self.assertEqual(res2.get("reason_code"), "incompatible_idempotency_replay")
|
||||
|
||||
def test_exact_next_action_satisfiable_via_mcp(self):
|
||||
"""Review #525 Finding 4: exact_next_action provides satisfiable MCP actions, not shell commands."""
|
||||
key = "test_key_next_action_mcp"
|
||||
stale_sha = "0000000000000000000000000000000000000000"
|
||||
res = author_issue_bootstrap.bootstrap_author_issue_worktree(
|
||||
issue_number=850,
|
||||
canonical_repo_root=self.repo_dir,
|
||||
expected_base_sha=stale_sha,
|
||||
lock_dir=self.lock_dir,
|
||||
)
|
||||
next_action = res.get("exact_next_action", "")
|
||||
self.assertNotIn("scripts/worktree-start", next_action)
|
||||
self.assertNotIn("git worktree add", next_action)
|
||||
self.assertNotIn("bash", next_action.lower())
|
||||
|
||||
def test_task_capability_map_integration(self):
|
||||
"""Verify task_capability_map has bootstrap_author_issue_worktree configured correctly."""
|
||||
self.assertEqual(task_capability_map.required_role("bootstrap_author_issue_worktree"), "author")
|
||||
|
||||
@@ -28,6 +28,21 @@ class TestPathUnderBranches(unittest.TestCase):
|
||||
amw.is_path_under_branches("/repo/other-checkout", self.ROOT)
|
||||
)
|
||||
|
||||
def test_unrelated_branches_dir_fails(self):
|
||||
self.assertFalse(
|
||||
amw.is_path_under_branches("/tmp/branches/evil", self.ROOT)
|
||||
)
|
||||
|
||||
def test_prefix_confusion_fails(self):
|
||||
self.assertFalse(
|
||||
amw.is_path_under_branches(f"{self.ROOT}/branches-other/foo", self.ROOT)
|
||||
)
|
||||
|
||||
def test_traversal_fails(self):
|
||||
self.assertFalse(
|
||||
amw.is_path_under_branches(f"{self.ROOT}/branches/../evil", self.ROOT)
|
||||
)
|
||||
|
||||
|
||||
class TestAssessAuthorMutationWorktree(unittest.TestCase):
|
||||
ROOT = "/repo/Gitea-Tools"
|
||||
|
||||
Reference in New Issue
Block a user