fix(bootstrap): address PR #853 REQUEST_CHANGES findings (#850)

- Remove /branches/ string-split fallback in resolve_canonical_repo_root;
  recover roots via commonpath ancestry only (review #531 F2).
- Refuse existing branches that do not contain live master; no weak
  merge-base acceptance (F3).
- Verify caller-supplied assignment_id/lease_id against the control plane
  or fail closed (F4).
- Compensating recovery releases bound workflow leases via lease_lifecycle (F5).
- Regression tests for each finding.

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,27 +807,27 @@ 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,
-                )
-                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]})"
+                    f"existing branch '{target_branch}' HEAD ({branch_head[:12]}) "
+                    f"does not contain 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]})."
+                        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": (
-                            "Inspect or sync the existing branch with master before bootstrapping."
+                        "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

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("\\", "/")
+    cur = fallback
-    if "/branches/" in norm:
+    for _ in range(64):
-        return os.path.realpath(norm.split("/branches/")[0])
+        parent = os.path.dirname(cur)
-    elif norm.endswith("/branches"):
+        if parent == cur:
-        return os.path.realpath(os.path.dirname(fallback))
+            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
 

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",
+            # assignment_id/lease_id omitted: optional unless verified live.
-            lease_id="lease-67890",
             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()

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 <repo>/branches/<worktree>, recover
+        # <repo> 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):