From 871a45eef11aff3e92cd685bce6960318c702730 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 22 Jul 2026 16:18:47 +0200 Subject: [PATCH 1/3] Stabilize PR status lookups and normalize Git remotes - Cache GitHub PR lookups while preserving the last known result on failures - Support older gh repository response fields - Reuse remotes across SSH and HTTPS URL variants - Route refreshes through full VCS status invalidation --- apps/server/src/git/GitManager.test.ts | 45 ++++++++- apps/server/src/git/GitManager.ts | 97 ++++++++++++++++--- .../src/sourceControl/GitHubCli.test.ts | 55 +++++++++++ .../src/sourceControl/gitHubPullRequests.ts | 18 +++- apps/server/src/vcs/GitVcsDriverCore.test.ts | 34 +++++++ apps/server/src/vcs/GitVcsDriverCore.ts | 15 ++- .../src/vcs/VcsStatusBroadcaster.test.ts | 5 + apps/server/src/vcs/VcsStatusBroadcaster.ts | 7 +- 8 files changed, 254 insertions(+), 22 deletions(-) diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index e1924c03ade..0de9bd1da3c 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,47 @@ 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("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..6aa920bca1f 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -95,6 +95,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; @@ -768,6 +771,82 @@ 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"); + return findLatestPr(cwd, { + branch, + upstreamRef: upstreamRef.length > 0 ? upstreamRef : null, + }); + }, + { + 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. + const lastKnownPrByBranchKey = new Map | null>(); + const rememberLastKnownPr = (branchKey: string, pr: ReturnType | null) => { + 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, pr); + }; + const lookupStatusPr = Effect.fn("lookupStatusPr")(function* ( + cwd: string, + details: { branch: string; upstreamRef: string | null; isDefaultBranch: boolean }, + ) { + const branchKey = `${cwd}\u0000${details.branch}\u0000${details.upstreamRef ?? ""}`; + return yield* Cache.get(prLookupCache, prLookupCacheKey(cwd, details)).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.tap((pr) => Effect.sync(() => rememberLastKnownPr(branchKey, 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.as(lastKnownPrByBranchKey.get(branchKey) ?? null), + ), + ), + ); + }); const readRemoteStatus = Effect.fn("readRemoteStatus")(function* ( cwd: string, options?: GitVcsDriver.GitRemoteStatusOptions, @@ -781,19 +860,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 { @@ -1442,6 +1513,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/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..29e113eb895 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -690,6 +690,40 @@ 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 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..3021b3bc3d3 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -236,11 +236,24 @@ function sanitizeRemoteName(value: string): string { } function normalizeRemoteUrl(value: string): string { - return value + const normalized = value .trim() .replace(/\/+$/g, "") .replace(/\.git$/i, "") .toLowerCase(); + // Reduce transport variants of the same repository to `host/path` so + // ensureRemote reuses an existing remote instead of adding an ssh/https + // duplicate (git@host:owner/repo, ssh://git@host/owner/repo, + // https://host/owner/repo all compare equal). + const scpLike = /^(?:ssh:\/\/)?(?:[^@/:]+@)([^:/]+)[:/](.+)$/.exec(normalized); + if (scpLike) { + return `${scpLike[1]}/${scpLike[2]}`; + } + const urlLike = /^(?:https?|git):\/\/(?:[^@/]+@)?(.+)$/.exec(normalized); + if (urlLike) { + return urlLike[1] ?? normalized; + } + return normalized; } function parseRemoteFetchUrls(stdout: string): Map { diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts index 032e48e4612..4235240fd28 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; + }), }), ), ); 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" }, From 0cb345dbc51ab5e1217f548158f515a810dd19e1 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 22 Jul 2026 16:30:46 +0200 Subject: [PATCH 2/3] Address review: reuse shared remote URL normalization, per-branch PR fallback, complete mocks - Replace the hand-rolled normalizeRemoteUrl in GitVcsDriverCore with the shared normalizeGitRemoteUrl, which correctly drops explicit ssh ports (ssh://git@host:22/owner/repo now matches https://host/owner/repo). - Key the last-known-PR fallback by (cwd, branch) only so an upstream change (e.g. first push -u) does not orphan the sticky value. - Implement invalidateStatus in the remaining GitWorkflowService/GitManager test mocks; the missing method made the background-refresh server test hang for 120s in CI. Co-Authored-By: Claude Fable 5 --- apps/server/src/git/GitManager.ts | 4 ++- apps/server/src/server.test.ts | 3 +++ apps/server/src/vcs/GitVcsDriverCore.test.ts | 7 +++++ apps/server/src/vcs/GitVcsDriverCore.ts | 27 +++---------------- .../src/vcs/VcsStatusBroadcaster.test.ts | 5 ++++ 5 files changed, 21 insertions(+), 25 deletions(-) diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts index 6aa920bca1f..d5b21901a79 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -822,7 +822,9 @@ export const make = Effect.gen(function* () { cwd: string, details: { branch: string; upstreamRef: string | null; isDefaultBranch: boolean }, ) { - const branchKey = `${cwd}\u0000${details.branch}\u0000${details.upstreamRef ?? ""}`; + // 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) => { if (!latest) return null; 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/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 29e113eb895..ecd2d0ad2b0 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -713,6 +713,13 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { }); 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", diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 3021b3bc3d3..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,27 +235,6 @@ function sanitizeRemoteName(value: string): string { return sanitized.length > 0 ? sanitized : "fork"; } -function normalizeRemoteUrl(value: string): string { - const normalized = value - .trim() - .replace(/\/+$/g, "") - .replace(/\.git$/i, "") - .toLowerCase(); - // Reduce transport variants of the same repository to `host/path` so - // ensureRemote reuses an existing remote instead of adding an ssh/https - // duplicate (git@host:owner/repo, ssh://git@host/owner/repo, - // https://host/owner/repo all compare equal). - const scpLike = /^(?:ssh:\/\/)?(?:[^@/:]+@)([^:/]+)[:/](.+)$/.exec(normalized); - if (scpLike) { - return `${scpLike[1]}/${scpLike[2]}`; - } - const urlLike = /^(?:https?|git):\/\/(?:[^@/]+@)?(.+)$/.exec(normalized); - if (urlLike) { - return urlLike[1] ?? normalized; - } - return normalized; -} - function parseRemoteFetchUrls(stdout: string): Map { const remotes = new Map(); for (const line of stdout.split("\n")) { @@ -1105,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, @@ -1113,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 4235240fd28..ee595b1f836 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts @@ -211,6 +211,11 @@ describe("VcsStatusBroadcaster", () => { Effect.sync(() => { state.remoteInvalidationCalls += 1; }), + invalidateStatus: () => + Effect.sync(() => { + state.localInvalidationCalls += 1; + state.remoteInvalidationCalls += 1; + }), }), ), ); From c0b9ceecc74af403190a1de74782c699115a504a Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 22 Jul 2026 17:41:53 +0200 Subject: [PATCH 3/3] Address review: don't reuse a sticky PR fallback across a retargeted upstream The last-known-PR fallback (added to avoid blanking the badge on a transient gh failure) is keyed by branch only, so a retarget to a different upstream/fork mid-failure could surface a PR that no longer matches the current head. Track the upstream the fallback was resolved against and withhold it when both the cached and current upstreamRef are known and differ, while still tolerating an upstream merely appearing or disappearing. Co-Authored-By: Claude Fable 5 --- apps/server/src/git/GitManager.test.ts | 135 +++++++++++++++++++++++++ apps/server/src/git/GitManager.ts | 98 +++++++++++++++--- 2 files changed, 217 insertions(+), 16 deletions(-) diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index 0de9bd1da3c..464c3afad64 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -1379,6 +1379,141 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + 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 d5b21901a79..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, @@ -145,6 +146,7 @@ interface BranchHeadContext { headSelectors: ReadonlyArray; preferredHeadSelector: string; remoteName: string | null; + headRemoteUrlKey: string | null; headRepositoryNameWithOwner: string | null; headRepositoryOwnerLogin: string | null; isCrossRepository: boolean; @@ -792,10 +794,17 @@ export const make = Effect.gen(function* () { const prLookupCache = yield* Cache.makeWith( (key: string) => { const [cwd = "", branch = "", upstreamRef = ""] = key.split("\u0000"); - return findLatestPr(cwd, { + 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, @@ -804,9 +813,17 @@ export const make = Effect.gen(function* () { ); // 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. - const lastKnownPrByBranchKey = new Map | null>(); - const rememberLastKnownPr = (branchKey: string, pr: ReturnType | null) => { + // 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 @@ -816,7 +833,32 @@ export const make = Effect.gen(function* () { lastKnownPrByBranchKey.delete(oldestKey); } } - lastKnownPrByBranchKey.set(branchKey, pr); + 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, @@ -826,14 +868,27 @@ export const make = Effect.gen(function* () { // `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) => { - if (!latest) return null; + 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 null; - return toStatusPr(latest); + if (details.isDefaultBranch && latest.state !== "open") { + return { pr: null, headContext }; + } + return { pr: toStatusPr(latest), headContext }; }), - Effect.tap((pr) => Effect.sync(() => rememberLastKnownPr(branchKey, pr))), + 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({ @@ -844,7 +899,15 @@ export const make = Effect.gen(function* () { ? String(error._tag) : typeof error, }), - Effect.as(lastKnownPrByBranchKey.get(branchKey) ?? null), + Effect.andThen(resolveBranchHeadContext(cwd, details)), + Effect.map((headContext) => + resolveLastKnownPr(branchKey, { + upstreamRef: details.upstreamRef, + headBranch: headContext.headBranch, + remoteName: headContext.remoteName, + headRemoteUrlKey: headContext.headRemoteUrlKey, + }), + ), ), ), ); @@ -910,6 +973,7 @@ export const make = Effect.gen(function* () { ) { if (!remoteName) { return { + remoteUrlKey: null, repositoryNameWithOwner: null, ownerLogin: null, }; @@ -918,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), }; @@ -988,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, @@ -1033,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) { @@ -1064,7 +1131,6 @@ export const make = Effect.gen(function* () { } return parsed[0] ?? null; }); - const buildCompletionToast = Effect.fn("buildCompletionToast")(function* ( cwd: string, result: Pick,