From 5881dcab34e61aeaf4d798003168f21f96357230 Mon Sep 17 00:00:00 2001 From: fixfon <12512757+fixfon@users.noreply.github.com> Date: Fri, 16 Oct 2026 21:42:08 +0300 Subject: [PATCH 1/4] fix(server): PR sync ignores tokenized avatar URLs on ghe.com On GitHub Enterprise Cloud with data residency (*.ghe.com), the author's avatarUrl carries a `token` query parameter that changes on every read. snapshotFieldsEqual compared the raw URL, so a stored snapshot never equalled a fresh one: every open linked PR took a REST /stacks read and a stored link event on every sweep. Compare the avatar URL without its query string (the raw string when it does not parse), so a different avatar path still counts as a change. Fixes #20273 --- .../PullRequestSyncReactor.test.ts | 46 +++++++++++++++++++ .../PullRequestSyncReactor.ts | 12 ++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts b/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts index 841e87c274fa3..e28289028b3a9 100644 --- a/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts +++ b/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts @@ -580,6 +580,52 @@ describe("PullRequestSyncReactor", () => { ), ); + it.effect("ignores the per-read token in the author's avatar URL", () => + Effect.scoped( + Effect.gen(function* () { + yield* TestClock.setTime(Date.parse(NOW)); + let reads = 0; + let avatarPath = "/u/1"; + const fixture = yield* makeHarness({ + snapshot: makeSnapshot([makeThread("one", { pullRequests: [makeLink(42)] })]), + summary: (input) => + Effect.succeed( + makeSummary(input, { + author: { + login: "octocat", + name: null, + avatarUrl: `https://avatars.example.ghe.com${avatarPath}?token=read-${++reads}`, + }, + }), + ), + }); + + yield* Effect.gen(function* () { + const reactor = yield* startAndSweep(fixture); + const firstSweep = yield* Ref.get(fixture.syncCommands); + yield* Ref.update(fixture.snapshots, (snapshot) => applySync(snapshot, firstSweep)); + + yield* sweepAgain(fixture, reactor); + + assert.strictEqual(reads, 2); + assert.strictEqual((yield* Ref.get(fixture.stackCalls)).length, 1); + assert.strictEqual((yield* Ref.get(fixture.syncCommands)).length, 1); + + avatarPath = "/u/2"; + yield* sweepAgain(fixture, reactor); + + assert.strictEqual((yield* Ref.get(fixture.stackCalls)).length, 2); + const commands = yield* Ref.get(fixture.syncCommands); + assert.strictEqual(commands.length, 2); + assert.strictEqual( + commands.at(-1)?.snapshot.author?.avatarUrl, + "https://avatars.example.ghe.com/u/2?token=read-3", + ); + }).pipe(Effect.provide(fixture.layer)); + }), + ), + ); + it.effect("refreshes closed links through the reactor's project after reopening elsewhere", () => Effect.scoped( Effect.gen(function* () { diff --git a/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts b/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts index c2cfe497d35ae..8a141b75ac575 100644 --- a/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts @@ -68,6 +68,16 @@ function snapshotFieldsOf(summary: PullRequestSummary): SnapshotFields { }; } +// ghe.com avatar URLs carry a `token` query that changes on every read, so a stored snapshot would +// never equal a fresh one. The origin and path still tell avatars apart. +function avatarIdentity(avatarUrl: string | null | undefined): string | null { + if (avatarUrl === null || avatarUrl === undefined) return null; + const url = URL.parse(avatarUrl); + if (url === null) return avatarUrl; + url.search = ""; + return url.href; +} + function snapshotFieldsEqual(left: SnapshotFields, right: SnapshotFields): boolean { return ( left.state === right.state && @@ -79,7 +89,7 @@ function snapshotFieldsEqual(left: SnapshotFields, right: SnapshotFields): boole (left.closedAt ?? null) === (right.closedAt ?? null) && (left.mergedAt ?? null) === (right.mergedAt ?? null) && (left.author?.login ?? null) === (right.author?.login ?? null) && - (left.author?.avatarUrl ?? null) === (right.author?.avatarUrl ?? null) && + avatarIdentity(left.author?.avatarUrl) === avatarIdentity(right.author?.avatarUrl) && left.additions === right.additions && left.deletions === right.deletions && left.changedFiles === right.changedFiles && From 006f031b354ffcad311f1c0bebf6508127ae1191 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 05:59:32 +0000 Subject: [PATCH 2/4] test(server): constrain pull request avatar token comparison --- .../PullRequestSyncReactor.test.ts | 79 +++++++++++++------ 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts b/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts index 2fedd1c53b962..bbb336ff68bdd 100644 --- a/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts +++ b/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts @@ -580,47 +580,78 @@ describe("PullRequestSyncReactor", () => { ), ); - it.effect("ignores the per-read token in the author's avatar URL", () => + it.effect.each([ + { + change: "ghe.com token rotation", + host: "avatars.example.ghe.com", + nextPath: "/u/1?token=new&v=4&s=40&format=png", + expectedSyncs: 1, + }, + { + change: "avatar version", + host: "avatars.example.ghe.com", + nextPath: "/u/1?token=old&v=5&s=40&format=png", + expectedSyncs: 2, + }, + { + change: "avatar size", + host: "avatars.example.ghe.com", + nextPath: "/u/1?token=old&v=4&s=80&format=png", + expectedSyncs: 2, + }, + { + change: "another avatar query parameter", + host: "avatars.example.ghe.com", + nextPath: "/u/1?token=old&v=4&s=40&format=webp", + expectedSyncs: 2, + }, + { + change: "avatar path", + host: "avatars.example.ghe.com", + nextPath: "/u/2?token=old&v=4&s=40&format=png", + expectedSyncs: 2, + }, + { + change: "token outside ghe.com", + host: "avatars.example.com", + nextPath: "/u/1?token=new&v=4&s=40&format=png", + expectedSyncs: 2, + }, + ])("syncs avatar changes selectively: $change", ({ host, nextPath, expectedSyncs }) => Effect.scoped( Effect.gen(function* () { yield* TestClock.setTime(Date.parse(NOW)); - let reads = 0; - let avatarPath = "/u/1"; + const avatarUrl = yield* Ref.make(`https://${host}/u/1?token=old&v=4&s=40&format=png`); const fixture = yield* makeHarness({ snapshot: makeSnapshot([makeThread("one", { pullRequests: [makeLink(42)] })]), summary: (input) => - Effect.succeed( - makeSummary(input, { - author: { - login: "octocat", - name: null, - avatarUrl: `https://avatars.example.ghe.com${avatarPath}?token=read-${++reads}`, - }, - }), + Ref.get(avatarUrl).pipe( + Effect.map((avatarUrl) => + makeSummary(input, { author: { login: "octocat", name: null, avatarUrl } }), + ), ), }); yield* Effect.gen(function* () { const reactor = yield* startAndSweep(fixture); const firstSweep = yield* Ref.get(fixture.syncCommands); + assert.strictEqual(firstSweep.length, 1); + assert.strictEqual(firstSweep[0]?.snapshot.author?.avatarUrl, yield* Ref.get(avatarUrl)); yield* Ref.update(fixture.snapshots, (snapshot) => applySync(snapshot, firstSweep)); + yield* Ref.set(avatarUrl, `https://${host}${nextPath}`); yield* sweepAgain(fixture, reactor); - assert.strictEqual(reads, 2); - assert.strictEqual((yield* Ref.get(fixture.stackCalls)).length, 1); - assert.strictEqual((yield* Ref.get(fixture.syncCommands)).length, 1); - - avatarPath = "/u/2"; - yield* sweepAgain(fixture, reactor); - - assert.strictEqual((yield* Ref.get(fixture.stackCalls)).length, 2); + assert.strictEqual((yield* Ref.get(fixture.summaryCalls)).length, 2); + assert.strictEqual((yield* Ref.get(fixture.stackCalls)).length, expectedSyncs); const commands = yield* Ref.get(fixture.syncCommands); - assert.strictEqual(commands.length, 2); - assert.strictEqual( - commands.at(-1)?.snapshot.author?.avatarUrl, - "https://avatars.example.ghe.com/u/2?token=read-3", - ); + assert.strictEqual(commands.length, expectedSyncs); + if (expectedSyncs === 2) { + assert.strictEqual( + commands.at(-1)?.snapshot.author?.avatarUrl, + yield* Ref.get(avatarUrl), + ); + } }).pipe(Effect.provide(fixture.layer)); }), ), From 4e8b91b78a1c589998f4860abbdc9e7f6a4ff65b Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 06:01:15 +0000 Subject: [PATCH 3/4] fix(server): limit avatar comparison to ghe token changes --- .../src/orchestration-v2/PullRequestSyncReactor.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts b/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts index 27af0b4ea796c..e23848787f058 100644 --- a/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts +++ b/apps/server/src/orchestration-v2/PullRequestSyncReactor.ts @@ -68,13 +68,14 @@ function snapshotFieldsOf(summary: PullRequestSummary): SnapshotFields { }; } -// ghe.com avatar URLs carry a `token` query that changes on every read, so a stored snapshot would -// never equal a fresh one. The origin and path still tell avatars apart. +// Rotating ghe.com avatar tokens should not make otherwise identical snapshots differ. function avatarIdentity(avatarUrl: string | null | undefined): string | null { if (avatarUrl === null || avatarUrl === undefined) return null; const url = URL.parse(avatarUrl); - if (url === null) return avatarUrl; - url.search = ""; + if (url === null || (url.hostname !== "ghe.com" && !url.hostname.endsWith(".ghe.com"))) { + return avatarUrl; + } + url.searchParams.delete("token"); return url.href; } From 480d600f1e9996e79c7b3965f496950f182de48e Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 20 Oct 2026 04:22:57 +0000 Subject: [PATCH 4/4] test(server): keep avatar sync fixture on active cadence --- .../src/orchestration-v2/PullRequestSyncReactor.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts b/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts index d91b03cd050d4..64c40a545e88c 100644 --- a/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts +++ b/apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts @@ -709,7 +709,10 @@ describe("PullRequestSyncReactor", () => { summary: (input) => Ref.get(avatarUrl).pipe( Effect.map((avatarUrl) => - makeSummary(input, { author: { login: "octocat", name: null, avatarUrl } }), + makeSummary(input, { + updatedAt: NOW, + author: { login: "octocat", name: null, avatarUrl }, + }), ), ), });