[codex] Fix PR worktree checkout materialization#4643
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
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:
📝 Walkthrough<review_stack_artifact_start> Adds PR branch materialization utilities, integrates them into workspace creation, covers behavior with unit and integration tests, and updates CLI/SDK/docs, desktop warnings, and planning docs.Defines PR branch types, synthetic/verified-ref helpers, OID resolution/verification, same-repo and synthetic fetch strategies, branch creation/configuration, safe-delete, and exports materializePrBranch / deleteMaterializedPrBranchIfSafe.range_939f80582a3c range_63305c6cef88 range_f8efb900a3be range_47ad63a5ea2d range_63a34aca2e2f range_a1f8fa31e451 range_59262bae7933 range_8fad53eec151 range_f84d6b574ca8 ```mermaid sequenceDiagram participant WorkspacesCreate participant FetchPrMetadata participant ExecGh participant MaterializePrBranch participant GitClient WorkspacesCreate->>FetchPrMetadata: request PR metadata (pr number) FetchPrMetadata->>ExecGh: gh pr view --json (headRefOid, headRepository, ...) ExecGh-->>FetchPrMetadata: PR metadata (headRefOid, headRepository) WorkspacesCreate->>MaterializePrBranch: materializePrBranch(pr metadata) MaterializePrBranch->>GitClient: fetch refs/heads/ or refs/pull//head -> verified ref GitClient-->>MaterializePrBranch: fetched ref OID MaterializePrBranch->>GitClient: rev-parse --verify ^{commit} GitClient-->>MaterializePrBranch: commit OID alt OID matches headRefOid MaterializePrBranch->>GitClient: create local branch, configure branch..remote/merge and pushRemote MaterializePrBranch-->>WorkspacesCreate: MaterializePrBranchResult (branch,startPoint,sourceKind,tracking,..) WorkspacesCreate->>GitClient: git worktree add else OID mismatch MaterializePrBranch-->>WorkspacesCreate: reject "did not match GitHub headRefOid" end ```Mocks Git to assert fetch/create/configure sequences for same-repo and cross-repo PRs; tests deleteMaterializedPrBranchIfSafe, adoption, abort-on-OID-mismatch, concurrency/adoption, and rollback error reporting.range_37e6426dbd8a range_091a95094dce range_70b484b28878 range_9a2c4fadeb85 range_d58482a2b98b range_115646e0d6e9 range_fe4f41ebf030 range_aaab1ac8a3ee range_0882130f61aa range_7a1f70e560e7Bun-based integration tests that create a bare remote and local repo, install a post-checkout hook that dirties tracked files, exercise old detached-flow failure and new materialize flow success, and test deleteMaterializedPrBranchIfSafe cleanup behavior.range_3cac2d34c345 range_0372b5907537 range_1f34cb997d11 range_cac156c618eb range_1b26ef006c61Injects ExecGh into fetchPrMetadata, replaces detached worktree + gh pr checkout with materializePrBranch + git worktree add, adds per-PR locks, tracks attempted worktree add and rolls back on failure, accumulates warnings, and attempts safe branch deletion when needed.range_319b81513a0d range_7c49c3568769 range_03c887363b22 range_2b594664f9b2 range_18e68ab288ea range_53656e7ff20dIntegration tests that stub execGh to allow only gh pr view, push PR head refs, run workspaces.create, assert pr checkout not called, verify persisted worktreePath, worktree HEAD, package-lock.json status, stale headRefOid failure, same-repo success/fallback, conflict path, and concurrency serialization.range_4b949fa0b73c range_b0b94d38d4e8 range_b3a7715edb57 range_84fffe11c856 range_dec9794a97be range_02cafd48485b range_41f819567762 range_506dca3e9a9bUpdate CLI option and command schema, CLI reference, MCP tool schema, SDK JSDoc, and skills docs to state the server checks out the verified PR head and derives the branch; extend workspaces.create response with warnings array and surface warnings to desktop.range_d9abf0a063ab range_b49bc10b46bc range_b64d656c3c21 range_5a991ceb76b4 range_9cc8e6822540 range_b6d4c5bb5fbd range_98172e24e06cPlanning doc describing the materialize-first v2 flow, failure modes of detached placeholder + gh pr checkout, implementation breakdown, test plan, rollback constraints, and current implementation status with follow-ups.range_7f012be442c7 range_cd0413831b7a range_547293c2cb9f range_870a04b4daad range_658fb4ebc539 range_388be5c9e7b3 range_0cc5f0f2493e range_7a7f41957632 range_047e86c181b8Adds toast wiring to surface server-side materialization warnings after workspace creation by iterating result.warnings and calling toast.warning for each.range_91e5db477922 range_a3ccc120f5cc🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Ready to review this PR? Stage has broken it down into 8 individual chapters for you: 
Chapters generated by Stage for commit 143decc on May 17, 2026 7:44pm UTC. |
Greptile SummaryThis PR replaces the old "detached placeholder worktree →
Confidence Score: 3/5Safe to merge for same-repo PRs; two correctness gaps in the cross-repo (synthetic-ref) path need addressing before the change is fully reliable. The core materialize-first approach is sound and well-tested. Two issues stand out: when
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts | New helper that fetches, verifies, and materializes a PR branch without a checkout. FETCH_HEAD is used as the start-point for the branch-create command after a separate verification step, creating a race window where a concurrent fetch can replace FETCH_HEAD with an unverified commit. |
| packages/host-service/src/trpc/router/workspaces/workspaces.ts | Replaces detached-worktree + gh pr checkout with materialize-first flow. Branch created by materializePrBranch is not cleaned up if the subsequent git worktree add fails, leaving an orphaned local branch in the repository. |
| packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts | Unit tests for materializePrBranch covering same-repo, cross-repo, and OID-mismatch cases; mocks are correctly aligned with the actual git command sequence. |
| packages/host-service/test/integration/pr-branch-materialize.integration.test.ts | Real-git integration test that proves the dirty-placeholder regression (post-checkout hook dirtying package-lock.json) no longer blocks PR workspace creation with the new flow. |
| packages/host-service/test/integration/workspace-create-pr.integration.test.ts | End-to-end integration test for the full workspaces.create PR path; verifies gh pr checkout is never called and the correct PR head is checked out. |
| packages/cli/src/commands/workspaces/create/command.ts | CLI flag description updated to reflect the new PR checkout mechanism; no logic changes. |
| packages/mcp-v2/src/tools/workspaces/create.ts | MCP tool description updated to match the new PR checkout behavior; no logic changes. |
| packages/sdk/src/resources/workspaces.ts | SDK JSDoc for WorkspaceCreateParams.pr updated; no logic changes. |
| plans/done/20260516-pr-worktree-checkout-flow.md | Design doc explaining the old flow's failure mode, comparisons with Worktrunk/T3Code/gh CLI, and the recommended materialize-first approach with remaining follow-ups. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/src/trpc/router/workspaces/workspaces.ts:639-668
**Orphaned local branch on `git worktree add` failure**
When `materializePrBranch` creates a new local branch (`createdBranch: true`) but the subsequent `git worktree add` throws, `rollbackWorktree()` is called — but `rollbackWorktree` only removes the worktree via `git worktree remove`, not the newly created branch. The plan document explicitly calls this out: "If `git worktree add` fails after a new branch was created only for this operation, delete that branch if and only if it still points at the verified PR head and is not checked out anywhere." That cleanup is absent here. Concretely, if the host filesystem is full or the branch is somehow already registered in another worktree at the time of `git worktree add`, the branch named e.g. `contributor/feature-x` will be left in the repo forever unless the user manually deletes it.
### Issue 2 of 2
packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts:113-138
**`FETCH_HEAD` race condition in concurrent same-repo fetches**
`FETCH_HEAD` is a single repo-level file overwritten by every `git fetch` that lacks a named destination ref. `fetchSyntheticPrBranch` fetches `refs/pull/N/head` into `FETCH_HEAD`, immediately verifies it against `headRefOid`, then returns `startPoint: "FETCH_HEAD"`. Back in `materializePrBranch`, the `git branch --no-track -- <branch> FETCH_HEAD` runs as a separate git command. If any other fetch (a second concurrent PR workspace creation, a background remote update, or a post-checkout hook) runs in the same clone between those two calls, `FETCH_HEAD` will point to a different, unverified commit, and the local branch will be created from the wrong OID — silently passing the OID guard. The fix is to fetch into a named ref (e.g., `refs/superset/pr-fetch/<number>`) instead of relying on `FETCH_HEAD`.
Reviews (1): Last reviewed commit: "fix PR worktree checkout materialization" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let attemptedWorktreeAdd = false; | ||
| try { | ||
| await git.raw(["worktree", "add", "--detach", worktreePath]); | ||
| const materialized = await materializePrBranch({ | ||
| git, | ||
| branch: resolvedBranch, | ||
| remoteName: localProject.remoteName ?? "origin", | ||
| pr: prMetadata, | ||
| }); | ||
| if (materialized.warning) { | ||
| console.warn(`[workspaces.create] ${materialized.warning}`); | ||
| } | ||
| attemptedWorktreeAdd = true; | ||
| await git.raw([ | ||
| "worktree", | ||
| "add", | ||
| worktreePath, | ||
| resolvedBranch, | ||
| ]); | ||
| } catch (err) { | ||
| if (attemptedWorktreeAdd) { | ||
| await rollbackWorktree(); | ||
| } | ||
| throw new TRPCError({ | ||
| code: "CONFLICT", | ||
| message: | ||
| err instanceof Error | ||
| ? err.message | ||
| : "Failed to add detached worktree", | ||
| : "Failed to prepare PR worktree", | ||
| }); | ||
| } |
There was a problem hiding this comment.
git worktree add failure
When materializePrBranch creates a new local branch (createdBranch: true) but the subsequent git worktree add throws, rollbackWorktree() is called — but rollbackWorktree only removes the worktree via git worktree remove, not the newly created branch. The plan document explicitly calls this out: "If git worktree add fails after a new branch was created only for this operation, delete that branch if and only if it still points at the verified PR head and is not checked out anywhere." That cleanup is absent here. Concretely, if the host filesystem is full or the branch is somehow already registered in another worktree at the time of git worktree add, the branch named e.g. contributor/feature-x will be left in the repo forever unless the user manually deletes it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspaces/workspaces.ts
Line: 639-668
Comment:
**Orphaned local branch on `git worktree add` failure**
When `materializePrBranch` creates a new local branch (`createdBranch: true`) but the subsequent `git worktree add` throws, `rollbackWorktree()` is called — but `rollbackWorktree` only removes the worktree via `git worktree remove`, not the newly created branch. The plan document explicitly calls this out: "If `git worktree add` fails after a new branch was created only for this operation, delete that branch if and only if it still points at the verified PR head and is not checked out anywhere." That cleanup is absent here. Concretely, if the host filesystem is full or the branch is somehow already registered in another worktree at the time of `git worktree add`, the branch named e.g. `contributor/feature-x` will be left in the repo forever unless the user manually deletes it.
How can I resolve this? If you propose a fix, please make it concise.| async function fetchSyntheticPrBranch(args: { | ||
| git: GitClient; | ||
| remoteName: string; | ||
| pr: PrBranchMetadata; | ||
| warning?: string; | ||
| }): Promise<PrBranchSource> { | ||
| const syntheticRef = getSyntheticPrHeadRef(args.pr.number); | ||
| await args.git.raw([ | ||
| "fetch", | ||
| "--no-tags", | ||
| "--quiet", | ||
| args.remoteName, | ||
| syntheticRef, | ||
| ]); | ||
| await assertRefMatchesExpectedOid({ | ||
| git: args.git, | ||
| ref: "FETCH_HEAD", | ||
| expectedHeadOid: args.pr.headRefOid, | ||
| }); | ||
| return { | ||
| kind: "synthetic-pr-ref", | ||
| startPoint: "FETCH_HEAD", | ||
| mergeRef: syntheticRef, | ||
| warning: args.warning, | ||
| }; | ||
| } |
There was a problem hiding this comment.
FETCH_HEAD race condition in concurrent same-repo fetches
FETCH_HEAD is a single repo-level file overwritten by every git fetch that lacks a named destination ref. fetchSyntheticPrBranch fetches refs/pull/N/head into FETCH_HEAD, immediately verifies it against headRefOid, then returns startPoint: "FETCH_HEAD". Back in materializePrBranch, the git branch --no-track -- <branch> FETCH_HEAD runs as a separate git command. If any other fetch (a second concurrent PR workspace creation, a background remote update, or a post-checkout hook) runs in the same clone between those two calls, FETCH_HEAD will point to a different, unverified commit, and the local branch will be created from the wrong OID — silently passing the OID guard. The fix is to fetch into a named ref (e.g., refs/superset/pr-fetch/<number>) instead of relying on FETCH_HEAD.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts
Line: 113-138
Comment:
**`FETCH_HEAD` race condition in concurrent same-repo fetches**
`FETCH_HEAD` is a single repo-level file overwritten by every `git fetch` that lacks a named destination ref. `fetchSyntheticPrBranch` fetches `refs/pull/N/head` into `FETCH_HEAD`, immediately verifies it against `headRefOid`, then returns `startPoint: "FETCH_HEAD"`. Back in `materializePrBranch`, the `git branch --no-track -- <branch> FETCH_HEAD` runs as a separate git command. If any other fetch (a second concurrent PR workspace creation, a background remote update, or a post-checkout hook) runs in the same clone between those two calls, `FETCH_HEAD` will point to a different, unverified commit, and the local branch will be created from the wrong OID — silently passing the OID guard. The fix is to fetch into a named ref (e.g., `refs/superset/pr-fetch/<number>`) instead of relying on `FETCH_HEAD`.
How can I resolve this? If you propose a fix, please make it concise.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts:134">
P1: Do not use `FETCH_HEAD` as the persisted start point for branch creation. `FETCH_HEAD` is repo-global and can be overwritten by another fetch between verification and `git branch`, which can create the PR branch at an unverified commit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.ts:258">
P2: Avoid swallowing cleanup errors with an empty catch; surface cleanup failure details so branch-materialization failures remain diagnosable.
(Based on your team's feedback about handling async rejections explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts:211">
P2: The negative branch-creation assertion uses `fetchRef`, but branch creation now uses the verified head OID. This can let an unintended `branch --no-track` call slip through without failing the test.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1 participant
Summary
gh pr checkout.headRefOid, creates/configures the local branch, then letsworkspaces.createadd the worktree from that branch.gh pr checkoutrecovery helper/tests.git pushtargets the contributor branch instead ofrefs/pull/<n>/head.workspaces.createand surface them in the desktop workspace-create flow.--prno longer claims the host runsgh pr checkout.plans/done/20260516-pr-worktree-checkout-flow.md.Why
The old flow performed two checkouts in the target path.
git worktree add --detachcan run hooks, filters, or watchers that dirty tracked files such aspackage-lock.json; the latergh pr checkout --forcecan still fail withlocal changes would be overwritten by checkout.The new flow avoids the dirty-placeholder state entirely: it materializes the verified branch first, then performs one worktree checkout.
Known follow-ups
gh pr checkoutpath and should get a separate backport.maintainerCanModify, deleted forks, inaccessible forks) if exact GitHub CLI parity is needed for every fork push edge case.Validation
bun test packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-materialize.test.ts packages/host-service/test/integration/workspace-create-pr.integration.test.ts packages/host-service/test/integration/pr-branch-materialize.integration.test.ts(21 pass, 0 fail)bun test packages/host-service/test/integration(206 pass, 8 existing todos, 0 fail)bun run --cwd packages/host-service test:e2e(12 pass, 0 fail)bun run typecheckbun run lintSummary by cubic
Fix PR workspace creation by materializing the verified PR head before creating the worktree. This removes the detached worktree +
gh pr checkoutflow, prevents “local changes would be overwritten by checkout” errors, and configures fork pushes for cross‑repo PRs.Bug Fixes
headRefOid, create/configure the local branch, then run onegit worktree add <path> <branch>.refs/heads/<branch>and cross‑repo or deleted head branches viarefs/pull/<n>/head; abort on OID mismatch. For cross‑repo PRs with fork metadata, add asuperset-pr-<n>remote and setpushRemotewithHEAD:refs/heads/<headRefName>; warn when fork metadata is missing or when falling back to the synthetic PR ref.Refactors
materializePrBranch,normalizePrBranchTracking, and cleanup utilities with unit/integration tests covering edge cases;workspaces.createnow materializes the branch then runsgit worktree add.fetchPrMetadatauses injectedctx.execGh; integration tests confirm the host never runsgh pr checkoutin worktree mode.@mcp-v2/@superset/sdk/docs:--pr“checks out the verified PR head” andworkspaces.createreturns awarningsarray. Added design docplans/done/20260516-pr-worktree-checkout-flow.md. Removed the oldgh pr checkoutrecovery helper and its tests.Written for commit 143decc. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
UI