Handle a stale completed branch when creating an ODW addendum worktree (#42)#52
Open
pandalump wants to merge 2 commits into
Open
Handle a stale completed branch when creating an ODW addendum worktree (#42)#52pandalump wants to merge 2 commits into
pandalump wants to merge 2 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @pandalump, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughThe ODW workflow now detects deterministic addendum branch collisions, reclaims merged branches with clean worktrees, and fails closed for unsafe states. A pure disposition helper, tests, fixtures, ADR, roadmap note, and mirrored JavaScript workflow support the change. ChangesWorktree collision handling
Sequence Diagram(s)sequenceDiagram
participant ODWWorkflow
participant GitRepository
participant Worktree
ODWWorkflow->>GitRepository: Inspect deterministic branch and merge status
GitRepository-->>ODWWorkflow: Return collision facts
ODWWorkflow->>Worktree: Create or reclaim the worktree
Worktree-->>ODWWorkflow: Return resolved HEAD
ODWWorkflow->>GitRepository: Validate HEAD against origin/BASE
Possibly related issues
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 6 warnings, 1 inconclusive)
✅ Passed checks (10 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)workflows/df12-build-odw.jsFile contains syntax errors that prevent linting: Line 3954: Illegal return statement outside of a function Comment |
added 2 commits
July 17, 2026 23:36
`createWorktree` derives a deterministic branch name (`roadmap-<id>[-addendum]`, no unique suffix) and runs `git worktree add -b <branch>` with no pre-check. When a completed addendum round from a prior run leaves its branch behind (worktree hoovered, branch never deleted), the next selection of that task re-derives the same name, the add throws, and `runTask` turns it into a `failed`/`worktree` halt. A harmless, fully-merged leftover therefore blocks real addendum work (issue #42). Detect the collision inside `createWorktree` and decide its fate through a pure, unit-tested helper (`decideWorktreeDisposition`) kept out of the verified recovery-decision twin. The rule is reclaim-when-safe, fail-closed otherwise: reclaim only when the branch tip is a merged ancestor of `origin/BASE` and any worktree on it is clean; fail closed (preserving today's halt) on unmerged commits or a dirty worktree. `candidateRoadmapComplete` only corroborates a reclaim and never licenses discarding work. Reclaim uses sandbox-permitted git commands only (`worktree prune`, `branch -f`, plain `worktree add`, `reset --hard origin/BASE`), then re-runs the existing HEAD-versus-base verification. The roadmap read is lazy, taken only on the rare collision path. Record the scoped destructive-operation policy in ADR 003 and against roadmap item 4.2.1: on-demand reclaim of one deterministic name is an exception; the general `discard`-branch sweeper stays deferred. Cover the decision table exhaustively with a pure unit test and model the real-git leftover state with a `withStaleAddendumBranch` fixture option. Regenerate the committed artefact.
The collision-detection and reclaim wiring added for issue #42 pushed createWorktree over the CodeScene Complex Method threshold (cc 11). Extract the fresh-create-or-reclaim disposition into provisionWorktreeForBranch so createWorktree stays a straight-line orchestration that delegates, and pass the branch and worktree path as a WorktreeTarget object rather than two loose primitives to avoid tripping the module's Primitive Obsession rule. Extract the duplicated exec-failure formatting shared by createWorktree and readRoadmapForSelection into execFailureDetail in exec.ts, its natural home alongside execFileText which throws these errors. This removes the last of createWorktree's excess complexity and de-duplicates the detail-flattening logic. Behaviour is unchanged: the reclaim-when-safe and fail-closed decisions, the HEAD verification, and every returned note are identical. The file's code-health score rises from 5.58 to 5.74 and createWorktree no longer trips any smell.
leynos
force-pushed
the
issue-42-addendum-worktree-collision
branch
from
July 17, 2026 22:36
5bdb862 to
e7cbb88
Compare
leynos
marked this pull request as ready for review
July 22, 2026 22:25
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7cbb88d4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reason: 'pre-existing branch carries commits not merged into the base; refusing to discard unmerged work', | ||
| } | ||
| } | ||
| if (facts.worktreeExists && facts.worktreeDirty) { |
There was a problem hiding this comment.
When another workflow run has just provisioned this deterministic branch, its registered worktree is initially clean and its tip is still an ancestor of origin/BASE, so it passes this check and falls through to reclaim. reclaimStaleWorktree then resets and returns that same path, allowing both runs to dispatch agents into one worktree; subsequent edits, commits, and integration can race or be discarded by the reset. A clean registered worktree is not proof that it is stale, so reclamation needs an ownership/liveness check or must refuse branches that still have a live worktree.
Useful? React with 👍 / 👎.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workflows/df12-build-odw/main.ts`:
- Around line 375-438: Add the established exec timeout option to every new git
invocation in worktreeBranchExists, inspectWorktreeCollision, and
reclaimStaleWorktree, including merge-base, worktree list, status, prune, reset,
branch, and worktree add. Also add a higher-level timeout guard around
createWorktree so the workflow cannot wait indefinitely when git hangs, reusing
the existing timeout configuration rather than introducing unrelated changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48f4cdb2-1938-4a7f-9b3f-cafc3ec4becb
📒 Files selected for processing (8)
docs/adr-003-reclaim-stale-addendum-worktree-branches.mddocs/roadmap.mdsrc/workflows/df12-build-odw/exec.tssrc/workflows/df12-build-odw/main.tssrc/workflows/df12-build-odw/worktree-collision.tstests/fixtures/recovery-repo.mjstests/modules/worktree-collision.test.tsworkflows/df12-build-odw.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/nixie(auto-detected)
Comment on lines
+375
to
+438
| async function worktreeBranchExists(branch: string): Promise<boolean> { | ||
| const probe = await execFileStatus('git', ['rev-parse', '--verify', '--quiet', `refs/heads/${branch}`]) | ||
| return probe.ok | ||
| } | ||
|
|
||
| // Read the git facts a collision disposition is decided from. Only invoked on | ||
| // the rare path where the deterministic branch already exists, so the roadmap | ||
| // read (a corroborating signal only) stays off the common no-collision path. | ||
| async function inspectWorktreeCollision(branch: string): Promise<WorktreeCollision> { | ||
| const merged = await execFileStatus('git', ['merge-base', '--is-ancestor', branch, `origin/${BASE}`]) | ||
| const list = await execFileStatus('git', ['worktree', 'list', '--porcelain']) | ||
| const entry = list.ok ? parseWorktreeList(list.stdout).find((candidate) => candidate.branch === branch) : undefined | ||
| const existingWorktreePath = entry?.worktreePath || '' | ||
| let worktreeDirty = false | ||
| if (existingWorktreePath) { | ||
| const status = await execFileStatus('git', ['-C', existingWorktreePath, 'status', '--porcelain']) | ||
| // A worktree the host cannot probe is treated as dirty: fail closed rather | ||
| // than reset a tree whose cleanliness is unverifiable. | ||
| worktreeDirty = !status.ok || status.stdout.trim() !== '' | ||
| } | ||
| let candidateComplete = false | ||
| const parsed = branchToRoadmapId(branch) | ||
| if (parsed) { | ||
| try { | ||
| const roadmap = await readRoadmapForSelection() | ||
| const roadmapTask = roadmapTaskIndex(roadmap.text).get(parsed.id) | ||
| if (roadmapTask) candidateComplete = candidateRoadmapComplete(roadmapTask, parsed.isAddendum) | ||
| } catch { | ||
| // Roadmap completeness only corroborates a reclaim; a read failure must | ||
| // never upgrade a fail to a reclaim, so it simply stays false here. | ||
| } | ||
| } | ||
| return { | ||
| facts: { | ||
| branchExists: true, | ||
| branchMergedIntoBase: merged.ok, | ||
| worktreeExists: Boolean(existingWorktreePath), | ||
| worktreeDirty, | ||
| candidateRoadmapComplete: candidateComplete, | ||
| }, | ||
| existingWorktreePath, | ||
| } | ||
| } | ||
|
|
||
| // Reclaim a stale, fully-merged branch onto origin/BASE using only | ||
| // sandbox-permitted git commands (see the supervisor skill's environment | ||
| // safety-net constraints — no `worktree remove --force`, no `branch -D`): | ||
| // reset an existing clean worktree in place, or force the orphaned branch to | ||
| // the base and attach a fresh worktree at the deterministic path. Returns the | ||
| // reclaimed worktree path so the caller can verify its HEAD. | ||
| async function reclaimStaleWorktree( | ||
| branch: string, | ||
| worktreePath: string, | ||
| existingWorktreePath: string, | ||
| ): Promise<string> { | ||
| await execFileStatus('git', ['worktree', 'prune']) | ||
| if (existingWorktreePath) { | ||
| await execFileText('git', ['-C', existingWorktreePath, 'reset', '--hard', `origin/${BASE}`]) | ||
| return existingWorktreePath | ||
| } | ||
| await execFileText('git', ['branch', '-f', branch, `origin/${BASE}`]) | ||
| await execFileText('git', ['worktree', 'add', worktreePath, branch]) | ||
| return worktreePath | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'timeoutMs' src/workflows/df12-build-odwRepository: leynos/df12-build
Length of output: 681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- exec.ts ---'
cat -n src/workflows/df12-build-odw/exec.ts
echo
echo '--- timeout callsites in main.ts ---'
rg -n 'timeoutMs|execFile(Status|Text)|createWorktree|worktreeBranchExists|inspectWorktreeCollision|reclaimStaleWorktree' src/workflows/df12-build-odw/main.tsRepository: leynos/df12-build
Length of output: 7155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- execFileText/execFileStatus timeout usages ---'
rg -n "execFile(Text|Status)\([^)]*timeoutMs|timeoutMs\s*:" src/workflows/df12-build-odw
echo
echo '--- createWorktree and nearby control flow ---'
sed -n '477,530p' src/workflows/df12-build-odw/main.ts
echo
echo '--- top-level loop / orchestration ---'
sed -n '880,950p' src/workflows/df12-build-odw/main.tsRepository: leynos/df12-build
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- createWorktree and nearby control flow ---'
sed -n '477,530p' src/workflows/df12-build-odw/main.ts
echo
echo '--- top-level orchestration ---'
sed -n '880,950p' src/workflows/df12-build-odw/main.tsRepository: leynos/df12-build
Length of output: 3927
Add a timeout to the new git calls. exec.ts already supports timeoutMs, but these merge-base, worktree list, status, worktree prune, reset --hard, branch -f, and worktree add calls do not use it, and createWorktree has no higher-level guard. A hung git process can stall the workflow indefinitely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflows/df12-build-odw/main.ts` around lines 375 - 438, Add the
established exec timeout option to every new git invocation in
worktreeBranchExists, inspectWorktreeCollision, and reclaimStaleWorktree,
including merge-base, worktree list, status, prune, reset, branch, and worktree
add. Also add a higher-level timeout guard around createWorktree so the workflow
cannot wait indefinitely when git hangs, reusing the existing timeout
configuration rather than introducing unrelated changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Normal addendum selection halted when
git worktree add -b roadmap-<id>-addendumcollided with a stale, already-completed addendumbranch of the same deterministic name left behind by a prior run. This makes
createWorktreedetect the collision and reclaim the leftover when it is safe,instead of failing the run.
What changed
src/workflows/df12-build-odw/worktree-collision.ts(new): a pure,I/O-free
decideWorktreeDispositionhelper that turns host-observed gitfacts into
create|reclaim|fail. Kept out of the formally-verifiedrecovery-decision twin. Rule: reclaim only when the branch tip is a merged
ancestor of
origin/BASEand any worktree on it is clean; fail closed onunmerged commits or a dirty worktree.
candidateRoadmapCompleteonlycorroborates a reclaim and never licenses discarding work.
main.ts:createWorktreenow probes for the existing branch(non-throwing
rev-parse --verify), gathers collision facts only on thatrare path (merged-ancestor, worktree presence/cleanliness, lazy roadmap
completeness), and executes the disposition with sandbox-permitted git
commands only (
worktree prune,branch -f, plainworktree add,reset --hard origin/BASE), re-running the existing HEAD-versus-base verification.The no-collision path is unchanged. A
faildisposition preserves today'sfailed/worktreehalt semantics.roadmap item 4.2.1 gets a note that on-demand reclaim of one deterministic
name is an exception while the general
discard-branch sweeper staysdeferred.
(
tests/modules/worktree-collision.test.ts); awithStaleAddendumBranchoption added to
tests/fixtures/recovery-repo.mjsto model the real-gitmerged-and-clean leftover.
workflows/df12-build-odw.js.Testing
make workflow-buildsucceeds. Commit gates (check-fmt,lint,typecheck,module and artefact suites,
workflow-freshness,verify-modules) are pendingand will be run by the orchestrator.
Deviations from the CodeRabbit plan
createWorktreeinline inmain.ts(Phase 2), as the plan directed; thealready-present-but-unused imports (
parseWorktreeList,candidateRoadmapComplete,roadmapTaskIndex) confirmed this intent.main.tsisnot host-importable (top-level ambient
args), socreateWorktreecannot bedriven from a
bun/node --testharness without the full ODW runtime. Thepure disposition unit test is the primary logic guard (as the plan itself
labels it), and the
withStaleAddendumBranchfixture models the git state forany future runtime-level coverage.
Closes #42