stacker: fix PR file-diff links + unify slot release lifecycle · snowytrees.dev/project-manager@56cfb26

+35 -5

src/project_manager/check.py

··· 5 5 from pathlib import Path 6 6 7 7 from project_manager.paths import Paths 8 + from project_manager.pool import slot as slot_mod 8 9 from project_manager.pool.db import Owner, OwnerKind, PoolDB 9 10 from project_manager.project import db, discovery 10 11 from project_manager.render import Column 12 + from project_manager.stacker import git as stacker_git 13 + from project_manager.stacker import ops_slot 11 14 12 15 13 16 class Kind(StrEnum): ··· 18 21 BROKEN = "broken" # forward points at a missing slot dir 19 22 ORPHAN_FORWARD = "orphan-forward" # forward exists, no db row 20 23 ORPHAN_OWNER = "orphan-owner" # project-owned pool row with no backing forward 21 - OPS_OWNED = "ops-owned" # pool row with owner_kind == stacker 24 + OPS_OWNED = "ops-owned" # stacker-owned slot mid-op (resumable state present) 25 + STALE_OPS = "stale-ops" # stacker-owned slot with a clean worktree — leaked 22 26 23 27 24 28 @dataclass(frozen=True) ··· 50 54 Kind.ORPHAN_FORWARD: "red", 51 55 Kind.ORPHAN_OWNER: "red", 52 56 Kind.OPS_OWNED: "cyan", 57 + Kind.STALE_OPS: "red", 53 58 } 54 59 55 60 ··· 171 176 if owner.kind == OwnerKind.STACKER: 172 177 if scope is not None: 173 178 continue 179 + # Split stacker-ops claims by whether the worktree still has 180 + # work to preserve. STALE_OPS means a previous push/sync 181 + # leaked the claim — `--fix` can safely release it. OPS_OWNED 182 + # means there's a paused cherry-pick or uncommitted change 183 + # that `pm stacker continue` will pick up; we must hold. 184 + stale = slot_path.is_dir() and not stacker_git.has_resumable_state( 185 + slot_path, 186 + ) 174 187 yield Finding( 175 - kind=Kind.OPS_OWNED, 188 + kind=Kind.STALE_OPS if stale else Kind.OPS_OWNED, 176 189 wt=None, 177 190 repo=repo, 178 191 slot_path=slot_path, 179 192 forward_path=None, 180 - detail="slot held by a stacker ops operation", 193 + detail=( 194 + "stacker ops claim with a clean worktree — leaked" 195 + if stale 196 + else "slot held by a stacker ops operation" 197 + ), 181 198 ) 182 199 continue 183 200 if scope is not None and owner.id != scope: ··· 273 290 """Apply safe fixes. Returns count of applied fixes. 274 291 275 292 Fixed: BROKEN (unlink forward), ORPHAN_FORWARD (unlink forward), 276 - ORPHAN_OWNER (release pool row), STALE (unlink forward). 293 + ORPHAN_OWNER (release pool row), STALE (unlink forward), 294 + STALE_OPS (detach HEAD + release pool row). 277 295 Left alone: DETACHED (intentional), ACTIVE (healthy), DRIFT (info), 278 - OPS_OWNED (intentional). 296 + OPS_OWNED (mid-op — `pm stacker continue` owns it). 279 297 """ 280 298 pooldb = PoolDB(paths.pool_db()) 281 299 n = 0 ··· 286 304 n += 1 287 305 elif f.kind == Kind.ORPHAN_OWNER and f.slot_path is not None: 288 306 pooldb.release(f.slot_path.parent.name, f.slot_path.name) 307 + n += 1 308 + elif f.kind == Kind.STALE_OPS and f.slot_path is not None and f.repo is not None: 309 + # Re-check `has_resumable_state` here: the classification was 310 + # observed earlier in the run, and a concurrent stacker op 311 + # could have entered a paused state since. Cheap insurance 312 + # against detaching from a worktree that's now mid-conflict. 313 + if stacker_git.has_resumable_state(f.slot_path): 314 + continue 315 + ops_slot.release( 316 + pooldb, 317 + slot_mod.Slot(repo=f.repo, uuid=f.slot_path.name, path=f.slot_path), 318 + ) 289 319 n += 1 290 320 return n

+81 -49

src/project_manager/stacker/cherry_pick/driver.py

