From 3572fed3d28886a6b07dc4199505724123b5e54f Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 22 Jul 2026 18:04:38 +0200 Subject: [PATCH] fix(server): don't drop sticky PR fallback when remote URL can't be resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sticky-PR fallback (introduced in #4281 to avoid blanking the badge on a transient gh failure) compared the resolved head remote URL strictly: if either the cached or current key was null, it treated that as a mismatch and dropped the fallback. But readConfigValueNullable maps ANY failed git-config read to null the same way it maps a genuinely absent remote — so a one-off config-read hiccup while resolving the *current* head context looked identical to "the remote actually changed," and cleared an otherwise-valid PR badge. Only invalidate the fallback when both the cached and current remote-URL keys are known and disagree; treat a null on either side as inconclusive and fall through to the next signal (remoteName, gated the same way). This preserves the fix for a genuinely retargeted branch while no longer punishing a transient read failure. Co-Authored-By: Claude Fable 5 --- apps/server/src/git/GitManager.test.ts | 48 ++++++++++++++++++++++++++ apps/server/src/git/GitManager.ts | 23 ++++++++---- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index 464c3afad64..000a9d99c6b 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -1514,6 +1514,54 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => { }), ); + it.effect("status keeps the last known PR when the current remote URL can't be resolved", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-config-hiccup"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/pr-config-hiccup"]); + + const existingPr = { + number: 217, + title: "Config hiccup PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/217", + baseRefName: "main", + headRefName: "feature/pr-config-hiccup", + }; + 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(217); + + // `remote.origin.url` reads go through readConfigValueNullable, which + // maps ANY failed read (a real "no remote configured" state or a + // transient git-config hiccup) to null the same way. Unsetting the + // key here reproduces that ambiguity without touching branch + // tracking (refs/remotes/origin/* and branch..remote are + // untouched) — the remote identity has not actually changed, so the + // sticky PR must survive even though the current lookup can no + // longer resolve a remote URL to compare against. + yield* runGit(repoDir, ["config", "--unset", "remote.origin.url"]); + yield* manager.invalidateStatus(repoDir); + + const second = yield* manager.status({ cwd: repoDir }); + expect(second.pr?.number).toBe(217); + }), + ); + 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 58bd8602df1..e59613f7cc8 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -846,16 +846,25 @@ export const make = Effect.gen(function* () { } // 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) { + // alias being repointed. Both sides must be resolved before treating a + // mismatch as real: `readConfigValueNullable` swallows any git-config + // read failure into `null`, so a transient failure to resolve the + // *current* remote URL must read as "unknown", not as "no remote" — the + // latter would otherwise drop an already-known PR badge on every hiccup. + 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) { + // If the remote URL can't be compared, fall back to the remote identity + // encoded by tracked branches — same "both sides known" requirement, for + // the same reason. A null-to-non-null transition (upstream/remoteName) + // is allowed because that is the expected first-push case. + if ( + lastKnown.upstreamRef !== null && + current.upstreamRef !== null && + lastKnown.remoteName !== null && + current.remoteName !== null + ) { return lastKnown.remoteName === current.remoteName ? lastKnown.pr : null; } return lastKnown.pr;