diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index e1924c03ade..464c3afad64 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -50,6 +50,8 @@ interface FakeGhScenario { }; repositoryCloneUrls?: Record; failWith?: GitHubCli.GitHubCliError; + /** Let this many gh calls succeed before failWith kicks in (default 0 = fail immediately). */ + failAfterCalls?: number; } function fakeGhOutput(stdout: string): VcsProcess.VcsProcessOutput { @@ -382,7 +384,7 @@ function createGitHubCliWithFakeGh(scenario: FakeGhScenario = {}): { const args = [...input.args]; ghCalls.push(args.join(" ")); - if (scenario.failWith) { + if (scenario.failWith && ghCalls.length > (scenario.failAfterCalls ?? 0)) { return Effect.fail(scenario.failWith); } @@ -1336,6 +1338,182 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("status keeps the last known PR when a later lookup fails", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-sticky"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/pr-sticky"]); + + const existingPr = { + number: 214, + title: "Sticky PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/214", + baseRefName: "main", + headRefName: "feature/pr-sticky", + }; + const { manager } = yield* makeManager({ + ghScenario: { + // @effect-diagnostics-next-line preferSchemaOverJson:off + prListSequence: [JSON.stringify([existingPr])], + failWith: new GitHubCli.GitHubCliUnavailableError({ + command: "gh", + cwd: repoDir, + cause: new Error("rate limited"), + }), + failAfterCalls: 1, + }, + }); + + const first = yield* manager.status({ cwd: repoDir }); + expect(first.pr?.number).toBe(214); + + // An explicit invalidation (user refresh, git action) bypasses the PR + // cache and forces a live lookup — which now fails. The badge must keep + // the last known PR instead of blanking out. + yield* manager.invalidateStatus(repoDir); + const second = yield* manager.status({ cwd: repoDir }); + expect(second.pr?.number).toBe(214); + }), + ); + + it.effect( + "status does not reuse a stale PR after the branch is retargeted to a different upstream", + () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-retarget"]); + + const originRemote = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", originRemote]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/pr-retarget"]); + + const existingPr = { + number: 214, + title: "Sticky PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/214", + baseRefName: "main", + headRefName: "feature/pr-retarget", + }; + const { manager } = yield* makeManager({ + ghScenario: { + // @effect-diagnostics-next-line preferSchemaOverJson:off + prListSequence: [JSON.stringify([existingPr])], + failWith: new GitHubCli.GitHubCliUnavailableError({ + command: "gh", + cwd: repoDir, + cause: new Error("rate limited"), + }), + failAfterCalls: 1, + }, + }); + + const first = yield* manager.status({ cwd: repoDir }); + expect(first.pr?.number).toBe(214); + + // Retarget the branch to a different remote/upstream (e.g. the PR was + // reopened against a fork). The previously cached PR belonged to the + // old upstream and must not be shown against the new one. + const forkRemote = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "fork", forkRemote]); + yield* runGit(repoDir, ["push", "fork", "feature/pr-retarget"]); + yield* runGit(repoDir, [ + "branch", + "--set-upstream-to=fork/feature/pr-retarget", + "feature/pr-retarget", + ]); + + yield* manager.invalidateStatus(repoDir); + const second = yield* manager.status({ cwd: repoDir }); + expect(second.pr).toBeNull(); + }), + ); + + it.effect("status keeps the last known PR when the branch gains its first upstream", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-sticky-first-push"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + + const existingPr = { + number: 215, + title: "Sticky first-push PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/215", + baseRefName: "main", + headRefName: "feature/pr-sticky-first-push", + }; + const { manager } = yield* makeManager({ + ghScenario: { + // @effect-diagnostics-next-line preferSchemaOverJson:off + prListSequence: [JSON.stringify([existingPr])], + failWith: new GitHubCli.GitHubCliUnavailableError({ + command: "gh", + cwd: repoDir, + cause: new Error("rate limited"), + }), + failAfterCalls: 1, + }, + }); + + const first = yield* manager.status({ cwd: repoDir }); + expect(first.pr?.number).toBe(215); + + yield* runGit(repoDir, ["push", "-u", "origin", "feature/pr-sticky-first-push"]); + yield* manager.invalidateStatus(repoDir); + + const second = yield* manager.status({ cwd: repoDir }); + expect(second.pr?.number).toBe(215); + }), + ); + + it.effect("status drops the last known PR when the tracked remote is repointed", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-repointed"]); + const originalRemoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", originalRemoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/pr-repointed"]); + + const existingPr = { + number: 216, + title: "Old remote PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/216", + baseRefName: "main", + headRefName: "feature/pr-repointed", + }; + const { manager } = yield* makeManager({ + ghScenario: { + // @effect-diagnostics-next-line preferSchemaOverJson:off + prListSequence: [JSON.stringify([existingPr])], + failWith: new GitHubCli.GitHubCliUnavailableError({ + command: "gh", + cwd: repoDir, + cause: new Error("rate limited"), + }), + failAfterCalls: 1, + }, + }); + + const first = yield* manager.status({ cwd: repoDir }); + expect(first.pr?.number).toBe(216); + + const replacementRemoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "replacement", replacementRemoteDir]); + yield* runGit(repoDir, ["push", "replacement", "feature/pr-repointed"]); + yield* runGit(repoDir, ["remote", "set-url", "origin", replacementRemoteDir]); + yield* manager.invalidateStatus(repoDir); + + const second = yield* manager.status({ cwd: repoDir }); + expect(second.pr).toBeNull(); + }), + ); + it.effect("creates a commit when working tree is dirty", () => Effect.gen(function* () { const repoDir = yield* makeTempDir("t3code-git-manager-"); diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts index f1fb03e7e45..58bd8602df1 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -32,6 +32,7 @@ import { import { detectSourceControlProviderFromGitRemoteUrl, mergeGitStatusParts, + normalizeGitRemoteUrl, resolveAutoFeatureBranchName, sanitizeBranchFragment, sanitizeFeatureBranchName, @@ -95,6 +96,9 @@ const SHORT_SHA_LENGTH = 7; const TOAST_DESCRIPTION_MAX = 72; const STATUS_RESULT_CACHE_TTL = Duration.seconds(1); const STATUS_RESULT_CACHE_CAPACITY = 2_048; +const PR_LOOKUP_CACHE_TTL = Duration.minutes(2); +const PR_LOOKUP_FAILURE_TTL = Duration.seconds(20); +const PR_LOOKUP_CACHE_CAPACITY = 2_048; type StripProgressContext = T extends any ? Omit : never; type GitActionProgressPayload = StripProgressContext; type GitActionProgressEmitter = (event: GitActionProgressPayload) => Effect.Effect; @@ -142,6 +146,7 @@ interface BranchHeadContext { headSelectors: ReadonlyArray; preferredHeadSelector: string; remoteName: string | null; + headRemoteUrlKey: string | null; headRepositoryNameWithOwner: string | null; headRepositoryOwnerLogin: string | null; isCrossRepository: boolean; @@ -768,6 +773,145 @@ export const make = Effect.gen(function* () { normalizeStatusCacheKey(cwd).pipe( Effect.flatMap((cacheKey) => Cache.invalidate(localStatusResultCache, cacheKey)), ); + // PR lookups hit the hosting provider's API (gh/glab/...), so they refresh + // on their own, slower cadence: ahead/behind counts stay fresh on every + // status poll while the PR association is re-fetched at most once per + // PR_LOOKUP_CACHE_TTL per branch. Git actions and user-driven refreshes bump + // the epoch (invalidateStatus) to bypass the cache immediately. + const prLookupEpochByCwd = new Map(); + const prLookupEpoch = (cwd: string) => prLookupEpochByCwd.get(cwd) ?? 0; + const bumpPrLookupEpoch = (cwd: string) => + normalizeStatusCacheKey(cwd).pipe( + Effect.map((cacheKey) => { + prLookupEpochByCwd.set(cacheKey, prLookupEpoch(cacheKey) + 1); + }), + ); + // Cache keys are NUL-joined [cwd, branch, upstreamRef, epoch] — none of the + // segments can contain a NUL byte, and refs are never empty, so "" decodes + // back to a null upstreamRef. + const prLookupCacheKey = (cwd: string, details: { branch: string; upstreamRef: string | null }) => + [cwd, details.branch, details.upstreamRef ?? "", String(prLookupEpoch(cwd))].join("\u0000"); + const prLookupCache = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", branch = "", upstreamRef = ""] = key.split("\u0000"); + const details = { + branch, + upstreamRef: upstreamRef.length > 0 ? upstreamRef : null, + }; + return resolveBranchHeadContext(cwd, details).pipe( + Effect.flatMap((headContext) => + findLatestPrForHeadContext(cwd, headContext).pipe( + Effect.map((latest) => ({ latest, headContext })), + ), + ), + ); + }, + { + capacity: PR_LOOKUP_CACHE_CAPACITY, + timeToLive: (exit) => (Exit.isSuccess(exit) ? PR_LOOKUP_CACHE_TTL : PR_LOOKUP_FAILURE_TTL), + }, + ); + // A transient lookup failure (rate limit, network blip) must not clear an + // already-known PR badge, so the last successful answer per branch sticks + // around as the fallback. Keep the resolved head context with it so a + // branch retargeted to another remote/fork cannot inherit the old badge. + interface LastKnownPr { + readonly pr: ReturnType | null; + readonly upstreamRef: string | null; + readonly headBranch: string; + readonly remoteName: string | null; + readonly headRemoteUrlKey: string | null; + } + const lastKnownPrByBranchKey = new Map(); + const rememberLastKnownPr = (branchKey: string, entry: LastKnownPr) => { + if ( + !lastKnownPrByBranchKey.has(branchKey) && + lastKnownPrByBranchKey.size >= PR_LOOKUP_CACHE_CAPACITY + ) { + const oldestKey = lastKnownPrByBranchKey.keys().next().value; + if (oldestKey !== undefined) { + lastKnownPrByBranchKey.delete(oldestKey); + } + } + lastKnownPrByBranchKey.set(branchKey, entry); + }; + const resolveLastKnownPr = ( + branchKey: string, + current: Pick, + ): ReturnType | null => { + const lastKnown = lastKnownPrByBranchKey.get(branchKey); + if (!lastKnown) return null; + if (lastKnown.headBranch !== current.headBranch) { + return null; + } + + // The normalized URL catches both remote-alias changes and an existing + // alias being repointed. It also lets an upstream appear after `push -u` + // without invalidating the fallback when it still targets the same repo. + if (lastKnown.headRemoteUrlKey !== null || current.headRemoteUrlKey !== null) { + return lastKnown.headRemoteUrlKey === current.headRemoteUrlKey ? lastKnown.pr : null; + } + + // If neither remote URL is available, fall back to the remote identity + // encoded by tracked branches. A null-to-non-null transition is allowed + // because that is the expected first-push case. + if (lastKnown.upstreamRef !== null && current.upstreamRef !== null) { + return lastKnown.remoteName === current.remoteName ? lastKnown.pr : null; + } + return lastKnown.pr; + }; + const lookupStatusPr = Effect.fn("lookupStatusPr")(function* ( + cwd: string, + details: { branch: string; upstreamRef: string | null; isDefaultBranch: boolean }, + ) { + // Keyed by (cwd, branch) only: the upstream ref changing (e.g. a first + // `push -u`) must not orphan the fallback value for the same branch. + const branchKey = `${cwd}\u0000${details.branch}`; + return yield* Cache.get(prLookupCache, prLookupCacheKey(cwd, details)).pipe( + Effect.map(({ latest, headContext }) => { + if (!latest) return { pr: null, headContext }; + // On the default branch, only surface open PRs. + // Merged/closed matches are usually reverse-merge history, not the thread's PR context. + if (details.isDefaultBranch && latest.state !== "open") { + return { pr: null, headContext }; + } + return { pr: toStatusPr(latest), headContext }; + }), + Effect.tap(({ pr, headContext }) => + Effect.sync(() => + rememberLastKnownPr(branchKey, { + pr, + upstreamRef: details.upstreamRef, + headBranch: headContext.headBranch, + remoteName: headContext.remoteName, + headRemoteUrlKey: headContext.headRemoteUrlKey, + }), + ), + ), + Effect.map(({ pr }) => pr), + Effect.catch((error) => + Effect.logWarning("PR lookup failed; keeping last known PR state.").pipe( + Effect.annotateLogs({ + operation: "lookupStatusPr", + branch: details.branch, + errorTag: + typeof error === "object" && error !== null && "_tag" in error + ? String(error._tag) + : typeof error, + }), + Effect.andThen(resolveBranchHeadContext(cwd, details)), + Effect.map((headContext) => + resolveLastKnownPr(branchKey, { + upstreamRef: details.upstreamRef, + headBranch: headContext.headBranch, + remoteName: headContext.remoteName, + headRemoteUrlKey: headContext.headRemoteUrlKey, + }), + ), + ), + ), + ); + }); const readRemoteStatus = Effect.fn("readRemoteStatus")(function* ( cwd: string, options?: GitVcsDriver.GitRemoteStatusOptions, @@ -781,19 +925,11 @@ export const make = Effect.gen(function* () { const pr = details.branch !== null - ? yield* findLatestPr(cwd, { + ? yield* lookupStatusPr(cwd, { branch: details.branch, upstreamRef: details.upstreamRef, - }).pipe( - Effect.map((latest) => { - if (!latest) return null; - // On the default branch, only surface open PRs. - // Merged/closed matches are usually reverse-merge history, not the thread's PR context. - if (details.isDefaultBranch && latest.state !== "open") return null; - return toStatusPr(latest); - }), - Effect.orElseSucceed(() => null), - ) + isDefaultBranch: details.isDefaultBranch, + }) : null; return { @@ -837,6 +973,7 @@ export const make = Effect.gen(function* () { ) { if (!remoteName) { return { + remoteUrlKey: null, repositoryNameWithOwner: null, ownerLogin: null, }; @@ -845,6 +982,7 @@ export const make = Effect.gen(function* () { const remoteUrl = yield* readConfigValueNullable(cwd, `remote.${remoteName}.url`); const repositoryNameWithOwner = parseGitHubRepositoryNameWithOwnerFromRemoteUrl(remoteUrl); return { + remoteUrlKey: remoteUrl ? normalizeGitRemoteUrl(remoteUrl) : null, repositoryNameWithOwner, ownerLogin: parseRepositoryOwnerLogin(repositoryNameWithOwner), }; @@ -915,6 +1053,9 @@ export const make = Effect.gen(function* () { preferredHeadSelector: ownerHeadSelector && isCrossRepository ? ownerHeadSelector : headBranch, remoteName, + headRemoteUrlKey: + remoteRepository.remoteUrlKey ?? + (remoteName === null ? originRepository.remoteUrlKey : null), headRepositoryNameWithOwner: remoteRepository.repositoryNameWithOwner, headRepositoryOwnerLogin: remoteRepository.ownerLogin, isCrossRepository, @@ -960,11 +1101,10 @@ export const make = Effect.gen(function* () { return null; }); - const findLatestPr = Effect.fn("findLatestPr")(function* ( + const findLatestPrForHeadContext = Effect.fn("findLatestPrForHeadContext")(function* ( cwd: string, - details: { branch: string; upstreamRef: string | null }, + headContext: BranchHeadContext, ) { - const headContext = yield* resolveBranchHeadContext(cwd, details); const parsedByNumber = new Map(); for (const headSelector of headContext.headSelectors) { @@ -991,7 +1131,6 @@ export const make = Effect.gen(function* () { } return parsed[0] ?? null; }); - const buildCompletionToast = Effect.fn("buildCompletionToast")(function* ( cwd: string, result: Pick, @@ -1442,6 +1581,10 @@ export const make = Effect.gen(function* () { function* (cwd) { yield* invalidateLocalStatusResultCache(cwd); yield* invalidateRemoteStatusResultCache(cwd); + // Full invalidation is the explicit-freshness path (git actions, user + // refresh); it also bypasses the slow PR-lookup cache. The periodic + // status poll only invalidates local/remote and keeps the PR cache warm. + yield* bumpPrLookupEpoch(cwd); }, ); diff --git a/apps/server/src/server.test.ts b/apps/server/src/server.test.ts index 8a5e0b713e6..c8c4ff377e8 100644 --- a/apps/server/src/server.test.ts +++ b/apps/server/src/server.test.ts @@ -5285,6 +5285,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => { gitManager: { invalidateLocalStatus: () => Effect.void, invalidateRemoteStatus: () => Effect.void, + invalidateStatus: () => Effect.void, localStatus: () => Effect.succeed({ isRepo: true, @@ -5331,6 +5332,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => { gitManager: { invalidateLocalStatus: () => Effect.void, invalidateRemoteStatus: () => Effect.void, + invalidateStatus: () => Effect.void, localStatus: () => Effect.succeed({ isRepo: true, @@ -5407,6 +5409,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => { gitManager: { invalidateLocalStatus: () => Effect.void, invalidateRemoteStatus: () => Effect.void, + invalidateStatus: () => Effect.void, localStatus: () => Deferred.succeed(localRefreshStarted, undefined).pipe( Effect.ignore, diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 5df4862b409..5daf7676d60 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -208,6 +208,61 @@ describe("GitHubCli.layer", () => { }).pipe(Effect.provide(layer)), ); + it.effect("keeps pull requests from gh versions without headRepository.nameWithOwner", () => + // gh < 2.47 (e.g. Ubuntu-packaged 2.46) exports headRepository as + // {id, name} only. These entries must decode instead of being dropped, + // with nameWithOwner rebuilt from the owner login. + Effect.gen(function* () { + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify([ + { + number: 2829, + title: "Codex turn mapping", + url: "https://github.com/pingdotgg/codething-mvp/pull/2829", + baseRefName: "main", + headRefName: "t3code/codex-turn-mapping", + state: "OPEN", + mergedAt: null, + isCrossRepository: false, + headRepository: { + id: "R_kgDORLtfbQ", + name: "codething-mvp", + }, + headRepositoryOwner: { + id: "MDEyOk9yZ2FuaXphdGlvbjg5MTkxNzI3", + login: "pingdotgg", + }, + }, + ]), + ), + ), + ); + + const gh = yield* GitHubCli.GitHubCli; + const result = yield* gh.listOpenPullRequests({ + cwd: "/repo", + headSelector: "t3code/codex-turn-mapping", + }); + + assert.deepStrictEqual(result, [ + { + number: 2829, + title: "Codex turn mapping", + url: "https://github.com/pingdotgg/codething-mvp/pull/2829", + baseRefName: "main", + headRefName: "t3code/codex-turn-mapping", + state: "open", + isCrossRepository: false, + headRepositoryNameWithOwner: "pingdotgg/codething-mvp", + headRepositoryOwnerLogin: "pingdotgg", + }, + ]); + }).pipe(Effect.provide(layer)), + ); + it.effect("reads repository clone URLs", () => Effect.gen(function* () { mockRun.mockReturnValueOnce( diff --git a/apps/server/src/sourceControl/gitHubPullRequests.ts b/apps/server/src/sourceControl/gitHubPullRequests.ts index d9dcb7f9ad1..ded3c0a90b0 100644 --- a/apps/server/src/sourceControl/gitHubPullRequests.ts +++ b/apps/server/src/sourceControl/gitHubPullRequests.ts @@ -30,17 +30,21 @@ const GitHubPullRequestSchema = Schema.Struct({ mergedAt: Schema.optional(Schema.NullOr(Schema.String)), updatedAt: Schema.optional(Schema.OptionFromNullOr(Schema.DateTimeUtcFromString)), isCrossRepository: Schema.optional(Schema.Boolean), + // gh < 2.47 exports headRepository as {id, name} only; nameWithOwner was + // added later. Both fields stay optional so a version-drifted gh CLI can + // never fail the decode and silently drop the PR from the list. headRepository: Schema.optional( Schema.NullOr( Schema.Struct({ - nameWithOwner: Schema.String, + nameWithOwner: Schema.optional(Schema.NullOr(Schema.String)), + name: Schema.optional(Schema.NullOr(Schema.String)), }), ), ), headRepositoryOwner: Schema.optional( Schema.NullOr( Schema.Struct({ - login: Schema.String, + login: Schema.optional(Schema.NullOr(Schema.String)), }), ), ), @@ -71,11 +75,15 @@ function normalizeGitHubPullRequestState(input: { function normalizeGitHubPullRequestRecord( raw: Schema.Schema.Type, ): NormalizedGitHubPullRequestRecord { - const headRepositoryNameWithOwner = trimOptionalString(raw.headRepository?.nameWithOwner); + const explicitNameWithOwner = trimOptionalString(raw.headRepository?.nameWithOwner); + const headRepositoryName = trimOptionalString(raw.headRepository?.name); const headRepositoryOwnerLogin = trimOptionalString(raw.headRepositoryOwner?.login) ?? - (typeof headRepositoryNameWithOwner === "string" && headRepositoryNameWithOwner.includes("/") - ? (headRepositoryNameWithOwner.split("/")[0] ?? null) + (explicitNameWithOwner?.includes("/") ? (explicitNameWithOwner.split("/")[0] ?? null) : null); + const headRepositoryNameWithOwner = + explicitNameWithOwner ?? + (headRepositoryOwnerLogin && headRepositoryName + ? `${headRepositoryOwnerLogin}/${headRepositoryName}` : null); return { diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 9ffd3ed696d..ecd2d0ad2b0 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -690,6 +690,47 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { ); }); + describe("remote operations", () => { + it.effect("ensureRemote reuses an existing remote across ssh/https transport variants", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* initRepoWithCommit(cwd); + const driver = yield* GitVcsDriver.GitVcsDriver; + + yield* git(cwd, ["remote", "add", "origin", "https://github.com/pingdotgg/t3code.git"]); + + const reusedForSsh = yield* driver.ensureRemote({ + cwd, + preferredName: "pingdotgg", + url: "git@github.com:pingdotgg/t3code.git", + }); + assert.equal(reusedForSsh, "origin"); + + const reusedForSshScheme = yield* driver.ensureRemote({ + cwd, + preferredName: "pingdotgg", + url: "ssh://git@github.com/pingdotgg/t3code", + }); + assert.equal(reusedForSshScheme, "origin"); + + const reusedForSshWithPort = yield* driver.ensureRemote({ + cwd, + preferredName: "pingdotgg", + url: "ssh://git@github.com:22/pingdotgg/t3code.git", + }); + assert.equal(reusedForSshWithPort, "origin"); + + const addedForFork = yield* driver.ensureRemote({ + cwd, + preferredName: "octocat", + url: "git@github.com:octocat/t3code.git", + }); + assert.equal(addedForFork, "octocat"); + assert.equal(yield* git(cwd, ["remote"]), "octocat\norigin"); + }), + ); + }); + describe("commit context", () => { it.effect("stages selected files and commits only those files", () => Effect.gen(function* () { diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 471ec10b566..33eb6d40d76 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -25,7 +25,7 @@ import { type ReviewDiffPreviewSource, type VcsRef, } from "@t3tools/contracts"; -import { dedupeRemoteBranchesWithLocalMatches } from "@t3tools/shared/git"; +import { dedupeRemoteBranchesWithLocalMatches, normalizeGitRemoteUrl } from "@t3tools/shared/git"; import { compactTraceAttributes } from "@t3tools/shared/observability"; import { decodeJsonResult } from "@t3tools/shared/schemaJson"; import { gitCommandDuration, gitCommandsTotal, withMetrics } from "../observability/Metrics.ts"; @@ -235,14 +235,6 @@ function sanitizeRemoteName(value: string): string { return sanitized.length > 0 ? sanitized : "fork"; } -function normalizeRemoteUrl(value: string): string { - return value - .trim() - .replace(/\/+$/g, "") - .replace(/\.git$/i, "") - .toLowerCase(); -} - function parseRemoteFetchUrls(stdout: string): Map { const remotes = new Map(); for (const line of stdout.split("\n")) { @@ -1092,7 +1084,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* "ensureRemote", )(function* (input) { const preferredName = sanitizeRemoteName(input.preferredName); - const normalizedTargetUrl = normalizeRemoteUrl(input.url); + const normalizedTargetUrl = normalizeGitRemoteUrl(input.url); const remoteFetchUrls = yield* runGitStdout( "GitVcsDriver.ensureRemote.listRemoteUrls", input.cwd, @@ -1100,7 +1092,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ).pipe(Effect.map((stdout) => parseRemoteFetchUrls(stdout))); for (const [remoteName, remoteUrl] of remoteFetchUrls.entries()) { - if (normalizeRemoteUrl(remoteUrl) === normalizedTargetUrl) { + if (normalizeGitRemoteUrl(remoteUrl) === normalizedTargetUrl) { return remoteName; } } diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts index 032e48e4612..ee595b1f836 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts @@ -94,6 +94,11 @@ function makeTestLayer(state: { Effect.sync(() => { state.remoteInvalidationCalls += 1; }), + invalidateStatus: () => + Effect.sync(() => { + state.localInvalidationCalls += 1; + state.remoteInvalidationCalls += 1; + }), }), ), ); @@ -206,6 +211,11 @@ describe("VcsStatusBroadcaster", () => { Effect.sync(() => { state.remoteInvalidationCalls += 1; }), + invalidateStatus: () => + Effect.sync(() => { + state.localInvalidationCalls += 1; + state.remoteInvalidationCalls += 1; + }), }), ), ); diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.ts b/apps/server/src/vcs/VcsStatusBroadcaster.ts index c238154f58c..b02ca9deb66 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.ts @@ -366,10 +366,9 @@ export const make = Effect.gen(function* () { "VcsStatusBroadcaster.refreshStatus", )(function* (rawCwd) { const cwd = yield* withFileSystem(normalizeCwd(rawCwd)); - yield* Effect.all([workflow.invalidateLocalStatus(cwd), workflow.invalidateRemoteStatus(cwd)], { - concurrency: "unbounded", - discard: true, - }); + // invalidateStatus (not the two partial invalidations) so an explicit + // refresh also bypasses GitManager's slow PR-lookup cache. + yield* workflow.invalidateStatus(cwd); const [local, remote] = yield* Effect.all( [workflow.localStatus({ cwd }), workflow.remoteStatus({ cwd })], { concurrency: "unbounded" },