··· 98 98 logs = [] 99 99 op = ctx.db.get_operation(repo_name) 100 100 assert op 101 - while True: 102 - if op.branch: 103 - if handle is None: 104 - slot_path = worktree.require_checked_out(ctx, repo_name, op.branch) 105 - handle = DriveHandle( 106 - slot_path=slot_path, 107 - acquired_ops=worktree.existing_ops_slot(ctx, op, slot_path), 101 + # Outer try/finally: a `KeyboardInterrupt` (or any unexpected raise) 102 + # mid-loop must still hand the in-flight slot back to the pool when 103 + # the worktree is clean. Without it, cancelling a downstream_sync 104 + # leaves the just-checked-out branch claimed as `stacker|ops`, 105 + # blocking the next stacker op until manual recovery. 106 + try: 107 + while True: 108 + if op.branch: 109 + if handle is None: 110 + slot_path = worktree.require_checked_out(ctx, repo_name, op.branch) 111 + handle = DriveHandle( 112 + slot_path=slot_path, 113 + acquired_ops=worktree.existing_ops_slot(ctx, op, slot_path), 114 + ) 115 + result = drive_local( 116 + ctx, op, slot_path=handle.slot_path, continuing=continuing, logs=logs 108 117 ) 109 - result = drive_local( 110 - ctx, op, slot_path=handle.slot_path, continuing=continuing, logs=logs 111 - ) 112 - continuing = False 113 - if result is not None: 114 - if handle.acquired_ops is not None: 115 - ops_slot.release(PoolDB(ctx.paths.pool_db()), handle.acquired_ops) 116 - return fmt.finish(ctx, logs, result) 117 - op = ctx.db.get_operation(repo_name) 118 - assert op 119 - handle = None 120 - continue 121 - single_op_done = _SINGLE_OP_DONE.get(op.op_type) 122 - if single_op_done is not None: 123 - ctx.db.clear_operation(repo_name) 124 - return fmt.finish(ctx, logs, single_op_done) 125 - if op.op_type == "downstream_sync": 126 - if op.current_index >= len(op.queue): 118 + continuing = False 119 + if result is not None: 120 + _release_handle_if_clean(ctx, handle) 121 + handle = None 122 + return fmt.finish(ctx, logs, result) 123 + op = ctx.db.get_operation(repo_name) 124 + assert op 125 + handle = None 126 + continue 127 + single_op_done = _SINGLE_OP_DONE.get(op.op_type) 128 + if single_op_done is not None: 127 129 ctx.db.clear_operation(repo_name) 128 - return fmt.finish(ctx, logs, "Sync complete.") 129 - handle = advance_downstream(ctx, repo_name, op, logs) 130 - op = ctx.db.get_operation(repo_name) 131 - assert op 132 - continue 133 - raise git.GitError(f"Unknown operation type {op.op_type}.") 130 + return fmt.finish(ctx, logs, single_op_done) 131 + if op.op_type == "downstream_sync": 132 + if op.current_index >= len(op.queue): 133 + ctx.db.clear_operation(repo_name) 134 + return fmt.finish(ctx, logs, "Sync complete.") 135 + handle = advance_downstream(ctx, repo_name, op, logs) 136 + op = ctx.db.get_operation(repo_name) 137 + assert op 138 + continue 139 + raise git.GitError(f"Unknown operation type {op.op_type}.") 140 + finally: 141 + _release_handle_if_clean(ctx, handle) 142 + 143 + 144 + def _release_handle_if_clean(ctx: StackerCtx, handle: DriveHandle | None) -> None: 145 + """Hand `handle.acquired_ops` back to the pool when safe. 146 + 147 + The cherry-pick driver carries the active slot in the handle. We 148 + release it the same way `slot.release_if_clean` releases a fresh 149 + `AcquiredSlot`: skip when no claim is held, hold when the worktree 150 + has resumable state (uncommitted changes / `CHERRY_PICK_HEAD`), 151 + otherwise detach + release. 152 + """ 153 + if handle is None or handle.acquired_ops is None: 154 + return 155 + if git.has_resumable_state(handle.slot_path): 156 + return 157 + ops_slot.release(PoolDB(ctx.paths.pool_db()), handle.acquired_ops) 134 158 135 159 136 160 def advance_downstream( ··· 152 176 f"{selectors.selector_for(repo_name, next_branch)}" 153 177 ) 154 178 acquired = worktree.acquire(ctx, repo_name, next_branch) 155 - parent_head, _, _ = sync_plan(ctx, tracked, acquired.path) 156 - if parent_head == tracked.managed_base_commit: 157 - child_label = selectors.selector_for(tracked.repo_name, tracked.branch) 158 - parent_label = selectors.selector_for( 159 - tracked.parent_repo_name, tracked.parent_branch 160 - ) 161 - fmt.record( 162 - ctx, 163 - logs, 164 - f"Skipping {child_label}; " 165 - f"parent {parent_label} is unchanged at {fmt.short(parent_head)}", 179 + # Bridge the acquire → DriveHandle handoff: an interrupt or raise 180 + # between here and `return DriveHandle(...)` would orphan the 181 + # just-claimed slot since the caller's outer finally only sees 182 + # `handle = advance_downstream(...)` after this returns. 183 + try: 184 + parent_head, _, _ = sync_plan(ctx, tracked, acquired.path) 185 + if parent_head == tracked.managed_base_commit: 186 + child_label = selectors.selector_for(tracked.repo_name, tracked.branch) 187 + parent_label = selectors.selector_for( 188 + tracked.parent_repo_name, tracked.parent_branch 189 + ) 190 + fmt.record( 191 + ctx, 192 + logs, 193 + f"Skipping {child_label}; " 194 + f"parent {parent_label} is unchanged at {fmt.short(parent_head)}", 195 + ) 196 + op.current_index += 1 197 + ctx.db.put_operation(op) 198 + worktree.release_if_clean(ctx, acquired) 199 + return None 200 + prepare_local_operation( 201 + ctx, tracked, op_type="downstream_sync", slot_path=acquired.path, logs=logs 166 202 ) 167 - op.current_index += 1 168 - ctx.db.put_operation(op) 169 - worktree.release_if_owned(ctx, acquired) 170 - return None 171 - prepare_local_operation( 172 - ctx, tracked, op_type="downstream_sync", slot_path=acquired.path, logs=logs 173 - ) 203 + except BaseException: 204 + worktree.release_if_clean(ctx, acquired) 205 + raise 174 206 return DriveHandle(slot_path=acquired.path, acquired_ops=acquired.ops) 175 207 176 208

+26

src/project_manager/stacker/git.py

··· 226 226 return bool(out.strip()) 227 227 228 228 229 + def detach_head(path: Path) -> None: 230 + """`git checkout --detach HEAD` — drop the slot's branch attachment. 231 + 232 + Single point of truth for the "this slot is no longer bound to a 233 + branch" action. Callers handle the dirty-check themselves and only 234 + invoke this once they've decided the working state is OK to discard 235 + (or has nothing to discard). Used by: 236 + - `ops_slot.release` (return slot to pool with detached HEAD). 237 + - `stacker.ops.remove` (free the branch ref so `git branch -D` works). 238 + - `pm check --fix` for stale stacker-ops claims. 239 + """ 240 + git(path, "checkout", "--detach", "HEAD") 241 + 242 + 243 + def has_resumable_state(path: Path) -> bool: 244 + """True if the worktree has work that detach-HEAD would lose. 245 + 246 + Tracked-file modifications (staged or unstaged) and an in-progress 247 + cherry-pick (CHERRY_PICK_HEAD) both represent uncommitted resolution 248 + work — releasing the slot under either would discard it. This is 249 + the predicate every "release the slot when safe" call site consults 250 + before handing the slot back to the pool. 251 + """ 252 + return has_tracked_changes(path) or cherry_pick_in_progress(path) 253 + 254 + 229 255 def cherry_pick_in_progress(path: Path) -> bool: 230 256 cherry_pick_head = git(path, "rev-parse", "--git-path", "CHERRY_PICK_HEAD").stdout.strip() 231 257 # `--git-path` returns a path relative to the repo working tree when

+32 -36

src/project_manager/stacker/ops/absorb.py

··· 51 51 child_label = selectors.selector_for(repo_name, child_branch) 52 52 parent_label = selectors.selector_for(repo_name, parent_branch) 53 53 return f"Nothing to absorb from {child_label} into {parent_label}." 54 - acquired = worktree.acquire(ctx, repo_name, parent_branch) 55 - try: 54 + with worktree.acquired_for_op(ctx, repo_name, parent_branch) as acquired: 56 55 ensure_syncable(acquired.path) 57 - except git.GitError: 58 - worktree.release_if_owned(ctx, acquired) 59 - raise 60 - parent_slot_head = git.rev_parse(acquired.path, "HEAD") 61 - ctx.db.put_operation( 62 - OperationState( 63 - repo_name=repo_name, 64 - op_type="local_absorb", 65 - status="running", 66 - # op.branch = the branch that's checked out in the slot (parent); 67 - # op.parent_branch carries the source child so failure messages 68 - # and `pm stacker abort` can render both sides of the absorb. 69 - branch=parent_branch, 70 - parent_branch=child_branch, 71 - start_head=parent_slot_head, 72 - target_parent_head=parent_slot_head, 73 - commit_list=commit_list, 74 - next_commit_index=0, 56 + parent_slot_head = git.rev_parse(acquired.path, "HEAD") 57 + ctx.db.put_operation( 58 + OperationState( 59 + repo_name=repo_name, 60 + op_type="local_absorb", 61 + status="running", 62 + # op.branch = the branch that's checked out in the slot (parent); 63 + # op.parent_branch carries the source child so failure messages 64 + # and `pm stacker abort` can render both sides of the absorb. 65 + branch=parent_branch, 66 + parent_branch=child_branch, 67 + start_head=parent_slot_head, 68 + target_parent_head=parent_slot_head, 69 + commit_list=commit_list, 70 + next_commit_index=0, 71 + ) 75 72 ) 76 - ) 77 - logs: list[str] = [] 78 - fmt.record( 79 - ctx, 80 - logs, 81 - f"Absorbing {len(commit_list)} commit(s) from " 82 - f"{selectors.selector_for(repo_name, child_branch)} into " 83 - f"{selectors.selector_for(repo_name, parent_branch)} " 84 - f"({fmt.short(parent_slot_head)})", 85 - ) 86 - return cp_driver.run_until_pause_or_finish( 87 - ctx, 88 - repo_name, 89 - cp_driver.DriveHandle(slot_path=acquired.path, acquired_ops=acquired.ops), 90 - logs=logs, 91 - ) 73 + logs: list[str] = [] 74 + fmt.record( 75 + ctx, 76 + logs, 77 + f"Absorbing {len(commit_list)} commit(s) from " 78 + f"{selectors.selector_for(repo_name, child_branch)} into " 79 + f"{selectors.selector_for(repo_name, parent_branch)} " 80 + f"({fmt.short(parent_slot_head)})", 81 + ) 82 + return cp_driver.run_until_pause_or_finish( 83 + ctx, 84 + repo_name, 85 + cp_driver.DriveHandle(slot_path=acquired.path, acquired_ops=acquired.ops), 86 + logs=logs, 87 + )

+6 -5

src/project_manager/stacker/ops/push.py

··· 52 52 leaf = resolved[-1] 53 53 focus_pr: gh.PullRequest | None = None 54 54 for item in resolved: 55 - acquired = worktree.acquire(ctx, item.repo_name, item.branch) 56 - try: 55 + # `acquired_for_op` releases the slot on exit when the worktree 56 + # has nothing to preserve — covers success, exception, and Ctrl+C 57 + # uniformly. Push has no resumable state of its own; if a cherry 58 + # -pick somehow remains in progress at exit time, the predicate 59 + # holds the slot for `pm stacker continue` to pick up. 60 + with worktree.acquired_for_op(ctx, item.repo_name, item.branch): 57 61 worktree.run_single_pp(ctx, item, logs) 58 62 if pr_ctx is not None: 59 63 is_leaf = item.branch == leaf.branch ··· 67 71 ) 68 72 if item.branch == target.branch: 69 73 focus_pr = pr 70 - finally: 71 - if item.branch != target.branch: 72 - worktree.release_if_owned(ctx, acquired) 73 74 if pr_ctx is None: 74 75 return fmt.finish(ctx, logs, "Push complete.") 75 76 assert focus_pr is not None

+1 -1

src/project_manager/stacker/ops/remove.py

··· 71 71 slot_path = locate.locate_worktree(ctx.paths, tracked.repo_name, tracked.branch) 72 72 if slot_path is not None: 73 73 # Detach so `git branch -D` won't refuse because it's checked out. 74 - git.git(slot_path, "checkout", "--detach", "HEAD") 74 + git.detach_head(slot_path) 75 75 repo_path = ctx.paths.repo(tracked.repo_name) 76 76 git.git(repo_path, "branch", "-D", tracked.branch, check=False)

+33 -31

src/project_manager/stacker/ops/sync.py

··· 66 66 parent_branch=tracked.parent_branch, 67 67 ) 68 68 ) 69 - try: 70 - acquired = worktree.acquire(ctx, tracked.repo_name, tracked.branch) 71 - except Exception: 72 - ctx.db.clear_operation(tracked.repo_name) 73 - raise 74 - parent_head, _, _ = cp_driver.sync_plan(ctx, tracked, acquired.path) 75 - if parent_head == tracked.managed_base_commit: 76 - ctx.db.clear_operation(tracked.repo_name) 77 - worktree.release_if_owned(ctx, acquired) 78 - child_label = selectors.selector_for(tracked.repo_name, tracked.branch) 79 - parent_label = selectors.selector_for( 80 - tracked.parent_repo_name, tracked.parent_branch 81 - ) 82 - return ( 83 - f"Nothing to sync for {child_label}.\n" 84 - f"Parent {parent_label} is unchanged at {fmt.short(parent_head)}." 85 - ) 69 + # The context manager releases the slot iff the worktree is clean on 70 + # exit — covers success, exception, and `KeyboardInterrupt` in one 71 + # place. A pause-on-conflict mid-cherry-pick keeps the slot (predicate 72 + # sees `CHERRY_PICK_HEAD`) so `pm stacker continue` can resume. 86 73 try: 87 - ensure_syncable(acquired.path) 88 - except git.GitError: 89 - ctx.db.clear_operation(tracked.repo_name) 90 - worktree.release_if_owned(ctx, acquired) 74 + with worktree.acquired_for_op(ctx, tracked.repo_name, tracked.branch) as acquired: 75 + parent_head, _, _ = cp_driver.sync_plan(ctx, tracked, acquired.path) 76 + if parent_head == tracked.managed_base_commit: 77 + ctx.db.clear_operation(tracked.repo_name) 78 + child_label = selectors.selector_for(tracked.repo_name, tracked.branch) 79 + parent_label = selectors.selector_for( 80 + tracked.parent_repo_name, tracked.parent_branch 81 + ) 82 + return ( 83 + f"Nothing to sync for {child_label}.\n" 84 + f"Parent {parent_label} is unchanged at {fmt.short(parent_head)}." 85 + ) 86 + ensure_syncable(acquired.path) 87 + logs: list[str] = [] 88 + cp_driver.prepare_local_operation( 89 + ctx, tracked, op_type="local_sync", slot_path=acquired.path, logs=logs 90 + ) 91 + return cp_driver.run_until_pause_or_finish( 92 + ctx, 93 + tracked.repo_name, 94 + cp_driver.DriveHandle(slot_path=acquired.path, acquired_ops=acquired.ops), 95 + logs=logs, 96 + ) 97 + except BaseException: 98 + # The slot is already handled by the context manager above; we 99 + # just need to clear the OperationState so a stale row doesn't 100 + # block the next stacker invocation in this repo. 101 + if ctx.db.get_operation(tracked.repo_name): 102 + ctx.db.clear_operation(tracked.repo_name) 91 103 raise 92 - logs: list[str] = [] 93 - cp_driver.prepare_local_operation( 94 - ctx, tracked, op_type="local_sync", slot_path=acquired.path, logs=logs 95 - ) 96 - return cp_driver.run_until_pause_or_finish( 97 - ctx, 98 - tracked.repo_name, 99 - cp_driver.DriveHandle(slot_path=acquired.path, acquired_ops=acquired.ops), 100 - logs=logs, 101 - ) 102 104 103 105 104 106 def repair(ctx: StackerCtx, target: SelectorTarget, base_ref: str) -> str:

+17

src/project_manager/stacker/ops/worktree.py

··· 1 1 from __future__ import annotations 2 2 3 + from collections.abc import Iterator 4 + from contextlib import contextmanager 3 5 from pathlib import Path 4 6 from typing import TYPE_CHECKING 5 7 ··· 31 33 32 34 def release_if_owned(ctx: StackerCtx, acquired: AcquiredSlot) -> None: 33 35 slot.release_if_owned(ctx, acquired) 36 + 37 + 38 + def release_if_clean(ctx: StackerCtx, acquired: AcquiredSlot) -> None: 39 + slot.release_if_clean(ctx, acquired) 40 + 41 + 42 + @contextmanager 43 + def acquired_for_op( 44 + ctx: StackerCtx, 45 + repo_name: str, 46 + branch: str, 47 + ) -> Iterator[AcquiredSlot]: 48 + """Re-export of `slot.acquired_for_op` for the `worktree.` namespace.""" 49 + with slot.acquired_for_op(ctx, repo_name, branch) as acquired: 50 + yield acquired 34 51 35 52 36 53 def slot_path_for_active_op(

+10 -5

src/project_manager/stacker/ops_slot.py

··· 102 102 ) -> slot_mod.Slot: 103 103 """Claim a pool slot and check `branch` out in it. 104 104 105 - Steady-state release happens via `release` on clean completion. On crash 106 - mid-op the slot stays claimed so `stacker continue` / `abort` can drive 107 - resolution — do NOT wrap this in try/finally. 105 + Steady-state release happens via `release` on clean completion. The 106 + pre-completion window — claim succeeded, checkout running — catches 107 + `BaseException` so a `KeyboardInterrupt` (or any unexpected error) 108 + releases the just-taken claim instead of orphaning it. There's no 109 + resumable op for a single-shot acquire and the worktree is clean by 110 + construction at this point, so the slot is safe to return to the 111 + pool. Once this returns, the caller's own `try/finally` (with 112 + `release_if_clean`) governs. 108 113 """ 109 114 target = claim(paths, pooldb, repo_name, wait=wait) 110 115 try: 111 116 git.git(target.path, "checkout", branch) 112 - except git.GitError: 117 + except BaseException: 113 118 pooldb.release(repo_name, target.uuid) 114 119 raise 115 120 return target ··· 117 122 118 123 def release(pooldb: PoolDB, slot: slot_mod.Slot) -> None: 119 124 """Return a stacker-ops slot to the pool with detached HEAD.""" 120 - git.git(slot.path, "checkout", "--detach", "HEAD") 125 + git.detach_head(slot.path) 121 126 pooldb.release(slot.repo, slot.uuid)

+25

src/project_manager/stacker/pr/create_update.py

··· 119 119 pr_map=pr_map, 120 120 config=config, 121 121 current_repo=pr_ctx.current_repo, 122 + live_heads=_live_heads(ctx, component), 122 123 ) 123 124 for node in component: 124 125 pr = pr_map.get(node.branch) ··· 140 141 f"Updated stack block for PR #{pr.number} " 141 142 f"({selectors.selector_for(node.repo_name, node.branch)})", 142 143 ) 144 + 145 + 146 + def _live_heads( 147 + ctx: StackerCtx, component: list[TrackedBranch] 148 + ) -> dict[str, str]: 149 + """Map `branch -> git rev-parse <branch>` from the canonical repo. 150 + 151 + Source of truth for the head SHA in `_files_url`. Reads the branch 152 + ref directly so it tracks the latest commit regardless of which 153 + worktree (if any) has the branch checked out — refs are shared 154 + across all worktrees of a repo. The DB's `last_clean_head` is only 155 + refreshed by sync/init/repair, so it goes stale as soon as the user 156 + adds a local commit; reading the ref live closes that gap. 157 + Branches missing locally fall through to the `last_clean_head` 158 + fallback in `_files_url`. 159 + """ 160 + heads: dict[str, str] = {} 161 + for node in component: 162 + repo_path = ctx.paths.repo(node.repo_name) 163 + try: 164 + heads[node.branch] = git.rev_parse(repo_path, node.branch) 165 + except git.GitError: 166 + continue 167 + return heads 143 168 144 169 145 170 def erase_component_pr_bodies(

+23 -7

src/project_manager/stacker/pr/stack_block.py

··· 23 23 pr_map: dict[str, gh.PullRequest] 24 24 config: RepoPRConfig 25 25 current_repo: gh.RepoInfo 26 + # Live `git rev-parse <branch>` from the canonical repo, keyed by branch. 27 + # Lets `_files_url` render `<base>..<head>` against the SHA actually 28 + # pushed (or about to be) instead of the `last_clean_head` cached at 29 + # init/sync time, which goes stale the moment a local commit lands. 30 + live_heads: dict[str, str] 26 31 27 32 28 33 def render_stack_block( ··· 96 101 97 102 98 103 def _files_url(node: TrackedBranch, render_ctx: _StackRender) -> str | None: 99 - """Build `<pr-url>/files/<base>..<head>` for reviewer navigation. 104 + """Build a Files-changed URL for reviewer navigation. 100 105 101 - Scoped to the PR so reviewers see only the commits unique to that 102 - branch. For a merged PR the range is dropped (`<pr>/files`) since 103 - the commit range no longer reflects reviewable changes. Uses 104 - `last_clean_head` (persisted by sync/init/repair), so ancestors 105 - and siblings render correctly even when not currently checked out. 106 + The range form `<pr>/files/<base>..<head>` only renders when both 107 + SHAs are reachable in the PR's commit graph — true for branches 108 + nested under another tracked, non-merged branch (parent commits are 109 + part of the child's repo-pr commit list) but not for branches whose 110 + parent is the trunk. For root-of-stack branches the base SHA is a 111 + trunk commit outside the PR graph, so GitHub 404s; we emit 112 + `<pr>/files` instead, mirroring universe gitstack's `get_diff_link`. 113 + A merged parent collapses to the same case (its commits land on 114 + trunk and the parent ref is gone). Head SHA prefers the live branch 115 + ref via `render_ctx.live_heads`, falling back to the `last_clean_head` 116 + cached by sync/init/repair. 106 117 """ 107 118 pr = render_ctx.pr_map.get(node.branch) 108 119 if pr is None: 109 120 return None 110 121 if pr.state == "MERGED": 111 122 return f"{pr.url}/files" 112 - head_sha = node.last_clean_head 123 + parent_pr = render_ctx.pr_map.get(node.parent_branch) 124 + parent_is_trunk = node.parent_branch == render_ctx.config.trunk_branch 125 + parent_is_merged = parent_pr is not None and parent_pr.state == "MERGED" 126 + if parent_is_trunk or parent_is_merged: 127 + return f"{pr.url}/files" 128 + head_sha = render_ctx.live_heads.get(node.branch) or node.last_clean_head 113 129 if not head_sha: 114 130 return None 115 131 base_sha = node.managed_base_commit

+101 -6

src/project_manager/stacker/slot.py

··· 14 14 """ 15 15 from __future__ import annotations 16 16 17 + import contextlib 18 + from collections.abc import Iterator 17 19 from dataclasses import dataclass 18 20 from pathlib import Path 19 21 from typing import TYPE_CHECKING ··· 69 71 """Pick a worktree to run an op for `branch` in, checking it out as needed. 70 72 71 73 Resolution order: existing checkout → cwd reuse (when policy permits) → 72 - fresh ops slot. Pool exhaustion in the last step raises 73 - `slot_mod.PoolExhaustedError` (same as today's `ops_slot.acquire`). 74 + fresh ops slot → cwd reuse with branch-switch (last-resort fallback). 75 + The fallback kicks in only when `ops_slot.acquire` raises 76 + `PoolExhaustedError`; without it the user is stuck whenever every slot 77 + is project-claimed and they're sitting in one of those slots — adding 78 + a slot just to push is heavyweight, and clobbering the cwd's branch 79 + is recoverable (cwd is the user's, by definition). 74 80 """ 75 81 existing = locate.locate_worktree(ctx.paths, repo_name, branch) 76 82 if existing is not None: ··· 80 86 git.git(cwd_path, "checkout", branch) 81 87 return AcquiredSlot(path=cwd_path, ops=None) 82 88 pooldb = PoolDB(ctx.paths.pool_db()) 83 - claimed = ops_slot.acquire( 84 - ctx.paths, pooldb, repo_name, branch, 85 - wait=ops_slot.WaitOptions(progress=ctx.progress), 86 - ) 89 + try: 90 + claimed = ops_slot.acquire( 91 + ctx.paths, pooldb, repo_name, branch, 92 + wait=ops_slot.WaitOptions(progress=ctx.progress), 93 + ) 94 + except slot_mod.PoolExhaustedError: 95 + fallback = _cwd_reuse_path( 96 + ctx, repo_name, 97 + CwdReusePolicy(enabled=cwd_reuse.enabled, allow_branch_switch=True), 98 + ) 99 + if fallback is None: 100 + raise 101 + git.git(fallback, "checkout", branch) 102 + return AcquiredSlot(path=fallback, ops=None) 87 103 return AcquiredSlot(path=claimed.path, ops=claimed) 88 104 89 105 ··· 114 130 def release_if_owned(ctx: StackerCtx, acquired: AcquiredSlot) -> None: 115 131 """Release a fresh ops slot. No-op when the slot was a cwd reuse or 116 132 locate-fast-path hit (i.e. `acquired.ops is None`). 133 + 134 + Prefer `release_if_clean` for steady-state post-op cleanup — this 135 + variant is for paths that have already decided the worktree state 136 + doesn't need preserving (e.g. early-return after a no-op sync). 117 137 """ 118 138 if acquired.ops is not None: 119 139 ops_slot.release(PoolDB(ctx.paths.pool_db()), acquired.ops) 140 + 141 + 142 + @contextlib.contextmanager 143 + def acquired_for_op( 144 + ctx: StackerCtx, 145 + repo_name: str, 146 + branch: str, 147 + *, 148 + cwd_reuse: CwdReusePolicy = CwdReusePolicy(), 149 + ) -> Iterator[AcquiredSlot]: 150 + """Context-managed slot acquisition with automatic clean-release. 151 + 152 + Wraps `resolve_slot` + `release_if_clean` into a single `with` 153 + block — the body of the block is the op, and on exit (success, 154 + exception, `KeyboardInterrupt`) the slot is returned to the pool 155 + iff the worktree has nothing to preserve. A mid-cherry-pick 156 + (`CHERRY_PICK_HEAD` present) or uncommitted change keeps the slot 157 + held so `pm stacker continue` can resume. 158 + 159 + Use for any single-acquire op (push iteration, sync setup, absorb, 160 + create, etc.). The cherry-pick driver manages slot handoffs across 161 + its own queue and uses `release_if_clean` directly. 162 + """ 163 + acquired = resolve_slot(ctx, repo_name, branch, cwd_reuse=cwd_reuse) 164 + try: 165 + yield acquired 166 + finally: 167 + release_if_clean(ctx, acquired) 168 + 169 + 170 + def release_if_clean(ctx: StackerCtx, acquired: AcquiredSlot) -> None: 171 + """Release a stacker-minted slot iff the worktree has nothing to 172 + preserve. 173 + 174 + Designed for `try/finally` cleanup that fires on success, exception, 175 + and `KeyboardInterrupt` alike. Returns the slot to the pool when: 176 + - The slot wasn't stacker-minted (`acquired.ops is None`). 177 + - The worktree has no resumable state (clean tree, no 178 + `CHERRY_PICK_HEAD`). 179 + Otherwise holds the slot so `pm stacker continue` can pick up the 180 + user's mid-flight resolution. Resume relies on the DB-side 181 + `OperationState`, not on which slot the work lives in — releasing a 182 + clean slot is safe because the next continue rebinds via that state. 183 + """ 184 + if acquired.ops is None: 185 + return 186 + try_release_ops_slot(ctx, acquired.ops.repo, acquired.ops.uuid) 187 + 188 + 189 + def try_release_ops_slot(ctx: StackerCtx, repo: str, uuid: str) -> bool: 190 + """Release the stacker-ops slot at (repo, uuid) when its worktree has 191 + no resumable state. Returns True if the slot was released, False if 192 + held. 193 + 194 + Single source of truth for "is this stacker slot reclaimable?" — 195 + consulted by both `release_if_clean` (post-op cleanup) and 196 + `pm check --fix` (steady-state recovery of leaked claims). The 197 + predicate (`git.has_resumable_state`) and the action 198 + (`ops_slot.release` → `git.detach_head` + `pooldb.release`) live 199 + here so future callers don't drift into divergent dirty checks. 200 + 201 + Caller is responsible for already knowing the slot is owned by 202 + `OWNER_STACKER_OPS`; the function will silently no-op if not, since 203 + it only constructs the `slot_mod.Slot` view used by 204 + `ops_slot.release` and that release is itself idempotent against 205 + a missing pool row. 206 + """ 207 + slot_path = ctx.paths.slot(repo, uuid) 208 + if git.has_resumable_state(slot_path): 209 + return False 210 + ops_slot.release( 211 + PoolDB(ctx.paths.pool_db()), 212 + slot_mod.Slot(repo=repo, uuid=uuid, path=slot_path), 213 + ) 214 + return True 120 215 121 216 122 217 def _cwd_reuse_path(

+8 -4

tests/stacker/test_ancestor_chain.py

··· 1 1 from __future__ import annotations 2 2 3 + import contextlib 4 + from collections.abc import Iterator 3 5 from pathlib import Path 4 6 5 7 import pytest ··· 92 94 pp_order.append(tb.branch) 93 95 return True 94 96 95 - def _fake_acquire(_ctx: object, _repo: str, _branch: str) -> _Acquired: 96 - return _Acquired(path=service.paths.repo("demo"), ops=None) 97 + @contextlib.contextmanager 98 + def _fake_acquired_for_op( 99 + _ctx: object, _repo: str, _branch: str 100 + ) -> Iterator[_Acquired]: 101 + yield _Acquired(path=service.paths.repo("demo"), ops=None) 97 102 98 103 def _fake_create( 99 104 _ctx: object, tb: TrackedBranch, *_args: object, **_kwargs: object ··· 109 114 # push_ops uses namespace imports (`worktree.run_single_pp`) so patching the 110 115 # module attribute takes effect. Same for pr_create_update.*. 111 116 monkeypatch.setattr(push_ops.worktree, "run_single_pp", _fake_pp) 112 - monkeypatch.setattr(push_ops.worktree, "acquire", _fake_acquire) 113 - monkeypatch.setattr(push_ops.worktree, "release_if_owned", lambda *_a, **_k: None) 117 + monkeypatch.setattr(push_ops.worktree, "acquired_for_op", _fake_acquired_for_op) 114 118 monkeypatch.setattr(pr_create_update, "create_or_update_current_pr", _fake_create) 115 119 monkeypatch.setattr( 116 120 push_ops.pr_create_update, "refresh_component_pr_bodies", lambda *_a, **_k: None

+155

tests/stacker/test_create_slot_resolution.py

··· 176 176 assert pooldb.get_owner(here.repo, here.uuid) is None 177 177 178 178 179 + def test_release_if_clean_no_op_when_not_owned( 180 + stacker_repo: tuple[str, Path], # noqa: ARG001 — bootstraps demo repo for service 181 + pooldb: PoolDB, 182 + service: StackerService, 183 + ) -> None: 184 + """`acquired.ops is None` (locate-fast-path or cwd-reuse) is a no-op.""" 185 + not_owned = slot.AcquiredSlot(path=Path("/dev/null"), ops=None) 186 + slot.release_if_clean(service.ctx, not_owned) # must not raise 187 + assert pooldb.list_owned("demo") == [] 188 + 189 + 190 + def test_release_if_clean_releases_when_worktree_clean( 191 + stacker_repo: tuple[str, Path], # noqa: ARG001 — bootstraps demo repo 192 + three_slots: list[slot_mod.Slot], 193 + pooldb: PoolDB, 194 + service: StackerService, 195 + ) -> None: 196 + pooldb.claim(three_slots[0].repo, three_slots[0].uuid, OWNER_STACKER_OPS) 197 + stacker_git.git(three_slots[0].path, "checkout", "-b", "feature-clean") 198 + acquired = slot.AcquiredSlot(path=three_slots[0].path, ops=three_slots[0]) 199 + 200 + slot.release_if_clean(service.ctx, acquired) 201 + 202 + assert pooldb.get_owner(three_slots[0].repo, three_slots[0].uuid) is None 203 + # Released slots are detached so the next claimant doesn't inherit a branch. 204 + head = stacker_git.git( 205 + three_slots[0].path, "symbolic-ref", "-q", "HEAD", check=False, 206 + ) 207 + assert head.returncode != 0 208 + 209 + 210 + def test_release_if_clean_holds_when_worktree_has_tracked_changes( 211 + stacker_repo: tuple[str, Path], # noqa: ARG001 — bootstraps demo repo 212 + three_slots: list[slot_mod.Slot], 213 + pooldb: PoolDB, 214 + service: StackerService, 215 + ) -> None: 216 + pooldb.claim(three_slots[0].repo, three_slots[0].uuid, OWNER_STACKER_OPS) 217 + stacker_git.git(three_slots[0].path, "checkout", "-b", "feature-dirty") 218 + # Modify a tracked file so `git status --porcelain -uno` is non-empty. 219 + (three_slots[0].path / "README.md").write_text("# changed\n") 220 + acquired = slot.AcquiredSlot(path=three_slots[0].path, ops=three_slots[0]) 221 + 222 + slot.release_if_clean(service.ctx, acquired) 223 + 224 + # Slot stays claimed; the user's modification would otherwise be discarded 225 + # by the detach-HEAD step inside `ops_slot.release`. 226 + assert pooldb.get_owner(three_slots[0].repo, three_slots[0].uuid) == OWNER_STACKER_OPS 227 + assert stacker_git.current_branch(three_slots[0].path) == "feature-dirty" 228 + 229 + 230 + def test_release_if_clean_holds_when_cherry_pick_in_progress( 231 + stacker_repo: tuple[str, Path], 232 + three_slots: list[slot_mod.Slot], 233 + pooldb: PoolDB, 234 + service: StackerService, 235 + ) -> None: 236 + """`CHERRY_PICK_HEAD` indicates a paused cherry-pick — `pm stacker continue` 237 + must find the slot still claimed when it resumes.""" 238 + _repo_name, _ = stacker_repo 239 + # Stage a deliberate cherry-pick conflict: two divergent commits on the 240 + # same line, then cherry-pick one onto the other so the cherry-pick 241 + # pauses with CHERRY_PICK_HEAD set. All work happens in slot[0] so it 242 + # doesn't fight the main repo's worktree. 243 + work = three_slots[0] 244 + stacker_git.git(work.path, "checkout", "-b", "feature-side", "main") 245 + (work.path / "README.md").write_text("side\n") 246 + stacker_git.git(work.path, "add", "README.md") 247 + stacker_git.git( 248 + work.path, 249 + "-c", "user.email=t@e.com", "-c", "user.name=t", 250 + "commit", "-m", "side: divergent", 251 + ) 252 + side_sha = stacker_git.rev_parse(work.path, "HEAD") 253 + 254 + stacker_git.git(work.path, "checkout", "-b", "feature-main", "main") 255 + (work.path / "README.md").write_text("main\n") 256 + stacker_git.git(work.path, "add", "README.md") 257 + stacker_git.git( 258 + work.path, 259 + "-c", "user.email=t@e.com", "-c", "user.name=t", 260 + "commit", "-m", "main: divergent", 261 + ) 262 + 263 + stacker_git.git(work.path, "cherry-pick", side_sha, check=False) 264 + # Conflict — `CHERRY_PICK_HEAD` exists. 265 + assert stacker_git.cherry_pick_in_progress(work.path) 266 + 267 + pooldb.claim(work.repo, work.uuid, OWNER_STACKER_OPS) 268 + acquired = slot.AcquiredSlot(path=work.path, ops=work) 269 + 270 + slot.release_if_clean(service.ctx, acquired) 271 + 272 + # CHERRY_PICK_HEAD survives → slot must too, so `continue` can drive it. 273 + assert pooldb.get_owner(work.repo, work.uuid) == OWNER_STACKER_OPS 274 + assert stacker_git.cherry_pick_in_progress(work.path) 275 + 276 + 277 + def test_acquired_for_op_releases_on_kbinterrupt_when_clean( 278 + stacker_repo: tuple[str, Path], 279 + three_slots: list[slot_mod.Slot], # noqa: ARG001 — pool-population side-effect 280 + pooldb: PoolDB, 281 + service: StackerService, 282 + ) -> None: 283 + """The context manager must release on `KeyboardInterrupt` — that's the 284 + cancel-during-sync case the broader cleanup is targeted at.""" 285 + repo_name, repo_path = stacker_repo 286 + stacker_git.git(repo_path, "branch", "feature-target", "main") 287 + 288 + with pytest.raises(KeyboardInterrupt), slot.acquired_for_op( 289 + service.ctx, repo_name, "feature-target", 290 + ): 291 + raise KeyboardInterrupt 292 + 293 + stacker_owned = [ 294 + uuid for repo, uuid, owner in pooldb.list_owned(repo_name) 295 + if owner.kind == OwnerKind.STACKER 296 + ] 297 + assert stacker_owned == [], ( 298 + "interrupt with a clean worktree must not leak the stacker claim" 299 + ) 300 + 301 + 302 + def test_resolve_slot_falls_back_to_cwd_when_pool_exhausted( 303 + stacker_repo: tuple[str, Path], 304 + three_slots: list[slot_mod.Slot], 305 + pooldb: PoolDB, 306 + service: StackerService, 307 + monkeypatch: pytest.MonkeyPatch, 308 + ) -> None: 309 + """Last-resort fallback: pool fully claimed by projects and cwd is on a 310 + live branch — default policy says "skip cwd" and `ops_slot.claim` 311 + fails fast (no stacker-owned slot to wait on). Without a fallback the 312 + user sees `pool ... has no free slot...` and is stuck. Reuse cwd 313 + instead, switching the live branch — same as `allow_branch_switch=True` 314 + but only when the pool can't help. 315 + """ 316 + repo_name, repo_path = stacker_repo 317 + stacker_git.git(repo_path, "branch", "feature-target", "main") 318 + # All slots project-owned: no free slot, no stacker slot to wait on. 319 + for i, s in enumerate(three_slots): 320 + pooldb.claim(s.repo, s.uuid, Owner(OwnerKind.PROJECT, f"proj-{i}")) 321 + cwd_slot = three_slots[0] 322 + stacker_git.git(cwd_slot.path, "checkout", "-b", "user-feature") 323 + monkeypatch.chdir(cwd_slot.path) 324 + 325 + acquired = slot.resolve_slot(service.ctx, repo_name, "feature-target") 326 + 327 + assert acquired.path.resolve() == cwd_slot.path.resolve(), ( 328 + "pool exhausted → resolve_slot should fall back to cwd reuse" 329 + ) 330 + assert stacker_git.current_branch(cwd_slot.path) == "feature-target" 331 + assert acquired.ops is None, "cwd-reuse fallback must not take a stacker claim" 332 + 333 + 179 334 def test_resolve_slot_disabled_policy_skips_cwd( 180 335 stacker_repo: tuple[str, Path], 181 336 three_slots: list[slot_mod.Slot],

+38

tests/stacker/test_ops_slot.py

··· 72 72 assert branch == "refs/heads/feature-crash" 73 73 74 74 75 + def test_acquire_releases_claim_when_checkout_interrupted( 76 + pm_env: Paths, 77 + stacker_repo: tuple[str, Path], 78 + three_slots: list[slot_mod.Slot], # noqa: ARG001 — populates the pool 79 + monkeypatch: pytest.MonkeyPatch, 80 + ) -> None: 81 + """Regression: a Ctrl+C during `git checkout` (or any non-`GitError` 82 + raise inside the post-claim block) must still release the pool claim. 83 + 84 + The original `except GitError` clause caught the typed checkout 85 + failure but not `KeyboardInterrupt`; canceling a `pm stacker push` 86 + mid-acquire left the slot stuck as `stacker|ops` with no resumable 87 + state to recover, blocking subsequent pushes on pool exhaustion. 88 + """ 89 + repo_name, _ = stacker_repo 90 + pooldb = _pooldb(pm_env) 91 + before_free = {s.uuid for s in slot_mod.free_slots(pm_env, pooldb, repo_name)} 92 + 93 + real_git = stacker_git.git 94 + 95 + def _interrupt_checkout( 96 + path: Path, *args: str, check: bool = True, 97 + ) -> object: 98 + if args[:1] == ("checkout",): 99 + raise KeyboardInterrupt 100 + return real_git(path, *args, check=check) 101 + 102 + monkeypatch.setattr(stacker_git, "git", _interrupt_checkout) 103 + 104 + with pytest.raises(KeyboardInterrupt): 105 + ops_slot.acquire(pm_env, pooldb, repo_name, "feature-irrelevant") 106 + 107 + after_free = {s.uuid for s in slot_mod.free_slots(pm_env, pooldb, repo_name)} 108 + assert after_free == before_free, ( 109 + "interrupted acquire must not leak a pool claim: every slot tried got released" 110 + ) 111 + 112 + 75 113 def test_acquire_of_branch_held_elsewhere_releases_claim( 76 114 pm_env: Paths, 77 115 stacker_repo: tuple[str, Path],

+100

tests/stacker/test_pr_body_rendering.py

··· 254 254 assert "[[Files changed](" in body_b 255 255 256 256 257 + def test_repo_pr_files_url_uses_current_head_after_local_commit( 258 + service: StackerService, 259 + backend: RecordingPRBackend, 260 + stacker_repo: tuple[str, Path], 261 + three_slots: list[slot_mod.Slot], 262 + monkeypatch: pytest.MonkeyPatch, 263 + ) -> None: 264 + """Regression: `Files changed` head SHA must reflect the pushed HEAD, 265 + not the stale `last_clean_head` written at init time. 266 + 267 + Real universe PRs (#1845320, #1845321) rendered 268 + `/files/<sha>..<sha>` — an empty diff — because after `pm stacker init` 269 + seeded `last_clean_head = parent_head`, a follow-up local commit 270 + advanced HEAD but `pm stacker pp` never refreshed `last_clean_head` 271 + in the DB before rendering the stack block. 272 + """ 273 + repo_name, _leaf, _slot_a, slot_b = _two_branch_stack( 274 + service, 275 + stacker_repo, 276 + three_slots, 277 + monkeypatch, 278 + ) 279 + _set_config(service, repo_name, "repo-pr") 280 + 281 + # Advance feature-b's HEAD past the value `init_adopt_branch` recorded 282 + # in `last_clean_head`. This is the user-perspective scenario: branch 283 + # was tracked, then more local commits landed before pp. 284 + (slot_b.path / "feature-b-extra.txt").write_text("more\n") 285 + stacker_git.git(slot_b.path, "add", "feature-b-extra.txt") 286 + stacker_git.git( 287 + slot_b.path, 288 + "-c", "user.email=t@e.com", "-c", "user.name=t", 289 + "commit", "-m", "B: extra local commit", 290 + ) 291 + actual_head_b = stacker_git.rev_parse(slot_b.path, "HEAD") 292 + 293 + service.push(SelectorTarget(repo_name=repo_name, branch="feature-b"), PushOptions(draft=True)) 294 + 295 + final = {r.number: b for (r, b) in backend.edited if b is not None} 296 + body_b = final[2] 297 + match = re.search(r"/pull/2/files/([0-9a-f]+)\.\.([0-9a-f]+)", body_b) 298 + assert match is not None, f"Files-changed URL missing from body: {body_b!r}" 299 + base_sha, head_sha = match.group(1), match.group(2) 300 + assert head_sha == actual_head_b, ( 301 + f"head SHA in stack block ({head_sha}) does not match feature-b's " 302 + f"current HEAD ({actual_head_b}); last_clean_head was not refreshed " 303 + f"after the local commit" 304 + ) 305 + assert base_sha != head_sha, ( 306 + f"base..head collapsed to {base_sha}..{head_sha} (empty diff)" 307 + ) 308 + 309 + 310 + def test_repo_pr_root_branch_files_link_omits_range( 311 + service: StackerService, 312 + backend: RecordingPRBackend, 313 + stacker_repo: tuple[str, Path], 314 + three_slots: list[slot_mod.Slot], 315 + monkeypatch: pytest.MonkeyPatch, 316 + ) -> None: 317 + """Regression: branches whose parent is the trunk must render 318 + `<pr>/files` (no range) — the base SHA is a trunk commit that lives 319 + outside the PR's commit graph, so GitHub 404s a `/files/<base>..<head>` 320 + URL. Mirrors universe gitstack's `get_diff_link`, which suppresses 321 + the range when `parent == default_branch`. 322 + 323 + Real universe PR #1845320 (stack/stacker-test-a, parent=master) 324 + surfaced this: GitHub returned 404 even after we fixed the head SHA 325 + staleness — the URL itself was structurally wrong for a root branch. 326 + """ 327 + repo_name, _leaf, _slot_a, _slot_b = _two_branch_stack( 328 + service, 329 + stacker_repo, 330 + three_slots, 331 + monkeypatch, 332 + ) 333 + _set_config(service, repo_name, "repo-pr") 334 + 335 + service.push(SelectorTarget(repo_name=repo_name, branch="feature-b"), PushOptions(draft=True)) 336 + 337 + final = {r.number: b for (r, b) in backend.edited if b is not None} 338 + body_a = final[1] 339 + # feature-a's self-line in its own stack block. Should link to /files, 340 + # not /files/<sha>..<sha>. 341 + assert re.search( 342 + r"\[\*\*feature-a\*\*\]\(https://github\.com/acme/widgets/pull/1\) " 343 + r"\[\[Files changed\]\(https://github\.com/acme/widgets/pull/1/files\)\]", 344 + body_a, 345 + ), f"feature-a (root, parent=trunk) must render /files without a range: {body_a!r}" 346 + 347 + # The body of PR B should also render feature-a's ancestor row without 348 + # a range, since the rule is per-branch (depends on its parent). 349 + body_b = final[2] 350 + assert re.search( 351 + r"\[feature-a\]\(https://github\.com/acme/widgets/pull/1\) " 352 + r"\[\[Files changed\]\(https://github\.com/acme/widgets/pull/1/files\)\]", 353 + body_b, 354 + ), f"feature-a row in PR B must also use /files without a range: {body_b!r}" 355 + 356 + 257 357 def test_pr_pr_mode_stack_block_omits_compare_link( 258 358 service: StackerService, 259 359 backend: RecordingPRBackend,

+109 -1

tests/stacker/test_push.py

··· 6 6 import pytest 7 7 8 8 from project_manager.paths import Paths 9 + from project_manager.pool import slot as slot_mod 10 + from project_manager.pool.db import OwnerKind, PoolDB 9 11 from project_manager.stacker import git as stacker_git 10 12 from project_manager.stacker.db import StackerDB 11 13 from project_manager.stacker.models import ( 14 + ParentLocator, 12 15 PushOptions, 13 16 ScopeSpec, 14 17 SelectorTarget, 18 + WorktreeInit, 15 19 ) 16 20 from project_manager.stacker.ops import worktree as ops_worktree 17 21 from project_manager.stacker.service import StackerService 18 22 19 - from .conftest import TrackedStack 23 + from .conftest import TrackedStack, commit_file 20 24 from .fakes import RecordingPRBackend 21 25 22 26 ··· 154 158 target = SelectorTarget(repo_name=tracked_stack.repo_name, branch="b") 155 159 with pytest.raises(stacker_git.GitError, match="mutually exclusive"): 156 160 push_service.push(target, PushOptions(draft=True, publish=True)) 161 + 162 + 163 + def _setup_fake_upstream(slot_path: Path, branch: str) -> None: 164 + stacker_git.git(slot_path, "remote", "add", "origin-fake", 165 + "git@github.com:acme/widgets.git", check=False) 166 + stacker_git.git(slot_path, "config", f"branch.{branch}.remote", "origin-fake") 167 + stacker_git.git(slot_path, "config", f"branch.{branch}.merge", 168 + f"refs/heads/{branch}") 169 + 170 + 171 + def _seed_unattached_branch( 172 + pm_env: Paths, 173 + stacker_repo: tuple[str, Path], 174 + init_slot: slot_mod.Slot, 175 + backend: RecordingPRBackend, 176 + branch: str, 177 + ) -> StackerService: 178 + """Track `branch` off `main` in `init_slot`, then detach so the slot is 179 + free of branch-bindings — `locate_worktree` returns None for `branch`. 180 + 181 + Mirrors the production "user committed and ran pp from a different 182 + cwd" path: the branch exists locally as a ref but no worktree has it 183 + checked out, so push must mint an ops slot to operate on it. 184 + """ 185 + repo_name, _ = stacker_repo 186 + svc = StackerService(StackerDB(pm_env.stacker_db()), pm_env, pr_backend=backend) 187 + svc.db.set_config(repo_name, "pr.mode", "repo-pr") 188 + svc.db.set_config(repo_name, "pr.trunk", "main") 189 + svc.init_new_branch(WorktreeInit( 190 + repo_name=repo_name, worktree_path=init_slot.path, branch=branch, 191 + parent=ParentLocator(repo_name=repo_name, branch="main"), 192 + )) 193 + commit_file(init_slot.path, f"{branch}.txt", f"{branch}\n", f"{branch}: first commit") 194 + _setup_fake_upstream(init_slot.path, branch) 195 + stacker_git.git(init_slot.path, "checkout", "--detach", "HEAD") 196 + return svc 197 + 198 + 199 + def test_push_releases_target_slot_after_clean_completion( 200 + pm_env: Paths, 201 + backend: RecordingPRBackend, 202 + stacker_repo: tuple[str, Path], 203 + three_slots: list[slot_mod.Slot], 204 + monkeypatch: pytest.MonkeyPatch, 205 + ) -> None: 206 + """Regression: stacker-owned slots must not leak across pushes. 207 + 208 + Real workflow: `pm stacker push --branch X` minted an ops slot for X, 209 + held it on completion, and the next push of an unrelated branch 210 + waited indefinitely for that slot to free. The hold was unconditional 211 + — even when the worktree was clean, no work was in progress, and the 212 + user wasn't `cd`'d into it. Release stacker-owned slots after a 213 + successful push when there's nothing to preserve. 214 + """ 215 + repo_name, _repo_path = stacker_repo 216 + monkeypatch.setattr(ops_worktree, "run_single_pp", lambda *_a, **_kw: True) 217 + svc = _seed_unattached_branch(pm_env, stacker_repo, three_slots[0], backend, "feature-x") 218 + 219 + pooldb = PoolDB(pm_env.pool_db()) 220 + svc.push(SelectorTarget(repo_name=repo_name, branch="feature-x"), PushOptions(draft=True)) 221 + 222 + stacker_owned = [ 223 + uuid for repo, uuid, owner in pooldb.list_owned(repo_name) 224 + if owner.kind == OwnerKind.STACKER and repo == repo_name 225 + ] 226 + assert stacker_owned == [], ( 227 + f"clean push left a stacker-owned slot leaked: {stacker_owned}" 228 + ) 229 + 230 + 231 + def test_push_holds_target_slot_when_worktree_dirty( 232 + pm_env: Paths, 233 + backend: RecordingPRBackend, 234 + stacker_repo: tuple[str, Path], 235 + three_slots: list[slot_mod.Slot], 236 + monkeypatch: pytest.MonkeyPatch, 237 + ) -> None: 238 + """Dirty worktrees retain their stacker slot — releasing would detach 239 + HEAD and silently throw away the user's uncommitted work. 240 + 241 + Tracked changes (modifications to committed files) are the trigger. 242 + `git status -uno` is the same predicate stacker uses elsewhere. 243 + """ 244 + repo_name, _repo_path = stacker_repo 245 + monkeypatch.setattr(ops_worktree, "run_single_pp", lambda *_a, **_kw: True) 246 + svc = _seed_unattached_branch(pm_env, stacker_repo, three_slots[0], backend, "feature-y") 247 + 248 + # Inject dirtiness right before the slot-release decision: stub 249 + # `has_tracked_changes` so push sees the worktree as having unsaved 250 + # work. (Writing files into the slot path the test doesn't know in 251 + # advance is brittle; this is the cleanest way to control the 252 + # predicate stacker actually consults.) 253 + monkeypatch.setattr(stacker_git, "has_tracked_changes", lambda _path: True) 254 + 255 + pooldb = PoolDB(pm_env.pool_db()) 256 + svc.push(SelectorTarget(repo_name=repo_name, branch="feature-y"), PushOptions(draft=True)) 257 + 258 + stacker_owned = [ 259 + uuid for repo, uuid, owner in pooldb.list_owned(repo_name) 260 + if owner.kind == OwnerKind.STACKER and repo == repo_name 261 + ] 262 + assert len(stacker_owned) == 1, ( 263 + f"dirty worktree should keep the stacker slot held: {stacker_owned}" 264 + ) 157 265 158 266 159 267 def test_push_empty_scope_when_target_has_no_lineage(

+1

tests/stacker/test_stack_block.py

··· 71 71 pr_map=pr_map, 72 72 config=_config(mode), 73 73 current_repo=gh.RepoInfo(name_with_owner="acme/widgets", owner="acme", name="widgets"), 74 + live_heads={}, 74 75 ) 75 76 return ctx, child 76 77

+36 -3

tests/test_check.py

··· 109 109 110 110 111 111 def test_ops_owned_slot_is_classified(pm_env: Paths) -> None: 112 - _mk_pool(pm_env, "foo", ["a"]) 112 + """A stacker ops claim mid-cherry-pick reports OPS_OWNED and survives --fix. 113 + 114 + `CHERRY_PICK_HEAD` is the canonical "paused, resumable" signal — a 115 + `pm stacker continue` is on the user's roadmap, so we mustn't release. 116 + """ 117 + [slot_obj] = git_pool(pm_env, "foo", n=1) 113 118 pooldb = PoolDB(pm_env.pool_db()) 114 - pooldb.claim("foo", "a", OWNER_STACKER_OPS) 119 + pooldb.claim(slot_obj.repo, slot_obj.uuid, OWNER_STACKER_OPS) 120 + # Mark the worktree as resumable: a real `CHERRY_PICK_HEAD` under the 121 + # slot's per-worktree git dir. Linked worktrees keep their git dir 122 + # under `.git/worktrees/<uuid>/` in the main repo, with a `gitdir:` 123 + # pointer in the slot's `.git` file — resolve through it. 124 + git_dir = slot_obj.path / ".git" 125 + if git_dir.is_file(): 126 + gitdir_line = git_dir.read_text().splitlines()[0] 127 + real_git_dir = (slot_obj.path / gitdir_line.removeprefix("gitdir: ").strip()).resolve() 128 + else: 129 + real_git_dir = git_dir 130 + (real_git_dir / "CHERRY_PICK_HEAD").write_text("0" * 40 + "\n") 115 131 116 132 findings = check.check(pm_env) 117 133 assert _kinds(findings) == [check.Kind.OPS_OWNED] 118 134 assert check.fix(pm_env, findings) == 0 119 - assert pooldb.get_owner("foo", "a") == OWNER_STACKER_OPS 135 + assert pooldb.get_owner(slot_obj.repo, slot_obj.uuid) == OWNER_STACKER_OPS 136 + 137 + 138 + def test_stale_ops_slot_is_classified_and_fixed(pm_env: Paths) -> None: 139 + """A stacker ops claim on a clean worktree is a leaked claim. 140 + 141 + Common shape: a previous `pm stacker push` finished cleanly but 142 + didn't release (pre-fix behavior). `pm check --fix` must hand the 143 + slot back to the pool. 144 + """ 145 + [slot_obj] = git_pool(pm_env, "foo", n=1) 146 + pooldb = PoolDB(pm_env.pool_db()) 147 + pooldb.claim(slot_obj.repo, slot_obj.uuid, OWNER_STACKER_OPS) 148 + 149 + findings = check.check(pm_env) 150 + assert _kinds(findings) == [check.Kind.STALE_OPS] 151 + assert check.fix(pm_env, findings) == 1 152 + assert pooldb.get_owner(slot_obj.repo, slot_obj.uuid) is None

Configure Feed

Configure Feed