fix(engine): refresh pooled worktree ownership - #2348
Conversation
Reinstall the task identity guard after a pooled checkout switches branches so stale ownership metadata cannot block the new task's commits. Fusion-Task-Id: FN-8400
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPooled worktree acquisition now refreshes task identity and attribution guard metadata before reuse. Unit and real-git tests verify reassigned worktrees reject stale-owner commits and accept commits for the new task. A patch changeset documents the fix. ChangesPooled worktree identity refresh
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 refreshes ownership metadata when a pooled worktree is assigned to a new task. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "test(FN-8400): tighten ownership guard a..." | Re-trigger Greptile |
Reproduce the stale-owner rejection in a recycled linked worktree, then prove reassignment refreshes the marker and permits the new task commit. Fusion-Task-Id: FN-8400
Addressed in 532b39c. The new real-Git regression reproduces a pooled worktree carrying the previous task marker, asserts that the new task commit is rejected, refreshes ownership, and then asserts the same commit path succeeds. The focused pool and identity-guard suite passes 81/81. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/src/__tests__/reliability-interactions/precommit-identity-guard.real-git.test.ts (1)
37-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a more targeted assertion and simplify path resolution.
Two minor improvements to consider:
- Since the identity guard hook explicitly exits with
1, you can use.toBe(1)instead of.not.toBe(0)to better align with the coding guidelines' preference for "targeted assertions".- Node's
path.resolveautomatically treats absolute paths as the new root, making theisAbsoluteternary check redundant.💡 Proposed tweaks
- expect(staleOwnerCommit.status).not.toBe(0); + expect(staleOwnerCommit.status).toBe(1); expect(`${staleOwnerCommit.stderr}${staleOwnerCommit.stdout}`).toContain( "fusion: refusing commit — worktree owns FN-OLD but HEAD is fusion/fn-new", ); await installTaskWorktreeIdentityGuard({ worktreePath: worktreeDir, taskId: "FN-NEW" }); const refreshedOwnerCommit = spawnSync("git", ["commit", "-m", "fix(FN-NEW): accepted after owner refresh"], { cwd: worktreeDir, encoding: "utf-8", }); expect(refreshedOwnerCommit.status).toBe(0); const taskIdPathRaw = git(worktreeDir, "git rev-parse --git-path fusion-task-id"); - const taskIdPath = isAbsolute(taskIdPathRaw) ? taskIdPathRaw : resolve(worktreeDir, taskIdPathRaw); + const taskIdPath = resolve(worktreeDir, taskIdPathRaw);🤖 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/engine/src/__tests__/reliability-interactions/precommit-identity-guard.real-git.test.ts` around lines 37 - 51, Update the staleOwnerCommit assertion to expect an exit status of exactly 1, matching the identity guard’s contract. Simplify taskIdPath construction by passing taskIdPathRaw directly to resolve, removing the isAbsolute check and any now-unused import.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@packages/engine/src/__tests__/reliability-interactions/precommit-identity-guard.real-git.test.ts`:
- Around line 37-51: Update the staleOwnerCommit assertion to expect an exit
status of exactly 1, matching the identity guard’s contract. Simplify taskIdPath
construction by passing taskIdPathRaw directly to resolve, removing the
isAbsolute check and any now-unused import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7b60b7f-a3e0-48e4-b0e9-5573786c781f
📒 Files selected for processing (1)
packages/engine/src/__tests__/reliability-interactions/precommit-identity-guard.real-git.test.ts
Assert the exact refusal status and use path resolution directly in the pooled worktree symptom test. Fusion-Task-Id: FN-8400
Fixed in e85d217: the stale-owner commit now asserts exit status 1, and the task marker path uses |
Summary
Pooled worktrees no longer retain the previous task's identity after reassignment, preventing valid commits from being rejected when the checked-out branch and stale ownership marker disagree. Fusion now refreshes the identity guard immediately after the pool prepares the new branch and before the checkout is exposed to the task, preserving the cross-task commit safety check. Regression coverage exercises the pooled acquisition path and confirms the new task identity is installed.
Related: FN-8400
Validation
@fusion/enginetypecheck passed.Summary by CodeRabbit
Bug Fixes
Tests