From dfcc7401870b7b4dc9982aa4b6b65e9010de3f90 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 10 Apr 2026 16:29:54 -0700 Subject: [PATCH 1/6] Handle missing git directories as non-repositories - Treat deleted/missing cwd paths as explicit non-repo status - Cover GitCore and GitManager with integration tests --- apps/server/src/git/Layers/GitCore.test.ts | 64 +++++++++++++++++++ apps/server/src/git/Layers/GitCore.ts | 44 ++++++++++++- apps/server/src/git/Layers/GitManager.test.ts | 47 ++++++++++++++ 3 files changed, 153 insertions(+), 2 deletions(-) diff --git a/apps/server/src/git/Layers/GitCore.test.ts b/apps/server/src/git/Layers/GitCore.test.ts index 53b881b666..8ab541e675 100644 --- a/apps/server/src/git/Layers/GitCore.test.ts +++ b/apps/server/src/git/Layers/GitCore.test.ts @@ -40,6 +40,24 @@ function writeTextFile( }); } +function removePath( + targetPath: string, +): Effect.Effect { + return Effect.gen(function* () { + const fileSystem = yield* FileSystem.FileSystem; + yield* fileSystem.remove(targetPath, { recursive: true, force: true }); + }); +} + +function makeDirectory( + dirPath: string, +): Effect.Effect { + return Effect.gen(function* () { + const fileSystem = yield* FileSystem.FileSystem; + yield* fileSystem.makeDirectory(dirPath, { recursive: true }); + }); +} + /** Run a raw git command for test setup (not under test). */ function git( cwd: string, @@ -299,6 +317,21 @@ it.layer(TestLayer)("git integration", (it) => { }), ); + it.effect("returns isRepo: false for deleted directories", () => + Effect.gen(function* () { + const tmp = yield* makeTmpDir(); + const deletedDir = path.join(tmp, "deleted-repo"); + yield* makeDirectory(deletedDir); + yield* removePath(deletedDir); + + const result = yield* (yield* GitCore).listBranches({ cwd: deletedDir }); + + expect(result.isRepo).toBe(false); + expect(result.hasOriginRemote).toBe(false); + expect(result.branches).toEqual([]); + }), + ); + it.effect("returns the current branch with current: true", () => Effect.gen(function* () { const tmp = yield* makeTmpDir(); @@ -1626,6 +1659,37 @@ it.layer(TestLayer)("git integration", (it) => { }), ); + it.effect("returns a non-repo status for deleted directories", () => + Effect.gen(function* () { + const tmp = yield* makeTmpDir(); + const deletedDir = path.join(tmp, "deleted-repo"); + yield* makeDirectory(deletedDir); + yield* removePath(deletedDir); + const core = yield* GitCore; + + const status = yield* core.statusDetails(deletedDir); + const localStatus = yield* core.statusDetailsLocal(deletedDir); + + expect(status).toEqual({ + isRepo: false, + hasOriginRemote: false, + isDefaultBranch: false, + branch: null, + upstreamRef: null, + hasWorkingTreeChanges: false, + workingTree: { + files: [], + insertions: 0, + deletions: 0, + }, + hasUpstream: false, + aheadCount: 0, + behindCount: 0, + }); + expect(localStatus).toEqual(status); + }), + ); + it.effect("computes ahead count against base branch when no upstream is configured", () => Effect.gen(function* () { const tmp = yield* makeTmpDir(); diff --git a/apps/server/src/git/Layers/GitCore.ts b/apps/server/src/git/Layers/GitCore.ts index 62f8405aea..33b9b97d2f 100644 --- a/apps/server/src/git/Layers/GitCore.ts +++ b/apps/server/src/git/Layers/GitCore.ts @@ -27,6 +27,7 @@ import { type ExecuteGitProgress, type GitCommitOptions, type GitCoreShape, + type GitStatusDetails, type ExecuteGitInput, type ExecuteGitResult, } from "../Services/GitCore.ts"; @@ -59,6 +60,18 @@ const STATUS_UPSTREAM_REFRESH_FAILURE_COOLDOWN = Duration.seconds(5); const STATUS_UPSTREAM_REFRESH_CACHE_CAPACITY = 2_048; const DEFAULT_BASE_BRANCH_CANDIDATES = ["main", "master"] as const; const GIT_LIST_BRANCHES_DEFAULT_LIMIT = 100; +const NON_REPOSITORY_STATUS_DETAILS = { + isRepo: false, + hasOriginRemote: false, + isDefaultBranch: false, + branch: null, + upstreamRef: null, + hasWorkingTreeChanges: false, + workingTree: { files: [], insertions: 0, deletions: 0 }, + hasUpstream: false, + aheadCount: 0, + behindCount: 0, +} satisfies GitStatusDetails; type TraceTailState = { processedChars: number; @@ -359,6 +372,16 @@ function quoteGitCommand(args: ReadonlyArray): string { return `git ${args.join(" ")}`; } +function isMissingGitCwdError(error: GitCommandError): boolean { + const normalized = `${error.detail}\n${error.message}`.toLowerCase(); + return ( + normalized.includes("no such file or directory") || + normalized.includes("notfound: filesystem.access") || + normalized.includes("enoent") || + normalized.includes("not a directory") + ); +} + function toGitCommandError( input: Pick, detail: string, @@ -1190,7 +1213,11 @@ export const makeGitCore = Effect.fn("makeGitCore")(function* (options?: { { allowNonZeroExit: true, }, - ); + ).pipe(Effect.catchIf(isMissingGitCwdError, () => Effect.succeed(null))); + + if (statusResult === null) { + return NON_REPOSITORY_STATUS_DETAILS; + } if (statusResult.code !== 0) { const stderr = statusResult.stderr.trim(); @@ -1322,7 +1349,10 @@ export const makeGitCore = Effect.fn("makeGitCore")(function* (options?: { ); const statusDetails: GitCoreShape["statusDetails"] = Effect.fn("statusDetails")(function* (cwd) { - yield* refreshStatusUpstreamIfStale(cwd).pipe(Effect.ignoreCause({ log: true })); + yield* refreshStatusUpstreamIfStale(cwd).pipe( + Effect.catchIf(isMissingGitCwdError, () => Effect.void), + Effect.ignoreCause({ log: true }), + ); return yield* readStatusDetailsLocal(cwd); }); @@ -1719,6 +1749,16 @@ export const makeGitCore = Effect.fn("makeGitCore")(function* (options?: { timeoutMs: 10_000, allowNonZeroExit: true, }, + ).pipe( + Effect.catchIf(isMissingGitCwdError, () => + Effect.succeed({ + code: 128, + stdout: "", + stderr: "fatal: not a git repository", + stdoutTruncated: false, + stderrTruncated: false, + }), + ), ); if (localBranchResult.code !== 0) { diff --git a/apps/server/src/git/Layers/GitManager.test.ts b/apps/server/src/git/Layers/GitManager.test.ts index 38cbd13014..1cf7884f3c 100644 --- a/apps/server/src/git/Layers/GitManager.test.ts +++ b/apps/server/src/git/Layers/GitManager.test.ts @@ -186,6 +186,24 @@ function makeTempDir( }); } +function removePath( + targetPath: string, +): Effect.Effect { + return Effect.gen(function* () { + const fileSystem = yield* FileSystem.FileSystem; + yield* fileSystem.remove(targetPath, { recursive: true, force: true }); + }); +} + +function makeDirectory( + dirPath: string, +): Effect.Effect { + return Effect.gen(function* () { + const fileSystem = yield* FileSystem.FileSystem; + yield* fileSystem.makeDirectory(dirPath, { recursive: true }); + }); +} + function runGit( cwd: string, args: readonly string[], @@ -720,6 +738,35 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("status returns an explicit non-repo result for deleted directories", () => + Effect.gen(function* () { + const rootDir = yield* makeTempDir("t3code-git-manager-missing-dir-"); + const cwd = path.join(rootDir, "deleted-repo"); + yield* makeDirectory(cwd); + yield* removePath(cwd); + const { manager } = yield* makeManager(); + + const status = yield* manager.status({ cwd }); + + expect(status).toEqual({ + isRepo: false, + hasOriginRemote: false, + isDefaultBranch: false, + branch: null, + hasWorkingTreeChanges: false, + workingTree: { + files: [], + insertions: 0, + deletions: 0, + }, + hasUpstream: false, + aheadCount: 0, + behindCount: 0, + pr: null, + }); + }), + ); + it.effect("status briefly caches repeated lookups for the same cwd", () => Effect.gen(function* () { const repoDir = yield* makeTempDir("t3code-git-manager-"); From c5adf234850842bb661404e37a565f4eddf77038 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 10 Apr 2026 16:34:25 -0700 Subject: [PATCH 2/6] Trim whitespace from PR metadata in git status - Normalize PR title, URL, and branch names before publishing status - Skip entries whose required fields become empty after trimming --- apps/server/src/git/Layers/GitManager.test.ts | 38 +++++++++++++++++++ apps/server/src/git/Layers/GitManager.ts | 29 ++++++++------ 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/apps/server/src/git/Layers/GitManager.test.ts b/apps/server/src/git/Layers/GitManager.test.ts index 1cf7884f3c..119fd0c0dc 100644 --- a/apps/server/src/git/Layers/GitManager.test.ts +++ b/apps/server/src/git/Layers/GitManager.test.ts @@ -712,6 +712,44 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("status trims PR metadata returned by gh before publishing it", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/status-trimmed-pr"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/status-trimmed-pr"]); + + const { manager } = yield* makeManager({ + ghScenario: { + prListSequence: [ + JSON.stringify([ + { + number: 14, + title: " Existing PR title \n", + url: " https://github.com/pingdotgg/codething-mvp/pull/14 ", + baseRefName: " main ", + headRefName: "\tfeature/status-trimmed-pr\t", + }, + ]), + ], + }, + }); + + const status = yield* manager.status({ cwd: repoDir }); + + expect(status.pr).toEqual({ + number: 14, + title: "Existing PR title", + url: "https://github.com/pingdotgg/codething-mvp/pull/14", + baseBranch: "main", + headBranch: "feature/status-trimmed-pr", + state: "open", + }); + }), + ); + it.effect("status returns an explicit non-repo result for non-git directories", () => Effect.gen(function* () { const cwd = yield* makeTempDir("t3code-git-manager-non-repo-"); diff --git a/apps/server/src/git/Layers/GitManager.ts b/apps/server/src/git/Layers/GitManager.ts index 33e9719804..3c936b3b9b 100644 --- a/apps/server/src/git/Layers/GitManager.ts +++ b/apps/server/src/git/Layers/GitManager.ts @@ -276,15 +276,16 @@ function parsePullRequestList(raw: unknown): PullRequestInfo[] { : typeof headRepositoryOwnerRecord?.login === "string" ? headRepositoryOwnerRecord.login : null; + const normalizedTitle = typeof title === "string" ? title.trim() : null; + const normalizedUrl = typeof url === "string" ? url.trim() : null; + const normalizedBaseRefName = typeof baseRefName === "string" ? baseRefName.trim() : null; + const normalizedHeadRefName = typeof headRefName === "string" ? headRefName.trim() : null; + const normalizedHeadRepositoryNameWithOwner = headRepositoryNameWithOwner?.trim() || null; + const normalizedHeadRepositoryOwnerLogin = headRepositoryOwnerLogin?.trim() || null; if (typeof number !== "number" || !Number.isInteger(number) || number <= 0) { continue; } - if ( - typeof title !== "string" || - typeof url !== "string" || - typeof baseRefName !== "string" || - typeof headRefName !== "string" - ) { + if (!normalizedTitle || !normalizedUrl || !normalizedBaseRefName || !normalizedHeadRefName) { continue; } @@ -305,15 +306,19 @@ function parsePullRequestList(raw: unknown): PullRequestInfo[] { parsed.push({ number, - title, - url, - baseRefName, - headRefName, + title: normalizedTitle, + url: normalizedUrl, + baseRefName: normalizedBaseRefName, + headRefName: normalizedHeadRefName, state: normalizedState, updatedAt: typeof updatedAt === "string" && updatedAt.trim().length > 0 ? updatedAt : null, ...(typeof isCrossRepository === "boolean" ? { isCrossRepository } : {}), - ...(headRepositoryNameWithOwner ? { headRepositoryNameWithOwner } : {}), - ...(headRepositoryOwnerLogin ? { headRepositoryOwnerLogin } : {}), + ...(normalizedHeadRepositoryNameWithOwner + ? { headRepositoryNameWithOwner: normalizedHeadRepositoryNameWithOwner } + : {}), + ...(normalizedHeadRepositoryOwnerLogin + ? { headRepositoryOwnerLogin: normalizedHeadRepositoryOwnerLogin } + : {}), }); } return parsed; From e0532782c81d8677e747e424c6e1489580cc2eb1 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 10 Apr 2026 16:43:56 -0700 Subject: [PATCH 3/6] Normalize GitHub PR JSON decoding - Trim GitHub CLI pull request fields before use - Share PR JSON decoding between GitHub CLI and git manager - Improve errors when gh returns invalid PR JSON --- apps/server/src/git/Layers/GitHubCli.test.ts | 47 +++++++ apps/server/src/git/Layers/GitHubCli.ts | 118 ++++++------------ apps/server/src/git/Layers/GitManager.ts | 123 +++++-------------- apps/server/src/git/githubPullRequests.ts | 113 +++++++++++++++++ 4 files changed, 231 insertions(+), 170 deletions(-) create mode 100644 apps/server/src/git/githubPullRequests.ts diff --git a/apps/server/src/git/Layers/GitHubCli.test.ts b/apps/server/src/git/Layers/GitHubCli.test.ts index aafc796db3..7347536c1b 100644 --- a/apps/server/src/git/Layers/GitHubCli.test.ts +++ b/apps/server/src/git/Layers/GitHubCli.test.ts @@ -76,6 +76,53 @@ layer("GitHubCliLive", (it) => { }), ); + it.effect("trims pull request fields decoded from gh json", () => + Effect.gen(function* () { + mockedRunProcess.mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 42, + title: " Add PR thread creation \n", + url: " https://github.com/pingdotgg/codething-mvp/pull/42 ", + baseRefName: " main ", + headRefName: "\tfeature/pr-threads\t", + state: "OPEN", + mergedAt: null, + isCrossRepository: true, + headRepository: { + nameWithOwner: " octocat/codething-mvp ", + }, + headRepositoryOwner: { + login: " octocat ", + }, + }), + stderr: "", + code: 0, + signal: null, + timedOut: false, + }); + + const result = yield* Effect.gen(function* () { + const gh = yield* GitHubCli; + return yield* gh.getPullRequest({ + cwd: "/repo", + reference: "#42", + }); + }); + + assert.deepStrictEqual(result, { + number: 42, + title: "Add PR thread creation", + url: "https://github.com/pingdotgg/codething-mvp/pull/42", + baseRefName: "main", + headRefName: "feature/pr-threads", + state: "open", + isCrossRepository: true, + headRepositoryNameWithOwner: "octocat/codething-mvp", + headRepositoryOwnerLogin: "octocat", + }); + }), + ); + it.effect("reads repository clone URLs", () => Effect.gen(function* () { mockedRunProcess.mockResolvedValueOnce({ diff --git a/apps/server/src/git/Layers/GitHubCli.ts b/apps/server/src/git/Layers/GitHubCli.ts index ce56e91d53..1a687b0e8d 100644 --- a/apps/server/src/git/Layers/GitHubCli.ts +++ b/apps/server/src/git/Layers/GitHubCli.ts @@ -1,5 +1,5 @@ -import { Effect, Layer, Schema, SchemaIssue } from "effect"; -import { PositiveInt, TrimmedNonEmptyString } from "@t3tools/contracts"; +import { Effect, Layer, Result, Schema, SchemaIssue } from "effect"; +import { TrimmedNonEmptyString } from "@t3tools/contracts"; import { runProcess } from "../../processRunner"; import { GitHubCliError } from "@t3tools/contracts"; @@ -7,8 +7,12 @@ import { GitHubCli, type GitHubRepositoryCloneUrls, type GitHubCliShape, - type GitHubPullRequestSummary, } from "../Services/GitHubCli.ts"; +import { + decodeGitHubPullRequestJson, + decodeGitHubPullRequestListJson, + formatGitHubJsonDecodeError, +} from "../githubPullRequests.ts"; const DEFAULT_TIMEOUT_MS = 30_000; @@ -63,76 +67,12 @@ function normalizeGitHubCliError(operation: "execute" | "stdout", error: unknown }); } -function normalizePullRequestState(input: { - state?: string | null | undefined; - mergedAt?: string | null | undefined; -}): "open" | "closed" | "merged" { - const mergedAt = input.mergedAt; - const state = input.state; - if ((typeof mergedAt === "string" && mergedAt.trim().length > 0) || state === "MERGED") { - return "merged"; - } - if (state === "CLOSED") { - return "closed"; - } - return "open"; -} - -const RawGitHubPullRequestSchema = Schema.Struct({ - number: PositiveInt, - title: TrimmedNonEmptyString, - url: TrimmedNonEmptyString, - baseRefName: TrimmedNonEmptyString, - headRefName: TrimmedNonEmptyString, - state: Schema.optional(Schema.NullOr(Schema.String)), - mergedAt: Schema.optional(Schema.NullOr(Schema.String)), - isCrossRepository: Schema.optional(Schema.Boolean), - headRepository: Schema.optional( - Schema.NullOr( - Schema.Struct({ - nameWithOwner: Schema.String, - }), - ), - ), - headRepositoryOwner: Schema.optional( - Schema.NullOr( - Schema.Struct({ - login: Schema.String, - }), - ), - ), -}); - const RawGitHubRepositoryCloneUrlsSchema = Schema.Struct({ nameWithOwner: TrimmedNonEmptyString, url: TrimmedNonEmptyString, sshUrl: TrimmedNonEmptyString, }); -function normalizePullRequestSummary( - raw: Schema.Schema.Type, -): GitHubPullRequestSummary { - const headRepositoryNameWithOwner = raw.headRepository?.nameWithOwner ?? null; - const headRepositoryOwnerLogin = - raw.headRepositoryOwner?.login ?? - (typeof headRepositoryNameWithOwner === "string" && headRepositoryNameWithOwner.includes("/") - ? (headRepositoryNameWithOwner.split("/")[0] ?? null) - : null); - return { - number: raw.number, - title: raw.title, - url: raw.url, - baseRefName: raw.baseRefName, - headRefName: raw.headRefName, - state: normalizePullRequestState(raw), - ...(typeof raw.isCrossRepository === "boolean" - ? { isCrossRepository: raw.isCrossRepository } - : {}), - ...(headRepositoryNameWithOwner ? { headRepositoryNameWithOwner } : {}), - ...(headRepositoryOwnerLogin ? { headRepositoryOwnerLogin } : {}), - }; -} - function normalizeRepositoryCloneUrls( raw: Schema.Schema.Type, ): GitHubRepositoryCloneUrls { @@ -194,14 +134,24 @@ const makeGitHubCli = Effect.sync(() => { Effect.flatMap((raw) => raw.length === 0 ? Effect.succeed([]) - : decodeGitHubJson( - raw, - Schema.Array(RawGitHubPullRequestSchema), - "listOpenPullRequests", - "GitHub CLI returned invalid PR list JSON.", + : Effect.sync(() => decodeGitHubPullRequestListJson(raw)).pipe( + Effect.flatMap((decoded) => { + if (!Result.isSuccess(decoded)) { + return Effect.fail( + new GitHubCliError({ + operation: "listOpenPullRequests", + detail: `GitHub CLI returned invalid PR list JSON: ${formatGitHubJsonDecodeError(decoded.failure)}`, + cause: decoded.failure, + }), + ); + } + + return Effect.succeed( + decoded.success.map(({ updatedAt: _updatedAt, ...summary }) => summary), + ); + }), ), ), - Effect.map((pullRequests) => pullRequests.map(normalizePullRequestSummary)), ), getPullRequest: (input) => execute({ @@ -216,14 +166,24 @@ const makeGitHubCli = Effect.sync(() => { }).pipe( Effect.map((result) => result.stdout.trim()), Effect.flatMap((raw) => - decodeGitHubJson( - raw, - RawGitHubPullRequestSchema, - "getPullRequest", - "GitHub CLI returned invalid pull request JSON.", + Effect.sync(() => decodeGitHubPullRequestJson(raw)).pipe( + Effect.flatMap((decoded) => { + if (!Result.isSuccess(decoded)) { + return Effect.fail( + new GitHubCliError({ + operation: "getPullRequest", + detail: `GitHub CLI returned invalid pull request JSON: ${formatGitHubJsonDecodeError(decoded.failure)}`, + cause: decoded.failure, + }), + ); + } + + return Effect.succeed( + (({ updatedAt: _updatedAt, ...summary }) => summary)(decoded.success), + ); + }), ), ), - Effect.map(normalizePullRequestSummary), ), getRepositoryCloneUrls: (input) => execute({ diff --git a/apps/server/src/git/Layers/GitManager.ts b/apps/server/src/git/Layers/GitManager.ts index 3c936b3b9b..a84427a194 100644 --- a/apps/server/src/git/Layers/GitManager.ts +++ b/apps/server/src/git/Layers/GitManager.ts @@ -1,7 +1,18 @@ import { randomUUID } from "node:crypto"; import { realpathSync } from "node:fs"; -import { Cache, Duration, Effect, Exit, FileSystem, Layer, Option, Path, Ref } from "effect"; +import { + Cache, + Duration, + Effect, + Exit, + FileSystem, + Layer, + Option, + Path, + Ref, + Result, +} from "effect"; import { GitActionProgressEvent, GitActionProgressPhase, @@ -34,6 +45,10 @@ import { ProjectSetupScriptRunner } from "../../project/Services/ProjectSetupScr import { extractBranchNameFromRemoteRef } from "../remoteRefs.ts"; import { ServerSettingsService } from "../../serverSettings.ts"; import type { GitManagerServiceError } from "@t3tools/contracts"; +import { + decodeGitHubPullRequestListJson, + formatGitHubJsonDecodeError, +} from "../githubPullRequests.ts"; const COMMIT_TIMEOUT_MS = 10 * 60_000; const MAX_PROGRESS_TEXT_LENGTH = 500; @@ -240,90 +255,6 @@ function matchesBranchHeadContext( return true; } -function parsePullRequestList(raw: unknown): PullRequestInfo[] { - if (!Array.isArray(raw)) return []; - - const parsed: PullRequestInfo[] = []; - for (const entry of raw) { - if (!entry || typeof entry !== "object") continue; - const record = entry as Record; - const number = record.number; - const title = record.title; - const url = record.url; - const baseRefName = record.baseRefName; - const headRefName = record.headRefName; - const state = record.state; - const mergedAt = record.mergedAt; - const updatedAt = record.updatedAt; - const isCrossRepository = record.isCrossRepository; - const headRepositoryRecord = - typeof record.headRepository === "object" && record.headRepository !== null - ? (record.headRepository as Record) - : null; - const headRepositoryOwnerRecord = - typeof record.headRepositoryOwner === "object" && record.headRepositoryOwner !== null - ? (record.headRepositoryOwner as Record) - : null; - const headRepositoryNameWithOwner = - typeof record.headRepositoryNameWithOwner === "string" - ? record.headRepositoryNameWithOwner - : typeof headRepositoryRecord?.nameWithOwner === "string" - ? headRepositoryRecord.nameWithOwner - : null; - const headRepositoryOwnerLogin = - typeof record.headRepositoryOwnerLogin === "string" - ? record.headRepositoryOwnerLogin - : typeof headRepositoryOwnerRecord?.login === "string" - ? headRepositoryOwnerRecord.login - : null; - const normalizedTitle = typeof title === "string" ? title.trim() : null; - const normalizedUrl = typeof url === "string" ? url.trim() : null; - const normalizedBaseRefName = typeof baseRefName === "string" ? baseRefName.trim() : null; - const normalizedHeadRefName = typeof headRefName === "string" ? headRefName.trim() : null; - const normalizedHeadRepositoryNameWithOwner = headRepositoryNameWithOwner?.trim() || null; - const normalizedHeadRepositoryOwnerLogin = headRepositoryOwnerLogin?.trim() || null; - if (typeof number !== "number" || !Number.isInteger(number) || number <= 0) { - continue; - } - if (!normalizedTitle || !normalizedUrl || !normalizedBaseRefName || !normalizedHeadRefName) { - continue; - } - - let normalizedState: "open" | "closed" | "merged"; - if ( - (typeof mergedAt === "string" && mergedAt.trim().length > 0) || - state === "MERGED" || - state === "merged" - ) { - normalizedState = "merged"; - } else if (state === "OPEN" || state === "open" || state === undefined || state === null) { - normalizedState = "open"; - } else if (state === "CLOSED" || state === "closed") { - normalizedState = "closed"; - } else { - continue; - } - - parsed.push({ - number, - title: normalizedTitle, - url: normalizedUrl, - baseRefName: normalizedBaseRefName, - headRefName: normalizedHeadRefName, - state: normalizedState, - updatedAt: typeof updatedAt === "string" && updatedAt.trim().length > 0 ? updatedAt : null, - ...(typeof isCrossRepository === "boolean" ? { isCrossRepository } : {}), - ...(normalizedHeadRepositoryNameWithOwner - ? { headRepositoryNameWithOwner: normalizedHeadRepositoryNameWithOwner } - : {}), - ...(normalizedHeadRepositoryOwnerLogin - ? { headRepositoryOwnerLogin: normalizedHeadRepositoryOwnerLogin } - : {}), - }); - } - return parsed; -} - function toPullRequestInfo(summary: GitHubPullRequestSummary): PullRequestInfo { return { number: summary.number, @@ -952,13 +883,23 @@ export const makeGitManager = Effect.fn("makeGitManager")(function* () { continue; } - const parsedJson = yield* Effect.try({ - try: () => JSON.parse(raw) as unknown, - catch: (cause) => - gitManagerError("findLatestPr", "GitHub CLI returned invalid PR list JSON.", cause), - }); + const pullRequests = yield* Effect.sync(() => decodeGitHubPullRequestListJson(raw)).pipe( + Effect.flatMap((decoded) => { + if (!Result.isSuccess(decoded)) { + return Effect.fail( + gitManagerError( + "findLatestPr", + `GitHub CLI returned invalid PR list JSON: ${formatGitHubJsonDecodeError(decoded.failure)}`, + decoded.failure, + ), + ); + } + + return Effect.succeed(decoded.success); + }), + ); - for (const pr of parsePullRequestList(parsedJson)) { + for (const pr of pullRequests) { if (!matchesBranchHeadContext(pr, headContext)) { continue; } diff --git a/apps/server/src/git/githubPullRequests.ts b/apps/server/src/git/githubPullRequests.ts new file mode 100644 index 0000000000..082dfa7b45 --- /dev/null +++ b/apps/server/src/git/githubPullRequests.ts @@ -0,0 +1,113 @@ +import { Cause, Result, Schema } from "effect"; +import { PositiveInt, TrimmedNonEmptyString } from "@t3tools/contracts"; +import { decodeJsonResult, formatSchemaError } from "@t3tools/shared/schemaJson"; + +export interface NormalizedGitHubPullRequestRecord { + readonly number: number; + readonly title: string; + readonly url: string; + readonly baseRefName: string; + readonly headRefName: string; + readonly state: "open" | "closed" | "merged"; + readonly updatedAt: string | null; + readonly isCrossRepository?: boolean; + readonly headRepositoryNameWithOwner?: string | null; + readonly headRepositoryOwnerLogin?: string | null; +} + +const GitHubPullRequestSchema = Schema.Struct({ + number: PositiveInt, + title: TrimmedNonEmptyString, + url: TrimmedNonEmptyString, + baseRefName: TrimmedNonEmptyString, + headRefName: TrimmedNonEmptyString, + state: Schema.optional(Schema.NullOr(Schema.String)), + mergedAt: Schema.optional(Schema.NullOr(Schema.String)), + updatedAt: Schema.optional(Schema.NullOr(Schema.String)), + isCrossRepository: Schema.optional(Schema.Boolean), + headRepository: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nameWithOwner: TrimmedNonEmptyString, + }), + ), + ), + headRepositoryOwner: Schema.optional( + Schema.NullOr( + Schema.Struct({ + login: TrimmedNonEmptyString, + }), + ), + ), +}); + +function normalizeGitHubPullRequestState(input: { + state?: string | null | undefined; + mergedAt?: string | null | undefined; +}): "open" | "closed" | "merged" { + if ( + (typeof input.mergedAt === "string" && input.mergedAt.trim().length > 0) || + input.state === "MERGED" + ) { + return "merged"; + } + if (input.state === "CLOSED") { + return "closed"; + } + return "open"; +} + +function normalizeGitHubPullRequestRecord( + raw: Schema.Schema.Type, +): NormalizedGitHubPullRequestRecord { + const headRepositoryNameWithOwner = raw.headRepository?.nameWithOwner ?? null; + const headRepositoryOwnerLogin = + raw.headRepositoryOwner?.login ?? + (typeof headRepositoryNameWithOwner === "string" && headRepositoryNameWithOwner.includes("/") + ? (headRepositoryNameWithOwner.split("/")[0] ?? null) + : null); + + return { + number: raw.number, + title: raw.title, + url: raw.url, + baseRefName: raw.baseRefName, + headRefName: raw.headRefName, + state: normalizeGitHubPullRequestState(raw), + updatedAt: + typeof raw.updatedAt === "string" && raw.updatedAt.trim().length > 0 ? raw.updatedAt : null, + ...(typeof raw.isCrossRepository === "boolean" + ? { isCrossRepository: raw.isCrossRepository } + : {}), + ...(headRepositoryNameWithOwner ? { headRepositoryNameWithOwner } : {}), + ...(headRepositoryOwnerLogin ? { headRepositoryOwnerLogin } : {}), + }; +} + +const decodeGitHubPullRequestList = decodeJsonResult(Schema.Array(GitHubPullRequestSchema)); +const decodeGitHubPullRequest = decodeJsonResult(GitHubPullRequestSchema); + +export const formatGitHubJsonDecodeError = formatSchemaError; + +export function decodeGitHubPullRequestListJson( + raw: string, +): Result.Result< + ReadonlyArray, + Cause.Cause +> { + const result = decodeGitHubPullRequestList(raw); + if (Result.isSuccess(result)) { + return Result.succeed(result.success.map(normalizeGitHubPullRequestRecord)); + } + return Result.fail(result.failure); +} + +export function decodeGitHubPullRequestJson( + raw: string, +): Result.Result> { + const result = decodeGitHubPullRequest(raw); + if (Result.isSuccess(result)) { + return Result.succeed(normalizeGitHubPullRequestRecord(result.success)); + } + return Result.fail(result.failure); +} From efb9dc7bfc058e05baa13d8323dea7971a0a4165 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 10 Apr 2026 17:07:03 -0700 Subject: [PATCH 4/6] Ignore invalid PR entries in GitHub list parsing - Skip malformed or blank GitHub PR list entries instead of failing the whole parse - Keep valid PRs normalized so status can still report directories with usable matches - Add regression coverage for CLI and manager PR listing --- apps/server/src/git/Layers/GitHubCli.test.ts | 52 +++++++++++++++++++ apps/server/src/git/Layers/GitManager.test.ts | 51 ++++++++++++++++++ apps/server/src/git/githubPullRequests.ts | 28 +++++++--- 3 files changed, 124 insertions(+), 7 deletions(-) diff --git a/apps/server/src/git/Layers/GitHubCli.test.ts b/apps/server/src/git/Layers/GitHubCli.test.ts index 7347536c1b..0ee4b3f09a 100644 --- a/apps/server/src/git/Layers/GitHubCli.test.ts +++ b/apps/server/src/git/Layers/GitHubCli.test.ts @@ -123,6 +123,58 @@ layer("GitHubCliLive", (it) => { }), ); + it.effect("skips invalid entries when parsing pr lists", () => + Effect.gen(function* () { + mockedRunProcess.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 0, + title: "invalid", + url: "https://github.com/pingdotgg/codething-mvp/pull/0", + baseRefName: "main", + headRefName: "feature/invalid", + }, + { + number: 43, + title: " Valid PR ", + url: " https://github.com/pingdotgg/codething-mvp/pull/43 ", + baseRefName: " main ", + headRefName: " feature/pr-list ", + headRepository: { + nameWithOwner: " ", + }, + headRepositoryOwner: { + login: " ", + }, + }, + ]), + stderr: "", + code: 0, + signal: null, + timedOut: false, + }); + + const result = yield* Effect.gen(function* () { + const gh = yield* GitHubCli; + return yield* gh.listOpenPullRequests({ + cwd: "/repo", + headSelector: "feature/pr-list", + }); + }); + + assert.deepStrictEqual(result, [ + { + number: 43, + title: "Valid PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/43", + baseRefName: "main", + headRefName: "feature/pr-list", + state: "open", + }, + ]); + }), + ); + it.effect("reads repository clone URLs", () => Effect.gen(function* () { mockedRunProcess.mockResolvedValueOnce({ diff --git a/apps/server/src/git/Layers/GitManager.test.ts b/apps/server/src/git/Layers/GitManager.test.ts index 119fd0c0dc..a57576823d 100644 --- a/apps/server/src/git/Layers/GitManager.test.ts +++ b/apps/server/src/git/Layers/GitManager.test.ts @@ -750,6 +750,57 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("status ignores invalid gh pr list entries and keeps valid ones", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/status-valid-pr-entry"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/status-valid-pr-entry"]); + + const { manager } = yield* makeManager({ + ghScenario: { + prListSequence: [ + JSON.stringify([ + { + number: 0, + title: "invalid", + url: "https://github.com/pingdotgg/codething-mvp/pull/0", + baseRefName: "main", + headRefName: "feature/invalid", + }, + { + number: 15, + title: " Valid PR title ", + url: " https://github.com/pingdotgg/codething-mvp/pull/15 ", + baseRefName: " main ", + headRefName: "\tfeature/status-valid-pr-entry\t", + headRepository: { + nameWithOwner: " ", + }, + headRepositoryOwner: { + login: " ", + }, + }, + ]), + ], + }, + }); + + const status = yield* manager.status({ cwd: repoDir }); + + expect(status.pr).toEqual({ + number: 15, + title: "Valid PR title", + url: "https://github.com/pingdotgg/codething-mvp/pull/15", + baseBranch: "main", + headBranch: "feature/status-valid-pr-entry", + state: "open", + }); + }), + ); + it.effect("status returns an explicit non-repo result for non-git directories", () => Effect.gen(function* () { const cwd = yield* makeTempDir("t3code-git-manager-non-repo-"); diff --git a/apps/server/src/git/githubPullRequests.ts b/apps/server/src/git/githubPullRequests.ts index 082dfa7b45..790b362c65 100644 --- a/apps/server/src/git/githubPullRequests.ts +++ b/apps/server/src/git/githubPullRequests.ts @@ -1,4 +1,4 @@ -import { Cause, Result, Schema } from "effect"; +import { Cause, Exit, Result, Schema } from "effect"; import { PositiveInt, TrimmedNonEmptyString } from "@t3tools/contracts"; import { decodeJsonResult, formatSchemaError } from "@t3tools/shared/schemaJson"; @@ -28,19 +28,24 @@ const GitHubPullRequestSchema = Schema.Struct({ headRepository: Schema.optional( Schema.NullOr( Schema.Struct({ - nameWithOwner: TrimmedNonEmptyString, + nameWithOwner: Schema.String, }), ), ), headRepositoryOwner: Schema.optional( Schema.NullOr( Schema.Struct({ - login: TrimmedNonEmptyString, + login: Schema.String, }), ), ), }); +function trimOptionalString(value: string | null | undefined): string | null { + const trimmed = value?.trim() ?? ""; + return trimmed.length > 0 ? trimmed : null; +} + function normalizeGitHubPullRequestState(input: { state?: string | null | undefined; mergedAt?: string | null | undefined; @@ -60,9 +65,9 @@ function normalizeGitHubPullRequestState(input: { function normalizeGitHubPullRequestRecord( raw: Schema.Schema.Type, ): NormalizedGitHubPullRequestRecord { - const headRepositoryNameWithOwner = raw.headRepository?.nameWithOwner ?? null; + const headRepositoryNameWithOwner = trimOptionalString(raw.headRepository?.nameWithOwner); const headRepositoryOwnerLogin = - raw.headRepositoryOwner?.login ?? + trimOptionalString(raw.headRepositoryOwner?.login) ?? (typeof headRepositoryNameWithOwner === "string" && headRepositoryNameWithOwner.includes("/") ? (headRepositoryNameWithOwner.split("/")[0] ?? null) : null); @@ -84,8 +89,9 @@ function normalizeGitHubPullRequestRecord( }; } -const decodeGitHubPullRequestList = decodeJsonResult(Schema.Array(GitHubPullRequestSchema)); +const decodeGitHubPullRequestList = decodeJsonResult(Schema.Array(Schema.Unknown)); const decodeGitHubPullRequest = decodeJsonResult(GitHubPullRequestSchema); +const decodeGitHubPullRequestEntry = Schema.decodeUnknownExit(GitHubPullRequestSchema); export const formatGitHubJsonDecodeError = formatSchemaError; @@ -97,7 +103,15 @@ export function decodeGitHubPullRequestListJson( > { const result = decodeGitHubPullRequestList(raw); if (Result.isSuccess(result)) { - return Result.succeed(result.success.map(normalizeGitHubPullRequestRecord)); + const pullRequests: NormalizedGitHubPullRequestRecord[] = []; + for (const entry of result.success) { + const decodedEntry = decodeGitHubPullRequestEntry(entry); + if (Exit.isFailure(decodedEntry)) { + continue; + } + pullRequests.push(normalizeGitHubPullRequestRecord(decodedEntry.value)); + } + return Result.succeed(pullRequests); } return Result.fail(result.failure); } From 1da72f03e1a5a27fb067d93005851de5a950637c Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 10 Apr 2026 17:18:07 -0700 Subject: [PATCH 5/6] readonly --- apps/server/src/git/Layers/GitCore.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/server/src/git/Layers/GitCore.ts b/apps/server/src/git/Layers/GitCore.ts index 33b9b97d2f..fb5d908575 100644 --- a/apps/server/src/git/Layers/GitCore.ts +++ b/apps/server/src/git/Layers/GitCore.ts @@ -60,7 +60,7 @@ const STATUS_UPSTREAM_REFRESH_FAILURE_COOLDOWN = Duration.seconds(5); const STATUS_UPSTREAM_REFRESH_CACHE_CAPACITY = 2_048; const DEFAULT_BASE_BRANCH_CANDIDATES = ["main", "master"] as const; const GIT_LIST_BRANCHES_DEFAULT_LIMIT = 100; -const NON_REPOSITORY_STATUS_DETAILS = { +const NON_REPOSITORY_STATUS_DETAILS = Object.freeze({ isRepo: false, hasOriginRemote: false, isDefaultBranch: false, @@ -71,7 +71,7 @@ const NON_REPOSITORY_STATUS_DETAILS = { hasUpstream: false, aheadCount: 0, behindCount: 0, -} satisfies GitStatusDetails; +}); type TraceTailState = { processedChars: number; From 99e0eb430f7b366f79c298b5b529dca47a9cc2b3 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 10 Apr 2026 18:02:08 -0700 Subject: [PATCH 6/6] Normalize GitHub PR state casing - Treat lowercase `merged` and `closed` states from `gh` JSON as expected - Add a GitManager test covering lowercase PR states --- apps/server/src/git/Layers/GitManager.test.ts | 49 +++++++++++++++++++ apps/server/src/git/githubPullRequests.ts | 5 +- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/apps/server/src/git/Layers/GitManager.test.ts b/apps/server/src/git/Layers/GitManager.test.ts index a57576823d..fd991273d1 100644 --- a/apps/server/src/git/Layers/GitManager.test.ts +++ b/apps/server/src/git/Layers/GitManager.test.ts @@ -801,6 +801,55 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("status preserves lowercase merged and closed PR states from gh json", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/status-lowercase-state"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/status-lowercase-state"]); + + const { manager } = yield* makeManager({ + ghScenario: { + prListSequence: [ + JSON.stringify([ + { + number: 16, + title: "Closed PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/16", + baseRefName: "main", + headRefName: "feature/status-lowercase-state", + state: "closed", + updatedAt: "2026-01-01T00:00:00.000Z", + }, + { + number: 17, + title: "Merged PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/17", + baseRefName: "main", + headRefName: "feature/status-lowercase-state", + state: "merged", + updatedAt: "2026-01-02T00:00:00.000Z", + }, + ]), + ], + }, + }); + + const status = yield* manager.status({ cwd: repoDir }); + + expect(status.pr).toEqual({ + number: 17, + title: "Merged PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/17", + baseBranch: "main", + headBranch: "feature/status-lowercase-state", + state: "merged", + }); + }), + ); + it.effect("status returns an explicit non-repo result for non-git directories", () => Effect.gen(function* () { const cwd = yield* makeTempDir("t3code-git-manager-non-repo-"); diff --git a/apps/server/src/git/githubPullRequests.ts b/apps/server/src/git/githubPullRequests.ts index 790b362c65..d137a46d6f 100644 --- a/apps/server/src/git/githubPullRequests.ts +++ b/apps/server/src/git/githubPullRequests.ts @@ -50,13 +50,14 @@ function normalizeGitHubPullRequestState(input: { state?: string | null | undefined; mergedAt?: string | null | undefined; }): "open" | "closed" | "merged" { + const normalizedState = input.state?.trim().toUpperCase(); if ( (typeof input.mergedAt === "string" && input.mergedAt.trim().length > 0) || - input.state === "MERGED" + normalizedState === "MERGED" ) { return "merged"; } - if (input.state === "CLOSED") { + if (normalizedState === "CLOSED") { return "closed"; } return "open";