refactor(host-service): split workspace-creation router into focused files#3693
Open
refactor(host-service): split workspace-creation router into focused files#3693
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new, modular workspace-creation subsystem to the host service: git/worktree discovery and management, branch name generation, GitHub issue/PR fetching and searching, progress tracking, and multiple protected TRPC procedures for creating, checking out, and adopting workspaces. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant TRPC as TRPC Handler
participant Git as Git (git/gh)
participant GitHub as GitHub API
participant Host as Host Service
participant DB as Database
Client->>TRPC: create/checkout/adopt request
TRPC->>DB: validate project row
TRPC->>Git: list refs / worktrees / resolve default branch
alt PR flow
TRPC->>GitHub: fetch PR metadata
TRPC->>Git: create worktree, gh pr checkout -> branch
else Branch flow
TRPC->>Git: resolve ref, fetch remote-tracking if needed
TRPC->>Git: git worktree add (possibly -b / --track)
end
TRPC->>Git: set push.autoSetupRemote (worktree-config)
TRPC->>Host: finishCheckout (ensure host, create cloud workspace)
Host->>DB: insert local `workspaces` row
Host->>Git: check/run .superset/setup.sh (optional)
Host-->>TRPC: result { workspace, terminals?, warnings }
TRPC-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
⚔️ Resolve merge conflicts
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 refactors the 1790-line Confidence Score: 5/5Safe to merge — pure structural refactor with no behavioral changes and all tests passing. All 72 tests pass, typechecks clean, and lint is green. The only finding is a P2 style suggestion (SQL vs JS filtering in adopt.ts) that doesn't affect correctness. Every extracted module faithfully mirrors the original monolith logic. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts | Reduced from 1790 lines to a 27-line router composition that imports and re-exports all procedures; no logic changes. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts | Extracted adopt procedure; stale workspace lookup loads all project workspaces in memory then JS-filters by branch instead of filtering in SQL — minor inefficiency, no behavioral change. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | Extracted checkout procedure covering both branch and PR paths; delegates to finishCheckout for the shared postlude. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts | Extracted create procedure with branch deduplication, start-point resolution, worktree creation, and fire-and-forget AI rename — behavior unchanged. |
| packages/host-service/src/trpc/router/workspace-creation/finish-checkout.ts | Shared postlude for checkout and PR-checkout paths; extracted cleanly with no logic changes. |
| packages/host-service/src/trpc/router/workspace-creation/branch-helpers.ts | Git worktree/branch enumeration helpers extracted from the monolith; all four functions are identical to their original forms. |
| packages/host-service/src/trpc/router/workspace-creation/progress.ts | In-memory progress map and step definitions extracted; types promoted from private interfaces to exported ones, required by consuming procedure files. |
| packages/host-service/src/trpc/router/workspace-creation/helpers.ts | Shared helpers (GitClient type alias, projectNotSetupError, safeResolveWorktreePath, resolveGithubRepo) extracted cleanly. |
| packages/host-service/src/trpc/router/workspace-creation/github-schemas.ts | IssueSchema and PrSchema Zod definitions moved from the bottom of the monolith into their own file. |
| packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts | searchBranches procedure with remote-fetch TTL, cursor-based pagination, and worktree/branch filtering — extracted with module-level state (lastRemoteRefetch Map) preserved correctly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
WC[workspace-creation.ts\nrouter composition] --> P1[procedures/create.ts]
WC --> P2[procedures/checkout.ts]
WC --> P3[procedures/adopt.ts]
WC --> P4[procedures/search-branches.ts]
WC --> P5[procedures/get-context.ts]
WC --> P6[procedures/generate-branch-name.ts]
WC --> P7[procedures/get-progress.ts]
WC --> P8[procedures/search-github-issues.ts]
WC --> P9[procedures/search-pull-requests.ts]
WC --> P10[procedures/get-github-issue-content.ts]
WC --> P11[procedures/get-github-pull-request-content.ts]
P1 --> FC[finish-checkout.ts]
P2 --> FC
P1 --> BH[branch-helpers.ts]
P3 --> BH
P4 --> BH
P1 --> PR[progress.ts]
P2 --> PR
FC --> PR
P2 --> H[helpers.ts]
P3 --> H
P4 --> H
P8 --> GS[github-schemas.ts]
P9 --> GS
P10 --> GS
P11 --> GS
Comments Outside Diff (1)
-
packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts, line 560-565 (link)In-memory branch filter instead of SQL predicate
The stale-row lookup fetches every workspace row for the project and then filters by
branchin JavaScript. For a project with many branches this loads unnecessary data; the branch filter should be pushed into the SQLWHEREclause. Sinceandis not currently imported, this may have been a deliberate trade-off, but it's easy to fix:Note:
andwould need to be added to thedrizzle-ormimport at the top of the file.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts Line: 560-565 Comment: **In-memory branch filter instead of SQL predicate** The stale-row lookup fetches every workspace row for the project and then filters by `branch` in JavaScript. For a project with many branches this loads unnecessary data; the branch filter should be pushed into the SQL `WHERE` clause. Since `and` is not currently imported, this may have been a deliberate trade-off, but it's easy to fix: Note: `and` would need to be added to the `drizzle-orm` import at the top of the file. How can I resolve this? If you propose a fix, please make it concise.
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/adopt.ts
Line: 560-565
Comment:
**In-memory branch filter instead of SQL predicate**
The stale-row lookup fetches every workspace row for the project and then filters by `branch` in JavaScript. For a project with many branches this loads unnecessary data; the branch filter should be pushed into the SQL `WHERE` clause. Since `and` is not currently imported, this may have been a deliberate trade-off, but it's easy to fix:
Note: `and` would need to be added to the `drizzle-orm` import at the top of the file.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "refactor(host-service): split workspace-..." | Re-trigger Greptile
![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 17 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/search-pull-requests.ts">
<violation number="1" location="packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts:11">
P2: Validate `limit` as an integer before passing it to `per_page` to avoid invalid GitHub API requests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| z.object({ | ||
| projectId: z.string(), | ||
| query: z.string().optional(), | ||
| limit: z.number().min(1).max(100).optional(), |
Contributor
There was a problem hiding this comment.
P2: Validate limit as an integer before passing it to per_page to avoid invalid GitHub API requests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts, line 11:
<comment>Validate `limit` as an integer before passing it to `per_page` to avoid invalid GitHub API requests.</comment>
<file context>
@@ -0,0 +1,79 @@
+ z.object({
+ projectId: z.string(),
+ query: z.string().optional(),
+ limit: z.number().min(1).max(100).optional(),
+ }),
+ )
</file context>
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![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: 3
🧹 Nitpick comments (12)
packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts (1)
43-48: Consider preserving the original error ascause.Flattening the underlying error to just a message drops the stack/typed info, which makes debugging
ghfailures and schema-validation failures harder (and will appear as a generic 500 to clients regardless of root cause — auth, NOT_FOUND from gh, zod parse, etc.). Attachingcausekeeps the original for server-side logs without changing the client-facing code.♻️ Suggested change
} catch (err) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: `Failed to fetch PR #${input.prNumber}: ${err instanceof Error ? err.message : String(err)}`, + cause: err, }); }🤖 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/get-github-pull-request-content.ts` around lines 43 - 48, In the catch block of get-github-pull-request-content (where a TRPCError is thrown), preserve the original error by passing it as the cause property to the TRPCError instead of only flattening its message; keep the current message formatting but add cause: err (ensuring err is forwarded as-is or wrapped if necessary) so downstream logging/stack/typed info from the original gh/zod/auth error is retained while the client-facing message stays unchanged.packages/host-service/src/trpc/router/workspace-creation/procedures/get-context.ts (1)
14-29: Optional: prefer an explicit return type overas string | null.The
as string | nullon Line 18 exists only to widen the no-project-branch so bothreturns unify. An explicit handler return type (or a sharedGetContextResulttype) is a bit cleaner and keeps the type discipline outside the object literals.♻️ Suggested shape
- .query(async ({ ctx, input }) => { + .query(async ({ ctx, input }): Promise<{ + projectId: string; + hasLocalRepo: boolean; + defaultBranch: string | null; + }> => { const localProject = ctx.db.query.projects .findFirst({ where: eq(projects.id, input.projectId) }) .sync(); if (!localProject) { return { projectId: input.projectId, hasLocalRepo: false, - defaultBranch: null as string | null, + defaultBranch: null, }; }🤖 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/get-context.ts` around lines 14 - 29, The two return branches in get-context.ts currently use an inline cast "as string | null" to unify shapes; instead declare and use an explicit return type (e.g., export or local GetContextResult) for the function that includes projectId: string, hasLocalRepo: boolean, and defaultBranch: string | null, then remove the inline cast from the no-local-repo return and let both returns conform to that declared type; locate the function that uses localProject, resolveDefaultBranchName, and ctx.git to apply the explicit return type to its signature so both object literals type-check without "as".packages/host-service/src/trpc/router/workspace-creation/finish-checkout.ts (1)
60-69:worktree removewithout--forcemay not actually clean up on rollback.
git worktree removerefuses to remove a worktree with untracked or modified files. Since the setup flow might partially populate the worktree (e.g.,gh pr checkoutside-effects before failure), considerworktree remove --forcehere to match the cleanup path incheckout.ts:169. The current code merely logs a warning and leaves behind an orphan worktree directory + git registration.♻️ Suggested change
const rollbackWorktree = async () => { try { - await args.git.raw(["worktree", "remove", args.worktreePath]); + await args.git.raw(["worktree", "remove", "--force", args.worktreePath]); } catch (err) { console.warn("[workspaceCreation.checkout] failed to rollback worktree", { worktreePath: args.worktreePath, err, }); } };🤖 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/finish-checkout.ts` around lines 60 - 69, The rollbackWorktree async function currently calls args.git.raw(["worktree", "remove", args.worktreePath]) which can fail to remove worktrees with untracked/modified files; change the call to include the --force flag (e.g., args.git.raw(["worktree", "remove", "--force", args.worktreePath"])) so the rollback cleans up the worktree directory and git registration like the cleanup in checkout.ts (line referenced in review); keep the existing try/catch and error logging around rollbackWorktree to preserve diagnostics.packages/host-service/src/trpc/router/workspace-creation/branch-helpers.ts (2)
26-43: Path-comparison asymmetry withfindWorktreeAtPath.
listWorktreeBranchescomparescurrentPath(raw string fromgit worktree list) directly againstworktreesRoot + sep, whereasfindWorktreeAtPath(lines 64, 72) correctlyresolve()s both sides before comparing. IfrepoPatharrives non-canonical (trailing slashes, symlink segments, or mixed separators on another platform),startsWith(worktreesRoot + sep)can false-negative and silently drop a Superset-managed worktree from the returnedworktreeMap.♻️ Suggested change
- for (const line of raw.split("\n")) { + for (const line of raw.split("\n")) { if (line.startsWith("worktree ")) { - currentPath = line.slice("worktree ".length).trim(); + currentPath = resolve(line.slice("worktree ".length).trim()); } else if (line.startsWith("branch refs/heads/") && currentPath) { const branch = line.slice("branch refs/heads/".length).trim(); if (!branch) continue; checkedOutBranches.add(branch); // Superset-managed worktrees live under <repoPath>/.worktrees/<name>; // the primary working tree is at repoPath itself and skipped here. if (currentPath.startsWith(worktreesRoot + sep)) { worktreeMap.set(branch, currentPath); }🤖 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/branch-helpers.ts` around lines 26 - 43, listWorktreeBranches currently compares the raw git worktree path string (currentPath) against worktreesRoot + sep which can miss matches if repoPath isn’t canonical; change the comparison to canonicalize both sides before checking (like path.resolve(currentPath).startsWith(path.resolve(worktreesRoot) + sep)) so Superset-managed worktrees are correctly detected and added to worktreeMap and checkedOutBranches (mirror the approach used in findWorktreeAtPath). Ensure you import/use the same path.resolve helper used elsewhere and update the conditional that sets worktreeMap accordingly.
10-86: Duplication betweenlistWorktreeBranchesandfindWorktreeAtPath.Both functions independently parse
git worktree list --porcelainoutput with the same state machine (worktree …→branch refs/heads/…). Consider extracting a singleparseWorktreePorcelain(raw): Array<{ path: string; branch: string | null }>helper; the two public functions then reduce to small, testable transformations over the parsed structure. Worth doing only if this parsing surface grows further — leaving this as an optional nit.🤖 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/branch-helpers.ts` around lines 10 - 86, Extract the duplicated parsing logic used in listWorktreeBranches and findWorktreeAtPath into a single helper parseWorktreePorcelain(raw: string): Array<{ path: string; branch: string | null }>, then make listWorktreeBranches call git.raw(...) and feed the output into parseWorktreePorcelain to build worktreeMap and checkedOutBranches, and make findWorktreeAtPath call git.raw(...), feed into parseWorktreePorcelain and then search the returned array for an entry with resolve(path) === resolve(worktreePath) and branch === expectedBranch; keep existing error handling and return types in listWorktreeBranches and findWorktreeAtPath.packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts (3)
92-92: Type annotation is looser than the actual return type.
resolveDefaultBranchNamealways returns astring(fallback"main"on error — seepackages/host-service/src/runtime/git/refs.tslines 132–145). The: string | nullannotation here and thedefaultBranch: null as string | nullin the early return at line 73 imply a nullable contract that the implementation never produces. Worth aligning the types so the downstream renderer doesn't over-handle a never-occurringnull.🤖 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/search-branches.ts` at line 92, The variable defaultBranch should be typed as string (not string | null) to match resolveDefaultBranchName's actual return type; change the declaration of defaultBranch to string and remove the nullable union, and update the early-return object that currently sets defaultBranch: null as string | null to provide a concrete string (e.g., "main") so the type aligns with resolveDefaultBranchName and downstream code no longer treats defaultBranch as nullable.
103-111: Load only thebranchcolumn forworkspaceBranches.The current
ctx.db.select().from(workspaces).where(…)pulls every column of every workspace row for this project just to extract.branch. A projected query reduces memory/CPU for projects with many workspaces and makes the intent clearer.♻️ Suggested change
- const workspaceBranches = new Set<string>( - ctx.db - .select() - .from(workspaces) - .where(eq(workspaces.projectId, input.projectId)) - .all() - .map((w) => w.branch) - .filter((b): b is string => !!b), - ); + const workspaceBranches = new Set<string>( + ctx.db + .select({ branch: workspaces.branch }) + .from(workspaces) + .where(eq(workspaces.projectId, input.projectId)) + .all() + .map((w) => w.branch) + .filter((b): b is string => !!b), + );🤖 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/search-branches.ts` around lines 103 - 111, The query building workspaceBranches currently selects all columns then maps to .branch; change it to project only the branch column to avoid loading full rows: update the ctx.db.select().from(workspaces).where(eq(workspaces.projectId, input.projectId)).all() call used to build workspaceBranches so it selects workspaces.branch (or uses a projection API like ctx.db.select({ branch: workspaces.branch }).from(...)) before mapping/filtering; keep the Set construction, filtering predicate (b is string) and variable name workspaceBranches unchanged.
40-50:lastRemoteRefetchmap is unbounded.Every distinct
projectIdever requested withrefresh: trueleaves a permanent entry inlastRemoteRefetchfor the life of the process. Small per-entry, and keyed by a usually-bounded set of projects on a host, so this is effectively fine — but worth a note. If it ever becomes a concern, a periodic sweep mirroringsweepStaleProgressinprogress.tswould fit the existing pattern.🤖 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/search-branches.ts` around lines 40 - 50, The lastRemoteRefetch Map used by shouldRefetchRemote and markRefetchRemote is unbounded and can grow forever; add a periodic cleanup (similar to sweepStaleProgress in progress.ts) to remove entries older than REMOTE_REFETCH_TTL_MS (or another configured threshold) so stale projectId keys are purged; implement the sweep as a timer-based task in the same module (or wired into existing cleanup routines) that iterates lastRemoteRefetch and deletes entries whose timestamp is older than Date.now() - REMOTE_REFETCH_TTL_MS, ensuring REMOTE_REFETCH_TTL_MS, lastRemoteRefetch, shouldRefetchRemote and markRefetchRemote remain the single sources of truth.packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts (1)
68-70: Minor:refineusesBoolean()XOR, which treats empty strings andundefinedthe same.
Boolean(v.branch) !== Boolean(v.pr)correctly enforces XOR, but becauseBoolean("")isfalse, a client can submitbranch: ""alongsidepr: {...}and still pass validation (then the PR path is taken and the empty branch is ignored). Also,branch: ""on its own passes the refine (false !== falseisfalse, so actually rejected — correct), butbranch: " "(whitespace) would pass the refine and then hit the dedicated empty check at lines 225–232. Current behavior is safe, but an explicit.transform((v) => ({ ...v, branch: v.branch?.trim() || undefined }))ahead of.refine()would make the invariant clearer.🤖 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 68 - 70, The XOR refine currently uses Boolean(v.branch) !== Boolean(v.pr) which treats empty/whitespace branches as truthy; add a prior transformation on the input object (e.g., via .transform((v) => ({ ...v, branch: v.branch?.trim() || undefined }))) before the .refine call so branch values that are empty or only whitespace become undefined, then keep the existing .refine((v) => Boolean(v.branch) !== Boolean(v.pr), ...) check to enforce exactly-one semantics; reference the checkout.ts schema where v.branch and v.pr are validated and the existing .refine invocation.packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts (1)
108-116: Consider filtering in SQL and wrapping in a transaction.Two minor improvements for the stale-row reconciliation:
- The
.find((w) => w.branch === branch)after loading all project rows could be pushed into the SQLWHEREclause (and(eq(workspaces.projectId, …), eq(workspaces.branch, branch))) to avoid loading the full set into memory.- The subsequent
delete+insertare two separate statements; wrapping them in a transaction would avoid leaving the local DB in a partially-reconciled state if the insert fails.♻️ Proposed refactor
- const stale = ctx.db - .select() - .from(workspaces) - .where(eq(workspaces.projectId, input.projectId)) - .all() - .find((w) => w.branch === branch); - if (stale && stale.id !== cloudRow.id) { - ctx.db.delete(workspaces).where(eq(workspaces.id, stale.id)).run(); - } - - ctx.db - .insert(workspaces) - .values({ - id: cloudRow.id, - projectId: input.projectId, - worktreePath, - branch, - }) - .run(); + ctx.db.transaction((tx) => { + const stale = tx + .select() + .from(workspaces) + .where( + and( + eq(workspaces.projectId, input.projectId), + eq(workspaces.branch, branch), + ), + ) + .get(); + if (stale && stale.id !== cloudRow.id) { + tx.delete(workspaces).where(eq(workspaces.id, stale.id)).run(); + } + tx.insert(workspaces) + .values({ + id: cloudRow.id, + projectId: input.projectId, + worktreePath, + branch, + }) + .run(); + });🤖 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/adopt.ts` around lines 108 - 116, The stale-row reconciliation currently loads all project rows and filters in memory then performs separate delete and insert operations; change the selection to query the workspaces table with both eq(workspaces.projectId, input.projectId) and eq(workspaces.branch, branch) so you fetch at most the matching stale row (replace the .select().from(...).where(...).all().find(...) pattern with a single where including branch), and wrap the delete of the stale workspace and the subsequent insert of cloudRow into a single transaction using ctx.db.transaction or the DB's transaction helper to ensure they succeed or rollback together (refer to the workspaces table, ctx.db.delete(workspaces).where(...), and the code that inserts cloudRow).packages/host-service/src/trpc/router/workspace-creation/progress.ts (2)
22-35: TightenactiveStepIdto a typed union; unknown values currently mark all steps as done.If
activeStepIddoesn't match any entry inSTEP_DEFINITIONS, theif (!reachedActive)branch runs for every step and the whole progress state is set to"done"— a misleading terminal state. The three known call sites pass literals, but the signatureactiveStepId: stringdoesn't surface this contract at compile time.♻️ Suggested change
-export const STEP_DEFINITIONS = [ +export const STEP_DEFINITIONS = [ { id: "ensuring_repo", label: "Ensuring local repository" }, { id: "creating_worktree", label: "Creating worktree" }, { id: "registering", label: "Registering workspace" }, ] as const; + +export type ProgressStepId = (typeof STEP_DEFINITIONS)[number]["id"]; -export function setProgress(pendingId: string, activeStepId: string): void { +export function setProgress(pendingId: string, activeStepId: ProgressStepId): void {🤖 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/progress.ts` around lines 22 - 35, The function setProgress currently accepts activeStepId: string which lets unknown values mark every step as "done"; change the parameter to a tightened union type derived from STEP_DEFINITIONS (e.g. declare type ActiveStepId = typeof STEP_DEFINITIONS[number]["id"] and use activeStepId: ActiveStepId in setProgress) and add a runtime guard: look up STEP_DEFINITIONS.find(d => d.id === activeStepId) before mapping and if not found, log/warn and treat as "no active step" (mark all steps "pending" or otherwise avoid marking everything "done") so createProgress.set(pendingId, ...) never records a misleading all-done state; update references to setProgress accordingly and keep createProgress.set usage unchanged.
20-20: Unbounded module-levelMap— confirmsweepStaleProgressis invoked.
createProgressis a process-wide Map. IfsweepStaleProgressis only called from withinget-progress.ts(on the poll path), an abandoned creation flow whose client never polls will never get swept — the entries accumulate for the lifetime of the host-service process. Low risk given the 5-minute cutoff and small per-entry size, but consider kicking off asetInterval-based sweeper at module init (cleared on shutdown) or ensuring everysetProgresscaller eventuallyclearProgresses on both success and failure paths. Looking atcheckout.ts, all error paths do callclearProgress, which is good.🤖 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/progress.ts` at line 20, The module-level Map createProgress can grow if sweepStaleProgress is only invoked on the poll path; to fix, start a periodic sweeper at module init that calls sweepStaleProgress (e.g., setInterval with the same cutoff period or a reasonable cadence) and ensure the interval is cleared on shutdown; also verify callers that mutate createProgress (setProgress, clearProgress, and checkout.ts paths) still clear on both success and failure so the periodic sweeper is a safety net rather than the only cleanup mechanism.
🤖 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/helpers.ts`:
- Around line 19-35: In safeResolveWorktreePath, tighten the path-traversal
check to also reject branchName values that resolve exactly to the worktrees
root (e.g., "", ".", "./"); update the conditional that currently allows
worktreePath === worktreesRoot so it throws a TRPCError when worktreePath ===
worktreesRoot OR when worktreePath does not start with worktreesRoot + sep,
ensuring any equal-to-root resolution is treated as invalid and prevented from
returning worktreesRoot.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts`:
- Around line 290-298: Wrap the local insert
(ctx.db.insert(workspaces).values({...}).run()) in a try/catch so that if the
insert throws you rollback the on-disk worktree and clear progress and then
remove the previously-created cloud row (use the same v2Workspace deletion API
you used to create cloudRow — pass cloudRow.id) before rethrowing the error;
mirror the error-handling pattern used around ensureV2Host and
v2Workspace.create (call the worktree rollback function and
clearProgress/clearCreateProgress, then delete the cloudRow by id) so no
cloud-only orphan remains if the local insert fails.
---
Nitpick comments:
In `@packages/host-service/src/trpc/router/workspace-creation/branch-helpers.ts`:
- Around line 26-43: listWorktreeBranches currently compares the raw git
worktree path string (currentPath) against worktreesRoot + sep which can miss
matches if repoPath isn’t canonical; change the comparison to canonicalize both
sides before checking (like
path.resolve(currentPath).startsWith(path.resolve(worktreesRoot) + sep)) so
Superset-managed worktrees are correctly detected and added to worktreeMap and
checkedOutBranches (mirror the approach used in findWorktreeAtPath). Ensure you
import/use the same path.resolve helper used elsewhere and update the
conditional that sets worktreeMap accordingly.
- Around line 10-86: Extract the duplicated parsing logic used in
listWorktreeBranches and findWorktreeAtPath into a single helper
parseWorktreePorcelain(raw: string): Array<{ path: string; branch: string | null
}>, then make listWorktreeBranches call git.raw(...) and feed the output into
parseWorktreePorcelain to build worktreeMap and checkedOutBranches, and make
findWorktreeAtPath call git.raw(...), feed into parseWorktreePorcelain and then
search the returned array for an entry with resolve(path) ===
resolve(worktreePath) and branch === expectedBranch; keep existing error
handling and return types in listWorktreeBranches and findWorktreeAtPath.
In `@packages/host-service/src/trpc/router/workspace-creation/finish-checkout.ts`:
- Around line 60-69: The rollbackWorktree async function currently calls
args.git.raw(["worktree", "remove", args.worktreePath]) which can fail to remove
worktrees with untracked/modified files; change the call to include the --force
flag (e.g., args.git.raw(["worktree", "remove", "--force", args.worktreePath"]))
so the rollback cleans up the worktree directory and git registration like the
cleanup in checkout.ts (line referenced in review); keep the existing try/catch
and error logging around rollbackWorktree to preserve diagnostics.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts`:
- Around line 108-116: The stale-row reconciliation currently loads all project
rows and filters in memory then performs separate delete and insert operations;
change the selection to query the workspaces table with both
eq(workspaces.projectId, input.projectId) and eq(workspaces.branch, branch) so
you fetch at most the matching stale row (replace the
.select().from(...).where(...).all().find(...) pattern with a single where
including branch), and wrap the delete of the stale workspace and the subsequent
insert of cloudRow into a single transaction using ctx.db.transaction or the
DB's transaction helper to ensure they succeed or rollback together (refer to
the workspaces table, ctx.db.delete(workspaces).where(...), and the code that
inserts cloudRow).
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts`:
- Around line 68-70: The XOR refine currently uses Boolean(v.branch) !==
Boolean(v.pr) which treats empty/whitespace branches as truthy; add a prior
transformation on the input object (e.g., via .transform((v) => ({ ...v, branch:
v.branch?.trim() || undefined }))) before the .refine call so branch values that
are empty or only whitespace become undefined, then keep the existing
.refine((v) => Boolean(v.branch) !== Boolean(v.pr), ...) check to enforce
exactly-one semantics; reference the checkout.ts schema where v.branch and v.pr
are validated and the existing .refine invocation.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/get-context.ts`:
- Around line 14-29: The two return branches in get-context.ts currently use an
inline cast "as string | null" to unify shapes; instead declare and use an
explicit return type (e.g., export or local GetContextResult) for the function
that includes projectId: string, hasLocalRepo: boolean, and defaultBranch:
string | null, then remove the inline cast from the no-local-repo return and let
both returns conform to that declared type; locate the function that uses
localProject, resolveDefaultBranchName, and ctx.git to apply the explicit return
type to its signature so both object literals type-check without "as".
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts`:
- Around line 43-48: In the catch block of get-github-pull-request-content
(where a TRPCError is thrown), preserve the original error by passing it as the
cause property to the TRPCError instead of only flattening its message; keep the
current message formatting but add cause: err (ensuring err is forwarded as-is
or wrapped if necessary) so downstream logging/stack/typed info from the
original gh/zod/auth error is retained while the client-facing message stays
unchanged.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts`:
- Line 92: The variable defaultBranch should be typed as string (not string |
null) to match resolveDefaultBranchName's actual return type; change the
declaration of defaultBranch to string and remove the nullable union, and update
the early-return object that currently sets defaultBranch: null as string | null
to provide a concrete string (e.g., "main") so the type aligns with
resolveDefaultBranchName and downstream code no longer treats defaultBranch as
nullable.
- Around line 103-111: The query building workspaceBranches currently selects
all columns then maps to .branch; change it to project only the branch column to
avoid loading full rows: update the
ctx.db.select().from(workspaces).where(eq(workspaces.projectId,
input.projectId)).all() call used to build workspaceBranches so it selects
workspaces.branch (or uses a projection API like ctx.db.select({ branch:
workspaces.branch }).from(...)) before mapping/filtering; keep the Set
construction, filtering predicate (b is string) and variable name
workspaceBranches unchanged.
- Around line 40-50: The lastRemoteRefetch Map used by shouldRefetchRemote and
markRefetchRemote is unbounded and can grow forever; add a periodic cleanup
(similar to sweepStaleProgress in progress.ts) to remove entries older than
REMOTE_REFETCH_TTL_MS (or another configured threshold) so stale projectId keys
are purged; implement the sweep as a timer-based task in the same module (or
wired into existing cleanup routines) that iterates lastRemoteRefetch and
deletes entries whose timestamp is older than Date.now() -
REMOTE_REFETCH_TTL_MS, ensuring REMOTE_REFETCH_TTL_MS, lastRemoteRefetch,
shouldRefetchRemote and markRefetchRemote remain the single sources of truth.
In `@packages/host-service/src/trpc/router/workspace-creation/progress.ts`:
- Around line 22-35: The function setProgress currently accepts activeStepId:
string which lets unknown values mark every step as "done"; change the parameter
to a tightened union type derived from STEP_DEFINITIONS (e.g. declare type
ActiveStepId = typeof STEP_DEFINITIONS[number]["id"] and use activeStepId:
ActiveStepId in setProgress) and add a runtime guard: look up
STEP_DEFINITIONS.find(d => d.id === activeStepId) before mapping and if not
found, log/warn and treat as "no active step" (mark all steps "pending" or
otherwise avoid marking everything "done") so createProgress.set(pendingId, ...)
never records a misleading all-done state; update references to setProgress
accordingly and keep createProgress.set usage unchanged.
- Line 20: The module-level Map createProgress can grow if sweepStaleProgress is
only invoked on the poll path; to fix, start a periodic sweeper at module init
that calls sweepStaleProgress (e.g., setInterval with the same cutoff period or
a reasonable cadence) and ensure the interval is cleared on shutdown; also
verify callers that mutate createProgress (setProgress, clearProgress, and
checkout.ts paths) still clear on both success and failure so the periodic
sweeper is a safety net rather than the only cleanup mechanism.
🪄 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: 47b75660-6b5a-47ca-a5fb-5b8ab1addd38
📒 Files selected for processing (17)
packages/host-service/src/trpc/router/workspace-creation/branch-helpers.tspackages/host-service/src/trpc/router/workspace-creation/finish-checkout.tspackages/host-service/src/trpc/router/workspace-creation/github-schemas.tspackages/host-service/src/trpc/router/workspace-creation/helpers.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tspackages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tspackages/host-service/src/trpc/router/workspace-creation/procedures/create.tspackages/host-service/src/trpc/router/workspace-creation/procedures/generate-branch-name.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-context.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-issue-content.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-progress.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.tspackages/host-service/src/trpc/router/workspace-creation/progress.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
Comment on lines
+19
to
+35
| export function safeResolveWorktreePath( | ||
| repoPath: string, | ||
| branchName: string, | ||
| ): string { | ||
| const worktreesRoot = resolve(repoPath, ".worktrees"); | ||
| const worktreePath = resolve(worktreesRoot, branchName); | ||
| if ( | ||
| worktreePath !== worktreesRoot && | ||
| !worktreePath.startsWith(worktreesRoot + sep) | ||
| ) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Invalid branch name: path traversal detected (${branchName})`, | ||
| }); | ||
| } | ||
| return worktreePath; | ||
| } |
Contributor
There was a problem hiding this comment.
Minor hardening: reject branchName that resolves to the worktrees root.
The traversal check allows worktreePath === worktreesRoot (i.e., a branch name that normalizes to the root itself, such as "", ., or ./). Git normally rejects such names upstream, but since this helper is named safeResolveWorktreePath and is the last line of defense, hardening it to reject an equal-to-root result would avoid ever creating/overwriting a worktree at the .worktrees/ directory itself.
🛡️ Proposed fix
const worktreesRoot = resolve(repoPath, ".worktrees");
const worktreePath = resolve(worktreesRoot, branchName);
- if (
- worktreePath !== worktreesRoot &&
- !worktreePath.startsWith(worktreesRoot + sep)
- ) {
+ if (!worktreePath.startsWith(worktreesRoot + sep)) {
throw new TRPCError({
code: "BAD_REQUEST",
message: `Invalid branch name: path traversal detected (${branchName})`,
});
}🤖 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/helpers.ts` around
lines 19 - 35, In safeResolveWorktreePath, tighten the path-traversal check to
also reject branchName values that resolve exactly to the worktrees root (e.g.,
"", ".", "./"); update the conditional that currently allows worktreePath ===
worktreesRoot so it throws a TRPCError when worktreePath === worktreesRoot OR
when worktreePath does not start with worktreesRoot + sep, ensuring any
equal-to-root resolution is treated as invalid and prevented from returning
worktreesRoot.
Comment on lines
+290
to
+298
| ctx.db | ||
| .insert(workspaces) | ||
| .values({ | ||
| id: cloudRow.id, | ||
| projectId: input.projectId, | ||
| worktreePath, | ||
| branch: branchName, | ||
| }) | ||
| .run(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the v2Workspace cloud router procedures to pick the right rollback mutation.
ast-grep --pattern 'v2Workspace: $_'
rg -nP --type=ts -C2 '\bv2Workspace\.(delete|remove|archive|destroy)\b'
rg -nP --type=ts -C2 '\bworkspaces\b.*\.run\(\)' -g '!**/node_modules/**'Repository: superset-sh/superset
Length of output: 3127
🏁 Script executed:
#!/bin/bash
# Read the full context of the three failure paths and the insert in question
sed -n '246,298p' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts
# Check if this pattern of unwrapped .run() on writes is standard in the file
rg -n '\.run\(\)' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts -B3
# Check other workspace-creation procedures for .run() error handling patterns
rg -n '\.run\(\)' packages/host-service/src/trpc/router/workspace-creation/procedures/ -B2 -A2Repository: superset-sh/superset
Length of output: 2990
🏁 Script executed:
#!/bin/bash
# Check if there's a separate cleanup process that handles orphaned cloud workspaces
rg -n 'v2Workspace.delete|workspace.*cleanup|orphan' packages/host-service/src/trpc/router/workspace-creation/ -C2
# Check what happens if a local workspace row is missing but a cloud row exists
rg -n 'cloudRow\|v2Workspace\|orphan' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts -C1
# Verify the exact signature of v2Workspace.delete to confirm the suggested fix
rg -n 'v2Workspace.*delete' packages/host-service/ -B2 -A3 | head -40Repository: superset-sh/superset
Length of output: 2880
Local insert failure leaves an orphaned cloud workspace.
The two earlier failure paths (ensureV2Host, v2Workspace.create) both clear progress and roll back the worktree on error. This synchronous .run() has no such safeguard: if it throws (disk full, constraint, crash between steps), cloudRow and the on-disk worktree both persist with no local row connecting them.
While the UI can surface orphan disk worktrees (showing them under "Create" in the Worktree tab), if the user later adopts that orphan, a new cloud row is created, leaving the original cloud row permanently orphaned with no cleanup path.
Wrap this insert with error handling that also rolls back the cloud row on failure:
Suggested error handling for local insert failure
- ctx.db
- .insert(workspaces)
- .values({
- id: cloudRow.id,
- projectId: input.projectId,
- worktreePath,
- branch: branchName,
- })
- .run();
+ try {
+ ctx.db
+ .insert(workspaces)
+ .values({
+ id: cloudRow.id,
+ projectId: input.projectId,
+ worktreePath,
+ branch: branchName,
+ })
+ .run();
+ } catch (err) {
+ console.error(
+ "[workspaceCreation.create] local workspaces insert failed",
+ err,
+ );
+ clearProgress(input.pendingId);
+ await rollbackWorktree();
+ // Best-effort cleanup of the cloud row so the user isn't left with
+ // an orphaned v2 workspace that has no local mapping.
+ await ctx.api.v2Workspace.delete
+ .mutate({ id: cloudRow.id })
+ .catch((delErr) => {
+ console.warn(
+ "[workspaceCreation.create] cloud rollback failed",
+ delErr,
+ );
+ });
+ throw new TRPCError({
+ code: "INTERNAL_SERVER_ERROR",
+ message: `Failed to persist workspace locally: ${err instanceof Error ? err.message : String(err)}`,
+ });
+ }🤖 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/create.ts`
around lines 290 - 298, Wrap the local insert
(ctx.db.insert(workspaces).values({...}).run()) in a try/catch so that if the
insert throws you rollback the on-disk worktree and clear progress and then
remove the previously-created cloud row (use the same v2Workspace deletion API
you used to create cloudRow — pass cloudRow.id) before rethrowing the error;
mirror the error-handling pattern used around ensureV2Host and
v2Workspace.create (call the worktree rollback function and
clearProgress/clearCreateProgress, then delete the cloudRow by id) so no
cloud-only orphan remains if the local insert fails.
Kitenite
force-pushed
the
fluorescent-machine
branch
from
5144bd4 to
8ae8a13
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts (1)
8-49: Optional: consider extracting a sharedgh viewhelper to reduce duplication withget-github-issue-content.ts.This procedure and
getGitHubIssueContentshare near-identical scaffolding: resolve repo →execGh([<kind>, "view", String(n), "--repo", "owner/name", "--json", fields])→ schema.parse(raw)→ normalize → wrap errors asINTERNAL_SERVER_ERRORwith aFailed to fetch ...message. A small helper (e.g.fetchGhJson({ kind: "pr" | "issue", number, repo, fields, schema })) would collapse the common path and keep the per-procedure files to just the input schema and the return-shape mapping. Not blocking for this refactor PR — just a follow-up opportunity now that the two procedures sit side-by-side.🤖 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/get-github-pull-request-content.ts` around lines 8 - 49, Both getGitHubPullRequestContent and getGitHubIssueContent duplicate the pattern of resolving a repo, calling execGh("view"...), parsing with a schema, normalizing fields, and wrapping errors; extract a shared helper (e.g. fetchGhJson) that accepts parameters like kind ("pr" | "issue"), number, repo (from resolveGithubRepo), json fields, and a Zod schema, and returns the parsed JSON or throws a standardized TRPCError; update getGitHubPullRequestContent to call fetchGhJson with PrSchema and map the returned parsed object to the same normalized return shape, and update getGitHubIssueContent similarly to remove the duplicated execGh/parse/error-handling logic while keeping the existing resolveGithubRepo, PrSchema, and error message format.packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts (2)
104-112: Project only thebranchcolumn.This query fires on every branch search request; selecting the whole row is unnecessary work when only
branchis consumed.♻️ Proposed diff
const workspaceBranches = new Set<string>( ctx.db - .select() + .select({ branch: workspaces.branch }) .from(workspaces) .where(eq(workspaces.projectId, input.projectId)) .all() .map((w) => w.branch) .filter((b): b is string => !!b), );🤖 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/search-branches.ts` around lines 104 - 112, The query builds workspaceBranches by selecting entire rows but only uses the branch field; change the DB call to project only the branch column so less data is fetched. Update the ctx.db.select().from(workspaces).where(eq(workspaces.projectId, input.projectId)).all() call used to construct workspaceBranches to select only the branch (e.g., use .select({ branch: workspaces.branch }) or the equivalent column-only select for your ORM) and keep the downstream .map((w) => w.branch) logic unchanged.
83-90: Boundgit fetchwith a timeout to prevent hanging the protected query.
git.fetch(["--prune", "--quiet", "--no-tags"])is awaited without timeout/abort. An unreachable remote (hung TCP, slow DNS, auth prompt) will stall the whole response; the surroundingtry/catchonly catches errors, not slowness. The codebase already demonstrates timeout patterns (e.g.,checkout.tsline 168 passes{ timeout: 120_000 }), and simple-git v3.30.0 supports both per-command timeout options and instance-level timeout plugins. Since the TTL is set before the await, adding a bounded timeout preserves the "offline → fall back to cached refs" intent without user-visible stalls.🤖 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/search-branches.ts` around lines 83 - 90, The git.fetch call in search-branches.ts (inside the input.refresh && shouldRefetchRemote(input.projectId) block) can hang; bound it with a timeout like other places (see checkout.ts pattern) by passing a timeout option to git.fetch (e.g., { timeout: 120_000 }) so the await cannot stall the protected query; keep the existing try/catch to fall back to cached refs on failure/timeouts and retain markRefetchRemote(input.projectId) behavior.packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts (1)
109-117: Stale-row lookup loads the entire project's workspaces and filters in JS — use a WHERE clause.This reads every
workspacesrow forprojectIdand then.find()s by branch in memory. It's both O(n) and inconsistent with the idempotency lookup inprocedures/checkout.ts(lines 93–100) which usesfindFirstwithand(eq(projectId), eq(branch)). Prefer a narrow query so the DB does the work and so the two procedures stay idiomatically aligned.♻️ Proposed refactor
- const stale = ctx.db - .select() - .from(workspaces) - .where(eq(workspaces.projectId, input.projectId)) - .all() - .find((w) => w.branch === branch); + const stale = ctx.db.query.workspaces + .findFirst({ + where: and( + eq(workspaces.projectId, input.projectId), + eq(workspaces.branch, branch), + ), + }) + .sync(); if (stale && stale.id !== cloudRow.id) { ctx.db.delete(workspaces).where(eq(workspaces.id, stale.id)).run(); }And add
andto thedrizzle-ormimport.🤖 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/adopt.ts` around lines 109 - 117, The current stale-row lookup in adopt.ts loads all workspaces for a project then .find()s by branch in JS; change it to query the DB for the specific row using ctx.db.select().from(workspaces).where(and(eq(workspaces.projectId, input.projectId), eq(workspaces.branch, branch))).then use the returned row (or findFirst equivalent) and, if its id !== cloudRow.id, delete it via ctx.db.delete(workspaces).where(eq(workspaces.id, stale.id)).run(); also add the 'and' import from 'drizzle-orm' so the narrower WHERE clause compiles and matches the idempotent pattern used in procedures/checkout.ts.
🤖 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/procedures/checkout.ts`:
- Around line 262-276: The fetch of a remote-tracking ref is currently swallowed
with console.warn which can hide the real cause of a subsequent worktree add
failure; in the checkout path where resolved.kind === "remote-tracking" (the
git.fetch([...resolved.remote, resolved.shortName, "--quiet","--no-tags"])
call), capture the thrown error and surface it: either rethrow a new error that
wraps the fetch error (so the procedure rejects with a descriptive message
referencing resolved.remoteShortName and the fetch failure) or append a
descriptive entry to the existing extraWarnings collection before continuing so
the caller sees the fetch failure cause when worktree add (--track -b ...) later
fails. Ensure the change references the same git.fetch call and
resolved.remoteShortName/resolved.shortName symbols so the error message or
extraWarnings clearly identifies which remote ref failed.
- Around line 53-68: The schema accepts linkedContext but the handler never
consumes it — either remove linkedContext from the input z.object or wire it
through: update the checkout handler paths that call finishCheckout (both branch
and PR flows) to read input.linkedContext and add a parameter to finishCheckout
(and its callers) to accept linkedContext, then persist it (e.g., save into
workspace metadata or forward into the agent prompt builder) so the server
stores/uses the internalIssueIds, githubIssueUrls, linkedPrUrl and attachments;
also update any related call sites and downstream consumers (refer to
linkedContext, finishCheckout, and buildIntentPayload) and remove the schema if
you choose not to implement persistence.
---
Nitpick comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts`:
- Around line 109-117: The current stale-row lookup in adopt.ts loads all
workspaces for a project then .find()s by branch in JS; change it to query the
DB for the specific row using
ctx.db.select().from(workspaces).where(and(eq(workspaces.projectId,
input.projectId), eq(workspaces.branch, branch))).then use the returned row (or
findFirst equivalent) and, if its id !== cloudRow.id, delete it via
ctx.db.delete(workspaces).where(eq(workspaces.id, stale.id)).run(); also add the
'and' import from 'drizzle-orm' so the narrower WHERE clause compiles and
matches the idempotent pattern used in procedures/checkout.ts.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts`:
- Around line 8-49: Both getGitHubPullRequestContent and getGitHubIssueContent
duplicate the pattern of resolving a repo, calling execGh("view"...), parsing
with a schema, normalizing fields, and wrapping errors; extract a shared helper
(e.g. fetchGhJson) that accepts parameters like kind ("pr" | "issue"), number,
repo (from resolveGithubRepo), json fields, and a Zod schema, and returns the
parsed JSON or throws a standardized TRPCError; update
getGitHubPullRequestContent to call fetchGhJson with PrSchema and map the
returned parsed object to the same normalized return shape, and update
getGitHubIssueContent similarly to remove the duplicated
execGh/parse/error-handling logic while keeping the existing resolveGithubRepo,
PrSchema, and error message format.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts`:
- Around line 104-112: The query builds workspaceBranches by selecting entire
rows but only uses the branch field; change the DB call to project only the
branch column so less data is fetched. Update the
ctx.db.select().from(workspaces).where(eq(workspaces.projectId,
input.projectId)).all() call used to construct workspaceBranches to select only
the branch (e.g., use .select({ branch: workspaces.branch }) or the equivalent
column-only select for your ORM) and keep the downstream .map((w) => w.branch)
logic unchanged.
- Around line 83-90: The git.fetch call in search-branches.ts (inside the
input.refresh && shouldRefetchRemote(input.projectId) block) can hang; bound it
with a timeout like other places (see checkout.ts pattern) by passing a timeout
option to git.fetch (e.g., { timeout: 120_000 }) so the await cannot stall the
protected query; keep the existing try/catch to fall back to cached refs on
failure/timeouts and retain markRefetchRemote(input.projectId) 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: 1d5d99b0-3394-42c8-b378-f044e97ca94b
📒 Files selected for processing (16)
packages/host-service/src/trpc/router/workspace-creation/branch-helpers.tspackages/host-service/src/trpc/router/workspace-creation/finish-checkout.tspackages/host-service/src/trpc/router/workspace-creation/github-schemas.tspackages/host-service/src/trpc/router/workspace-creation/helpers.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tspackages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tspackages/host-service/src/trpc/router/workspace-creation/procedures/create.tspackages/host-service/src/trpc/router/workspace-creation/procedures/generate-branch-name.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-context.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-issue-content.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-progress.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.tspackages/host-service/src/trpc/router/workspace-creation/progress.ts
✅ Files skipped from review due to trivial changes (3)
- packages/host-service/src/trpc/router/workspace-creation/github-schemas.ts
- packages/host-service/src/trpc/router/workspace-creation/progress.ts
- packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/host-service/src/trpc/router/workspace-creation/procedures/generate-branch-name.ts
- packages/host-service/src/trpc/router/workspace-creation/helpers.ts
- packages/host-service/src/trpc/router/workspace-creation/procedures/get-progress.ts
- packages/host-service/src/trpc/router/workspace-creation/branch-helpers.ts
- packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts
Comment on lines
+53
to
+68
| linkedContext: z | ||
| .object({ | ||
| internalIssueIds: z.array(z.string()).optional(), | ||
| githubIssueUrls: z.array(z.string()).optional(), | ||
| linkedPrUrl: z.string().optional(), | ||
| attachments: z | ||
| .array( | ||
| z.object({ | ||
| data: z.string(), | ||
| mediaType: z.string(), | ||
| filename: z.string().optional(), | ||
| }), | ||
| ) | ||
| .optional(), | ||
| }) | ||
| .optional(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether linkedContext is consumed anywhere in the workspace-creation flow (any file, any form).
rg -nP --type=ts -C3 '\blinkedContext\b'
# Cross-check: does the original pre-refactor file (if still in git history on the PR base) reference linkedContext usage beyond the schema?
rg -nP --type=ts -C3 '\b(linkedPrUrl|githubIssueUrls|internalIssueIds|attachments)\b' -g '!**/*.test.ts'Repository: superset-sh/superset
Length of output: 50377
🏁 Script executed:
#!/bin/bash
# Check the handler body of checkout.ts to see if input.linkedContext is referenced
echo "=== checkout.ts handler body (after input schema) ==="
sed -n '70,340p' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | head -150
# Check finishCheckout signature and calls
echo -e "\n=== finishCheckout signature and calls ==="
rg -n 'finishCheckout' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts -A 2 -B 2Repository: superset-sh/superset
Length of output: 5031
🏁 Script executed:
#!/bin/bash
# Also check create.ts to compare handling
echo "=== create.ts handler body (after input schema) ==="
sed -n '70,150p' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts
# Check if finishCheckout is called in create.ts
echo -e "\n=== finishCheckout in create.ts ==="
rg -n 'finishCheckout' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts -A 2 -B 2Repository: superset-sh/superset
Length of output: 3053
🏁 Script executed:
#!/bin/bash
# Get full file size to understand structure
echo "=== File sizes ==="
wc -l packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts
wc -l packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts
# Search for input.linkedContext usage specifically
echo -e "\n=== input.linkedContext in checkout.ts ==="
rg 'input\.linkedContext' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts -n
# Search for input.linkedContext usage specifically in create.ts
echo -e "\n=== input.linkedContext in create.ts ==="
rg 'input\.linkedContext' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts -nRepository: superset-sh/superset
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Find and view finishCheckout function signature
echo "=== finishCheckout function (finish-checkout.ts) ==="
find packages/host-service -name 'finish-checkout.ts' -o -name '*finish-checkout*' | head -5
# Show the file
cat packages/host-service/src/trpc/router/workspace-creation/procedures/finish-checkout.ts | head -100Repository: superset-sh/superset
Length of output: 328
🏁 Script executed:
#!/bin/bash
# Double-check: is linkedContext passed anywhere in finishCheckout calls?
echo "=== finishCheckout calls in checkout.ts (full context) ==="
rg -A 15 'return await finishCheckout' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: superset-sh/superset
Length of output: 884
🏁 Script executed:
#!/bin/bash
# Check create.ts for finishCheckout usage
echo "=== finishCheckout in create.ts ==="
rg 'finishCheckout' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts -A 10 -B 2 | head -50Repository: superset-sh/superset
Length of output: 99
🏁 Script executed:
#!/bin/bash
# View the finishCheckout function signature
cat packages/host-service/src/trpc/router/workspace-creation/finish-checkout.ts | head -80Repository: superset-sh/superset
Length of output: 2332
🏁 Script executed:
#!/bin/bash
# What does create.ts do at the end? Search for the return statement
echo "=== create.ts final return statement ==="
tail -60 packages/host-service/src/trpc/router/workspace-creation/procedures/create.tsRepository: superset-sh/superset
Length of output: 1746
🏁 Script executed:
#!/bin/bash
# Search for linkedContext in finish-checkout.ts
rg 'linkedContext' packages/host-service/src/trpc/router/workspace-creation/finish-checkout.tsRepository: superset-sh/superset
Length of output: 46
linkedContext is validated but never consumed — remove it or implement the persistence logic.
The input schema accepts a fully structured linkedContext (internal issue ids, GitHub issue URLs, linked PR URL, attachments) at lines 53–68, but neither the branch nor PR path in the handler reads input.linkedContext. The finishCheckout function (which both paths call) has no parameter for it, and nothing forwards it to any downstream consumer. The client-side code does build and send linkedContext objects (e.g., in buildIntentPayload.ts), but the server silently discards them. Either remove the field from the schema to match actual behavior, or implement the handler logic to persist or use the linked context (e.g., storing it in workspace metadata or passing it to an agent prompt builder).
Also applies to: lines 214–224, 328–338
🤖 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 53 - 68, The schema accepts linkedContext but the handler never
consumes it — either remove linkedContext from the input z.object or wire it
through: update the checkout handler paths that call finishCheckout (both branch
and PR flows) to read input.linkedContext and add a parameter to finishCheckout
(and its callers) to accept linkedContext, then persist it (e.g., save into
workspace metadata or forward into the agent prompt builder) so the server
stores/uses the internalIssueIds, githubIssueUrls, linkedPrUrl and attachments;
also update any related call sites and downstream consumers (refer to
linkedContext, finishCheckout, and buildIntentPayload) and remove the schema if
you choose not to implement persistence.
Comment on lines
+262
to
+276
| if (resolved.kind === "remote-tracking") { | ||
| try { | ||
| await git.fetch([ | ||
| resolved.remote, | ||
| resolved.shortName, | ||
| "--quiet", | ||
| "--no-tags", | ||
| ]); | ||
| } catch (err) { | ||
| console.warn( | ||
| `[workspaceCreation.checkout] fetch ${resolved.remoteShortName} failed:`, | ||
| err, | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent fetch failure can make the next worktree add fail confusingly.
If the explicit fetch of the remote-tracking ref fails (network, auth, etc.), it's only console.warn'd and execution continues into the worktree add --track -b … <remoteShortName> call. That subsequent call may then fail because the remote ref wasn't advanced, and the user sees a CONFLICT with a generic "Failed to add worktree" message rather than the real root cause. Consider surfacing the fetch error into the rejection message (or into extraWarnings on success) so the user has enough to act on.
🤖 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 262 - 276, The fetch of a remote-tracking ref is currently
swallowed with console.warn which can hide the real cause of a subsequent
worktree add failure; in the checkout path where resolved.kind ===
"remote-tracking" (the git.fetch([...resolved.remote, resolved.shortName,
"--quiet","--no-tags"]) call), capture the thrown error and surface it: either
rethrow a new error that wraps the fetch error (so the procedure rejects with a
descriptive message referencing resolved.remoteShortName and the fetch failure)
or append a descriptive entry to the existing extraWarnings collection before
continuing so the caller sees the fetch failure cause when worktree add (--track
-b ...) later fails. Ensure the change references the same git.fetch call and
resolved.remoteShortName/resolved.shortName symbols so the error message or
extraWarnings clearly identifies which remote ref failed.
…files The 1790-line workspace-creation.ts is now a 27-line router composition. Helpers, progress state, schemas, and each tRPC procedure live in their own file under procedures/. Public API (workspaceCreationRouter) unchanged.
Kitenite
force-pushed
the
fluorescent-machine
branch
from
8ae8a13 to
046d0e8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/host-service/src/trpc/router/workspace-creation/finish-checkout.ts (1)
118-126:⚠️ Potential issue | 🟡 MinorLocal
workspacesinsert has no rollback — failure here leaves an orphaned cloud workspace.Same pattern as the one flagged on
create.ts:ensureV2Hostandv2Workspace.createboth roll back the worktree on error, but this synchronous.run()does not. If the insert throws (disk full, constraint violation, crash),cloudRowand the on-disk worktree persist with no local row connecting them. A subsequent orphan adoption creates a new cloud row, leaving the first one unreachable.Wrap with error handling that rolls back the cloud row as well, mirroring the earlier failure paths:
🛠️ Proposed fix
- ctx.db - .insert(workspaces) - .values({ - id: cloudRow.id, - projectId: args.projectId, - worktreePath: args.worktreePath, - branch: args.branch, - }) - .run(); + try { + ctx.db + .insert(workspaces) + .values({ + id: cloudRow.id, + projectId: args.projectId, + worktreePath: args.worktreePath, + branch: args.branch, + }) + .run(); + } catch (err) { + console.error( + "[workspaceCreation.checkout] local workspaces insert failed", + err, + ); + clearProgress(args.pendingId); + await rollbackWorktree(); + await ctx.api.v2Workspace.delete + .mutate({ id: cloudRow.id }) + .catch((delErr) => { + console.warn( + "[workspaceCreation.checkout] cloud rollback failed", + delErr, + ); + }); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `Failed to persist workspace locally: ${err instanceof Error ? err.message : String(err)}`, + }); + }🤖 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/finish-checkout.ts` around lines 118 - 126, The local DB insert for workspaces (ctx.db.insert(workspaces).values(...).run()) can fail and leave an orphaned cloudRow/worktree; wrap this insert in a try/catch and on any error perform the same cleanup used earlier (undo the cloudRow and on-disk worktree) — e.g., call the v2 workspace rollback/teardown path used after ensureV2Host and v2Workspace.create (or explicitly delete the cloudRow via the same cloud API and remove the worktree) before rethrowing the error; ensure you reference cloudRow, ensureV2Host, and v2Workspace.create so the cleanup mirrors the existing failure paths.
🧹 Nitpick comments (2)
packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts (1)
109-117: Narrow the stale-row lookup in the DB rather than in JS.Currently all workspace rows for the project are loaded then filtered in memory. Pushing the
branchpredicate into thewhereclause reduces allocations and is idiomatic Drizzle.♻️ Proposed refactor
-import { eq } from "drizzle-orm"; +import { and, eq } from "drizzle-orm"; @@ - const stale = ctx.db - .select() - .from(workspaces) - .where(eq(workspaces.projectId, input.projectId)) - .all() - .find((w) => w.branch === branch); + const stale = ctx.db + .select() + .from(workspaces) + .where( + and( + eq(workspaces.projectId, input.projectId), + eq(workspaces.branch, branch), + ), + ) + .get();🤖 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/adopt.ts` around lines 109 - 117, The code loads all workspaces for the project and then filters by branch in JS; change the DB query to include the branch predicate so only the matching row is returned. Replace the select(...).from(workspaces).where(eq(workspaces.projectId, input.projectId)).all().find((w) => w.branch === branch) with a single query that includes both predicates (e.g. where(eq(workspaces.projectId, input.projectId), eq(workspaces.branch, branch))) and fetch a single row (use the query form that returns one row instead of .all()); keep the subsequent check that if stale && stale.id !== cloudRow.id then delete the stale workspace via ctx.db.delete(workspaces).where(eq(workspaces.id, stale.id)).run().packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts (1)
40-50: Consider boundinglastRemoteRefetchand timing out the opportunisticgit fetch.Two minor hygiene concerns in this TTL gate:
lastRemoteRefetchis a module-levelMap<string, number>that grows byprojectIdfor the lifetime of the host-service process; deleted projects never have their entries reclaimed. Low volume per user, but worth either periodic sweeping (analogous tosweepStaleProgress) or using a bounded LRU.git.fetch(...)here has no explicit timeout. A hung fetch (slow DNS, flaky network) will block thesearchBranchesquery for whatever the underlying timeout is, making the picker feel frozen on first page load. Thetry { ... } catch {}only masks errors, not hangs.Not blocking; flagging for a follow-up pass.
Also applies to: 83-90
🤖 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/search-branches.ts` around lines 40 - 50, The module-level Map lastRemoteRefetch used by shouldRefetchRemote/markRefetchRemote can grow unbounded; replace or wrap it with a bounded LRU (or add a periodic sweep similar to sweepStaleProgress) keyed by projectId so entries get reclaimed, and keep REMOTE_REFETCH_TTL_MS behavior intact. Also make the opportunistic git.fetch call in searchBranches abortable by adding an explicit timeout (e.g., AbortController or Promise.race with a short timeout) so a hung fetch doesn't stall the query; ensure the fetch is aborted on timeout and the existing try/catch handles aborted fetches gracefully.
🤖 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/branch-helpers.ts`:
- Around line 38-57: The code parses git worktree paths into currentPath but
compares them to knownPaths and managedRoot using OS-specific separators,
causing mismatches on Windows; update the loop in which currentPath is set (the
variable currentPath used with knownPaths.has(...) and
currentPath.startsWith(managedRoot + sep)) to normalize currentPath via
path.resolve() (or an equivalent path normalization) immediately after
extracting it from the git output so subsequent comparisons to managedRoot and
knownPaths use consistent separators (mirror the normalization approach used in
findWorktreeAtPath/projectWorktreesRoot).
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts`:
- Around line 83-189: The progress state is left as "creating_worktree" if any
of the pre-registering steps (listBranchNames, safeResolveWorktreePath,
mkdirSync, ctx.git/resolveStartPoint, or the git.raw(["worktree","add",...])
call) throw; wrap the block starting immediately after
setProgress(input.pendingId, "creating_worktree") through the worktree creation
call in a try/catch that calls clearProgress(input.pendingId) on any error and
then rethrows the error so existing error handling still runs; reference the
functions/variables setProgress, clearProgress, listBranchNames,
safeResolveWorktreePath, ctx.git / resolveStartPoint, and git.raw (worktree add)
when locating where to add the try/catch.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts`:
- Around line 8-12: The Zod schema for the procedure (the z.object with keys
projectId, query, limit) allows fractional numbers because it uses
z.number().min(1).max(100); change the limit validator to
z.number().int().min(1).max(100) so only integers are accepted before passing to
Octokit as per_page; apply the same change in the matching schema in
search-pull-requests.ts (look for the identical z.object with
projectId/query/limit).
In `@packages/host-service/src/trpc/router/workspace-creation/progress.ts`:
- Around line 22-35: setProgress currently treats an unknown activeStepId as if
all steps are done; update setProgress to validate that activeStepId exists in
STEP_DEFINITIONS before mapping: use STEP_DEFINITIONS.some or find to check for
a matching def.id (reference symbols: setProgress, STEP_DEFINITIONS,
createProgress.set) and if not found throw a descriptive Error (or assert)
indicating the invalid activeStepId and the expected ids, otherwise proceed with
the existing mapping logic and call createProgress.set as before.
---
Duplicate comments:
In `@packages/host-service/src/trpc/router/workspace-creation/finish-checkout.ts`:
- Around line 118-126: The local DB insert for workspaces
(ctx.db.insert(workspaces).values(...).run()) can fail and leave an orphaned
cloudRow/worktree; wrap this insert in a try/catch and on any error perform the
same cleanup used earlier (undo the cloudRow and on-disk worktree) — e.g., call
the v2 workspace rollback/teardown path used after ensureV2Host and
v2Workspace.create (or explicitly delete the cloudRow via the same cloud API and
remove the worktree) before rethrowing the error; ensure you reference cloudRow,
ensureV2Host, and v2Workspace.create so the cleanup mirrors the existing failure
paths.
---
Nitpick comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts`:
- Around line 109-117: The code loads all workspaces for the project and then
filters by branch in JS; change the DB query to include the branch predicate so
only the matching row is returned. Replace the
select(...).from(workspaces).where(eq(workspaces.projectId,
input.projectId)).all().find((w) => w.branch === branch) with a single query
that includes both predicates (e.g. where(eq(workspaces.projectId,
input.projectId), eq(workspaces.branch, branch))) and fetch a single row (use
the query form that returns one row instead of .all()); keep the subsequent
check that if stale && stale.id !== cloudRow.id then delete the stale workspace
via ctx.db.delete(workspaces).where(eq(workspaces.id, stale.id)).run().
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts`:
- Around line 40-50: The module-level Map lastRemoteRefetch used by
shouldRefetchRemote/markRefetchRemote can grow unbounded; replace or wrap it
with a bounded LRU (or add a periodic sweep similar to sweepStaleProgress) keyed
by projectId so entries get reclaimed, and keep REMOTE_REFETCH_TTL_MS behavior
intact. Also make the opportunistic git.fetch call in searchBranches abortable
by adding an explicit timeout (e.g., AbortController or Promise.race with a
short timeout) so a hung fetch doesn't stall the query; ensure the fetch is
aborted on timeout and the existing try/catch handles aborted fetches
gracefully.
🪄 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: 16e73fe3-8f41-4d55-8156-118be3b2820e
📒 Files selected for processing (17)
packages/host-service/src/trpc/router/workspace-creation/branch-helpers.tspackages/host-service/src/trpc/router/workspace-creation/finish-checkout.tspackages/host-service/src/trpc/router/workspace-creation/github-schemas.tspackages/host-service/src/trpc/router/workspace-creation/helpers.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tspackages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tspackages/host-service/src/trpc/router/workspace-creation/procedures/create.tspackages/host-service/src/trpc/router/workspace-creation/procedures/generate-branch-name.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-context.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-issue-content.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.tspackages/host-service/src/trpc/router/workspace-creation/procedures/get-progress.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.tspackages/host-service/src/trpc/router/workspace-creation/progress.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
✅ Files skipped from review due to trivial changes (1)
- packages/host-service/src/trpc/router/workspace-creation/github-schemas.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/host-service/src/trpc/router/workspace-creation/procedures/get-progress.ts
- packages/host-service/src/trpc/router/workspace-creation/procedures/generate-branch-name.ts
- packages/host-service/src/trpc/router/workspace-creation/helpers.ts
Comment on lines
+38
to
+57
| try { | ||
| const raw = await git.raw(["worktree", "list", "--porcelain"]); | ||
| let currentPath: string | null = null; | ||
| for (const line of raw.split("\n")) { | ||
| if (line.startsWith("worktree ")) { | ||
| currentPath = line.slice("worktree ".length).trim(); | ||
| } else if (line.startsWith("branch refs/heads/") && currentPath) { | ||
| const branch = line.slice("branch refs/heads/".length).trim(); | ||
| if (!branch) continue; | ||
| checkedOutBranches.add(branch); | ||
| if ( | ||
| knownPaths.has(currentPath) || | ||
| currentPath.startsWith(managedRoot + sep) | ||
| ) { | ||
| worktreeMap.set(branch, currentPath); | ||
| } | ||
| } else if (line === "") { | ||
| currentPath = null; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify projectWorktreesRoot implementation & separator usage
fd -t f 'helpers.ts' packages/host-service/src/trpc/router/workspace-creation
rg -nP -C3 'projectWorktreesRoot\s*=' packages/host-service/src/trpc/router/workspace-creation
# Check tests for Windows path scenarios
rg -nP -C2 '\\\\|projectWorktreesRoot|managedRoot' packages/host-service/src/trpc/router/workspace-creationRepository: superset-sh/superset
Length of output: 2856
🏁 Script executed:
rg -n -A 10 "const knownPaths" packages/host-service/src/trpc/router/workspace-creation/branch-helpers.tsRepository: superset-sh/superset
Length of output: 396
🏁 Script executed:
# Verify worktreePath handling in other parts of the codebase
rg -n "worktreePath" packages/host-service/src/trpc/router/workspace-creation --max-count=10Repository: superset-sh/superset
Length of output: 5285
🏁 Script executed:
# Check findWorktreeAtPath implementation to see how it handles path comparison
rg -n -B 5 -A 15 "function findWorktreeAtPath" packages/host-service/src/trpc/router/workspace-creation/branch-helpers.tsRepository: superset-sh/superset
Length of output: 1066
🏁 Script executed:
# Check the full listWorktreeBranches implementation to confirm the issue
rg -n -B 5 -A 35 "export async function listWorktreeBranches" packages/host-service/src/trpc/router/workspace-creation/branch-helpers.tsRepository: superset-sh/superset
Length of output: 1847
Path-separator mismatch can make managed-root check miss orphans on Windows.
git worktree list --porcelain emits paths with forward slashes on Windows, while projectWorktreesRoot() returns a path normalized by path.resolve() (using backslashes on Windows). Both knownPaths.has(currentPath) (line 49) and currentPath.startsWith(managedRoot + sep) (line 50) will fail on Windows because the paths have incompatible separators. This prevents orphan worktrees (on disk under the managed root but not in the workspaces table) from being detected for adoption, silently breaking the behavior described in the header comment ("partial create rollback").
Normalize currentPath via path.resolve() before the comparisons, mirroring the pattern already used correctly in findWorktreeAtPath() (line 86).
🔧 Proposed fix
for (const line of raw.split("\n")) {
if (line.startsWith("worktree ")) {
- currentPath = line.slice("worktree ".length).trim();
+ currentPath = resolve(line.slice("worktree ".length).trim());
} else if (line.startsWith("branch refs/heads/") && currentPath) {
const branch = line.slice("branch refs/heads/".length).trim();
if (!branch) continue;📝 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
| try { | |
| const raw = await git.raw(["worktree", "list", "--porcelain"]); | |
| let currentPath: string | null = null; | |
| for (const line of raw.split("\n")) { | |
| if (line.startsWith("worktree ")) { | |
| currentPath = line.slice("worktree ".length).trim(); | |
| } else if (line.startsWith("branch refs/heads/") && currentPath) { | |
| const branch = line.slice("branch refs/heads/".length).trim(); | |
| if (!branch) continue; | |
| checkedOutBranches.add(branch); | |
| if ( | |
| knownPaths.has(currentPath) || | |
| currentPath.startsWith(managedRoot + sep) | |
| ) { | |
| worktreeMap.set(branch, currentPath); | |
| } | |
| } else if (line === "") { | |
| currentPath = null; | |
| } | |
| } | |
| try { | |
| const raw = await git.raw(["worktree", "list", "--porcelain"]); | |
| let currentPath: string | null = null; | |
| for (const line of raw.split("\n")) { | |
| if (line.startsWith("worktree ")) { | |
| currentPath = resolve(line.slice("worktree ".length).trim()); | |
| } else if (line.startsWith("branch refs/heads/") && currentPath) { | |
| const branch = line.slice("branch refs/heads/".length).trim(); | |
| if (!branch) continue; | |
| checkedOutBranches.add(branch); | |
| if ( | |
| knownPaths.has(currentPath) || | |
| currentPath.startsWith(managedRoot + sep) | |
| ) { | |
| worktreeMap.set(branch, currentPath); | |
| } | |
| } else if (line === "") { | |
| currentPath = null; | |
| } | |
| } |
🤖 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/branch-helpers.ts`
around lines 38 - 57, The code parses git worktree paths into currentPath but
compares them to knownPaths and managedRoot using OS-specific separators,
causing mismatches on Windows; update the loop in which currentPath is set (the
variable currentPath used with knownPaths.has(...) and
currentPath.startsWith(managedRoot + sep)) to normalize currentPath via
path.resolve() (or an equivalent path normalization) immediately after
extracting it from the git output so subsequent comparisons to managedRoot and
knownPaths use consistent separators (mirror the normalization approach used in
findWorktreeAtPath/projectWorktreesRoot).
Comment on lines
+83
to
+189
| setProgress(input.pendingId, "creating_worktree"); | ||
|
|
||
| // 2. Validate + deduplicate branch name | ||
| // Renderer already sanitized/slugified. Host-service only validates | ||
| // and deduplicates — doesn't re-sanitize (which would strip case, | ||
| // slashes, etc. the user intended). | ||
| if (!input.names.branchName.trim()) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Branch name is empty", | ||
| }); | ||
| } | ||
|
|
||
| const existingBranches = await listBranchNames(ctx, localProject.repoPath); | ||
| const branchName = deduplicateBranchName( | ||
| input.names.branchName, | ||
| existingBranches, | ||
| ); | ||
|
|
||
| // 3. Create worktree | ||
| const worktreePath = safeResolveWorktreePath(localProject.id, branchName); | ||
| mkdirSync(dirname(worktreePath), { recursive: true }); | ||
|
|
||
| const git = await ctx.git(localProject.repoPath); | ||
|
|
||
| // Trust the picker's hint when provided: it knows whether the row | ||
| // the user clicked was local or remote-only. Re-resolving here | ||
| // races against stale cached refs (a workspace branch with an | ||
| // incidental `refs/remotes/origin/<name>` cache would silently win). | ||
| // Falls back to probing for callers that don't pass the hint. | ||
| let startPoint: ResolvedRef = | ||
| input.composer.baseBranch && input.composer.baseBranchSource | ||
| ? buildStartPointFromHint( | ||
| input.composer.baseBranch, | ||
| input.composer.baseBranchSource, | ||
| ) | ||
| : await resolveStartPoint(git, input.composer.baseBranch); | ||
|
|
||
| // Local default branches are rarely fast-forwarded; swap to the | ||
| // branch's configured upstream so we fork from the real tip, not a | ||
| // stale local ref. Non-default branches stay local-first by design. | ||
| if (startPoint.kind === "local") { | ||
| const defaultBranchName = await resolveDefaultBranchName(git); | ||
| if (startPoint.shortName === defaultBranchName) { | ||
| const upstream = await resolveUpstream(git, defaultBranchName); | ||
| if (upstream) { | ||
| const remoteRef = asRemoteRef(upstream.remote, upstream.remoteBranch); | ||
| const remoteExists = await git | ||
| .raw(["rev-parse", "--verify", "--quiet", `${remoteRef}^{commit}`]) | ||
| .then(() => true) | ||
| .catch(() => false); | ||
| if (remoteExists) { | ||
| startPoint = { | ||
| kind: "remote-tracking", | ||
| fullRef: remoteRef, | ||
| shortName: upstream.remoteBranch, | ||
| remote: upstream.remote, | ||
| remoteShortName: `${upstream.remote}/${upstream.remoteBranch}`, | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.log( | ||
| `[workspaceCreation.create] start point: ${startPoint.kind} (${ | ||
| input.composer.baseBranchSource ? "from hint" : "resolved" | ||
| })`, | ||
| ); | ||
|
|
||
| // If we resolved to a remote-tracking ref, fetch just that branch | ||
| // to ensure we're branching from the latest remote state. | ||
| if (startPoint.kind === "remote-tracking") { | ||
| try { | ||
| await git.fetch([ | ||
| startPoint.remote, | ||
| startPoint.shortName, | ||
| "--quiet", | ||
| "--no-tags", | ||
| ]); | ||
| } catch (err) { | ||
| console.warn( | ||
| `[workspaceCreation.create] fetch ${startPoint.remoteShortName} failed, proceeding with local ref:`, | ||
| err, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Always create a new branch — never check out an existing one. | ||
| // Checking out existing branches is a separate intent (createFromPr, | ||
| // or the picker's Check out action via the `checkout` procedure). | ||
| // --no-track keeps `git pull` / ahead-behind counts from treating | ||
| // the start point as the branch's home. Push targeting is handled | ||
| // separately by push.autoSetupRemote (set below). | ||
| const startPointArg = | ||
| startPoint.kind === "head" ? "HEAD" : startPoint.shortName; | ||
| await git.raw([ | ||
| "worktree", | ||
| "add", | ||
| "--no-track", | ||
| "-b", | ||
| branchName, | ||
| worktreePath, | ||
| startPoint.kind === "remote-tracking" | ||
| ? startPoint.remoteShortName | ||
| : startPointArg, | ||
| ]); |
Contributor
There was a problem hiding this comment.
Pre-registering failure paths don't clear progress — stale UI state until the 5-minute sweep.
After setProgress(pendingId, "creating_worktree") at line 83, the code runs several throw-capable operations (listBranchNames, safeResolveWorktreePath, mkdirSync, ctx.git, resolveStartPoint, and most notably git.raw(["worktree", "add", …]) at lines 179–189) without a clearProgress wrapper. Any of them throwing (branch path collision with an existing dir, filesystem perms, worktree add tripping over an existing branch ref, etc.) surfaces the TRPC error to the client but leaves createProgress.get(pendingId) pinned at creating_worktree until sweepStaleProgress fires 5 minutes later.
checkout.ts handles this symmetrically (wraps safeResolveWorktreePath and worktree add in try/catches that call clearProgress). A simple fix is a single top-level try/catch around everything between setProgress("creating_worktree") and the host/cloud block (which already has its own cleanup). Minor severity — the renderer eventually recovers — but it's a free UX win.
🛠️ Suggested shape
- setProgress(input.pendingId, "creating_worktree");
-
- // 2. Validate + deduplicate branch name
+ setProgress(input.pendingId, "creating_worktree");
+
+ let worktreePath: string;
+ let branchName: string;
+ let git: Awaited<ReturnType<typeof ctx.git>>;
+ let startPoint: ResolvedRef;
+ try {
+ // … everything from branch validation through worktree add + config …
+ } catch (err) {
+ clearProgress(input.pendingId);
+ throw err;
+ }🤖 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/create.ts`
around lines 83 - 189, The progress state is left as "creating_worktree" if any
of the pre-registering steps (listBranchNames, safeResolveWorktreePath,
mkdirSync, ctx.git/resolveStartPoint, or the git.raw(["worktree","add",...])
call) throw; wrap the block starting immediately after
setProgress(input.pendingId, "creating_worktree") through the worktree creation
call in a try/catch that calls clearProgress(input.pendingId) on any error and
then rethrows the error so existing error handling still runs; reference the
functions/variables setProgress, clearProgress, listBranchNames,
safeResolveWorktreePath, ctx.git / resolveStartPoint, and git.raw (worktree add)
when locating where to add the try/catch.
Comment on lines
+8
to
+12
| z.object({ | ||
| projectId: z.string(), | ||
| query: z.string().optional(), | ||
| limit: z.number().min(1).max(100).optional(), | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GitHub's REST search.issuesAndPullRequests per_page parameter require an integer?
💡 Result:
Yes, GitHub's REST API search.issuesAndPullRequests (GET /search/issues) per_page parameter requires an integer. The official documentation explicitly types it as "per_page (integer)", describing it as "The number of results per page (max 100)". Default is 30, max 100. This is consistent across pagination parameters in the REST API.
Citations:
- 1: https://docs.github.com/rest/search/search
- 2: https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api
- 3: https://github.com/octokit/plugin-rest-endpoint-methods.js/blob/main/docs/search/issuesAndPullRequests.md
- 4: https://docs.github.com/rest/issues/issues
Add .int() to validate limit as an integer.
z.number().min(1).max(100) accepts fractional values (e.g., 30.5), which are then passed as per_page to octokit. GitHub's REST API requires per_page to be an integer; add .int() for proper validation. Same fix applies to search-pull-requests.ts.
🔧 Proposed fix
- limit: z.number().min(1).max(100).optional(),
+ limit: z.number().int().min(1).max(100).optional(),📝 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
| z.object({ | |
| projectId: z.string(), | |
| query: z.string().optional(), | |
| limit: z.number().min(1).max(100).optional(), | |
| }), | |
| z.object({ | |
| projectId: z.string(), | |
| query: z.string().optional(), | |
| limit: z.number().int().min(1).max(100).optional(), | |
| }), |
🤖 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/search-github-issues.ts`
around lines 8 - 12, The Zod schema for the procedure (the z.object with keys
projectId, query, limit) allows fractional numbers because it uses
z.number().min(1).max(100); change the limit validator to
z.number().int().min(1).max(100) so only integers are accepted before passing to
Octokit as per_page; apply the same change in the matching schema in
search-pull-requests.ts (look for the identical z.object with
projectId/query/limit).
Comment on lines
+22
to
+35
| export function setProgress(pendingId: string, activeStepId: string): void { | ||
| let reachedActive = false; | ||
| const steps: ProgressStep[] = STEP_DEFINITIONS.map((def) => { | ||
| if (def.id === activeStepId) { | ||
| reachedActive = true; | ||
| return { id: def.id, label: def.label, status: "active" as const }; | ||
| } | ||
| if (!reachedActive) { | ||
| return { id: def.id, label: def.label, status: "done" as const }; | ||
| } | ||
| return { id: def.id, label: def.label, status: "pending" as const }; | ||
| }); | ||
| createProgress.set(pendingId, { steps, updatedAt: Date.now() }); | ||
| } |
Contributor
There was a problem hiding this comment.
setProgress silently marks all steps as done when the caller passes an unknown activeStepId.
If activeStepId does not match any entry in STEP_DEFINITIONS (typo, rename drift, or a caller passing a step id from a different schema), reachedActive never flips, so every step is emitted with status: "done". The renderer polling get-progress then sees a fully-completed state while the workspace-creation flow is still mid-execution — it'd be easy for this to quietly hide a real error in the future.
Consider validating activeStepId is a known id and failing loudly (or asserting via a union type):
🛠️ Proposed fix
-export function setProgress(pendingId: string, activeStepId: string): void {
- let reachedActive = false;
+type StepId = (typeof STEP_DEFINITIONS)[number]["id"];
+
+export function setProgress(pendingId: string, activeStepId: StepId): void {
+ if (!STEP_DEFINITIONS.some((d) => d.id === activeStepId)) {
+ // Fail loudly rather than silently reporting "done" for everything.
+ throw new Error(`[progress] unknown step id: ${activeStepId}`);
+ }
+ let reachedActive = false;🤖 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/progress.ts` around
lines 22 - 35, setProgress currently treats an unknown activeStepId as if all
steps are done; update setProgress to validate that activeStepId exists in
STEP_DEFINITIONS before mapping: use STEP_DEFINITIONS.some or find to check for
a matching def.id (reference symbols: setProgress, STEP_DEFINITIONS,
createProgress.set) and if not found throw a descriptive Error (or assert)
indicating the invalid activeStepId and the expected ids, otherwise proceed with
the existing mapping logic and call createProgress.set as before.
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
workspace-creation.tsinto focused files; the main file is now a 27-line router composition.progress.ts,helpers.ts,branch-helpers.ts,finish-checkout.ts,github-schemas.ts) and move each tRPC procedure into its own file underprocedures/.workspaceCreationRouterexported via./index.ts) is unchanged — no behavior changes.Test plan
bun run typecheck --filter=@superset/host-servicebun test packages/host-service/src/trpc/router/workspace-creation(72 pass)bun run lint:fixcleanSummary by cubic
Split the monolithic workspace-creation router into focused files. Public API
workspaceCreationRouterand behavior are unchanged.procedures/(e.g.,create,checkout,adopt,generate-branch-name,get-context,get-progress,get-github-issue-content,get-github-pull-request-content,search-branches,search-github-issues,search-pull-requests).branch-helpers.ts,finish-checkout.ts,helpers.ts,progress.ts, andgithub-schemas.ts.workspace-creation.tsto a small router composition and continued exporting via./index.ts.Written for commit 046d0e8. Summary will update on new commits.
Summary by CodeRabbit