diff --git a/author_issue_bootstrap.py b/author_issue_bootstrap.py index c246698..c7784fd 100644 --- a/author_issue_bootstrap.py +++ b/author_issue_bootstrap.py @@ -24,6 +24,7 @@ import subprocess from typing import Any, Mapping import author_mutation_worktree +import control_plane_db import issue_lock_store import issue_lock_worktree import lease_lifecycle @@ -123,10 +124,114 @@ def derive_default_idempotency_key( return ":".join(parts) +def _verify_assignment_and_lease_ids( + *, + assignment_id: str | None, + lease_id: str | None, + issue_number: int, + owner_session: str, + remote: str, + org: str | None, + repo: str | None, + db: control_plane_db.ControlPlaneDB | None = None, +) -> dict[str, Any] | None: + """Fail closed when caller-supplied assignment/lease IDs are unverified (#531 F4). + + Both IDs are optional together. When either is supplied, both must be + present and must resolve to the same live control-plane lease/assignment + bound to this issue and owner session. Fabricated identifiers never pass. + """ + asn = (assignment_id or "").strip() + lid = (lease_id or "").strip() + if not asn and not lid: + return None + if not asn or not lid: + return { + "success": False, + "reason_code": "incomplete_assignment_lease_ids", + "message": ( + "assignment_id and lease_id must be supplied together when " + "either is provided (fail closed)." + ), + "exact_next_action": ( + "Pass both identifiers from allocate_next_work / control-plane " + "assignment proof, or omit both." + ), + } + + try: + store = db if db is not None else control_plane_db.ControlPlaneDB() + state = store.get_lease_workflow_state(lid) + except Exception as exc: + return { + "success": False, + "reason_code": "assignment_lease_lookup_failed", + "message": f"Could not verify assignment/lease against control plane: {exc}", + "exact_next_action": ( + "Ensure the control-plane DB is available and retry with live IDs." + ), + } + if not state or not state.get("lease"): + return { + "success": False, + "reason_code": "unknown_lease_id", + "message": f"lease_id '{lid}' is not present in the control plane (fail closed).", + "exact_next_action": "Pass a live lease_id from control-plane assignment.", + } + lease = state["lease"] + assignment = state.get("assignment") or {} + work = state.get("work_item") or {} + recorded_asn = str(assignment.get("assignment_id") or "").strip() + if recorded_asn and recorded_asn != asn: + return { + "success": False, + "reason_code": "assignment_lease_mismatch", + "message": ( + f"assignment_id '{asn}' does not match lease '{lid}' " + f"(recorded assignment '{recorded_asn}') (fail closed)." + ), + "exact_next_action": "Re-read allocate_next_work proof and pass matching IDs.", + } + if not recorded_asn: + # Some lease rows may not yet have an assignment join; still require + # the lease itself to exist and bind to the claimed session/issue. + pass + lease_session = str(lease.get("session_id") or "").strip() + if lease_session and lease_session != owner_session: + return { + "success": False, + "reason_code": "lease_session_mismatch", + "message": ( + f"lease_id '{lid}' is owned by session '{lease_session}', not " + f"'{owner_session}' (fail closed)." + ), + "exact_next_action": "Use the session that holds the lease, or re-allocate.", + } + # Bind issue number when the work item records one. + work_number = work.get("number") or work.get("issue_number") or assignment.get("issue_number") + try: + if work_number is not None and int(work_number) != int(issue_number): + return { + "success": False, + "reason_code": "lease_issue_mismatch", + "message": ( + f"lease_id '{lid}' is bound to issue #{work_number}, not " + f"#{issue_number} (fail closed)." + ), + "exact_next_action": "Pass the lease issued for this issue number.", + } + except (TypeError, ValueError): + pass + _ = (remote, org, repo) # reserved for host-scoped DBs + return None + + def run_compensating_recovery( journal: dict[str, Any], canonical_repo_root: str, journal_dir: str | None = None, + *, + db: control_plane_db.ControlPlaneDB | None = None, ) -> dict[str, Any]: """Execute compensating recovery for artifacts created by this transition only.""" artifacts = journal.get("artifacts_created") or {} @@ -135,10 +240,22 @@ def run_compensating_recovery( worktree_path = journal.get("worktree_path") branch_name = journal.get("branch_name") + # Roll back workflow lease/assignment when this transition bound one (#531 F5). + lease_id = str(journal.get("lease_id") or "").strip() + session_id = str(journal.get("owner_session") or "").strip() + if lease_id and session_id: + try: + store = db if db is not None else control_plane_db.ControlPlaneDB() + lease_lifecycle.release_lease( + store, lease_id=lease_id, session_id=session_id + ) + rolled_back.append(f"lease:{lease_id}") + except Exception as exc: + rolled_back.append(f"lease_release_failed:{lease_id}:{type(exc).__name__}") + # Roll back issue lock if created if artifacts.get("lock_created") or pending.get("lock"): issue_num = journal.get("issue_number") - session_id = journal.get("owner_session") if issue_num and session_id: try: issue_lock_store.release_session_lock( @@ -510,6 +627,19 @@ def bootstrap_author_issue_worktree( lease_id=lease_id, ) + # Review #531 Finding 4: never embed unverified caller-supplied IDs. + id_block = _verify_assignment_and_lease_ids( + assignment_id=assignment_id, + lease_id=lease_id, + issue_number=issue_number, + owner_session=session, + remote=remote, + org=org, + repo=repo, + ) + if id_block is not None: + return id_block + # Acquire cross-process file lock scoped to the idempotency key / transition identity with BootstrapTransitionLock(key, journal_dir=lock_dir): # Idempotency check @@ -677,29 +807,29 @@ def bootstrap_author_issue_worktree( text=True, check=False, ) + # F-8 / review #531 Finding 3: require the existing branch head to + # *contain* live master (is-ancestor) or equal it. Sharing any + # historical merge-base is not enough — that would accept stale or + # diverged branches that merely share history with master. if anc_check.returncode != 0 and branch_head.lower() != live_master_sha.lower(): - # F-8: Check if branch shares a common ancestor with live master - mb_check = subprocess.run( - ["git", "-C", root, "merge-base", live_master_sha, branch_head], - capture_output=True, - text=True, - check=False, + journal["failure_reason"] = ( + f"existing branch '{target_branch}' HEAD ({branch_head[:12]}) " + f"does not contain live master ({live_master_sha[:12]})" ) - if mb_check.returncode != 0 or not mb_check.stdout.strip(): - journal["failure_reason"] = ( - f"existing branch '{target_branch}' HEAD ({branch_head[:12]}) is incompatible with live master ({live_master_sha[:12]})" - ) - save_phase_journal(journal, journal_dir=lock_dir) - return { - "success": False, - "reason_code": "incompatible_existing_branch", - "message": ( - f"Existing branch '{target_branch}' HEAD ({branch_head[:12]}) is incompatible with live master ({live_master_sha[:12]})." - ), - "exact_next_action": ( - "Inspect or sync the existing branch with master before bootstrapping." - ), - } + save_phase_journal(journal, journal_dir=lock_dir) + return { + "success": False, + "reason_code": "incompatible_existing_branch", + "message": ( + f"Existing branch '{target_branch}' HEAD ({branch_head[:12]}) " + f"does not contain live master ({live_master_sha[:12]}). " + "Stale or diverged branches are refused (fail closed)." + ), + "exact_next_action": ( + "Update the branch by merging current master (no rebase/" + "force-push), or choose a branch that already contains master." + ), + } # Preserve creation provenance monotonically across interruption and replay if was_branch_created_previously or pending_branch == target_branch: journal["artifacts_created"]["branch_created"] = True diff --git a/author_mutation_worktree.py b/author_mutation_worktree.py index bf69a8a..ee396e8 100644 --- a/author_mutation_worktree.py +++ b/author_mutation_worktree.py @@ -92,12 +92,31 @@ def resolve_canonical_repo_root(workspace_path: str, fallback_project_root: str) except Exception: pass + # Fallback when git metadata is unavailable. Never string-split on + # "/branches/" (review #531 Finding 2 / F-6): recover the repo root only + # via resolved-path commonpath ancestry against a parent that owns a + # real ``branches`` directory containing the fallback path. fallback = os.path.realpath(fallback_project_root or workspace_path or ".") - norm = fallback.replace("\\", "/") - if "/branches/" in norm: - return os.path.realpath(norm.split("/branches/")[0]) - elif norm.endswith("/branches"): - return os.path.realpath(os.path.dirname(fallback)) + cur = fallback + for _ in range(64): + parent = os.path.dirname(cur) + if parent == cur: + break + branches_dir = os.path.realpath(os.path.join(parent, "branches")) + if os.path.isdir(branches_dir): + try: + if os.path.commonpath([branches_dir, fallback]) == branches_dir: + return parent + except ValueError: + pass + # Also accept fallback itself being the branches directory. + if os.path.basename(cur) == "branches" and os.path.isdir(cur): + try: + if os.path.commonpath([cur, fallback]) == cur: + return parent + except ValueError: + pass + cur = parent return fallback diff --git a/tests/test_author_issue_bootstrap.py b/tests/test_author_issue_bootstrap.py index e6c147a..f1de515 100644 --- a/tests/test_author_issue_bootstrap.py +++ b/tests/test_author_issue_bootstrap.py @@ -69,8 +69,7 @@ class TestAuthorIssueBootstrap(unittest.TestCase): res = author_issue_bootstrap.bootstrap_author_issue_worktree( issue_number=850, canonical_repo_root=self.repo_dir, - assignment_id="asn-12345", - lease_id="lease-67890", + # assignment_id/lease_id omitted: optional unless verified live. expected_base_sha=self.master_sha, idempotency_key=key, remote="prgs", @@ -477,6 +476,125 @@ class TestAuthorIssueBootstrap(unittest.TestCase): self.assertTrue(task_capability_map.preflight_task_matches("work_issue", "bootstrap_author_issue_worktree")) self.assertTrue(task_capability_map.preflight_task_matches("bootstrap_author_issue_worktree", "lock_issue")) + def test_unverified_assignment_lease_ids_fail_closed(self): + """Review #531 Finding 4: fabricated assignment/lease IDs are refused.""" + res = author_issue_bootstrap.bootstrap_author_issue_worktree( + issue_number=850, + canonical_repo_root=self.repo_dir, + assignment_id="asn-fabricated", + lease_id="lease-fabricated", + expected_base_sha=self.master_sha, + lock_dir=self.lock_dir, + owner_session="session-test-1234", + ) + self.assertFalse(res["success"]) + self.assertIn( + res.get("reason_code"), + { + "unknown_lease_id", + "assignment_lease_lookup_failed", + "incomplete_assignment_lease_ids", + }, + ) + + def test_partial_assignment_lease_ids_fail_closed(self): + """Review #531 Finding 4: one of assignment_id/lease_id alone is incomplete.""" + res = author_issue_bootstrap.bootstrap_author_issue_worktree( + issue_number=850, + canonical_repo_root=self.repo_dir, + assignment_id="asn-only", + lock_dir=self.lock_dir, + owner_session="session-test-1234", + ) + self.assertFalse(res["success"]) + self.assertEqual(res.get("reason_code"), "incomplete_assignment_lease_ids") + + def test_stale_diverged_branch_is_not_accepted_via_merge_base(self): + """Review #531 Finding 3: any common ancestor is not enough; require master ⊆ branch.""" + branch = "fix/issue-850-stale-divergent" + # Create branch from current master, then advance master so branch lacks tip. + subprocess.run( + ["git", "-C", self.repo_dir, "branch", branch, self.master_sha], + check=True, + capture_output=True, + ) + # Make a new commit on master (orphan path so branch does not contain it). + marker = os.path.join(self.repo_dir, "master-advance.txt") + with open(marker, "w") as f: + f.write("advance master\n") + subprocess.run(["git", "-C", self.repo_dir, "add", "master-advance.txt"], check=True, capture_output=True) + subprocess.run( + ["git", "-C", self.repo_dir, "commit", "-m", "advance master past branch"], + check=True, + capture_output=True, + ) + new_master = subprocess.run( + ["git", "-C", self.repo_dir, "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + res = author_issue_bootstrap.bootstrap_author_issue_worktree( + issue_number=850, + canonical_repo_root=self.repo_dir, + branch_name=branch, + expected_base_sha=new_master, + lock_dir=self.lock_dir, + owner_session="session-test-1234", + ) + self.assertFalse(res["success"]) + self.assertEqual(res.get("reason_code"), "incompatible_existing_branch") + + def test_compensating_recovery_attempts_lease_release(self): + """Review #531 Finding 5: recovery invokes lease release when lease_id is present.""" + from unittest import mock + + journal = { + "idempotency_key": "test_lease_rec", + "issue_number": 850, + "owner_session": "session-test-1234", + "lease_id": "lease-abc", + "branch_name": "fix/issue-850-lease-rec", + "artifacts_created": {"lock_created": True}, + "failure_reason": "simulated", + "completed": False, + } + with mock.patch.object( + author_issue_bootstrap.lease_lifecycle, + "release_lease", + return_value={"success": True}, + ) as rel, mock.patch.object( + author_issue_bootstrap.control_plane_db, + "ControlPlaneDB", + return_value=mock.Mock(), + ): + rec = author_issue_bootstrap.run_compensating_recovery( + journal, self.repo_dir, journal_dir=self.lock_dir + ) + self.assertTrue(rec["executed"]) + rel.assert_called_once() + self.assertIn("lease:lease-abc", rec["rolled_back"]) + + +class TestCanonicalRootNoStringSplit(unittest.TestCase): + def test_fallback_uses_commonpath_not_substring_split(self): + """Review #531 Finding 2: no norm.split('/branches/') fallback.""" + import inspect + import author_mutation_worktree as amw + + src = inspect.getsource(amw.resolve_canonical_repo_root) + self.assertNotIn('split("/branches/")', src) + self.assertNotIn("split('/branches/')", src) + + # Fallback recovers repo root from a nested branches worktree path. + with tempfile.TemporaryDirectory() as tmp: + repo = os.path.join(tmp, "repo") + wt = os.path.join(repo, "branches", "fix-issue-850-x") + os.makedirs(wt) + # git unavailable path: pass missing workspace so fallback is used. + root = amw.resolve_canonical_repo_root("/missing/path", wt) + self.assertEqual(root, os.path.realpath(repo)) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_workspace_guard_alignment.py b/tests/test_workspace_guard_alignment.py index 3d41291..b65b7da 100644 --- a/tests/test_workspace_guard_alignment.py +++ b/tests/test_workspace_guard_alignment.py @@ -35,8 +35,10 @@ class TestCanonicalRepoRoot(unittest.TestCase): self.assertEqual(root, CONTROL_ROOT) def test_falls_back_when_git_unavailable(self): + # When fallback is a path under /branches/, recover + # via commonpath ancestry (never string-split on "/branches/"). root = amw.resolve_canonical_repo_root("/missing/path", MCP_PROCESS_ROOT) - self.assertEqual(root, os.path.realpath(MCP_PROCESS_ROOT)) + self.assertEqual(root, os.path.realpath(CONTROL_ROOT)) class TestWorkspaceRepoMembership(unittest.TestCase):