feat(worktrees): task-pinned worktrees under worktreeNaming "task-id" - #2233
Conversation
When worktreeNaming is "task-id", each task is pinned to one derivable directory <worktreesDir>/<lowercased-task-id> for its whole lifecycle. Acquisition runs derive → validate → reuse-or-recreate at that same path (never suffixed); a disagreeing task.worktree cache self-corrects and emits worktree:pin-rederived. The recycle pool is untouched for random/task-title. Task pinning and recycleWorktrees are mutually exclusive: enabling both is rejected at the settings-write boundary (assertWorktreeNamingRecycleExclusive, enforced in store.updateSettings + dashboard PUT /settings), and the runtime gates pinned mode on !recycleWorktrees so a legacy config carrying both degrades safely to recycling. Worktrunk-managed layouts bypass pinning. - new packages/engine/src/worktree-pinning.ts pure helpers - worktree-acquisition.ts pinned branch + branch-match reclaim-in-place - run-audit.ts worktree:pin-rederived type - core settings-validation mutual-exclusion validator + wiring - docs (settings-reference, architecture), types doc, changeset - unit + acquisition + settings-validation + route 400 tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesTask-pinned worktree behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Task
participant acquireTaskWorktree
participant WorktreePool
participant TaskStore
Task->>acquireTaskWorktree: request worktree
acquireTaskWorktree->>WorktreePool: derive and inspect task-pinned path
WorktreePool-->>acquireTaskWorktree: valid or stale directory
acquireTaskWorktree->>TaskStore: correct task.worktree metadata
acquireTaskWorktree->>WorktreePool: reuse or recreate same pinned path
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds task-pinned worktrees for Task ID naming. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "Address PR review feedback (#2233)" | Re-trigger Greptile |
- worktree-acquisition.ts (P1): pinned reclaim-in-place now fails loud when removeWorktree fails (e.g. active-session removal) instead of clearing sessionFile and recreating over the still-occupied path — preserves resume metadata and lets executor retry/self-healing recover. - WorktreesSection.tsx (P2): legacy-conflict escape hatch — when a stored config already has recycleWorktrees + worktreeNaming:"task-id", keep the recycle toggle enabled+checked (mirroring the runtime "recycling wins" backstop) so the operator can turn it off and unlock the naming select; never lock both. - settings-sections test: cover the legacy-conflict escape hatch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/src/routes/register-settings-memory-routes.ts (1)
764-772: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMap the backstop conflict error to a 400 status.
The explicit pre-check added above (lines 707-721) can be bypassed if a user sends
recycleWorktrees: nullto clear the project setting. In that scenario,isRecycleWorktreeNamingConflictevaluatesnull === trueasfalse, allowing the patch to proceed.When it proceeds, the
updateSettingsImplbackstop correctly evaluates thenulldeletion against the global fallback and safely throws an error if the fallback combination is invalid. However, because this catch block does not recognize the "mutually exclusive" error string, it will return a500 Internal Server Errorinstead of a clean400 Bad Request.Include the conflict message in this mapping to ensure a 400 status is reliably returned for all conflict edge cases.
♻️ Proposed fix to map the error and reduce duplication
} catch (err: unknown) { if (err instanceof ApiError) { throw err; } - const status = typeof (err instanceof Error ? err.message : String(err)) === "string" && ( - (err instanceof Error ? err.message : String(err)).includes("modelPresets") || (err instanceof Error ? err.message : String(err)).includes("must include both provider and modelId") - ) ? 400 : 500; - throw new ApiError(status, err instanceof Error ? err.message : String(err)); + const errorMessage = err instanceof Error ? err.message : String(err); + const status = typeof errorMessage === "string" && ( + errorMessage.includes("modelPresets") || + errorMessage.includes("must include both provider and modelId") || + errorMessage.includes("mutually exclusive") + ) ? 400 : 500; + throw new ApiError(status, errorMessage); }🤖 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 `@packages/dashboard/src/routes/register-settings-memory-routes.ts` around lines 764 - 772, Update the catch block’s status mapping around the register-settings route to classify the backstop “mutually exclusive” conflict error as 400, alongside the existing modelPresets and provider/modelId checks. Reuse a single normalized error-message value for the string conversion and conflict checks, while preserving ApiError passthrough and 500 status for unrelated errors.
🤖 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 `@packages/engine/src/__tests__/worktree-acquisition-pinned.test.ts`:
- Around line 5-7: Add a dated `FNXC:<Area-of-product>` heading in the comment
above the pinned-mode test seam explanation, using the required
`yyyy-MM-dd-hh:mm` format. Keep the existing rationale intact and make the
heading concisely identify the recorded testing decision.
In `@packages/engine/src/worktree-acquisition.ts`:
- Around line 174-188: Update pinnedWorktreeBranchMatches so failures from
canonicalizePath or getRegisteredWorktreeBranches propagate to the caller
instead of being caught and converted to false. Return false only when branch
enumeration succeeds and the matching worktree is absent or has a different
branch.
- Around line 542-548: Update the existing pinned-worktree reuse path in the
acquisition flow around classification.ok and branchMatches so that, when
adopting an orphaned worktree, it persists the task’s worktree path and resumed
branch metadata before returning reuseWarmWorktree. Preserve the current reuse
behavior for already-assigned tasks and ensure both task.worktree and
task.branch are restored.
---
Outside diff comments:
In `@packages/dashboard/src/routes/register-settings-memory-routes.ts`:
- Around line 764-772: Update the catch block’s status mapping around the
register-settings route to classify the backstop “mutually exclusive” conflict
error as 400, alongside the existing modelPresets and provider/modelId checks.
Reuse a single normalized error-message value for the string conversion and
conflict checks, while preserving ApiError passthrough and 500 status for
unrelated errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95cfe234-1270-4d10-9f44-fd2be32ff9f6
📒 Files selected for processing (18)
.changeset/task-pinned-worktrees.mddocs/architecture.mddocs/settings-reference.mdpackages/core/src/__tests__/worktree-naming-recycle-exclusive.test.tspackages/core/src/index.tspackages/core/src/settings-validation.tspackages/core/src/task-store/settings-ops.tspackages/core/src/types.tspackages/dashboard/app/__tests__/settings-sections.test.tsxpackages/dashboard/app/components/settings/sections/WorktreesSection.tsxpackages/dashboard/src/routes/__tests__/register-settings-memory-worktrunk.test.tspackages/dashboard/src/routes/register-settings-memory-routes.tspackages/engine/src/__tests__/worktree-acquisition-pinned.test.tspackages/engine/src/__tests__/worktree-pinning.test.tspackages/engine/src/run-audit.tspackages/engine/src/worktree-acquisition.tspackages/engine/src/worktree-pinning.tspackages/i18n/locales/en/app.json
- worktree-acquisition.ts pinnedWorktreeBranchMatches: reserve `false` for a proven branch mismatch. A transient `git worktree list` failure yields an empty enumeration (the helper swallows internally), which previously read as "foreign branch" and destructively reclaimed a valid warm worktree. Now throws (fail-safe) so acquisition retries with a fresh probe instead. - worktree-acquisition.ts warm reuse: persist worktree+branch when adopting an orphaned pinned dir (task.worktree null) so a successful acquisition never leaves the task unassigned; idempotent when the cache was already correct. - register-settings-memory-routes.ts: map the store's mutual-exclusion backstop error to 400 (not 500) for edge cases the route pre-check misses (e.g. a null-clear resolving to a conflicting fallback); dedupe the error-message read. - worktree-acquisition-pinned.test.ts: FNXC heading on the test-seam comment; cover orphan-adoption metadata persistence and the fail-safe probe throw. - worktrunk route test: cover the 400 backstop mapping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addressed in |
Summary
Adds task-pinned worktrees for
worktreeNaming: "task-id". Under task-id naming, a task is pinned to exactly one derivable directory<worktreesDir>/<lowercased-task-id>(e.g..worktrees/fn-7996) for its entire lifecycle — removing the ambiguity that let stale/foreigntask.worktreepointers strand a task (the FN-7996 shape).recycleWorktreesstays fully functional and is mutually exclusive with task-id pinning: the two can't be enabled together.Behavior
worktreeNaming: "task-id", recycling off):acquireTaskWorktreeruns derive → validate → reuse-or-recreate at the derived path — warm-reuse when the dir is a registered, usable worktree on the task's own branch; otherwise reclaim-in-place (removeWorktree+ recreate at the SAME path, never a sibling name). A disagreeingtask.worktreecache self-corrects and emits a newworktree:pin-rederivedaudit event, without consuming worktree-session retries. The recycle pool is never consulted in pinned mode.recycleWorktreesandworktreeNaming: "task-id"is rejected at the settings-write boundary — HTTP 400 atPUT /settings, and anErrorbackstop instore.updateSettingscovering the CLI and every other writer (assertWorktreeNamingRecycleExclusive). The runtime also gates pinned mode on!recycleWorktrees, so a legacy on-disk config carrying both degrades safely to recycling.random/task-titlenaming and the recycle pool (incl.merger.tsrelease) are unchanged; worktrunk-managed layouts bypass pinning.Acceptance criteria (from the plan)
<worktreesDir>/<task-id>on its own branchtask.worktreeself-corrects at next dispatch (worktree:pin-rederived) without consuming session retriesrecycleWorktrees: true|falseare byte-identical (existing pool tests pass unchanged)worktreeNamingtype doc); changeset (minor,feature); FNXC comments encode the invariantFiles
packages/engine/src/worktree-pinning.ts— new pure helpers (isTaskPinnedWorktreeNaming,pinnedWorktreePathForTask)packages/engine/src/worktree-acquisition.ts— pinned branch + branch-match reclaim-in-placepackages/engine/src/run-audit.ts—worktree:pin-rederivedaudit typepackages/core/src/settings-validation.ts(+index.ts,task-store/settings-ops.ts) — mutual-exclusion validator + wiringpackages/dashboard/src/routes/register-settings-memory-routes.ts— 400 on conflictpackages/dashboard/app/components/settings/sections/WorktreesSection.tsx(+packages/i18n/locales/en/app.json) — bidirectional UI exclusivitypackages/core/src/types.ts,docs/*,.changeset/*Verification
worktree-pinning(5) +worktree-acquisition-pinned(7); coreworktree-naming-recycle-exclusive(2); dashboard settings-route 400 (3) + WorktreesSection UI exclusivity (3)tsc --noEmitclean for@fusion/coreand@fusion/engine; changed source files clean; eslint cleanpnpm verify:fastPASS (build + scoped typecheck + boot smoke)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes