Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 26 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 大規模な構造的リファクタリング(24 ファイル新規作成)、多様な関心事の分散(手続き・スキーマ・共有ユーティリティ・型)、複雑な制御フロー(進捗追跡、ワークツリー・Git操作、クラウド・ローカルの同期化、ロールバック)が含まれるため。 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eb0a815c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .update(v2Workspaces) | ||
| .set(data) | ||
| .where(eq(v2Workspaces.id, workspace.id)) |
There was a problem hiding this comment.
Make expectedCurrentName guard atomic in updateNameFromHost
The race-protection for expectedCurrentName is currently split into a read-then-update sequence, but the UPDATE only filters by workspace id. If a user renames the workspace after the initial findFirst check and before this UPDATE executes, the AI rename still overwrites that newer user value, which is the exact race this endpoint is meant to prevent. Include expectedCurrentName in the UPDATE predicate (or use a transaction/locking strategy) so the guard is enforced atomically.
Useful? React with 👍 / 👎.
| ctx.db | ||
| .update(workspaces) | ||
| .set({ branch: deduped }) | ||
| .where(eq(workspaces.id, workspaceId)) |
There was a problem hiding this comment.
Keep worktree path consistent after AI branch rename
After applyAiWorkspaceRename renames the git branch, only workspaces.branch is updated locally while worktreePath is left at the old .worktrees/<old-branch> directory. New workspace creation derives paths from branch names (safeResolveWorktreePath), so reusing the old branch name later can collide with that still-existing directory and fail checkout/create despite no branch-name collision. The rename flow should either move the worktree directory too or account for occupied worktree paths when choosing/deduplicating branch names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
packages/host-service/src/trpc/router/workspace-creation/shared/types.ts (1)
5-30: オブジェクト型はinterfaceで定義する規約に合わせることを推奨します。
TerminalDescriptor/BranchRow/CheckoutResultはいずれもプレーンなオブジェクト形なので、規約に従いinterface化するのが望ましいです。GitClientはAwaited<ReturnType<...>>による派生型なのでtypeのままで問題ありません。♻️ 推奨パッチ
-export type TerminalDescriptor = { - id: string; - role: string; - label: string; -}; +export interface TerminalDescriptor { + id: string; + role: string; + label: string; +} -export type BranchRow = { +export interface BranchRow { name: string; lastCommitDate: number; isLocal: boolean; isRemote: boolean; recency: number | null; worktreePath: string | null; // True when a workspaces row exists for this (project, branch) on this // host. A worktree can exist on disk without one (orphan); the Worktree // tab distinguishes Open (hasWorkspace) from Create (orphan adopt). hasWorkspace: boolean; isCheckedOut: boolean; -}; +} -export type CheckoutResult = { - workspace: { id: string }; - terminals: TerminalDescriptor[]; - warnings: string[]; - alreadyExists?: false; -}; +export interface CheckoutResult { + workspace: { id: string }; + terminals: TerminalDescriptor[]; + warnings: string[]; + alreadyExists?: false; +}As per coding guidelines: 「Prefer
interfacefor defining object shapes in TypeScript」。🤖 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/shared/types.ts` around lines 5 - 30, The exported object-shaped types TerminalDescriptor, BranchRow, and CheckoutResult should be converted from type aliases to interfaces to follow the project's TS convention; update declarations for TerminalDescriptor, BranchRow, and CheckoutResult to use interface instead of type (preserving all properties, optional flags like alreadyExists?: false, and array/primitive types) while leaving any non-object derived types (e.g., GitClient-related Awaited/ReturnType usage) unchanged.packages/host-service/src/trpc/router/workspace-creation/utils/list-branch-names.ts (1)
33-35: エラーを完全に無視せず、ログに残すことを推奨します。
git rawの失敗や入力パース失敗の両方を[]で吸収しており、AI ブランチ名生成の入力が欠落しても静かに進行します。戻り値のフォールバックは維持しつつ、原因追跡のためにconsole.warnを 1 行追加するだけで運用時の切り分けが楽になります。🔧 推奨パッチ
- } catch { - return []; - } + } catch (err) { + console.warn( + "[workspaceCreation.listBranchNames] failed to enumerate refs", + { repoPath, err }, + ); + return []; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/utils/list-branch-names.ts` around lines 33 - 35, The catch block in list-branch-names.ts is silently swallowing errors (it currently reads "} catch { return []; }"); update the catch to capture the thrown error (e.g., "catch (err)") and add a single-line console.warn or logger call that includes contextual text and the error object (for example: "console.warn('listBranchNames: failed to list branches', err)"), then still return [] so the fallback behavior remains unchanged.packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts (1)
111-140: クラウドロールバック失敗時にゾンビワークスペースが残り得ます(軽微)。ローカル DB 挿入失敗 → クラウド
v2Workspace.delete失敗という二重障害時、catch内で警告ログのみ残して元エラーを throw するため、クラウド側に孤立 workspace 行が残存する可能性があります。現状の運用として許容範囲ですが、将来的には再試行キュー/リコンシリエーションジョブに回す、あるいはレスポンス中でcleanupErrorをクライアントに通知するなどの拡張を検討する余地があります。🤖 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/shared/finish-checkout.ts` around lines 111 - 140, The catch block for the local DB insert can leave a zombie cloud workspace if ctx.api.v2Workspace.delete.mutate fails; capture the cleanup error (cleanupErr) when delete.catch runs, log it with context, and include that cleanupErr information in the TRPCError you throw (e.g., append to the message or set as the error cause/metadata) so callers are aware a cloud rollback also failed; also consider enqueuing the failed cloud cleanup for retry if you have a retry/reconciliation mechanism (pass cloudRow.id and cleanupErr to that queue) and ensure rollbackWorktree and clearProgress are still executed.packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts (2)
214-230: Promise チェーン.catch()とawaitの混在
await ctx.api.v2Workspace.create.mutate(...).catch(async (err) => { ...; throw err; })は動作はしますが、同ファイル内の他箇所(Lines 198–212 の ensureV2Host 等)はtry/catchスタイルを採用しており不統一です。コーディングガイドラインの async/await 推奨方針にも沿うので、統一するとレビュアビリティが上がります。As per coding guidelines: "Use async/await for promises in TypeScript/JavaScript".
🤖 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 214 - 230, Replace the mixed await/.catch pattern around ctx.api.v2Workspace.create.mutate with a try/catch block to match the surrounding code style: move the awaited call to inside try, and in the catch call clearProgress(input.pendingId), await rollbackWorktree(), log the error (same message using console.error with the err and context like "[workspaceCreation.create] v2Workspace.create failed"), then rethrow the error so callers still see it; reference the cloudRow assignment, ctx.api.v2Workspace.create.mutate, clearProgress, rollbackWorktree, and input.pendingId when making the change.
197-270:createとfinishCheckout間でホスト登録・クラウド作成・ローカル永続化ロジックが重複ensureV2Host → v2Workspace.create → ローカル workspaces insert → 各段のロールバックという同一シーケンスが
create.ts(Lines 197–270) とfinish-checkout.tsにほぼコピペで存在します。今回は上流 split の忠実移植を優先してよいですが、将来的には共有ヘルパ(例:persistWorkspace(ctx, { cloudInput, localInput, rollbacks }))に集約することでロールバック漏れのリスクを減らせます。🤖 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 197 - 270, The host registration -> cloud workspace create -> local persist -> per-step rollback sequence is duplicated between create.ts and finish-checkout.ts; extract that flow into a shared helper (e.g., persistWorkspace) and call it from both places so rollback logic is centralized. Specifically, encapsulate the calls to ctx.api.device.ensureV2Host.mutate (ensureV2Host), ctx.api.v2Workspace.create.mutate (v2Workspace.create) and the local DB insert (ctx.db.insert(workspaces).values(...).run()) into the helper, accept inputs for cloud/local payloads and a rollback handler list, perform try/catch around each step inside the helper, run registered rollback actions on failure, and surface a uniform error (TRPCError) back to callers; then replace the inline sequence in create.ts and finish-checkout.ts with calls to this new persistWorkspace helper.packages/host-service/src/trpc/router/workspace-creation/utils/ai-workspace-names.ts (2)
183-212: ロールバック処理を async/await に統一することを推奨ロールバックの2箇所で
ctx.git(worktreePath).then((g) => g.raw(...)).catch(...)という Promise チェーンを使っていますが、関数自体はasyncなのでコーディングガイドライン通りtry/catch+awaitに統一するほうが読みやすく、スタックトレースも改善されます。♻️ Proposed refactor
} catch (err) { // Cloud update failed — roll back git rename to keep git and cloud in sync. if (gitRenamed) { - await ctx - .git(worktreePath) - .then((g) => g.raw(["branch", "-m", deduped, oldBranchName])) - .catch((rollbackErr) => { - console.warn( - `[applyAiWorkspaceRename] git branch rollback failed (workspace ${workspaceId}, ${deduped} → ${oldBranchName})`, - rollbackErr, - ); - }); + try { + const g = await ctx.git(worktreePath); + await g.raw(["branch", "-m", deduped, oldBranchName]); + } catch (rollbackErr) { + console.warn( + `[applyAiWorkspaceRename] git branch rollback failed (workspace ${workspaceId}, ${deduped} → ${oldBranchName})`, + rollbackErr, + ); + } } throw err; } // No-op detection: if the cloud row's name still equals oldWorkspaceName, // the expected-name guard fired (user renamed in the meantime). Revert // the git rename and skip local DB update so all three stay in lockstep. if (gitRenamed && cloudResult.branch !== deduped) { - await ctx - .git(worktreePath) - .then((g) => g.raw(["branch", "-m", deduped, oldBranchName])) - .catch((rollbackErr) => { - console.warn( - `[applyAiWorkspaceRename] no-op rollback failed (workspace ${workspaceId}, ${deduped} → ${oldBranchName})`, - rollbackErr, - ); - }); + try { + const g = await ctx.git(worktreePath); + await g.raw(["branch", "-m", deduped, oldBranchName]); + } catch (rollbackErr) { + console.warn( + `[applyAiWorkspaceRename] no-op rollback failed (workspace ${workspaceId}, ${deduped} → ${oldBranchName})`, + rollbackErr, + ); + } return; }As per coding guidelines: "Use async/await for promises in TypeScript/JavaScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/utils/ai-workspace-names.ts` around lines 183 - 212, Both rollback blocks use Promise chaining (ctx.git(worktreePath).then(...).catch(...)) inside an async function—replace each with await inside a try/catch to match async/await style: await ctx.git(worktreePath) to get g, then await g.raw(["branch", "-m", deduped, oldBranchName]) inside a try, and on error catch it and call console.warn with the same message and the caught rollbackErr; keep the original thrown err after the first rollback and the return after the second. Reference symbols: ctx.git, worktreePath, gitRenamed, deduped, oldBranchName, cloudResult.branch.
198-212: no-op 検出をcloudResult.branchではなくcloudResult.nameに対しても確認することを検討現在の no-op 検出は「
gitRenamedが true かつcloudResult.branch !== deduped」で expected-name ガードの発火を推定していますが、これは「ガード発火時は branch も含め patch 全体が無視される」という v2Workspace 側の実装に暗黙依存しています。将来updateNameFromHostの挙動が細粒度化(name だけガード、branch は更新)された場合、このチェックは静かに破綻します。
expectedCurrentNameを設定した場合に限り、cloudResult.name === oldWorkspaceNameもしくは明示的な no-op フラグを確認する方が契約に対して頑健です。少なくともコメントで「cloudResult.branch チェックは updateNameFromHost のオールオアナッシング契約に依存」という不変条件を明記しておくと安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/utils/ai-workspace-names.ts` around lines 198 - 212, The no-op detection in applyAiWorkspaceRename currently relies on gitRenamed && cloudResult.branch !== deduped and implicitly assumes updateNameFromHost updates branch and name together; change the check to also consider cloudResult.name when expectedCurrentName is provided (i.e., treat it as a no-op if expectedCurrentName is set and cloudResult.name === oldWorkspaceName), or alternatively detect an explicit no-op flag from the cloudResult; if you choose not to change logic, at minimum add a clear comment near gitRenamed/cloudResult.branch explaining that this check depends on updateNameFromHost being all-or-nothing for branch+name. Ensure you reference gitRenamed, cloudResult.branch, cloudResult.name, deduped, oldWorkspaceName, expectedCurrentName, and updateNameFromHost in the change so future readers/maintainers see the contract.packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts (1)
15-47: 進行状態creating_worktreeを冪等性チェック前に設定している点(軽微)Line 22 で
creating_worktreeを設定した直後、Line 31–38 で既存ワークスペースを検出し Line 40 で clearProgress しています。レンダラがcreating_worktree状態を一瞬観測して即座にクリアされる挙動になるため、進行状態を購読する UI 側でフラッシュが出る可能性があります。冪等ケースでは
setProgress(input.pendingId, "creating_worktree")を既存チェックの後ろに移すか、冪等ヒット時のみ専用状態(例: "reusing_existing")を通すと UX が滑らかになります。優先度は低いです。🤖 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 15 - 47, 現在は setProgress(input.pendingId, "creating_worktree") が既存ワークスペースの冪等性チェックより前に呼ばれており、UI に短いフラッシュを生じさせるため、PR パスの処理(derivePrLocalBranchName → ctx.db.query.workspaces.findFirst)で既存ワークスペースを確認するロジックの前に setProgress を移動するか、冪等ヒット時のみ別の状態を使うように修正してください(対象箇所: protectedProcedure checkout, 関数/識別子: setProgress, clearProgress, derivePrLocalBranchName, ctx.db.query.workspaces.findFirst, alreadyExists return path)。具体的には既存チェックを先に行い、既存があれば必要に応じて setProgress(input.pendingId, "reusing_existing") を設定してから clearProgress を呼び出して返却し、既存でなければその後に setProgress(input.pendingId, "creating_worktree") を呼ぶようにしてください。packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts (1)
106-152: ステール行削除と新規 insert がトランザクション外で実行される点Line 118 の
deleteと Line 122–134 のinsertがトランザクションにラップされていないため、停電やプロセスクラッシュが両者の間に入った場合、ローカル DB に (projectId, branch) のマッピングが一時的に消える隙間が生じます。さらに insert 失敗時のロールバック(Line 140)はクラウド側を消すだけで、先に消したステールローカル行は復元しません。実害は限定的(リトライで再 adopt 可能)ですが、Drizzle の同期 API でも
ctx.db.transaction((tx) => { ... })でまとめれば一貫性が保てます。優先度は低いので後続 PR でも問題ありません。♻️ Proposed fix
- 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(); - } - - try { - ctx.db - .insert(workspaces) - .values({ id: cloudRow.id, projectId: input.projectId, worktreePath, branch }) - .onConflictDoUpdate({ - target: workspaces.id, - set: { projectId: input.projectId, worktreePath, branch }, - }) - .run(); - } catch (err) { + try { + ctx.db.transaction((tx) => { + const stale = tx.query.workspaces + .findFirst({ + where: and( + eq(workspaces.projectId, input.projectId), + eq(workspaces.branch, branch), + ), + }) + .sync(); + 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 }) + .onConflictDoUpdate({ + target: workspaces.id, + set: { projectId: input.projectId, worktreePath, branch }, + }) + .run(); + }); + } catch (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/adopt.ts` around lines 106 - 152, The stale-row delete (ctx.db.delete(workspaces).where(...)) and the insert (ctx.db.insert(workspaces).values(...).onConflictDoUpdate(...)) must be executed inside a single transaction to avoid a window where the (projectId, branch) mapping is missing; wrap both operations in ctx.db.transaction(...) and use the transaction handle (tx.query.workspaces.findFirst, tx.delete(workspaces).where, tx.insert(workspaces)...run) so both the stale removal and upsert are atomic and will roll back together on error, and then remove or adjust the post-error cloud cleanup logic that currently calls ctx.api.v2Workspace.delete on insert failure (since the transaction will preserve/restore local state).packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts (3)
17-22:BranchAccumはtypeエイリアスでなくinterfaceで定義を。純粋なオブジェクト形状のため、リポジトリのコーディング規約に沿って
interface宣言が望ましいです。🛠 Proposed fix
-type BranchAccum = { - name: string; - lastCommitDate: number; - isLocal: boolean; - isRemote: boolean; -}; +interface BranchAccum { + name: string; + lastCommitDate: number; + isLocal: boolean; + isRemote: boolean; +}As per coding guidelines: "Prefer
interfacefor defining object shapes in TypeScript".🤖 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 17 - 22, Replace the type alias BranchAccum with an equivalent interface declaration (interface BranchAccum { name: string; lastCommitDate: number; isLocal: boolean; isRemote: boolean; }) to follow the repo's coding guideline preferring interfaces for plain object shapes; update any imports/uses that reference BranchAccum if needed but keep the property names and types unchanged.
43-50:git.fetchにタイムアウトやキャンセルを検討。ユーザー側のネットワーク断・プロキシ経路のハングでリモートフェッチが長時間ブロックすると、このクエリ自体がリクエスト・スレッドを掴んで応答が返らなくなります。
--quietだけでは時間上限はつかないので、AbortSignal 対応または外側でのタイムアウトラッパーを入れておくと体感レイテンシが安定します。Chill モードなので任意対応で構いません。🤖 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 43 - 50, The remote git.fetch call can hang indefinitely; wrap the fetch so it respects a timeout/abort: when input.refresh and shouldRefetchRemote(input.projectId) is true, create an AbortController (or equivalent timeout wrapper) tied to a short configurable timeout, pass its signal into git.fetch (or use Promise.race to reject after timeout if git.fetch doesn't accept signals), ensure you call markRefetchRemote(input.projectId) before attempting fetch, clear/abort the controller on success, and catch both fetch errors and abort/timeouts to proceed with cached refs as currently done; reference git.fetch, shouldRefetchRemote, markRefetchRemote, input.refresh, and input.projectId when making the change.
63-71:workspacesクエリはカラムを射影したほうが経済的です。
.select()は全カラムを取得しますが、後続でbranchしか利用していません。Drizzle の投影を使えば転送量とマッピングコストを抑えられます。1 プロジェクトあたりの workspace 行数は通常少ないので optional ですが、読みやすさの面でも明示的な方が良いです。🛠 Proposed fix
const workspaceBranches = new Set<string>( ctx.db - .select() + .select({ branch: workspaces.branch }) .from(workspaces) .where(eq(workspaces.projectId, input.projectId)) .all() .map((workspace) => workspace.branch) .filter((branch): branch is string => Boolean(branch)), );🤖 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 63 - 71, The query currently calls ctx.db.select() which fetches all columns but only uses the branch field; change the Drizzle projection to select only workspaces.branch (e.g., use ctx.db.select(workspaces.branch) or select({branch: workspaces.branch}) from workspaces) in the block that constructs workspaceBranches so you only retrieve and map the branch values; also update the subsequent mapping/filtering to match the projected result shape (string or {branch: string}) so workspaceBranches remains a Set<string>.packages/host-service/src/trpc/router/workspace-creation/shared/branch-search.ts (1)
94-121:findWorktreeAtPathでユーザー指定パスに symlink が含まれる場合、パス比較が失敗する可能性あり
resolvePathはnode:path.resolve()の別名で、相対パスと..の正規化のみ行い symlink を解決しません。adoptで v1→v2 移行時にユーザーが明示的にworktreePathを指定した場合、そのパスに symlink が含まれていると、git worktree list --porcelainが返す正準パス(macOS での/var→/private/varなど)との比較に失敗し、adopt が失敗する可能性があります。ただし、本フォークで Superset が管理するworktree は
<repoPath>/.worktrees/<branch>配下に統一されており(コメント"Worktrees are under /.worktrees/, not ~/.superset/worktrees"参照)、この場合はlistWorktreeBranchesが返す正規化済みパスが使用されるため、実際のリスクは adopt での ユーザー指定パス処理に限定されます。将来的にはfs.promises.realpathの併用を検討する価値があります。🤖 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/shared/branch-search.ts` around lines 94 - 121, findWorktreeAtPath currently uses resolvePath (path.resolve) which does not resolve symlinks, causing mismatches with git worktree's canonical paths; change the comparison to use fs.promises.realpath for both the input worktreePath and the currentPath parsed from git output (within findWorktreeAtPath) so symlinks are resolved before comparing to targetPath, and fall back to resolvePath if realpath fails (catch and degrade gracefully); reference resolvePath and the findWorktreeAtPath function and ensure you await realpath for both sides of the equality check.
🤖 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/create.ts`:
- Around line 165-182: checkout path is missing the remote-tracking-only guard
that create.ts uses: ensure finishCheckout does not unconditionally write
branch.<branch>.base for local-only names. Reconstruct the startPoint from the
provided baseBranchSource (like create.ts) or add the same check
(startPoint.kind === "remote-tracking") inside finishCheckout before calling
git.raw(["config", `branch.${branchName}.base`, ...]). Also update the
misleading comment "matches create's head-start-point behavior" to state that
create applies a kind filter so finishCheckout must mirror that behavior
(remote-tracking-only) rather than only checking presence of args.baseBranch.
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/branch-search.ts`:
- Around line 140-150: The regex used to parse reflog lines is greedy and thus
miscaptures branch names that themselves contain " to "; update the pattern used
in branch-search (variable re) to make the first .+ non-greedy (e.g. change
/^checkout: moving from .+ to (.+)$/ to /^checkout: moving from .*? to (.+)$/ or
equivalent), then keep the existing logic that trims name and skips SHA-like
values so branch names with " to " are captured correctly into order.
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts`:
- Around line 34-51: The finishCheckout call is being passed
input.composer.baseBranch unconditionally which allows local-only branches to be
written into branch.<name>.base and later referenced as
origin/<local-branch-name> by resolveBaseComparison; update both checkout.ts
call sites (the paths that currently forward input.composer.baseBranch) to only
pass baseBranch when the start point is remote-tracking (i.e. startPoint.kind
=== "remote-tracking") or otherwise validate using the same baseBranchSource
logic used in create.ts, so finishCheckout only receives a remote-tracking
baseBranch and local-only branches are never recorded.
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/progress-store.ts`:
- Around line 41-42: getProgress currently lets in-progress entries expire
because updatedAt is only set by setProgress, so long-running steps
(runSetupScript, heavy checkout) get swept as stale and return null; fix by
either refreshing the TTL on poll — e.g., when getProgress(pendingId) finds a
non-null progress, update its updatedAt (or call setProgress/refreshProgress) to
extend life — or change sweepStaleProgress to only remove entries whose status
is terminal (completed/failed) and rely on explicit cleanup on completion; apply
the same change to the related logic that uses sweepStaleProgress (lines around
the other get/cleanup calls referenced in the diff).
In `@packages/host-service/src/trpc/router/workspace-creation/shared/types.ts`:
- Line 29: The CheckoutResult discriminated-union is incorrect: currently it
defines alreadyExists?: false but callers return alreadyExists: true or omit the
field for new checkouts; update the CheckoutResult type (in
packages/host-service/src/trpc/router/workspace-creation/shared/types.ts) to
reflect both cases either by using a proper union of the two shapes (one variant
with alreadyExists: true and one without the field) or by making alreadyExists?:
true (optional true) so that result.alreadyExists === true can be used for
type-safe branching; adjust the exported CheckoutResult type definition
accordingly.
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/worktree-paths.ts`:
- Around line 13-18: The current check lets branchName "" or "." pass because
resolve(repoPath, ".worktrees", branchName) equals worktreesRoot; update the
validation around worktreesRoot/worktreePath to explicitly reject empty and
special-segment names by validating branchName first (e.g., if branchName === ""
|| branchName === "." || branchName === ".." then throw), then keep the existing
check that worktreePath is either equal to or a child of worktreesRoot (using
worktreesRoot and worktreePath) to prevent path traversal; reference branchName,
worktreesRoot, and worktreePath to locate and harden the guard before subsequent
create/delete logic.
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Around line 225-249: The current SELECT-then-UPDATE flow for handling
input.expectedCurrentName is vulnerable to a TOCTOU race because the name check
happens in JS instead of being enforced in the UPDATE WHERE clause; change the
dbWs.update(...) call to include the name equality guard when
input.expectedCurrentName is provided (i.e. add eq(v2Workspaces.name,
input.expectedCurrentName) into the where chain alongside eq(v2Workspaces.id,
workspace.id)), and then treat a 0-row update as a no-op by re-querying the
workspace by id (using dbWs or the existing read path) and returning that
current row (if still present) or throwing the existing NOT_FOUND error only if
the workspace truly no longer exists.
---
Nitpick comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts`:
- Around line 106-152: The stale-row delete
(ctx.db.delete(workspaces).where(...)) and the insert
(ctx.db.insert(workspaces).values(...).onConflictDoUpdate(...)) must be executed
inside a single transaction to avoid a window where the (projectId, branch)
mapping is missing; wrap both operations in ctx.db.transaction(...) and use the
transaction handle (tx.query.workspaces.findFirst, tx.delete(workspaces).where,
tx.insert(workspaces)...run) so both the stale removal and upsert are atomic and
will roll back together on error, and then remove or adjust the post-error cloud
cleanup logic that currently calls ctx.api.v2Workspace.delete on insert failure
(since the transaction will preserve/restore local state).
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts`:
- Around line 15-47: 現在は setProgress(input.pendingId, "creating_worktree")
が既存ワークスペースの冪等性チェックより前に呼ばれており、UI に短いフラッシュを生じさせるため、PR
パスの処理(derivePrLocalBranchName →
ctx.db.query.workspaces.findFirst)で既存ワークスペースを確認するロジックの前に setProgress
を移動するか、冪等ヒット時のみ別の状態を使うように修正してください(対象箇所: protectedProcedure checkout, 関数/識別子:
setProgress, clearProgress, derivePrLocalBranchName,
ctx.db.query.workspaces.findFirst, alreadyExists return
path)。具体的には既存チェックを先に行い、既存があれば必要に応じて setProgress(input.pendingId,
"reusing_existing") を設定してから clearProgress を呼び出して返却し、既存でなければその後に
setProgress(input.pendingId, "creating_worktree") を呼ぶようにしてください。
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts`:
- Around line 214-230: Replace the mixed await/.catch pattern around
ctx.api.v2Workspace.create.mutate with a try/catch block to match the
surrounding code style: move the awaited call to inside try, and in the catch
call clearProgress(input.pendingId), await rollbackWorktree(), log the error
(same message using console.error with the err and context like
"[workspaceCreation.create] v2Workspace.create failed"), then rethrow the error
so callers still see it; reference the cloudRow assignment,
ctx.api.v2Workspace.create.mutate, clearProgress, rollbackWorktree, and
input.pendingId when making the change.
- Around line 197-270: The host registration -> cloud workspace create -> local
persist -> per-step rollback sequence is duplicated between create.ts and
finish-checkout.ts; extract that flow into a shared helper (e.g.,
persistWorkspace) and call it from both places so rollback logic is centralized.
Specifically, encapsulate the calls to ctx.api.device.ensureV2Host.mutate
(ensureV2Host), ctx.api.v2Workspace.create.mutate (v2Workspace.create) and the
local DB insert (ctx.db.insert(workspaces).values(...).run()) into the helper,
accept inputs for cloud/local payloads and a rollback handler list, perform
try/catch around each step inside the helper, run registered rollback actions on
failure, and surface a uniform error (TRPCError) back to callers; then replace
the inline sequence in create.ts and finish-checkout.ts with calls to this new
persistWorkspace helper.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/search-branches.ts`:
- Around line 17-22: Replace the type alias BranchAccum with an equivalent
interface declaration (interface BranchAccum { name: string; lastCommitDate:
number; isLocal: boolean; isRemote: boolean; }) to follow the repo's coding
guideline preferring interfaces for plain object shapes; update any imports/uses
that reference BranchAccum if needed but keep the property names and types
unchanged.
- Around line 43-50: The remote git.fetch call can hang indefinitely; wrap the
fetch so it respects a timeout/abort: when input.refresh and
shouldRefetchRemote(input.projectId) is true, create an AbortController (or
equivalent timeout wrapper) tied to a short configurable timeout, pass its
signal into git.fetch (or use Promise.race to reject after timeout if git.fetch
doesn't accept signals), ensure you call markRefetchRemote(input.projectId)
before attempting fetch, clear/abort the controller on success, and catch both
fetch errors and abort/timeouts to proceed with cached refs as currently done;
reference git.fetch, shouldRefetchRemote, markRefetchRemote, input.refresh, and
input.projectId when making the change.
- Around line 63-71: The query currently calls ctx.db.select() which fetches all
columns but only uses the branch field; change the Drizzle projection to select
only workspaces.branch (e.g., use ctx.db.select(workspaces.branch) or
select({branch: workspaces.branch}) from workspaces) in the block that
constructs workspaceBranches so you only retrieve and map the branch values;
also update the subsequent mapping/filtering to match the projected result shape
(string or {branch: string}) so workspaceBranches remains a Set<string>.
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/branch-search.ts`:
- Around line 94-121: findWorktreeAtPath currently uses resolvePath
(path.resolve) which does not resolve symlinks, causing mismatches with git
worktree's canonical paths; change the comparison to use fs.promises.realpath
for both the input worktreePath and the currentPath parsed from git output
(within findWorktreeAtPath) so symlinks are resolved before comparing to
targetPath, and fall back to resolvePath if realpath fails (catch and degrade
gracefully); reference resolvePath and the findWorktreeAtPath function and
ensure you await realpath for both sides of the equality check.
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts`:
- Around line 111-140: The catch block for the local DB insert can leave a
zombie cloud workspace if ctx.api.v2Workspace.delete.mutate fails; capture the
cleanup error (cleanupErr) when delete.catch runs, log it with context, and
include that cleanupErr information in the TRPCError you throw (e.g., append to
the message or set as the error cause/metadata) so callers are aware a cloud
rollback also failed; also consider enqueuing the failed cloud cleanup for retry
if you have a retry/reconciliation mechanism (pass cloudRow.id and cleanupErr to
that queue) and ensure rollbackWorktree and clearProgress are still executed.
In `@packages/host-service/src/trpc/router/workspace-creation/shared/types.ts`:
- Around line 5-30: The exported object-shaped types TerminalDescriptor,
BranchRow, and CheckoutResult should be converted from type aliases to
interfaces to follow the project's TS convention; update declarations for
TerminalDescriptor, BranchRow, and CheckoutResult to use interface instead of
type (preserving all properties, optional flags like alreadyExists?: false, and
array/primitive types) while leaving any non-object derived types (e.g.,
GitClient-related Awaited/ReturnType usage) unchanged.
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/ai-workspace-names.ts`:
- Around line 183-212: Both rollback blocks use Promise chaining
(ctx.git(worktreePath).then(...).catch(...)) inside an async function—replace
each with await inside a try/catch to match async/await style: await
ctx.git(worktreePath) to get g, then await g.raw(["branch", "-m", deduped,
oldBranchName]) inside a try, and on error catch it and call console.warn with
the same message and the caught rollbackErr; keep the original thrown err after
the first rollback and the return after the second. Reference symbols: ctx.git,
worktreePath, gitRenamed, deduped, oldBranchName, cloudResult.branch.
- Around line 198-212: The no-op detection in applyAiWorkspaceRename currently
relies on gitRenamed && cloudResult.branch !== deduped and implicitly assumes
updateNameFromHost updates branch and name together; change the check to also
consider cloudResult.name when expectedCurrentName is provided (i.e., treat it
as a no-op if expectedCurrentName is set and cloudResult.name ===
oldWorkspaceName), or alternatively detect an explicit no-op flag from the
cloudResult; if you choose not to change logic, at minimum add a clear comment
near gitRenamed/cloudResult.branch explaining that this check depends on
updateNameFromHost being all-or-nothing for branch+name. Ensure you reference
gitRenamed, cloudResult.branch, cloudResult.name, deduped, oldWorkspaceName,
expectedCurrentName, and updateNameFromHost in the change so future
readers/maintainers see the contract.
In
`@packages/host-service/src/trpc/router/workspace-creation/utils/list-branch-names.ts`:
- Around line 33-35: The catch block in list-branch-names.ts is silently
swallowing errors (it currently reads "} catch { return []; }"); update the
catch to capture the thrown error (e.g., "catch (err)") and add a single-line
console.warn or logger call that includes contextual text and the error object
(for example: "console.warn('listBranchNames: failed to list branches', err)"),
then still return [] so the fallback behavior remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c06ddfe-ab02-456b-8cd4-1d69833b8383
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
packages/host-service/package.jsonpackages/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/index.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/schemas.tspackages/host-service/src/trpc/router/workspace-creation/shared/branch-search.tspackages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.tspackages/host-service/src/trpc/router/workspace-creation/shared/git-config.tspackages/host-service/src/trpc/router/workspace-creation/shared/local-project.tspackages/host-service/src/trpc/router/workspace-creation/shared/progress-store.tspackages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.tspackages/host-service/src/trpc/router/workspace-creation/shared/setup-terminal.tspackages/host-service/src/trpc/router/workspace-creation/shared/start-point.tspackages/host-service/src/trpc/router/workspace-creation/shared/types.tspackages/host-service/src/trpc/router/workspace-creation/shared/worktree-paths.tspackages/host-service/src/trpc/router/workspace-creation/utils/ai-workspace-names.tspackages/host-service/src/trpc/router/workspace-creation/utils/list-branch-names.tspackages/host-service/src/trpc/router/workspace-creation/workspace-creation.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
| // FORK NOTE: only write for remote-tracking start points. Downstream | ||
| // (resolveBaseComparison) always rebuilds the compare ref as | ||
| // `origin/${baseBranch}`, so a local-only branch name would resolve | ||
| // to a non-existent `origin/<local-name>` and the Changes tab would | ||
| // silently break. Skipping the write leaves baseBranch null for | ||
| // local-only bases — downstream falls back to the default branch. | ||
| // Upstream split uses `startPoint.kind !== "head"` which would write | ||
| // for local branches too — we intentionally diverge here. | ||
| if (startPoint.kind === "remote-tracking") { | ||
| await git | ||
| .raw(["config", `branch.${branchName}.base`, startPoint.shortName]) | ||
| .catch((err) => { | ||
| console.warn( | ||
| `[workspaceCreation.create] failed to record base branch ${startPoint.shortName}:`, | ||
| err, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify finish-checkout.ts applies the same "remote-tracking only" filter as create.ts
fd finish-checkout.ts --exec cat {}Repository: MocA-Love/superset
Length of output: 4335
🏁 Script executed:
# Find and examine checkout.ts to see how baseBranch flows from composer to finishCheckout
fd -e ts -e tsx | xargs rg -l "checkout" | head -20Repository: MocA-Love/superset
Length of output: 1376
🏁 Script executed:
# Search for where composer.baseBranch is used in checkout path
rg "composer\.baseBranch|\.baseBranch" --type ts --max-count=10 -B 2 -A 2Repository: MocA-Love/superset
Length of output: 23225
🏁 Script executed:
# Check if there's any validation on baseBranch in the checkout flow
rg "baseBranch.*kind|startPoint.*kind" --type ts -B 3 -A 3Repository: MocA-Love/superset
Length of output: 4520
checkout パスでもリモート追跡ブランチのみフィルタを適用する必要あり
create.ts は startPoint.kind === "remote-tracking" でフィルタしてから branch.<name>.base を書き込みます(165-182行)。一方、checkout.ts は input.composer.baseBranch(プレーン文字列)を finishCheckout にそのまま渡し、finishCheckout はそれを無条件に書き込みます(if (args.baseBranch) のみ)。
checkout 経路で composer.baseBranch に local-only ブランチが渡される場合、同じ fork フィルタが欠落しているため、Changes タブが origin/<local-name> で壊れる既知症状が再発します。checkout でも create のように baseBranchSource を受け取り、startPoint を再構成してから kind フィルタを適用するか、あるいは finishCheckout に同じリモート追跡フィルタを追加してください。
finishCheckout 内のコメント「matches create's head-start-point behavior」は誤解を招きます。create は kind フィルタも適用しており、単なる存在チェック(if (args.baseBranch))との一致ではありません。
🤖 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 165 - 182, checkout path is missing the remote-tracking-only guard
that create.ts uses: ensure finishCheckout does not unconditionally write
branch.<branch>.base for local-only names. Reconstruct the startPoint from the
provided baseBranchSource (like create.ts) or add the same check
(startPoint.kind === "remote-tracking") inside finishCheckout before calling
git.raw(["config", `branch.${branchName}.base`, ...]). Also update the
misleading comment "matches create's head-start-point behavior" to state that
create applies a kind filter so finishCheckout must mirror that behavior
(remote-tracking-only) rather than only checking presence of args.baseBranch.
| const re = /^checkout: moving from .+ to (.+)$/; | ||
| for (const line of raw.split("\n")) { | ||
| const match = re.exec(line); | ||
| if (!match?.[1]) continue; | ||
| const name = match[1].trim(); | ||
| if (!name || /^[0-9a-f]{7,40}$/.test(name)) continue; | ||
| if (!order.has(name)) { | ||
| order.set(name, order.size); | ||
| if (order.size >= limit) break; | ||
| } | ||
| } |
There was a problem hiding this comment.
reflog パース時の貪欲マッチで、ブランチ名に to を含む場合に誤パースが発生します。
/^checkout: moving from .+ to (.+)$/ の .+ は貪欲かつ to 前後がリテラル空白のため、移動先ブランチ名に to が含まれると最後の to でスプリットされ、キャプチャがブランチ名の末尾断片のみになります。例: checkout: moving from main to feat/add to cart は cart を捕捉し、本来の feat/add to cart は recencyMap に載らず、該当ブランチの並びが正しくなくなります。ソート品質のみの影響なので minor ですが、非貪欲化で確実に修正できます。
🛠 Proposed fix
- const re = /^checkout: moving from .+ to (.+)$/;
+ const re = /^checkout: moving from .+? to (.+)$/;🤖 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/shared/branch-search.ts`
around lines 140 - 150, The regex used to parse reflog lines is greedy and thus
miscaptures branch names that themselves contain " to "; update the pattern used
in branch-search (variable re) to make the first .+ non-greedy (e.g. change
/^checkout: moving from .+ to (.+)$/ to /^checkout: moving from .*? to (.+)$/ or
equivalent), then keep the existing logic that trims name and skips SHA-like
values so branch names with " to " are captured correctly into order.
| // Record the base branch for the Changes tab (skipped if unset — matches | ||
| // `create`'s head-start-point behavior). | ||
| if (args.baseBranch) { | ||
| await args.git | ||
| .raw([ | ||
| "-C", | ||
| args.worktreePath, | ||
| "config", | ||
| `branch.${args.branch}.base`, | ||
| args.baseBranch, | ||
| ]) | ||
| .catch((err) => { | ||
| console.warn( | ||
| `[workspaceCreation.checkout] failed to record base branch ${args.baseBranch}:`, | ||
| err, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# finishCheckout への baseBranch 引数の組み立て箇所を列挙し、
# ローカル専用判定(リモート追跡の存在確認など)が残っているかを確認する。
rg -nP --type=ts -C6 '\bfinishCheckout\s*\('
# checkout/adopt 手続きで baseBranch を決定している箇所の周辺
fd -e ts . 'packages/host-service/src/trpc/router/workspace-creation/procedures' \
--exec rg -nP -C3 '\bbaseBranch\b' {}Repository: MocA-Love/superset
Length of output: 6448
🏁 Script executed:
# View checkout.ts to see if baseBranch filtering occurs before finishCheckout calls
head -n 300 packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | tail -n 150Repository: MocA-Love/superset
Length of output: 4616
🏁 Script executed:
# Look for the complete context around the startPoint.kind check
rg -nP -B5 -A5 'startPoint\.kind' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Check if baseBranch is filtered based on remote-tracking logic
rg -nP -B3 -A3 'remote-tracking|refs/remotes' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: MocA-Love/superset
Length of output: 463
🏁 Script executed:
# Check adopt.ts for similar finishCheckout calls
rg -nP -B5 -A10 'finishCheckout' packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tsRepository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Find where composer.baseBranch is set or validated
rg -nP 'composer\.baseBranch|baseBranch.*=' packages/host-service/src/trpc/router/workspace-creation/ -B2 -A2Repository: MocA-Love/superset
Length of output: 5140
🏁 Script executed:
# Look for any filtering logic around baseBranch in the broader workspace-creation context
rg -nP 'local.*only|remote.*tracking|isRemoteTracking' packages/host-service/src/trpc/router/workspace-creation/ -B3 -A3Repository: MocA-Love/superset
Length of output: 21714
🏁 Script executed:
# Check if there's filtering in the input preparation/normalization
fd -e ts packages/host-service/src/trpc/router/workspace-creation/ -exec rg -l 'baseBranch' {} \; | head -20Repository: MocA-Love/superset
Length of output: 306
🏁 Script executed:
# View the complete finish-checkout.ts implementation to see if filtering happens there
cat -n packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts | head -100Repository: MocA-Love/superset
Length of output: 3593
🏁 Script executed:
# Check if checkout.ts uses baseBranchSource to filter baseBranch
rg -nP 'baseBranchSource' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts -B3 -A3Repository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# See the complete section in create.ts showing the filtering
sed -n '162,180p' packages/host-service/src/trpc/router/workspace-creation/procedures/create.tsRepository: MocA-Love/superset
Length of output: 981
🏁 Script executed:
# Check what composer object contains and where it's prepared
rg -nP 'composer\s*[:=]' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | head -20Repository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Check the full checkout.ts around the finishCheckout calls to see entire flow
sed -n '140,160p' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: MocA-Love/superset
Length of output: 668
🏁 Script executed:
# Check what input.composer contains in checkout schema/type
rg -nP 'input\.composer' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts -B2 -A2 | head -40Repository: MocA-Love/superset
Length of output: 433
🏁 Script executed:
# Verify the final piece: does checkout input schema enforce baseBranch filtering?
sed -n '1,50p' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: MocA-Love/superset
Length of output: 1889
🏁 Script executed:
# Check if there's any validation/filtering of baseBranch in checkoutInputSchema
cat -n packages/host-service/src/trpc/router/workspace-creation/schemas.ts | grep -A 20 "checkoutInputSchema"Repository: MocA-Love/superset
Length of output: 1002
🏁 Script executed:
# See if composer is built/filtered anywhere before being used in checkout
rg -nP 'composer\s*=' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts -B5 -A5Repository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Final check: confirm create.ts uses startPoint.kind filter but checkout doesn't
diff -u <(sed -n '165,176p' packages/host-service/src/trpc/router/workspace-creation/procedures/create.ts) <(sed -n '34,51p' packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts) || trueRepository: MocA-Love/superset
Length of output: 150
baseBranchの「ローカル専用ブランチ除外」ポリシーがcheckout.tsで実装されていません — 回帰リスク有り。
create.ts(165〜173行目)ではif (startPoint.kind === "remote-tracking")で明示的にフィルタされていますが、checkout.tsの両呼び出し箇所(147〜153行目と251〜257行目)はinput.composer.baseBranchを無条件にfinishCheckoutに渡します。
finishCheckout(34〜51行目)はこの値を無フィルタで書き込むため、ローカル専用ブランチがbranch.<name>.baseに記録される可能性があります。FORK NOTEの通り、下流のresolveBaseComparisonはorigin/${baseBranch}として参照するため、存在しないorigin/<local-branch-name>に解決され、Changes タブが無音で破綻します。
checkout.tsの両パスで、create.tsと同様にstartPoint.kind === "remote-tracking"チェックを追加するか、baseBranchSourceを用いた同等の検証が必要です。
🤖 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/shared/finish-checkout.ts`
around lines 34 - 51, The finishCheckout call is being passed
input.composer.baseBranch unconditionally which allows local-only branches to be
written into branch.<name>.base and later referenced as
origin/<local-branch-name> by resolveBaseComparison; update both checkout.ts
call sites (the paths that currently forward input.composer.baseBranch) to only
pass baseBranch when the start point is remote-tracking (i.e. startPoint.kind
=== "remote-tracking") or otherwise validate using the same baseBranchSource
logic used in create.ts, so finishCheckout only receives a remote-tracking
baseBranch and local-only branches are never recorded.
| export function getProgress(pendingId: string): ProgressStep[] | null { | ||
| return createProgress.get(pendingId)?.steps ?? null; |
There was a problem hiding this comment.
5分固定 TTL だと進行中ジョブの progress が途中で消えます。
updatedAt は setProgress() でしか更新されないので、同じステップが 5 分を超えると次の getProgress 呼び出しで sweepStaleProgress() に消され、まだ処理中でも null が返ります。runSetupScript や重い checkout だと普通に起こりうるので、ポーリング時に延命するか、完了/失敗時の明示的な cleanup に寄せた方がよいです。
差分案
export function getProgress(pendingId: string): ProgressStep[] | null {
- return createProgress.get(pendingId)?.steps ?? null;
+ const entry = createProgress.get(pendingId);
+ if (!entry) return null;
+ entry.updatedAt = Date.now();
+ return entry.steps;
}Also applies to: 49-53
🤖 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/shared/progress-store.ts`
around lines 41 - 42, getProgress currently lets in-progress entries expire
because updatedAt is only set by setProgress, so long-running steps
(runSetupScript, heavy checkout) get swept as stale and return null; fix by
either refreshing the TTL on poll — e.g., when getProgress(pendingId) finds a
non-null progress, update its updatedAt (or call setProgress/refreshProgress) to
extend life — or change sweepStaleProgress to only remove entries whose status
is terminal (completed/failed) and rely on explicit cleanup on completion; apply
the same change to the related logic that uses sweepStaleProgress (lines around
the other get/cleanup calls referenced in the diff).
| workspace: { id: string }; | ||
| terminals: TerminalDescriptor[]; | ||
| warnings: string[]; | ||
| alreadyExists?: false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# alreadyExists フラグの実際の利用箇所と、true を返す代替結果型を調べる。
rg -nP --type=ts -C3 '\balreadyExists\b'Repository: MocA-Love/superset
Length of output: 5329
🏁 Script executed:
cat -n packages/host-service/src/trpc/router/workspace-creation/shared/types.tsRepository: MocA-Love/superset
Length of output: 1060
🏁 Script executed:
# Check for related return types in the same directory
fd -t f '\.(ts|tsx)$' packages/host-service/src/trpc/router/workspace-creation/ | head -20Repository: MocA-Love/superset
Length of output: 1731
🏁 Script executed:
# Look for how this type is used/exported
rg -nP --type=ts 'export.*type|interface' packages/host-service/src/trpc/router/workspace-creation/shared/types.tsRepository: MocA-Love/superset
Length of output: 236
🏁 Script executed:
# Search for the complete return type of the checkout procedure
rg -nP --type=ts -A10 'checkout.*=.*procedure|const checkout' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | head -40Repository: MocA-Love/superset
Length of output: 468
🏁 Script executed:
# Check the complete return type of the checkout mutation
rg -nP --type=ts -B5 -A20 'export const checkout = protectedProcedure' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | head -80Repository: MocA-Love/superset
Length of output: 1153
🏁 Script executed:
# Search for any union types or type combinations with CheckoutResult
rg -nP --type=ts 'CheckoutResult|alreadyExists.*true' packages/host-service/src/trpc/router/workspace-creation/ -A2 -B2Repository: MocA-Love/superset
Length of output: 2855
🏁 Script executed:
# Check the adopt.ts procedure to see if it has a different return type
cat -n packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts | head -100Repository: MocA-Love/superset
Length of output: 4070
🏁 Script executed:
# Get more context from checkout.ts - see all return statements
rg -nP --type=ts -B2 -A5 'return {' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | head -100Repository: MocA-Love/superset
Length of output: 272
🏁 Script executed:
# Check how alreadyExists is used on the frontend
rg -nP --type=ts -B3 -A3 'alreadyExists.*true|alreadyExists.*===' apps/desktop/src/renderer/ --max-count=20Repository: MocA-Love/superset
Length of output: 1488
🏁 Script executed:
# Check if there's a union type or overloaded return type
rg -nP --type=ts 'type.*CheckoutResult|CheckoutResult.*=.*\|' packages/host-service/Repository: MocA-Love/superset
Length of output: 332
🏁 Script executed:
# Check if checkout.ts has explicit return type annotation
rg -nP --type=ts -B10 'export const checkout' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | head -30Repository: MocA-Love/superset
Length of output: 675
🏁 Script executed:
# Check all return statements in checkout.ts to understand return type variations
rg -nP --type=ts -B1 'return \{' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: MocA-Love/superset
Length of output: 117
🏁 Script executed:
# Check finish-checkout.ts which declares return type as CheckoutResult
cat -n packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts | head -80Repository: MocA-Love/superset
Length of output: 3029
🏁 Script executed:
# Check the complete checkout.ts to see all return paths
rg -nP --type=ts -B5 -A10 'finishCheckout' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts | head -60Repository: MocA-Love/superset
Length of output: 1898
🏁 Script executed:
# Search for explicit return type on checkout mutation
ast-grep --pattern 'export const checkout = $$$' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: MocA-Love/superset
Length of output: 28288
🏁 Script executed:
# Look at the full finish-checkout return to see all return statements
rg -nP --type=ts 'return \{' packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts -A5Repository: MocA-Love/superset
Length of output: 126
🏁 Script executed:
# Look at how frontend uses the result.alreadyExists check
rg -nP --type=ts -B5 -A5 'result\.alreadyExists|\.alreadyExists ===' apps/desktop/src/renderer/Repository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Search more broadly for how the checkout result is consumed
rg -nP --type=ts 'useCheckoutDashboardWorkspace|checkout\.mutate' apps/desktop/src/renderer/ -A10 | head -80Repository: MocA-Love/superset
Length of output: 9535
🏁 Script executed:
# Look for conditional logic related to alreadyExists on the client
rg -nP --type=ts 'alreadyExists' apps/desktop/src/renderer/ -B5 -A5Repository: MocA-Love/superset
Length of output: 4571
🏁 Script executed:
# Check if TypeScript compiles with this type mismatch
cd packages/host-service && npx tsc --noEmit 2>&1 | grep -A2 -B2 'alreadyExists' | head -30Repository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Check the actual inferred return type from checkout.ts mutation
rg -nP --type=ts -B2 'alreadyExists: true as const' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.ts -A3Repository: MocA-Love/superset
Length of output: 167
🏁 Script executed:
# Check if there are type errors when compiling
cd packages/host-service && npx tsc --noEmit 2>&1 | head -50Repository: MocA-Love/superset
Length of output: 321
🏁 Script executed:
# Search for how the return type is actually used/consumed in callers
rg -nP --type=ts 'checkout\.mutate' packages/host-service/ -A15 | head -60Repository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Look at the actual mutations calling this to see how the result is used
rg -nP --type=ts -B5 'checkout\.mutate\(' apps/desktop/src/renderer/ -A20 | grep -A15 'const result\|checkout\.mutate'Repository: MocA-Love/superset
Length of output: 2424
🏁 Script executed:
# Check if there's a return type override or if the type annotation exists
rg -nP --type=ts 'checkout.*:.*mutation' packages/host-service/src/trpc/router/workspace-creation/procedures/checkout.tsRepository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Check procedures/index.ts to see how checkout is exported
cat -n packages/host-service/src/trpc/router/workspace-creation/procedures/index.tsRepository: MocA-Love/superset
Length of output: 720
🏁 Script executed:
# Check how the result is used after mutation in the pending page
rg -nP --type=tsx 'checkoutWorkspace\(' apps/desktop/src/renderer/ -B3 -A15 | head -60Repository: MocA-Love/superset
Length of output: 90
🏁 Script executed:
# Look for type assertions or if statements checking alreadyExists
rg -nP --type=ts 'alreadyExists.*true|if.*alreadyExists' apps/desktop/src/renderer/ -B2 -A3Repository: MocA-Love/superset
Length of output: 2051
CheckoutResult 型の判別共用体パターンが不完全です。
現在の型定義は alreadyExists?: false となっていますが、実装では以下の2つの戻り値パターンがあります:
- 既存ワークスペースの場合(checkout.ts:45):
alreadyExists: true as constを返す - 新規作成の場合(checkout.ts:147, 251):
finishCheckout()を呼び出し、alreadyExistsフィールドを含まない結果を返す
型定義を修正し、判別共用体として両パターンを正しく表現してください:
export type CheckoutResult =
| {
workspace: { id: string };
terminals: TerminalDescriptor[];
warnings: string[];
alreadyExists: true;
}
| {
workspace: { id: string };
terminals: TerminalDescriptor[];
warnings: string[];
};または、より簡潔に:
export type CheckoutResult = {
workspace: { id: string };
terminals: TerminalDescriptor[];
warnings: string[];
alreadyExists?: true; // 存在する場合のみ true、省略時は新規作成を示す
};これにより、呼び出し側での型安全な判別(result.alreadyExists === true による分岐)が適切に型チェックされます。
🤖 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/shared/types.ts` at
line 29, The CheckoutResult discriminated-union is incorrect: currently it
defines alreadyExists?: false but callers return alreadyExists: true or omit the
field for new checkouts; update the CheckoutResult type (in
packages/host-service/src/trpc/router/workspace-creation/shared/types.ts) to
reflect both cases either by using a proper union of the two shapes (one variant
with alreadyExists: true and one without the field) or by making alreadyExists?:
true (optional true) so that result.alreadyExists === true can be used for
type-safe branching; adjust the exported CheckoutResult type definition
accordingly.
| const worktreesRoot = resolve(repoPath, ".worktrees"); | ||
| const worktreePath = resolve(worktreesRoot, branchName); | ||
| if ( | ||
| worktreePath !== worktreesRoot && | ||
| !worktreePath.startsWith(worktreesRoot + sep) | ||
| ) { |
There was a problem hiding this comment.
. / 空文字のブランチ名が .worktrees 直下として通ってしまいます。
この判定は「ルート外への脱出」は防げていますが、branchName === "" や "." のとき worktreePath === worktreesRoot になり、そのまま許可されます。結果として、後続の worktree 作成/削除処理がブランチ用サブディレクトリではなく .worktrees ルート自体を対象にできてしまいます。ここは少なくとも "" / "." / ".." を先に弾いた方が安全です。
差分案
export function safeResolveWorktreePath(
repoPath: string,
branchName: string,
): string {
+ if (!branchName || branchName === "." || branchName === "..") {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Invalid branch name",
+ });
+ }
+
const worktreesRoot = resolve(repoPath, ".worktrees");
const worktreePath = resolve(worktreesRoot, branchName);
if (
worktreePath !== worktreesRoot &&
!worktreePath.startsWith(worktreesRoot + sep)🤖 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/shared/worktree-paths.ts`
around lines 13 - 18, The current check lets branchName "" or "." pass because
resolve(repoPath, ".worktrees", branchName) equals worktreesRoot; update the
validation around worktreesRoot/worktreePath to explicitly reject empty and
special-segment names by validating branchName first (e.g., if branchName === ""
|| branchName === "." || branchName === ".." then throw), then keep the existing
check that worktreePath is either equal to or a child of worktreesRoot (using
worktreesRoot and worktreePath) to prevent path traversal; reference branchName,
worktreesRoot, and worktreePath to locate and harden the guard before subsequent
create/delete logic.
Codex レビュー指摘の race condition を修正。`updateNameFromHost` は find → check → update の 3 段階で、find と update の間にユーザーが rename を挟むと expectedCurrentName チェックをすり抜けて overwrite されていた。 expectedCurrentName を SQL レベルの WHERE に積んで UPDATE 自体を 条件付きにすることで、名前が競合して変わっていた場合は 0 行マッチ → 現在行を返す → 呼び元 (applyAiWorkspaceRename) で branch 不一致を 検出して git rename をロールバック、という既存の no-op パスに正しく 乗る。 - drizzle-orm から `and` を import - `expectedCurrentName` 指定時のみ WHERE に追加 (optional 動作維持) - UPDATE 0 行は "row was renamed between find and update" を意味する ので NOT_FOUND ではなく現在行を返す (旧挙動の NOT_FOUND は誤り)
… upstream superset-sh#3697) upstream superset-sh#3697 の split 構造 (24 ファイル) を fork に取り込み、fork 独自拡張を新 split 構造に再配置。workspace-creation.ts を 1758 行から 28 行の facade に縮小。 - schemas.ts: 全 procedure の zod schemas (baseBranchSource, workspaceNameWasAutoGenerated, checkoutPrSchema XOR refine を保持) - shared/: 11 ファイル (types, project-helpers, local-project, progress-store, git-config, setup-terminal, worktree-paths, start-point, branch-search, finish-checkout, types) - procedures/: 11 ファイル (adopt, checkout, create, generate-branch-name, get-context, get-github-issue-content, get-github-pull-request-content, get-progress, search-branches, search-github-issues, search-pull-requests) - utils/: 追加 (ai-workspace-names.ts, list-branch-names.ts) 1. **baseBranchSource** (picker hint: local/remote-tracking) → schemas.ts + procedures/create.ts + procedures/adopt.ts 2. **PR checkout mode** (derivePrLocalBranchName, detached worktree, gh pr checkout --force, pre-existing branch/closed/merged warnings) → procedures/checkout.ts 3. **FORK NOTE** コメント群 → 関連 procedure に復元 4. **GitHub link command wiring** (gh issue develop, gh pr create) → 各 procedure 5. **normalize-github-query.ts** (fork-only URL normalization) → 維持 6. **applyAiWorkspaceRename** + updateNameFromHost no-op detection → utils/ai-workspace-names.ts で復元 (PR #9 で fork main から欠落して いた関数を本 PR で供給) 7. **worktree path 方針**: <repoPath>/.worktrees/<branch> を維持 (upstream の ~/.superset/worktrees/<projectId>/<branch> は採用しない。 既存 workspace/file watcher/migration 互換性のため) packages/trpc/src/router/v2-workspace/v2-workspace.ts に `updateNameFromHost` JWT procedure を追加。expectedCurrentName WHERE guard で並行 user rename の race を検出し、no-op 時は既存 row を 返して host-service 側で git branch rollback できるようにする。 - ai-workspace-names.ts: getSmallModel() が unknown を返すため、 Agent の model parameter に biome-ignore 付きで cast - v2-workspace.updateNameFromHost: findFirst の columns に branch を追加 (no-op 時の caller での branch 比較に必要) - bun run typecheck: 27/27 successful - bun run lint: pass (rg 未 install の環境警告除く) - fork 固有機能: 19 procedure, ansi_up/@vscode/ripgrep/@xyflow/react, TERMINAL_OPTIONS, SUPERSET_WORKSPACE_NAME, dmg.size=4g, desktop 1.5.10 全健在 packages/host-service/package.json に @mastra/core: 1.25.0 を追加 (applyAiWorkspaceRename が Agent を使うため)。packages/chat が既に 同 version を持つため workspace 共有で解決される。 Closes #415
Codex レビュー指摘の race condition を修正。`updateNameFromHost` は find → check → update の 3 段階で、find と update の間にユーザーが rename を挟むと expectedCurrentName チェックをすり抜けて overwrite されていた。 expectedCurrentName を SQL レベルの WHERE に積んで UPDATE 自体を 条件付きにすることで、名前が競合して変わっていた場合は 0 行マッチ → 現在行を返す → 呼び元 (applyAiWorkspaceRename) で branch 不一致を 検出して git rename をロールバック、という既存の no-op パスに正しく 乗る。 - drizzle-orm から `and` を import - `expectedCurrentName` 指定時のみ WHERE に追加 (optional 動作維持) - UPDATE 0 行は "row was renamed between find and update" を意味する ので NOT_FOUND ではなく現在行を返す (旧挙動の NOT_FOUND は誤り)
b220e28 to
9abf35f
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Summary
Issue #415 対応: upstream commit
167542eb4(#3697) のworkspace-creation.tssplit refactor を fork に取り込み。workspace-creation.tsを 1758 行 → 28 行 facade に縮小、機能は 24 個の procedure/shared ファイルに分散。behind = 0 達成 (計画後 upstream の残 1 commit を取り込み)。
Fork 独自拡張を全て新 split 構造に再配置
baseBranchSource(picker hint)schemas.ts+procedures/create.ts+procedures/adopt.tsderivePrLocalBranchName, detached worktree,gh pr checkout --force, pre-existing/closed/merged warnings)procedures/checkout.tsgh issue develop,gh pr create)normalize-github-query.ts(fork-only URL normalization)applyAiWorkspaceRename+updateNameFromHostno-op detectionutils/ai-workspace-names.ts(PR #9 で fork main から欠落していた関数を本 PR で供給)重要な設計判断
<repoPath>/.worktrees/<branch>を維持。upstream の~/.superset/worktrees/<projectId>/<branch>は採用しない (既存 workspace / file watcher / migration 互換性のため)updateNameFromHost追加:packages/trpc/src/router/v2-workspace/v2-workspace.tsに JWT procedure 追加。expectedCurrentNameWHERE guard で並行 user rename の race を検出、no-op 時は既存 row を返して host-service で git branch rollback 可能に@mastra/core 1.25.0依存追加:packages/host-service/package.jsonに追加 (applyAiWorkspaceRenameが Agent 使用)。packages/chatが同 version を持つため workspace 共有で解決新 split 構造
Fork 固有機能ヘルスチェック
全維持 (実体は新 split ファイル内に配置):
ansi_up,@vscode/ripgrep,@xyflow/react依存TERMINAL_OPTIONS,SUPERSET_WORKSPACE_NAME,moonshot-ai.kimi-codedmg.size="4g"TiptapPromptEditor,v1MigrationStateTest plan
bun install正常完了bun run typecheckグリーン (27/27)bun run lintグリーンgh pr checkout+ fork/upstream remote 区別)baseBranchSourceが host-service 側に正しく伝搬expectedCurrentNamemismatch 時に git rename が rollback される関連
project_upstream_batch_workflow.mdCloses #415
Summary by CodeRabbit
ワークスペース作成機能の拡張
New Features
Dependencies
@mastra/core@1.25.0を追加