From cb22c605467dbb1c527e51a28e35ce0af3a41ae5 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Sun, 13 Sep 2026 03:03:46 -0600 Subject: [PATCH] refactor(server): share the no-git-repository checkout-root rewrap (#tech-debt) worktree.ts and pr.ts each declared a file-private resolveRepoRoot that wrapped resolveCheckoutRoot and rethrew the same 404 message, differing only in the domain error class. Replace both (and worktree.ts's pass-through resolveCurrentCheckoutRoot alias) with an exported resolveCheckoutRootOrThrow(cwd, runner, ErrorClass) in git-context.ts, and cover the no-repo path for all four entry points. Co-Authored-By: Claude Opus 5 (1M context) --- apps/server/src/shared/git/git-context.ts | 22 +++++++++++++- apps/server/src/shared/git/worktree.ts | 35 +++++++---------------- apps/server/src/shared/github/pr.ts | 28 +++++++----------- apps/server/test/git-worktree.test.ts | 22 ++++++++++++++ apps/server/test/github-pr.test.ts | 20 +++++++++++++ 5 files changed, 85 insertions(+), 42 deletions(-) diff --git a/apps/server/src/shared/git/git-context.ts b/apps/server/src/shared/git/git-context.ts index ce9215b70..75ba916f0 100644 --- a/apps/server/src/shared/git/git-context.ts +++ b/apps/server/src/shared/git/git-context.ts @@ -68,7 +68,8 @@ export async function resolveRepoRoot( * Root of the current checkout (`--show-toplevel`) — for a linked worktree * this is the worktree path, not the main repository root. No timeout is * applied unless one is passed. Throws when `cwd` is not inside a git - * working tree; callers that need a domain-specific error should rewrap. + * working tree; callers that need a domain-specific error should use + * resolveCheckoutRootOrThrow. */ export async function resolveCheckoutRoot( cwd: string, @@ -85,6 +86,25 @@ export async function resolveCheckoutRoot( ); } +/** + * resolveCheckoutRoot, rewrapping any failure as a 404 of the caller's + * domain error class so HTTP/MCP error mapping keys off that class. + */ +export async function resolveCheckoutRootOrThrow( + cwd: string, + commandRunner: CommandRunner, + ErrorClass: new (message: string, statusCode: number) => Error +): Promise { + try { + return await resolveCheckoutRoot(cwd, commandRunner); + } catch { + throw new ErrorClass( + "No git repository found for the provided working directory.", + 404 + ); + } +} + export async function resolveWorktreeRoot( cwd: string, opts: ProbeOptions = {} diff --git a/apps/server/src/shared/git/worktree.ts b/apps/server/src/shared/git/worktree.ts index 09e84a5be..a3aca9fe4 100644 --- a/apps/server/src/shared/git/worktree.ts +++ b/apps/server/src/shared/git/worktree.ts @@ -5,7 +5,7 @@ import { access } from "node:fs/promises"; import { runCommand, type CommandRunner } from "../lib/run-command.js"; import { normalizePath, - resolveCheckoutRoot, + resolveCheckoutRootOrThrow, resolveCurrentBranch, } from "./git-context.js"; @@ -115,7 +115,11 @@ export async function createGitWorktree( throw new GitWorktreeError("name is required.", 400); } - const repoRoot = await resolveRepoRoot(cwd, commandRunner); + const repoRoot = await resolveCheckoutRootOrThrow( + cwd, + commandRunner, + GitWorktreeError + ); const baseBranch = normalizeRefName(input.baseBranch, "main", "baseBranch"); const createNewBranch = input.createNewBranch ?? false; const branchName = createNewBranch @@ -230,7 +234,11 @@ export async function cleanupGitWorktree( throw new GitWorktreeError("cwd is required.", 400); } - const worktreePath = await resolveCurrentCheckoutRoot(cwd, commandRunner); + const worktreePath = await resolveCheckoutRootOrThrow( + cwd, + commandRunner, + GitWorktreeError + ); const repoRoot = await resolveCommonRepoRoot(worktreePath, commandRunner); const normalizedWorktreePath = normalizePath(worktreePath); const normalizedRepoRoot = normalizePath(repoRoot); @@ -344,27 +352,6 @@ export async function cleanupGitWorktree( }; } -async function resolveRepoRoot( - cwd: string, - commandRunner: CommandRunner -): Promise { - try { - return await resolveCheckoutRoot(cwd, commandRunner); - } catch { - throw new GitWorktreeError( - "No git repository found for the provided working directory.", - 404 - ); - } -} - -async function resolveCurrentCheckoutRoot( - cwd: string, - commandRunner: CommandRunner -): Promise { - return await resolveRepoRoot(cwd, commandRunner); -} - async function resolveCommonRepoRoot( cwd: string, commandRunner: CommandRunner diff --git a/apps/server/src/shared/github/pr.ts b/apps/server/src/shared/github/pr.ts index 81712d87e..939155621 100644 --- a/apps/server/src/shared/github/pr.ts +++ b/apps/server/src/shared/github/pr.ts @@ -1,5 +1,5 @@ import { - resolveCheckoutRoot, + resolveCheckoutRootOrThrow, resolveCurrentBranch, } from "../git/git-context.js"; import { runCommand, type CommandRunner } from "../lib/run-command.js"; @@ -62,7 +62,11 @@ export async function createPr( commandRunner: CommandRunner = runCommand ): Promise { const cwd = requireString(input.cwd, "cwd"); - const repoRoot = await resolveRepoRoot(cwd, commandRunner); + const repoRoot = await resolveCheckoutRootOrThrow( + cwd, + commandRunner, + GitHubPrError + ); const baseBranch = input.baseBranch?.trim() || "main"; const branchName = await resolveCurrentBranch(repoRoot, commandRunner); @@ -132,7 +136,11 @@ export async function getPrStatus( commandRunner: CommandRunner = runCommand ): Promise { const cwd = requireString(input.cwd, "cwd"); - const repoRoot = await resolveRepoRoot(cwd, commandRunner); + const repoRoot = await resolveCheckoutRootOrThrow( + cwd, + commandRunner, + GitHubPrError + ); const args = [ "pr", @@ -164,20 +172,6 @@ export async function getPrStatus( }; } -async function resolveRepoRoot( - cwd: string, - commandRunner: CommandRunner -): Promise { - try { - return await resolveCheckoutRoot(cwd, commandRunner); - } catch { - throw new GitHubPrError( - "No git repository found for the provided working directory.", - 404 - ); - } -} - async function ensureBaseBranchHasDiff( repoRoot: string, baseBranch: string, diff --git a/apps/server/test/git-worktree.test.ts b/apps/server/test/git-worktree.test.ts index 1c977a876..31209df29 100644 --- a/apps/server/test/git-worktree.test.ts +++ b/apps/server/test/git-worktree.test.ts @@ -452,6 +452,28 @@ describe("git worktree services", () => { }); }); + it("rejects with a 404 GitWorktreeError outside a git repository", async () => { + const cwd = path.join(tempRoot, "not-a-repo"); + + vi.mocked(runCommand).mockImplementation(async (_command, args) => { + throw new Error(`fatal: not a git repository: ${args.join(" ")}`); + }); + + const createPromise = createGitWorktree({ cwd, name: "feature" }); + await expect(createPromise).rejects.toBeInstanceOf(GitWorktreeError); + await expect(createPromise).rejects.toMatchObject({ + message: "No git repository found for the provided working directory.", + statusCode: 404, + }); + + const cleanupPromise = cleanupGitWorktree({ cwd }); + await expect(cleanupPromise).rejects.toBeInstanceOf(GitWorktreeError); + await expect(cleanupPromise).rejects.toMatchObject({ + message: "No git repository found for the provided working directory.", + statusCode: 404, + }); + }); + it("rejects cleanup when called from the primary checkout", async () => { const repoRoot = path.join(tempRoot, "repo"); diff --git a/apps/server/test/github-pr.test.ts b/apps/server/test/github-pr.test.ts index 2f247a14d..63295f0cb 100644 --- a/apps/server/test/github-pr.test.ts +++ b/apps/server/test/github-pr.test.ts @@ -163,6 +163,26 @@ describe("github pr services", () => { ); }); + it("rejects with a 404 GitHubPrError outside a git repository", async () => { + const runner = vi.fn(async (_command: string, args: string[]) => { + throw new Error(`fatal: not a git repository: ${args.join(" ")}`); + }); + + const createPromise = createPr({ cwd: "/tmp/not-a-repo" }, runner); + await expect(createPromise).rejects.toBeInstanceOf(GitHubPrError); + await expect(createPromise).rejects.toMatchObject({ + message: "No git repository found for the provided working directory.", + statusCode: 404, + }); + + const statusPromise = getPrStatus({ cwd: "/tmp/not-a-repo" }, runner); + await expect(statusPromise).rejects.toBeInstanceOf(GitHubPrError); + await expect(statusPromise).rejects.toMatchObject({ + message: "No git repository found for the provided working directory.", + statusCode: 404, + }); + }); + it("reports PR status details", async () => { const repoRoot = "/tmp/repo"; const runner = vi.fn(async (_command: string, args: string[]) => {