[codex] Recover PR checkout from archived refs#3725
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds PR head metadata to checkout payloads and schemas, implements git-based recovery for failed Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Desktop Client
participant HostSvc as Host Service
participant GH as GitHub CLI
participant Git as Git (local)
participant DB as Runtime/DB
User->>Client: Request PR checkout
Client->>Client: Build payload (includes headRefOid, headRepositoryName, isDraft)
Client->>HostSvc: Call checkout API with payload
HostSvc->>GH: Run `gh pr checkout`
alt gh checkout succeeds
GH-->>HostSvc: Success
HostSvc->>DB: finishCheckout(..., pullRequest)
DB-->>HostSvc: persisted
HostSvc-->>Client: Workspace created
else gh checkout fails
GH-->>HostSvc: Error
HostSvc->>HostSvc: classify error (getPrCheckoutRecoveryKind)
alt synthetic-pr-ref or fetch-head
HostSvc->>Git: fetch refs/pull/<N>/head (or use FETCH_HEAD)
Git-->>HostSvc: FETCH_HEAD
HostSvc->>Git: rev-parse FETCH_HEAD^{commit}
Git-->>HostSvc: Commit OID
alt OID == expectedHeadOid
HostSvc->>Git: checkout FETCH_HEAD --no-track
Git-->>HostSvc: Success
HostSvc->>DB: finishCheckout(..., pullRequest) with warning
HostSvc-->>Client: Workspace created (with warning)
else
HostSvc->>Git: git worktree remove --force
Git-->>HostSvc: Removed
HostSvc-->>Client: Error (recovery failed)
end
else unrecognized error
HostSvc->>Git: git worktree remove --force
HostSvc-->>Client: Error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 |
Greptile SummaryThis PR adds a two-stage recovery mechanism for Confidence Score: 5/5Safe to merge — recovery logic is well-guarded by OID verification and rollback, and all remaining findings are P2. All P2 findings: one missing warning text in a server log, and one broad error-pattern match that is still protected by OID verification from checking out the wrong commit. No correctness, data-integrity, or security issues found. pr-checkout-recovery.ts — the "is not a branch" pattern in getPrCheckoutRecoveryKind could be tightened.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts | New file implementing the two-path recovery strategy (synthetic PR ref vs FETCH_HEAD), with OID verification as a safety guard; one pattern-match in getPrCheckoutRecoveryKind is broader than necessary. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | Wraps gh pr checkout failure in a recovery block; rollback and error propagation logic is preserved; minor: server log omits the actual warning text. |
| packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.ts | Falls back to pr/ for cross-repo PRs with no owner; number made optional in the type so existing callers without number compile cleanly. |
| packages/host-service/src/trpc/router/workspace-creation/schemas.ts | headRefOid added to both checkoutPrSchema and pullRequestContentSchema with z.string().min(1) — correctly required and non-empty. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts | Removed early-exit for deleted-fork cross-repo PRs so host-service can attempt ref recovery; headRefOid threaded through the payload. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts | headRefOid added to the gh pr view --json fields and propagated to the return value. |
| packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts | Good coverage of both recovery paths, OID mismatch rejection, and unrecoverable errors; mock correctly simulates rev-parse output. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts | Test updated to reflect deleted-fork PRs now succeeding instead of throwing; headRefOid wired into test fixture. |
| packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts | New test verifies cross-repo PR with empty owner falls back to pr/. |
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.ts | headRefOid added to CheckoutWorkspaceInput interface to match the new schema requirement. |
Sequence Diagram
sequenceDiagram
participant Client
participant HostService as host-service (checkout.ts)
participant gh as gh CLI
participant Git as git
participant GitHub as GitHub API
Client->>HostService: checkoutInput (pr + headRefOid)
HostService->>Git: worktree add --detach path
HostService->>gh: pr checkout N --branch branch --force
alt gh succeeds
gh-->>HostService: ok
else gh fails
gh-->>HostService: error
HostService->>HostService: getPrCheckoutRecoveryKind(error)
alt kind = synthetic-pr-ref
HostService->>Git: fetch --no-tags origin refs/pull/N/head
Git->>GitHub: fetch refs/pull/N/head
GitHub-->>Git: commit
HostService->>Git: rev-parse --verify FETCH_HEAD
HostService->>HostService: assert OID == headRefOid
HostService->>Git: checkout -B branch --no-track FETCH_HEAD
else kind = fetch-head
HostService->>Git: rev-parse --verify FETCH_HEAD
HostService->>HostService: assert OID == headRefOid
HostService->>Git: checkout -B branch --no-track FETCH_HEAD
else unrecoverable
HostService->>Git: worktree remove --force path
HostService-->>Client: TRPCError INTERNAL_SERVER_ERROR
end
end
HostService->>HostService: enablePushAutoSetupRemote
HostService-->>Client: workspace + warnings[]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts
Line: 157-162
Comment:
**Warning message omitted from server log**
`prCheckoutRecoveryWarning` is logged in the `console.warn` call but the actual warning text is not included. Developers reading server logs won't see the specific warning message (e.g. which ref was used, what was unavailable) — only the PR number and branch name.
```suggestion
if (prCheckoutRecoveryWarning) {
console.warn(
"[workspaceCreation.checkout] recovered failed gh pr checkout",
{ prNumber: input.pr.number, branch, warning: prCheckoutRecoveryWarning },
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts
Line: 18-19
Comment:
**Broad pattern match for `"fetch-head"` recovery kind**
`"is not a branch"` is a common git error phrase (e.g., `fatal: 'HEAD~5' is not a branch`, `error: pathspec '...' is not a branch`) that can appear in contexts unrelated to tracking-ref failures. If triggered by an unrelated error, recovery would fall through to reading FETCH_HEAD — which may be stale from a prior, unrelated fetch. The OID verification does protect against checking out wrong commits, but the fetch-head path would silently consume these unrelated errors and produce a less precise error message on OID mismatch.
Consider tightening the pattern to something more specific to the `gh pr checkout` tracking-ref failure, e.g. checking for the `origin/<name>` prefix in the message.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Recover PR checkout from archived refs" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
157
to
162
| if (prCheckoutRecoveryWarning) { | ||
| console.warn( | ||
| "[workspaceCreation.checkout] recovered failed gh pr checkout", | ||
| { prNumber: input.pr.number, branch }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
prCheckoutRecoveryWarning is logged in the console.warn call but the actual warning text is not included. Developers reading server logs won't see the specific warning message (e.g. which ref was used, what was unavailable) — only the PR number and branch name.
Suggested change
| if (prCheckoutRecoveryWarning) { | |
| console.warn( | |
| "[workspaceCreation.checkout] recovered failed gh pr checkout", | |
| { prNumber: input.pr.number, branch }, | |
| ); | |
| } | |
| if (prCheckoutRecoveryWarning) { | |
| console.warn( | |
| "[workspaceCreation.checkout] recovered failed gh pr checkout", | |
| { prNumber: input.pr.number, branch, warning: prCheckoutRecoveryWarning }, | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts
Line: 157-162
Comment:
**Warning message omitted from server log**
`prCheckoutRecoveryWarning` is logged in the `console.warn` call but the actual warning text is not included. Developers reading server logs won't see the specific warning message (e.g. which ref was used, what was unavailable) — only the PR number and branch name.
```suggestion
if (prCheckoutRecoveryWarning) {
console.warn(
"[workspaceCreation.checkout] recovered failed gh pr checkout",
{ prNumber: input.pr.number, branch, warning: prCheckoutRecoveryWarning },
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+18
to
+19
| if (message.includes("is not a branch")) { | ||
| return "fetch-head"; |
There was a problem hiding this comment.
"fetch-head" recovery kind
"is not a branch" is a common git error phrase (e.g., fatal: 'HEAD~5' is not a branch, error: pathspec '...' is not a branch) that can appear in contexts unrelated to tracking-ref failures. If triggered by an unrelated error, recovery would fall through to reading FETCH_HEAD — which may be stale from a prior, unrelated fetch. The OID verification does protect against checking out wrong commits, but the fetch-head path would silently consume these unrelated errors and produce a less precise error message on OID mismatch.
Consider tightening the pattern to something more specific to the gh pr checkout tracking-ref failure, e.g. checking for the origin/<name> prefix in the message.
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-checkout-recovery.ts
Line: 18-19
Comment:
**Broad pattern match for `"fetch-head"` recovery kind**
`"is not a branch"` is a common git error phrase (e.g., `fatal: 'HEAD~5' is not a branch`, `error: pathspec '...' is not a branch`) that can appear in contexts unrelated to tracking-ref failures. If triggered by an unrelated error, recovery would fall through to reading FETCH_HEAD — which may be stale from a prior, unrelated fetch. The OID verification does protect against checking out wrong commits, but the fetch-head path would silently consume these unrelated errors and produce a less precise error message on OID mismatch.
Consider tightening the pattern to something more specific to the `gh pr checkout` tracking-ref failure, e.g. checking for the `origin/<name>` prefix in the message.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts (2)
157-162: Optional: include the recovery warning text in the success log for observability.Right now the success log records only
prNumberandbranch. IncludingprCheckoutRecoveryWarning(or a short summary of it) makes debugging easier when scanning host-service logs.if (prCheckoutRecoveryWarning) { console.warn( "[workspaceCreation.checkout] recovered failed gh pr checkout", - { prNumber: input.pr.number, branch }, + { prNumber: input.pr.number, branch, warning: prCheckoutRecoveryWarning }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts` around lines 157 - 162, The success log inside the workspaceCreation.checkout procedure currently logs only prNumber and branch; update that log to also include prCheckoutRecoveryWarning (or a short summary) so observers can see the recovery message. Locate the block that checks prCheckoutRecoveryWarning and the subsequent success console.log/console.info call in checkout.ts, and add prCheckoutRecoveryWarning (or its trimmed substring) into the logged object/message so the log contains { prNumber: input.pr.number, branch, prCheckoutRecoveryWarning } for improved observability.
121-155: Recovery control flow is correct but a bit subtle — consider a local boolean instead of re-throwingerr.The inner
if (!recovery.recovered) throw err;is caught by the outercatch (recoveryErr)and only therecoveryErr === erridentity check distinguishes "recovery declined" from "recovery itself errored". This works, but flattening the branches makes the two failure modes explicit and avoids the identity-comparison contract.♻️ Suggested refactor (illustrative)
} catch (err) { - try { - const recovery = await recoverPrCheckoutAfterGhFailure({ - git, - worktreePath, - branch, - prNumber: input.pr.number, - remoteName: localProject.remoteName ?? "origin", - expectedHeadOid: input.pr.headRefOid, - error: err, - }); - if (!recovery.recovered) { - throw err; - } - prCheckoutRecoveryWarning = recovery.warning; - } catch (recoveryErr) { - await git - .raw(["worktree", "remove", "--force", worktreePath]) - .catch((rollbackErr) => { - console.warn( - "[workspaceCreation.checkout] failed to rollback PR worktree", - { worktreePath, err: rollbackErr }, - ); - }); - clearProgress(input.pendingId); - const recoveryMessage = - recoveryErr === err - ? "" - : ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryErr)}`; - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `gh pr checkout failed: ${getErrorMessage(err)}${recoveryMessage}`, - }); - } + let recoverySuffix = ""; + let recovered = false; + try { + const recovery = await recoverPrCheckoutAfterGhFailure({ + git, + worktreePath, + branch, + prNumber: input.pr.number, + remoteName: localProject.remoteName ?? "origin", + expectedHeadOid: input.pr.headRefOid, + error: err, + }); + if (recovery.recovered) { + prCheckoutRecoveryWarning = recovery.warning; + recovered = true; + } + } catch (recoveryErr) { + recoverySuffix = ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryErr)}`; + } + if (!recovered) { + await git + .raw(["worktree", "remove", "--force", worktreePath]) + .catch((rollbackErr) => { + console.warn( + "[workspaceCreation.checkout] failed to rollback PR worktree", + { worktreePath, err: rollbackErr }, + ); + }); + clearProgress(input.pendingId); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `gh pr checkout failed: ${getErrorMessage(err)}${recoverySuffix}`, + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts` around lines 121 - 155, Replace the subtle reuse-of-err control flow by introducing a local boolean (e.g., recoveryAttempted/ recoverySucceeded) instead of re-throwing err from inside the recoverPrCheckoutAfterGhFailure handling block: call recoverPrCheckoutAfterGhFailure, set recoverySucceeded = recovery.recovered and prCheckoutRecoveryWarning = recovery.warning when available, and if recoverySucceeded is false throw a new, explicit error or proceed to the cleanup path so the outer catch only handles actual recovery errors (recoveryErr) rather than relying on identity checks; ensure the outer catch still removes the worktree via git.raw([... worktreePath]) .catch(...), calls clearProgress(input.pendingId), and constructs the TRPCError message using getErrorMessage(err) and getErrorMessage(recoveryErr) consistently.packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts (2)
18-32: Substring-based error classification is fragile across git/gh versions — the OID guard is doing the real work.The classifier matches on lowercased English error fragments (e.g.
"couldn't find remote ref","repository not found","is not a branch"). These strings come from upstream git/gh and may change over time or be localized in some environments. Two notes:
"does not appear to be a git repository"is fairly generic — it can fire for unrelated cases (e.g. a bad remote URL pointing somewhere that isn't a git server). Mapping it to"synthetic-pr-ref"is fine here only because the subsequentexpectedHeadOidcheck rejects bogus recoveries; just be aware this widens the "attempt recovery" funnel.- Consider centralizing the substrings as named constants and adding a brief code comment that the OID check (Line 65) is the actual correctness guarantee, not the substring match. That makes the intent obvious to future maintainers and tests.
No change required — flagging for visibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts` around lines 18 - 32, Centralize the substring literals used for error classification (e.g., the phrases that currently trigger returning "fetch-head" or "synthetic-pr-ref") into named constants (e.g., FETCH_HEAD_MARKER, SYNTHETIC_PR_REF_MARKERS array) and add a short comment above this classifier stating that these substring checks are heuristics and that the subsequent expectedHeadOid OID check (around line 65) is the real correctness guard; update the classifier to reference the constants/array and keep behavior unchanged so intent is clear to future maintainers and tests.
142-161: The two recovery paths are asymmetric; consider adding an explicit fetch to the "fetch-head" path or documenting the gh dependency.The
"synthetic-pr-ref"recovery (lines 142–154) explicitly fetchesrefs/pull/<N>/headbefore asserting the OID, whereas the"fetch-head"recovery (lines 156–161) skips the fetch and relies ongh pr checkouthaving already populatedFETCH_HEADbefore it fails with"is not a branch". The OID assertion is a solid safety net—a stale or unrelatedFETCH_HEADwill mismatch—but this creates an implicit dependency on gh's internal fetch behavior.Two ways to harden this without changing behavior in the happy case:
- Add an explicit
git fetch <remoteName> refs/pull/<N>/headbefore the OID check on this path too (mirrors the"synthetic-pr-ref"path, removes the implicit dependency on gh).- Document the assumption in the JSDoc (lines 114–121) that this recovery path depends on gh's pre-failure fetch populating
FETCH_HEAD, so future maintainers understand this contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts` around lines 142 - 161, The "fetch-head" recovery path currently relies on gh having populated FETCH_HEAD and skips fetching, creating an implicit dependency; to harden it, call the same fetch used for synthetic refs before asserting the OID — i.e. invoke fetchSyntheticPrHead({ git, worktreePath, remoteName, prNumber }) (or an equivalent git.fetch of refs/pull/<N>/head) prior to assertFetchHeadMatchesExpectedOid in the non-"synthetic-pr-ref" branch, then proceed to checkoutFetchHeadAsBranch; this mirrors the synthetic path and removes the hidden gh dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts`:
- Around line 150-153: Update the user-facing warning that currently says
"archived PR ref" to instead reference GitHub's synthetic/synthetic PR head ref
to avoid implying the PR was archived; locate the return that constructs the
recovery result (the object with recovered: true and the warning string) and
change the message to mention "GitHub's synthetic PR head ref
(refs/pull/<N>/head)" or "GitHub's PR head ref"—use the same terminology as
getSyntheticPrHeadRef and getPrCheckoutRecoveryKind so messaging matches
internal names and does not suggest the PR itself was archived.
---
Nitpick comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts`:
- Around line 157-162: The success log inside the workspaceCreation.checkout
procedure currently logs only prNumber and branch; update that log to also
include prCheckoutRecoveryWarning (or a short summary) so observers can see the
recovery message. Locate the block that checks prCheckoutRecoveryWarning and the
subsequent success console.log/console.info call in checkout.ts, and add
prCheckoutRecoveryWarning (or its trimmed substring) into the logged
object/message so the log contains { prNumber: input.pr.number, branch,
prCheckoutRecoveryWarning } for improved observability.
- Around line 121-155: Replace the subtle reuse-of-err control flow by
introducing a local boolean (e.g., recoveryAttempted/ recoverySucceeded) instead
of re-throwing err from inside the recoverPrCheckoutAfterGhFailure handling
block: call recoverPrCheckoutAfterGhFailure, set recoverySucceeded =
recovery.recovered and prCheckoutRecoveryWarning = recovery.warning when
available, and if recoverySucceeded is false throw a new, explicit error or
proceed to the cleanup path so the outer catch only handles actual recovery
errors (recoveryErr) rather than relying on identity checks; ensure the outer
catch still removes the worktree via git.raw([... worktreePath]) .catch(...),
calls clearProgress(input.pendingId), and constructs the TRPCError message using
getErrorMessage(err) and getErrorMessage(recoveryErr) consistently.
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts`:
- Around line 18-32: Centralize the substring literals used for error
classification (e.g., the phrases that currently trigger returning "fetch-head"
or "synthetic-pr-ref") into named constants (e.g., FETCH_HEAD_MARKER,
SYNTHETIC_PR_REF_MARKERS array) and add a short comment above this classifier
stating that these substring checks are heuristics and that the subsequent
expectedHeadOid OID check (around line 65) is the real correctness guard; update
the classifier to reference the constants/array and keep behavior unchanged so
intent is clear to future maintainers and tests.
- Around line 142-161: The "fetch-head" recovery path currently relies on gh
having populated FETCH_HEAD and skips fetching, creating an implicit dependency;
to harden it, call the same fetch used for synthetic refs before asserting the
OID — i.e. invoke fetchSyntheticPrHead({ git, worktreePath, remoteName, prNumber
}) (or an equivalent git.fetch of refs/pull/<N>/head) prior to
assertFetchHeadMatchesExpectedOid in the non-"synthetic-pr-ref" branch, then
proceed to checkoutFetchHeadAsBranch; this mirrors the synthetic path and
removes the hidden gh dependency.
🪄 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
Run ID: 1458ab46-885d-4003-adf8-fb2488691c6a
📒 Files selected for processing (10)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.tspackages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.tspackages/host-service/src/trpc/router/workspace-creation/schemas.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.tspackages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts
Comment on lines
+150
to
+153
| return { | ||
| recovered: true, | ||
| warning: `The PR head branch was unavailable, so Superset checked out GitHub's archived PR ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`, | ||
| }; |
Contributor
There was a problem hiding this comment.
Misleading "archived PR ref" wording in user-facing warning.
refs/pull/<N>/head is a synthetic ref GitHub exposes for every PR regardless of state — it is not specifically an "archived" ref. For a PR whose head branch/fork was just deleted, calling it "archived" may confuse users into thinking the PR itself was archived/closed. Consider phrasing it as GitHub's PR head ref (or "synthetic PR head ref"), which also matches the internal terminology used in getSyntheticPrHeadRef and getPrCheckoutRecoveryKind.
✏️ Suggested wording
- warning: `The PR head branch was unavailable, so Superset checked out GitHub's archived PR ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`,
+ warning: `The PR head branch was unavailable, so Superset checked out GitHub's PR head ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| recovered: true, | |
| warning: `The PR head branch was unavailable, so Superset checked out GitHub's archived PR ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`, | |
| }; | |
| return { | |
| recovered: true, | |
| warning: `The PR head branch was unavailable, so Superset checked out GitHub's PR head ref (${getSyntheticPrHeadRef(prNumber)}) with no upstream. Push a new branch if you need to continue from it.`, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.ts`
around lines 150 - 153, Update the user-facing warning that currently says
"archived PR ref" to instead reference GitHub's synthetic/synthetic PR head ref
to avoid implying the PR was archived; locate the return that constructs the
recovery result (the object with recovered: true and the warning string) and
change the message to mention "GitHub's synthetic PR head ref
(refs/pull/<N>/head)" or "GitHub's PR head ref"—use the same terminology as
getSyntheticPrHeadRef and getPrCheckoutRecoveryKind so messaging matches
internal names and does not suggest the PR itself was archived.
Contributor
🧹 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}
Contributor
There was a problem hiding this comment.
1 issue found across 10 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/procedures/checkout.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts:160">
P2: The actual warning text is omitted from the log output. `prCheckoutRecoveryWarning` is checked but never included in the logged metadata, so server logs won't show which ref was used or what was unavailable — only the PR number and branch name.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts (2)
173-178: Recovery-success log is gated on warning truthiness — recoveries with empty warnings won't be logged.If
recoverPrCheckoutAfterGhFailureever returns{ recovered: true, warning: null }(or empty), this log silently skips. If the intent is to always observe successful recoveries (useful for telemetry around archived-ref fallbacks), gate on a "did recover" boolean instead of the warning text. Also consider including the recovery message body in the log so operators can see why we recovered, not just that we did.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts` around lines 173 - 178, The recovery logging currently checks prCheckoutRecoveryWarning truthiness so a successful recovery that returns a null/empty warning is not logged; instead, change the condition to check the recovery success boolean returned by recoverPrCheckoutAfterGhFailure (e.g., recovered === true) and always log when recovered, and include the recovery message/body in the log payload (alongside input.pr.number and branch) so operators can see why the fallback occurred; update the logging around prCheckoutRecoveryWarning/recoverPrCheckoutAfterGhFailure in the checkout procedure to use the recovered flag and include the recovery message.
138-171: Recovery control flow relies on object-identity comparison; consider a clearer flag.The
throw errinside the inner try (Line 149) is later caught by the outercatch (recoveryErr)and identified viarecoveryErr === err(Line 163) to suppress the "Recovery via refs/pull/... also failed" suffix. This works, but it's subtle — anyone reading the catch sees two distinct meanings (recovery returned recovered:falsevsrecovery threw) collapsed into one path, distinguished only by reference equality. A small refactor with an explicit flag makes the intent obvious and avoids a future bug if the recovery utility ever wraps/rethrows the same error reference.♻️ Proposed refactor
let prCheckoutRecoveryWarning: string | null = null; try { await execGh( ... ); } catch (err) { - try { - const recovery = await recoverPrCheckoutAfterGhFailure({ ... }); - if (!recovery.recovered) { - throw err; - } - prCheckoutRecoveryWarning = recovery.warning; - } catch (recoveryErr) { - await git.raw(["worktree", "remove", "--force", worktreePath]).catch(...); - clearProgress(input.pendingId); - const recoveryMessage = - recoveryErr === err - ? "" - : ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryErr)}`; - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `gh pr checkout failed: ${getErrorMessage(err)}${recoveryMessage}`, - }); - } + let recoveryError: unknown = null; + let recovered = false; + try { + const recovery = await recoverPrCheckoutAfterGhFailure({ ... }); + recovered = recovery.recovered; + prCheckoutRecoveryWarning = recovery.warning; + } catch (e) { + recoveryError = e; + } + if (!recovered) { + await git.raw(["worktree", "remove", "--force", worktreePath]).catch(...); + clearProgress(input.pendingId); + const recoveryMessage = recoveryError + ? ` Recovery via refs/pull/${input.pr.number}/head also failed: ${getErrorMessage(recoveryError)}` + : ""; + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `gh pr checkout failed: ${getErrorMessage(err)}${recoveryMessage}`, + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts` around lines 138 - 171, The current recovery control flow in the checkout procedure relies on comparing recoveryErr === err to decide whether to append the "Recovery via refs/pull/..." message; this is fragile. Instead, change the inner try/catch logic around recoverPrCheckoutAfterGhFailure to set an explicit flag (e.g., recoveryThrew or recoveryAttempted and recovered boolean) that records whether recoverPrCheckoutAfterGhFailure threw vs returned recovered:false, then use that flag when building recoveryMessage; update references to recoverPrCheckoutAfterGhFailure, prCheckoutRecoveryWarning, err, recoveryErr, and the TRPCError message construction so the decision no longer depends on object identity, and preserve the existing rollback (git.raw worktree remove) and clearProgress behavior.packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)
400-422: ConditionalpullRequestIdclearing leaves a transient stale link when branch and upstream both change.The expression at Line 401-402 keeps
pullRequestIdwheneverupstreamis truthy, even ifbranchChanged === true. So if a user switches a workspace from a branch tracking PR A (upstream X) to a different branch tracking upstream Y (a different PR), the link to A is preserved here and only corrected on the nextperformProjectRefreshcycle (which clears it when the new key Y has no match).This is eventually consistent and probably acceptable, but worth deciding whether to also clear on
branchChanged && upstream-changedfor tighter semantics. Calling it out so the trade-off is explicit, not flagging as a bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around lines 400 - 422, The current update logic keeps pullRequestId when upstream is truthy, causing a transient stale PR link if branchChanged is true; change the assignment of pullRequestId in the this.db.update(workspaces).set({...}) block (the object that sets branch, headSha, upstreamOwner, upstreamRepo, upstreamBranch, pullRequestId) so that pullRequestId is cleared (set to null) whenever branchChanged is true or the upstream identity changes (e.g., compute upstreamChanged by comparing upstreamOwner/upstreamRepo/upstreamBranch to workspace.* and set pullRequestId = null if branchChanged || upstreamChanged), otherwise preserve the existing workspace.pullRequestId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 333-343: The method linkWorkspaceToCheckoutPullRequest currently
returns null when getProjectRepository returns null without any log or error;
update linkWorkspaceToCheckoutPullRequest to emit a warning (e.g.,
this.logger.warn or similar class logger) before returning null, mentioning the
workspaceId, projectId, and that repository metadata was missing so callers can
see why no PR link was created; alternatively, throw a specific error instead if
you prefer the caller to handle it — reference
linkWorkspaceToCheckoutPullRequest and getProjectRepository to locate where to
add the warning/throw.
---
Nitpick comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 400-422: The current update logic keeps pullRequestId when
upstream is truthy, causing a transient stale PR link if branchChanged is true;
change the assignment of pullRequestId in the
this.db.update(workspaces).set({...}) block (the object that sets branch,
headSha, upstreamOwner, upstreamRepo, upstreamBranch, pullRequestId) so that
pullRequestId is cleared (set to null) whenever branchChanged is true or the
upstream identity changes (e.g., compute upstreamChanged by comparing
upstreamOwner/upstreamRepo/upstreamBranch to workspace.* and set pullRequestId =
null if branchChanged || upstreamChanged), otherwise preserve the existing
workspace.pullRequestId.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts`:
- Around line 173-178: The recovery logging currently checks
prCheckoutRecoveryWarning truthiness so a successful recovery that returns a
null/empty warning is not logged; instead, change the condition to check the
recovery success boolean returned by recoverPrCheckoutAfterGhFailure (e.g.,
recovered === true) and always log when recovered, and include the recovery
message/body in the log payload (alongside input.pr.number and branch) so
operators can see why the fallback occurred; update the logging around
prCheckoutRecoveryWarning/recoverPrCheckoutAfterGhFailure in the checkout
procedure to use the recovered flag and include the recovery message.
- Around line 138-171: The current recovery control flow in the checkout
procedure relies on comparing recoveryErr === err to decide whether to append
the "Recovery via refs/pull/..." message; this is fragile. Instead, change the
inner try/catch logic around recoverPrCheckoutAfterGhFailure to set an explicit
flag (e.g., recoveryThrew or recoveryAttempted and recovered boolean) that
records whether recoverPrCheckoutAfterGhFailure threw vs returned
recovered:false, then use that flag when building recoveryMessage; update
references to recoverPrCheckoutAfterGhFailure, prCheckoutRecoveryWarning, err,
recoveryErr, and the TRPCError message construction so the decision no longer
depends on object identity, and preserve the existing rollback (git.raw worktree
remove) and clearProgress behavior.
🪄 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
Run ID: 49c9ea8f-1976-48b2-97cf-d3f8df3fd40f
📒 Files selected for processing (10)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.tspackages/host-service/src/runtime/pull-requests/index.tspackages/host-service/src/runtime/pull-requests/pull-requests.test.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.tspackages/host-service/src/trpc/router/workspace-creation/schemas.tspackages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/runtime/pull-requests/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts
Comment on lines
+333
to
+343
| async linkWorkspaceToCheckoutPullRequest({ | ||
| workspaceId, | ||
| projectId, | ||
| pullRequest, | ||
| }: { | ||
| workspaceId: string; | ||
| projectId: string; | ||
| pullRequest: CheckoutPullRequestMetadata; | ||
| }): Promise<string | null> { | ||
| const repo = await this.getProjectRepository(projectId); | ||
| if (!repo) return null; |
Contributor
There was a problem hiding this comment.
linkWorkspaceToCheckoutPullRequest silently returns null when project repo metadata is missing.
When getProjectRepository returns null (project row missing or unparseable remote), this method returns null without surfacing why. The only caller in finish-checkout.ts (Lines 147-163) wraps this in try/catch and only converts thrown errors into a user warning — a null return goes unnoticed and the workspace ends up with no PR link, no warning, and no log line.
This is an edge case (project metadata is normally populated by the time PR checkout runs), but consider either:
- logging a warn when returning
null, or - letting the caller distinguish "no repo metadata" from a successful no-op so it can surface a warning.
🛡️ Minimal fix: log when bailing
const repo = await this.getProjectRepository(projectId);
- if (!repo) return null;
+ if (!repo) {
+ console.warn(
+ "[host-service:pull-request-runtime] linkWorkspaceToCheckoutPullRequest: skipping; project repo metadata unavailable",
+ { projectId, workspaceId, prNumber: pullRequest.number },
+ );
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async linkWorkspaceToCheckoutPullRequest({ | |
| workspaceId, | |
| projectId, | |
| pullRequest, | |
| }: { | |
| workspaceId: string; | |
| projectId: string; | |
| pullRequest: CheckoutPullRequestMetadata; | |
| }): Promise<string | null> { | |
| const repo = await this.getProjectRepository(projectId); | |
| if (!repo) return null; | |
| async linkWorkspaceToCheckoutPullRequest({ | |
| workspaceId, | |
| projectId, | |
| pullRequest, | |
| }: { | |
| workspaceId: string; | |
| projectId: string; | |
| pullRequest: CheckoutPullRequestMetadata; | |
| }): Promise<string | null> { | |
| const repo = await this.getProjectRepository(projectId); | |
| if (!repo) { | |
| console.warn( | |
| "[host-service:pull-request-runtime] linkWorkspaceToCheckoutPullRequest: skipping; project repo metadata unavailable", | |
| { projectId, workspaceId, prNumber: pullRequest.number }, | |
| ); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around
lines 333 - 343, The method linkWorkspaceToCheckoutPullRequest currently returns
null when getProjectRepository returns null without any log or error; update
linkWorkspaceToCheckoutPullRequest to emit a warning (e.g., this.logger.warn or
similar class logger) before returning null, mentioning the workspaceId,
projectId, and that repository metadata was missing so callers can see why no PR
link was created; alternatively, throw a specific error instead if you prefer
the caller to handle it — reference linkWorkspaceToCheckoutPullRequest and
getProjectRepository to locate where to add the warning/throw.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/host-service/src/runtime/pull-requests/pull-requests.test.ts (2)
84-140: Fake DB ignoreswherepredicates — tests may pass for the wrong reasons.
query.pullRequests.findFirstand theselect(...).from(...).where(...)chains returnstate.pullRequest/state.workspaceregardless of the filter, so the production code could query for an unrelated id/projectId and the tests would still pass. For the third test (clears link whenheadShamismatches), this is particularly relevant: the assertion only proves thatpullRequestIdends upnull, not that the refresh logic correctly correlated the workspace with the right stored PR row. Consider keying the fakes off the actual filter args (or at least asserting the expected lookup keys) to make these tests resilient against regressions in the lookup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/pull-requests/pull-requests.test.ts` around lines 84 - 140, The fake DB in createFakeDb ignores where predicates so query.pullRequests.findFirst and select(...).from(...).where(...) always return state values; update createFakeDb to inspect the where/filter arguments passed into query.pullRequests.findFirst (and the select(...).from(...).where chain) and return matching rows (or empty) based on id/projectId/headSha as appropriate, so tests exercise real lookup logic; specifically modify the findFirst implementation and the where closures to accept filter params and compare them against state.pullRequest and state.workspace (and fail/return [] when keys don't match) so the third test verifies correct correlation rather than a coincidental match.
154-241: Good coverage of the new linking paths; consider one extra positive-refresh assertion.The three tests cover fork-with-upstream, deleted-fork persistence, and deleted-fork clearing on
headShadivergence — which matches the new behavior described in the PR. One small gap: there's no test for the "preservespullRequestIdwhen workspaceheadShastill matches the stored PRheadSha" path that the AI summary calls out. Adding a case wherestate.workspace.headSha = "abc123"(matching the linked PR) before callingrefreshPullRequestsByWorkspaceswould lock in that the no-op branch is exercised separately from the deleted-fork persistence case (which currently relies onheadShabeingnull).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/runtime/pull-requests/pull-requests.test.ts` around lines 154 - 241, Add a new test that exercises the "no-op refresh when workspace headSha matches PR headSha" path: create a state via makeState("pr/42"), link the PR using manager.linkWorkspaceToCheckoutPullRequest with headRefOid "abc123", set state.workspace.headSha = "abc123" (matching the stored PR headSha), call manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID]) and assert that state.workspace.pullRequestId remains the same; reference the existing test helpers and methods (makeState, createManager, linkWorkspaceToCheckoutPullRequest, refreshPullRequestsByWorkspaces, and state.workspace.headSha) so the new test mirrors the structure of the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.test.ts`:
- Around line 84-140: The fake DB in createFakeDb ignores where predicates so
query.pullRequests.findFirst and select(...).from(...).where(...) always return
state values; update createFakeDb to inspect the where/filter arguments passed
into query.pullRequests.findFirst (and the select(...).from(...).where chain)
and return matching rows (or empty) based on id/projectId/headSha as
appropriate, so tests exercise real lookup logic; specifically modify the
findFirst implementation and the where closures to accept filter params and
compare them against state.pullRequest and state.workspace (and fail/return []
when keys don't match) so the third test verifies correct correlation rather
than a coincidental match.
- Around line 154-241: Add a new test that exercises the "no-op refresh when
workspace headSha matches PR headSha" path: create a state via
makeState("pr/42"), link the PR using manager.linkWorkspaceToCheckoutPullRequest
with headRefOid "abc123", set state.workspace.headSha = "abc123" (matching the
stored PR headSha), call manager.refreshPullRequestsByWorkspaces([WORKSPACE_ID])
and assert that state.workspace.pullRequestId remains the same; reference the
existing test helpers and methods (makeState, createManager,
linkWorkspaceToCheckoutPullRequest, refreshPullRequestsByWorkspaces, and
state.workspace.headSha) so the new test mirrors the structure of the other
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0abd4959-a9d5-4e50-bffd-3e8a10a578ff
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.tspackages/host-service/src/runtime/pull-requests/pull-requests.test.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/trpc/router/workspace-creation/schemas.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/runtime/pull-requests/pull-requests.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.ts
…d contract Two coderabbit nitpicks worth taking: - checkout.ts: replace `recoveryErr === err` reference-identity check with an explicit `recoveryError` variable. The two failure modes (recovery declined vs. recovery itself threw) were collapsed into one catch block and distinguished only by object identity — a future change that wraps or rethrows the same error reference would silently break the message. - pr-checkout-recovery.ts: document that the `fetch-head` recovery path depends on gh having pre-populated FETCH_HEAD before failing. The `synthetic-pr-ref` path fetches itself; the asymmetry was implicit.
The comment explained the structure against the prior `recoveryErr === err` form. Future readers don't see that prior form — keep only the WHY (the two states need distinguishing).
saddlepaddle
added a commit
that referenced
this pull request
May 4, 2026
The recovery utility (`recoverPrCheckoutAfterGhFailure`) shipped on main via #3725, but was wired into the old `workspaceCreation.checkout` procedure that this PR deleted. Without recovery, `workspaces.create` fails for any merged PR whose head branch has been deleted from the remote — a common case post-merge. Plug the same recovery flow into the new PR-create catch block: - fetch prMetadata.headRefOid via `gh pr view` - on `gh pr checkout` failure, try the synthetic-pr-ref fallback or FETCH_HEAD recovery (utility decides based on the error) - rollback + throw only when recovery declined; otherwise continue past the catch with the worktree on FETCH_HEAD
saddlepaddle
added a commit
that referenced
this pull request
May 4, 2026
…(PR1) (#3893) * docs(plans): add v2 workspace create canonical refactor specs Two design docs guiding a clean reimplementation of the canonical workspace.create() flow: - 20260425-canonical-workspace-create-flow.md: umbrella design covering the unified host-service create API, host-scoped attachments, pane store registry, prompt boundary, and PR sequencing (PRs 1-7). - 20260425-host-agent-configs-pr1.md: PR 1 spec for the host-runtime agent config model (host_agent_configs table + settings router). Sourced from the prior v2-workspace-create-canonical branch so the reimplementation can land PR-by-PR per the plan. * docs(plans): switch host agent configs to argv-array launch spec Replace the single `launchCommand` string with a structured argv-array shape (`command` + `args[]` + `promptArgs[]` + `env`) matching VS Code ITerminalProfile / Tabby Shell / WezTerm SpawnCommand / Zellij panes. Empty launches drop `promptArgs` automatically, so codex/opencode/copilot no longer carry their prompt-mode flags into no-prompt sessions. Storing argv directly avoids shell-quoting bugs and makes prompt injection a list push instead of string concatenation. Adds first-class `env` overlay and the per-preset breakdown for the 8 in-scope agents. Implementation in PR1 will be redone against this shape in a separate branch; this branch is now docs-only. * docs(plans): address review on prompt contract + PR1 scope canonical-workspace-create-flow.md: - Make `prompt` required on agent launches in both code blocks (was optional in the API shape but later text said agent launches require one and promptless = raw terminal). Reconciled to the stricter rule and added a Notes line pointing readers to the Agent Configs section. 20260425-host-agent-configs-pr1.md: - Tighten the Summary to make explicit that this PR ships only the storage + tRPC router + V2 settings page. V2 modal and launch dispatch remain on legacy desktop presets and move to host configs in PR 5. * wip: canonical workspace create flow + optimistic attachment uploads Consolidate in-flight create lifecycle into one WorkspaceCreatesManager + useWorkspaceCreates hook (submit/retry/dismiss). Navigate immediately on submit; route renders creating/error/notfound states from the in-flight store. Sidebar reads the same store and supports dismiss on hover for failed entries. Optimistic attachment uploads via a module-scoped Zustand store keyed (fileId, hostUrl). Files upload to whichever host was active when added; switching hosts hides their pills without re-uploading and keeps cached attachment ids for return visits. Submit awaits in-flight uploads and joins file metadata for error messaging. Submit-time gating moved into handleSubmit so all paths (button, Enter, Cmd+Enter) respect preconditions. Plain Enter inserts a newline; submit is button or Cmd+Enter only. Server-side: workspaces.create detects an existing standard-path worktree and adopts it instead of failing on git worktree add — folds the picker's adopt path into the same submit. Layout for v2-workspace falls through to the page when no real row exists so the in-flight UI can render. Other cleanup: drop legacy in-flight-creates store + reconciler, the v2 useCreateWorkspace wrapper, useAdoptWorktree; collapse picker checkout+adopt callbacks; UploadingAttachmentPill replaces the library pill with subtle status overlays; PR link command keeps the Show closed checkbox (pagination concern); useShallow on the draft selector to fix an infinite-loop bug. * fix: drop simple-git --quiet on rev-parse and ship membership claim - refExists/localBranchExists/remoteExists: simple-git's `raw` resolves successfully with empty stdout when --quiet is on, so every "new branch" was being treated as already existing. Drop the flag and validate a 40+ hex sha was actually printed. - auth: include the user's full membership list in OAuth access-token claims so cross-org JWT checks pass downstream. - org-resource-access: split the not-found / wrong-org error paths so org-mismatch reports the actual ids in the diagnostic. - pull-requests: skip refresh for workspaces whose worktree was deleted on disk; simple-git would otherwise throw a confusing directory-does-not-exist error. - git/utils: stop logging on missing remote/origin — both null cases are expected and callers handle them. * chore(cli): override env vars from workspace .env in dev script Without -o, dotenv-cli leaves any pre-existing process.env vars alone, so a shell-level `SUPERSET_HOME_DIR=~/.superset` (set globally for the production CLI) wins over the workspace's `.env` value and dev CLI ops land in the prod data dir instead of `superset-dev-data/`. * feat(cli): add agents run command + variadic flag support - cli-framework: extend `.variadic()` to string flags so repeated `--flag value` invocations accumulate into an array. Drop the auto-`isRequired: true` from `.variadic()` — both existing positional callsites already chain `.required()` explicitly, no behavior change. - cli: new `superset agents run --workspace <id> --agent <preset|id> --prompt <text> [--attachment-id <uuid>]...` for spawning an agent inside an existing workspace. Wraps the host service `agents.run` mutation; resolves the workspace's host via the cloud lookup. * refactor: server-generate workspace name + per-side AI rename gate The renderer no longer generates a friendly-random fallback. Both `name` and `branch` are now optional on the create input — the server picks a friendly random for whichever side is missing, and the AI rename only replaces the side(s) the user didn't supply. - `workspaces.create` schema: drop `autogenerateName`, make `name` optional, allow both `branch` and `pr` to be absent (refine relaxed to "not both set"). Server generates+dedupes a friendly branch name when `branch` is undefined; falls back to PR title or branch for workspace name when `name` is undefined. - `applyAiWorkspaceRename`: take `renameTitle` / `renameBranch` flags; only apply the side the caller asked for. Both `true` from the manual `aiRename` mutation; computed per-create-input on the branch path; skipped entirely on the PR path. - Renderer: drop `friendlyFallback` from the draft store + context + `PromptGroup` placeholder; `resolveNames` returns `string | null`; `useSubmitWorkspace` sends only what the user typed (no synthetic name). In-flight states show "Creating workspace" with no subtitle when the name is absent. - plans: add the v2-workspaces-create-test-plan covering this PR's divergences from v1. * docs(plans): add comprehensive input × state matrix for workspaces.create Enumerates every input combination (PR vs branch mode, typed vs absent name/branch, taskIds, explicit id) crossed with the relevant pre-existing on-disk / DB state (idempotency, adoption, tag, remote-only ref, base-branch existence). Plus the failure/rollback paths and the post-refactor AI-rename behavior table. Drives the smoke-test plan and surfaces the regressions vs v1. * refactor(host-service): inline AI naming in workspaces.create Move AI naming (workspace title + branch) inline so it lands in the same v2Workspace.create round-trip rather than firing a post-create rename + Electric resync. Also overlap host.ensure with the git work. Restructure: - `host.ensure` kicks off at the top, awaited inside `registerCloudAndLocal` — keeps the cloud round-trip off the critical path. - AI naming kicks off in parallel when the user supplied a prompt but left at least one of (name, branch) blank. - Auto-gen branch path: `Promise.all([aiPromise, resolveNewBranchStartPoint, listBranchNames])` — branch name = AI suggestion (deduped), falling back to a friendly random when the LLM is absent or fails. No generated-then-renamed branch and no `git branch -m`. - Typed branch path: `Promise.all([planBranchSource, aiPromise])`. Existing-branch detection still uses planBranchSource; AI title rename can race with that lookup. - PR path skips AI naming entirely (PR title + derived branch are meaningful). Bench (presentations repo, p50, 5 runs each): typed branch, no prompt: 2400ms (no LLM) typed branch, with prompt: 2044ms (AI title only) auto-gen branch, with prompt: 1858ms (AI both) Inline-LLM is essentially free — the LLM (~700ms) overlaps with the ~900ms `git fetch`/`resolveStartPoint` step. v2_workspaces gets the right name from the start; no friendly-name flash. * fix(host-service): wire PR-checkout recovery into workspaces.create The recovery utility (`recoverPrCheckoutAfterGhFailure`) shipped on main via #3725, but was wired into the old `workspaceCreation.checkout` procedure that this PR deleted. Without recovery, `workspaces.create` fails for any merged PR whose head branch has been deleted from the remote — a common case post-merge. Plug the same recovery flow into the new PR-create catch block: - fetch prMetadata.headRefOid via `gh pr view` - on `gh pr checkout` failure, try the synthetic-pr-ref fallback or FETCH_HEAD recovery (utility decides based on the error) - rollback + throw only when recovery declined; otherwise continue past the catch with the worktree on FETCH_HEAD * fix(host-service): adopt PR-pre-existing local branch when OID matches When the local repo already has the PR's head branch (typical after a prior `gh pr checkout` outside Superset), compare its OID to `prMetadata.headRefOid`: - match → adopt the existing branch into a new worktree (no `gh pr checkout`, no `--force`, no data-loss risk) - diverge → CONFLICT with a specific message naming both OIDs Removes the unconditional CONFLICT for stale-but-clean PR branches — the dominant case for engineers who review teammates' PRs locally. Diverged-branch case still blocks (user has commits the PR doesn't). * feat(db): regenerate workspace_tasks migration as 0044 after merge Main landed 0043_submitted_prompts so our original 0043_add_workspace_tasks collided. Regenerated via `drizzle-kit generate` so it slots in cleanly on top of main's 0043. * chore: drop pr-test.ts dev bench script * chore: drop unused _navigate in DashboardSidebarWorkspaceItem * feat: stitch linked-context bodies into agent prompt + workspace task M:1 - Client-side prompt builder fetches PR/GitHub-issue/internal-task bodies optimistically on link-time and stitches them into the agent prompt at submit. Bodies are cached in a module-scoped Zustand store keyed by source:id so close-and-reopen of the modal reuses fetched content. The agent now spawns whenever ANY context (typed prompt, linked PR, linked issue, linked task, attachments) is present, and the host-side schema is relaxed to allow attachment-only launches. - Split the GH content fetches off the workspace-creation router into pullRequests.getContent and a new issues.getContent (issuesRouter) so the names reflect what they are (general PR/issue body fetches) rather than what flow first needed them. - Replace the workspace_tasks join table with a nullable task_id FK column on v2_workspaces. The product is one-task-per-workspace; M:N was over-engineered. Collapses linkTask + unlinkTask into one setTask({ workspaceId, taskId | null }) mutation. Updates the renderer, host service, mcp-v2, sdk, and trpc routers accordingly. Migration 0044 regenerated as add_task_id_to_v2_workspaces. - Drop completed plan / spec docs that were carried on this branch. --------- Co-authored-by: Satya Patel <satyapatel111@gmail.com>
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
…(PR1) (superset-sh#3893) * docs(plans): add v2 workspace create canonical refactor specs Two design docs guiding a clean reimplementation of the canonical workspace.create() flow: - 20260425-canonical-workspace-create-flow.md: umbrella design covering the unified host-service create API, host-scoped attachments, pane store registry, prompt boundary, and PR sequencing (PRs 1-7). - 20260425-host-agent-configs-pr1.md: PR 1 spec for the host-runtime agent config model (host_agent_configs table + settings router). Sourced from the prior v2-workspace-create-canonical branch so the reimplementation can land PR-by-PR per the plan. * docs(plans): switch host agent configs to argv-array launch spec Replace the single `launchCommand` string with a structured argv-array shape (`command` + `args[]` + `promptArgs[]` + `env`) matching VS Code ITerminalProfile / Tabby Shell / WezTerm SpawnCommand / Zellij panes. Empty launches drop `promptArgs` automatically, so codex/opencode/copilot no longer carry their prompt-mode flags into no-prompt sessions. Storing argv directly avoids shell-quoting bugs and makes prompt injection a list push instead of string concatenation. Adds first-class `env` overlay and the per-preset breakdown for the 8 in-scope agents. Implementation in PR1 will be redone against this shape in a separate branch; this branch is now docs-only. * docs(plans): address review on prompt contract + PR1 scope canonical-workspace-create-flow.md: - Make `prompt` required on agent launches in both code blocks (was optional in the API shape but later text said agent launches require one and promptless = raw terminal). Reconciled to the stricter rule and added a Notes line pointing readers to the Agent Configs section. 20260425-host-agent-configs-pr1.md: - Tighten the Summary to make explicit that this PR ships only the storage + tRPC router + V2 settings page. V2 modal and launch dispatch remain on legacy desktop presets and move to host configs in PR 5. * wip: canonical workspace create flow + optimistic attachment uploads Consolidate in-flight create lifecycle into one WorkspaceCreatesManager + useWorkspaceCreates hook (submit/retry/dismiss). Navigate immediately on submit; route renders creating/error/notfound states from the in-flight store. Sidebar reads the same store and supports dismiss on hover for failed entries. Optimistic attachment uploads via a module-scoped Zustand store keyed (fileId, hostUrl). Files upload to whichever host was active when added; switching hosts hides their pills without re-uploading and keeps cached attachment ids for return visits. Submit awaits in-flight uploads and joins file metadata for error messaging. Submit-time gating moved into handleSubmit so all paths (button, Enter, Cmd+Enter) respect preconditions. Plain Enter inserts a newline; submit is button or Cmd+Enter only. Server-side: workspaces.create detects an existing standard-path worktree and adopts it instead of failing on git worktree add — folds the picker's adopt path into the same submit. Layout for v2-workspace falls through to the page when no real row exists so the in-flight UI can render. Other cleanup: drop legacy in-flight-creates store + reconciler, the v2 useCreateWorkspace wrapper, useAdoptWorktree; collapse picker checkout+adopt callbacks; UploadingAttachmentPill replaces the library pill with subtle status overlays; PR link command keeps the Show closed checkbox (pagination concern); useShallow on the draft selector to fix an infinite-loop bug. * fix: drop simple-git --quiet on rev-parse and ship membership claim - refExists/localBranchExists/remoteExists: simple-git's `raw` resolves successfully with empty stdout when --quiet is on, so every "new branch" was being treated as already existing. Drop the flag and validate a 40+ hex sha was actually printed. - auth: include the user's full membership list in OAuth access-token claims so cross-org JWT checks pass downstream. - org-resource-access: split the not-found / wrong-org error paths so org-mismatch reports the actual ids in the diagnostic. - pull-requests: skip refresh for workspaces whose worktree was deleted on disk; simple-git would otherwise throw a confusing directory-does-not-exist error. - git/utils: stop logging on missing remote/origin — both null cases are expected and callers handle them. * chore(cli): override env vars from workspace .env in dev script Without -o, dotenv-cli leaves any pre-existing process.env vars alone, so a shell-level `SUPERSET_HOME_DIR=~/.superset` (set globally for the production CLI) wins over the workspace's `.env` value and dev CLI ops land in the prod data dir instead of `superset-dev-data/`. * feat(cli): add agents run command + variadic flag support - cli-framework: extend `.variadic()` to string flags so repeated `--flag value` invocations accumulate into an array. Drop the auto-`isRequired: true` from `.variadic()` — both existing positional callsites already chain `.required()` explicitly, no behavior change. - cli: new `superset agents run --workspace <id> --agent <preset|id> --prompt <text> [--attachment-id <uuid>]...` for spawning an agent inside an existing workspace. Wraps the host service `agents.run` mutation; resolves the workspace's host via the cloud lookup. * refactor: server-generate workspace name + per-side AI rename gate The renderer no longer generates a friendly-random fallback. Both `name` and `branch` are now optional on the create input — the server picks a friendly random for whichever side is missing, and the AI rename only replaces the side(s) the user didn't supply. - `workspaces.create` schema: drop `autogenerateName`, make `name` optional, allow both `branch` and `pr` to be absent (refine relaxed to "not both set"). Server generates+dedupes a friendly branch name when `branch` is undefined; falls back to PR title or branch for workspace name when `name` is undefined. - `applyAiWorkspaceRename`: take `renameTitle` / `renameBranch` flags; only apply the side the caller asked for. Both `true` from the manual `aiRename` mutation; computed per-create-input on the branch path; skipped entirely on the PR path. - Renderer: drop `friendlyFallback` from the draft store + context + `PromptGroup` placeholder; `resolveNames` returns `string | null`; `useSubmitWorkspace` sends only what the user typed (no synthetic name). In-flight states show "Creating workspace" with no subtitle when the name is absent. - plans: add the v2-workspaces-create-test-plan covering this PR's divergences from v1. * docs(plans): add comprehensive input × state matrix for workspaces.create Enumerates every input combination (PR vs branch mode, typed vs absent name/branch, taskIds, explicit id) crossed with the relevant pre-existing on-disk / DB state (idempotency, adoption, tag, remote-only ref, base-branch existence). Plus the failure/rollback paths and the post-refactor AI-rename behavior table. Drives the smoke-test plan and surfaces the regressions vs v1. * refactor(host-service): inline AI naming in workspaces.create Move AI naming (workspace title + branch) inline so it lands in the same v2Workspace.create round-trip rather than firing a post-create rename + Electric resync. Also overlap host.ensure with the git work. Restructure: - `host.ensure` kicks off at the top, awaited inside `registerCloudAndLocal` — keeps the cloud round-trip off the critical path. - AI naming kicks off in parallel when the user supplied a prompt but left at least one of (name, branch) blank. - Auto-gen branch path: `Promise.all([aiPromise, resolveNewBranchStartPoint, listBranchNames])` — branch name = AI suggestion (deduped), falling back to a friendly random when the LLM is absent or fails. No generated-then-renamed branch and no `git branch -m`. - Typed branch path: `Promise.all([planBranchSource, aiPromise])`. Existing-branch detection still uses planBranchSource; AI title rename can race with that lookup. - PR path skips AI naming entirely (PR title + derived branch are meaningful). Bench (presentations repo, p50, 5 runs each): typed branch, no prompt: 2400ms (no LLM) typed branch, with prompt: 2044ms (AI title only) auto-gen branch, with prompt: 1858ms (AI both) Inline-LLM is essentially free — the LLM (~700ms) overlaps with the ~900ms `git fetch`/`resolveStartPoint` step. v2_workspaces gets the right name from the start; no friendly-name flash. * fix(host-service): wire PR-checkout recovery into workspaces.create The recovery utility (`recoverPrCheckoutAfterGhFailure`) shipped on main via superset-sh#3725, but was wired into the old `workspaceCreation.checkout` procedure that this PR deleted. Without recovery, `workspaces.create` fails for any merged PR whose head branch has been deleted from the remote — a common case post-merge. Plug the same recovery flow into the new PR-create catch block: - fetch prMetadata.headRefOid via `gh pr view` - on `gh pr checkout` failure, try the synthetic-pr-ref fallback or FETCH_HEAD recovery (utility decides based on the error) - rollback + throw only when recovery declined; otherwise continue past the catch with the worktree on FETCH_HEAD * fix(host-service): adopt PR-pre-existing local branch when OID matches When the local repo already has the PR's head branch (typical after a prior `gh pr checkout` outside Superset), compare its OID to `prMetadata.headRefOid`: - match → adopt the existing branch into a new worktree (no `gh pr checkout`, no `--force`, no data-loss risk) - diverge → CONFLICT with a specific message naming both OIDs Removes the unconditional CONFLICT for stale-but-clean PR branches — the dominant case for engineers who review teammates' PRs locally. Diverged-branch case still blocks (user has commits the PR doesn't). * feat(db): regenerate workspace_tasks migration as 0044 after merge Main landed 0043_submitted_prompts so our original 0043_add_workspace_tasks collided. Regenerated via `drizzle-kit generate` so it slots in cleanly on top of main's 0043. * chore: drop pr-test.ts dev bench script * chore: drop unused _navigate in DashboardSidebarWorkspaceItem * feat: stitch linked-context bodies into agent prompt + workspace task M:1 - Client-side prompt builder fetches PR/GitHub-issue/internal-task bodies optimistically on link-time and stitches them into the agent prompt at submit. Bodies are cached in a module-scoped Zustand store keyed by source:id so close-and-reopen of the modal reuses fetched content. The agent now spawns whenever ANY context (typed prompt, linked PR, linked issue, linked task, attachments) is present, and the host-side schema is relaxed to allow attachment-only launches. - Split the GH content fetches off the workspace-creation router into pullRequests.getContent and a new issues.getContent (issuesRouter) so the names reflect what they are (general PR/issue body fetches) rather than what flow first needed them. - Replace the workspace_tasks join table with a nullable task_id FK column on v2_workspaces. The product is one-task-per-workspace; M:N was over-engineered. Collapses linkTask + unlinkTask into one setTask({ workspaceId, taskId | null }) mutation. Updates the renderer, host service, mcp-v2, sdk, and trpc routers accordingly. Migration 0044 regenerated as add_task_id_to_v2_workspaces. - Drop completed plan / spec docs that were carried on this branch. --------- Co-authored-by: Satya Patel <satyapatel111@gmail.com>
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
gh pr checkoutfails after a merged PR branch or fork is unavailablerefs/pull/<PR>/headref and verify it againstheadRefOidbefore creating the local branchheadRefOidthrough the PR content and checkout payload, with focused recovery testsRoot Cause
gh pr checkout --branch ... --forcecan fail after the PR head branch is deleted even though GitHub still exposes the PR head commit through the base repository pull ref. The previous v2 flow surfaced that as workspace creation failure.Validation
bun test packages/host-service/src/trpc/router/workspace-creation/utils/pr-branch-name.test.ts packages/host-service/src/trpc/router/workspace-creation/utils/pr-checkout-recovery.test.ts apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.tsbunx @biomejs/biome@2.4.2 check ...bun run --cwd packages/host-service typecheckbun run --cwd apps/desktop typecheckSummary by cubic
Recover failed PR checkouts when the PR head branch or fork is gone by fetching and verifying GitHub’s synthetic
refs/pull/<N>/heador a verifiedFETCH_HEAD, and link PRs to workspaces so the sidebar stays accurate. Also read the terminal daemon org id from request context or env to prevent test-time crashes.New Features
gh pr checkoutviarefs/pull/<PR>/heador verifiedFETCH_HEAD, checked againstheadRefOid, with a clear recovery warning.headRefOid,headRepositoryName,isDraft, and timestamps end-to-end.Bug Fixes
pr/<number>so the server can recover viarefs/pull/<PR>/head.process.envread in the daemon client.Written for commit d04b818. Summary will update on new commits.
Summary by CodeRabbit