From acc76f533abb8012a2b9f6ca90b04cf5b6cbfa0b Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Thu, 6 Aug 2026 06:52:07 -0400 Subject: [PATCH] fix(github-project): skip the fork alias when its own origin is unresolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review fix. `githubHostFromIdentityKey` cannot tell "origin resolved to github.com" from "origin did not resolve" — both yield no host. A GHES fork whose slug resolution had failed (auth lapse, unreachable runtime) therefore landed in the github.com namespace, so an unrelated public Project row matched it and Start work opened the wrong clone on the wrong server. Require a resolved origin before indexing the upstream alias: it is the only host evidence there is, and a repo with an unresolved origin was already absent from the origin index, so nothing is lost that origin matching had. --- .../tasks/github-project-repo-match.test.ts | 26 +++++++++++++-- mobile/src/tasks/github-project-repo-match.ts | 17 ++++++---- .../project-row-filtering.test.ts | 13 ++++++++ src/renderer/src/lib/repo-slug-cache.test.ts | 10 ++++-- src/renderer/src/lib/repo-slug-cache.ts | 32 +++++++++++-------- src/renderer/src/lib/repo-slug-index.test.ts | 23 +++++++++++-- src/renderer/src/lib/repo-slug-index.ts | 7 ++-- src/shared/github-repository-identity-key.ts | 1 + 8 files changed, 97 insertions(+), 32 deletions(-) diff --git a/mobile/src/tasks/github-project-repo-match.test.ts b/mobile/src/tasks/github-project-repo-match.test.ts index e81988b1f31..58c44a57830 100644 --- a/mobile/src/tasks/github-project-repo-match.test.ts +++ b/mobile/src/tasks/github-project-repo-match.test.ts @@ -172,7 +172,12 @@ describe('GitHub project repo matching', () => { } ] - expect(findRepoForGitHubProjectRepository('SciPhi-AI/R2R', forks, {})).toBeNull() + expect( + findRepoForGitHubProjectRepository('SciPhi-AI/R2R', forks, { + 'repo-1': { path: '/Users/me/a', repository: { owner: 'me', repo: 'a' } }, + 'repo-2': { path: '/Users/me/b', repository: { owner: 'me', repo: 'b' } } + }) + ).toBeNull() }) it('does not bind a github.com fork parent to a same-named Enterprise row', () => { @@ -198,8 +203,23 @@ describe('GitHub project repo matching', () => { ).toBeNull() }) - // Why: the host is stripped from the persisted `upstream`, so the fork's own - // origin host is the only evidence of which server its parent lives on. + it('drops the fork alias while its own origin is unresolved', () => { + const fork = { + id: 'repo-1', + path: '/Users/me/widgets-mirror', + displayName: 'widgets-mirror', + upstream: { owner: 'acme', repo: 'widgets' } + } + + for (const slugs of [ + {}, + { 'repo-1': { path: '/Users/me/widgets-mirror', repository: null } }, + { 'repo-1': { path: '/moved', repository: { owner: 'me', repo: 'widgets' } } } + ]) { + expect(findRepoForGitHubProjectRepository('acme/widgets', [fork], slugs)).toBeNull() + } + }) + it('scopes a host-less fork parent to the host the fork itself was cloned from', () => { const enterpriseFork = { id: 'repo-1', diff --git a/mobile/src/tasks/github-project-repo-match.ts b/mobile/src/tasks/github-project-repo-match.ts index d17c687405c..d4b0d228af2 100644 --- a/mobile/src/tasks/github-project-repo-match.ts +++ b/mobile/src/tasks/github-project-repo-match.ts @@ -48,10 +48,10 @@ function cachedSlugStateForRepo( return { status: 'resolved', repository: cached.repository } } -/** Identity key of the repo's fork parent, or null when it is not a fork. - * Why: the host is stripped from the persisted `upstream`, and a fork's parent - * lives on the fork's own server — without scoping the key to the repo's own - * origin host a GHES parent would collide with github.com. */ +/** Identity key of the repo's fork parent, or null when it is not a fork or its + * origin has not resolved. Why: the host is stripped from the persisted + * `upstream`, so the fork's own origin is the only evidence of which server its + * parent lives on — a host-less key would let a github.com row bind a GHES clone. */ function upstreamIdentityKeyForRepo( repo: GitHubProjectRepoMatch, originState: CachedSlugState | undefined @@ -60,8 +60,13 @@ function upstreamIdentityKeyForRepo( if (!upstream?.owner || !upstream.repo) { return null } - const originHost = originState?.status === 'resolved' ? originState.repository?.host : undefined - return githubRepoIdentityKey({ ...upstream, host: upstream.host ?? originHost }) + if (originState?.status !== 'resolved' || !originState.repository) { + return null + } + return githubRepoIdentityKey({ + ...upstream, + host: upstream.host ?? originState.repository.host + }) } export function findRepoForGitHubProjectRepository( diff --git a/src/renderer/src/components/github-project/project-row-filtering.test.ts b/src/renderer/src/components/github-project/project-row-filtering.test.ts index f100e35d37c..8871c047f7a 100644 --- a/src/renderer/src/components/github-project/project-row-filtering.test.ts +++ b/src/renderer/src/components/github-project/project-row-filtering.test.ts @@ -130,6 +130,19 @@ describe('filterProjectTableRowsBySelectedRepos', () => { expect(filtered.totalCount).toBe(0) }) + it('keeps a row whose only selected match is a fork of the row s repo', () => { + const rows = [row('fork-only', 'acme/orca')] + const filtered = filterProjectTableRowsBySelectedRepos( + table(rows), + () => ({ origin: [], upstream: [repo('fork')] }), + true, + new Set(['fork']) + ) + + expect(filtered.rows.map((r) => r.id)).toEqual(['fork-only']) + expect(filtered.totalCount).toBe(1) + }) + it('keeps a row with multiple selected matches for action ambiguity handling', () => { const rows = [row('ambiguous', 'acme/orca')] const filtered = filterProjectTableRowsBySelectedRepos( diff --git a/src/renderer/src/lib/repo-slug-cache.test.ts b/src/renderer/src/lib/repo-slug-cache.test.ts index c909286c6f9..d9de7ae3e7b 100644 --- a/src/renderer/src/lib/repo-slug-cache.test.ts +++ b/src/renderer/src/lib/repo-slug-cache.test.ts @@ -81,8 +81,6 @@ describe('repo slug cache host identity', () => { expect(lookupReposBySlugFromCache([fork], null, 'acme/widgets', 'ghe.example:8443')).toEqual([]) }) - // Why: persistence strips `upstream.host`, so the fork's own origin host is the - // only evidence of which server its parent lives on. it('scopes a host-less fork parent to the host the fork itself was cloned from', () => { const enterpriseFork = { ...repo('fork'), upstream: { owner: 'acme', repo: 'widgets' } } slugByRepoId.set( @@ -96,6 +94,14 @@ describe('repo slug cache host identity', () => { expect(lookupReposBySlugFromCache([enterpriseFork], null, 'acme/widgets')).toEqual([]) }) + it('drops the fork alias while its own origin is unresolved', () => { + const fork = { ...repo('fork'), upstream: { owner: 'acme', repo: 'widgets' } } + + expect(lookupReposBySlugFromCache([fork], null, 'acme/widgets')).toEqual([]) + slugByRepoId.set(slugCacheKey(fork.id, settingsForRepoOwner(fork, null)), null) + expect(lookupReposBySlugFromCache([fork], null, 'acme/widgets')).toEqual([]) + }) + it('expires negative slug resolutions so an external GHES login can recover', () => { const key = slugCacheKey('enterprise', null) rememberRepoSlug(key, null, 1_000) diff --git a/src/renderer/src/lib/repo-slug-cache.ts b/src/renderer/src/lib/repo-slug-cache.ts index e415dcaaeb6..503d800617a 100644 --- a/src/renderer/src/lib/repo-slug-cache.ts +++ b/src/renderer/src/lib/repo-slug-cache.ts @@ -17,23 +17,27 @@ export type SlugIndex = Map * unselected clone of the upstream repo shadow the selected fork. */ export type RepoSlugMatches = { origin: Repo[]; upstream: Repo[] } -/** Identity key of a fork's upstream parent, resolved once at repo-add time and - * persisted on the Repo record (`undefined` = unresolved, `null` = not a fork). - * Why: Project cards reference the upstream repo while a contributor's clone - * has the personal fork as `origin`, so upstream is a second identity a row - * may legitimately match. +/** Identity key of a fork's upstream parent — the second identity a Project row + * may legitimately match, since a contributor's clone has the personal fork as + * `origin`. `null` when the repo is not a fork or the key cannot be trusted. * - * `originHost` is the host of the repo's own origin remote. Persistence strips - * `upstream.host` (`sanitizeRepoUpstream`), and a fork's parent always lives on - * the fork's own server — without this the key lands in the github.com - * namespace, so a GHES row would never match and a github.com row would bind - * the wrong clone. */ -export function repoUpstreamIdentityKey(repo: Repo, originHost?: string): string | null { + * Why `originIdentityKey` is required: persistence strips `upstream.host` + * (`sanitizeRepoUpstream`), so the fork's own origin is the only evidence of + * which server its parent lives on. Without it the key falls into the + * github.com namespace, where a GHES row would never match and an unrelated + * public row would bind the Enterprise clone. */ +export function repoUpstreamIdentityKey( + repo: Repo, + originIdentityKey: string | null | undefined +): string | null { const upstream = repo.upstream - if (!upstream?.owner || !upstream.repo) { + if (!upstream?.owner || !upstream.repo || !originIdentityKey) { return null } - return githubRepoIdentityKey({ ...upstream, host: upstream.host ?? originHost }) + return githubRepoIdentityKey({ + ...upstream, + host: upstream.host ?? githubHostFromIdentityKey(originIdentityKey) + }) } /** Module-scope cache keyed by runtime scope + repo.id. A Repo that has already @@ -134,7 +138,7 @@ export function lookupReposBySlugFromCache( const originKey = slugByRepoId.get(cacheKey) if (originKey === target) { matched.push(repo) - } else if (repoUpstreamIdentityKey(repo, githubHostFromIdentityKey(originKey)) === target) { + } else if (repoUpstreamIdentityKey(repo, originKey) === target) { upstreamMatched.push(repo) } } diff --git a/src/renderer/src/lib/repo-slug-index.test.ts b/src/renderer/src/lib/repo-slug-index.test.ts index f347610c34c..106c9ec7182 100644 --- a/src/renderer/src/lib/repo-slug-index.test.ts +++ b/src/renderer/src/lib/repo-slug-index.test.ts @@ -91,8 +91,6 @@ describe('useRepoSlugIndex fork upstream matching', () => { expect(repoSlug).toHaveBeenCalledTimes(1) }) - // Why: persistence strips `upstream.host`, so the fork's own origin host is the - // only evidence of which server its parent lives on. it('scopes a host-less fork parent to the host the fork itself was cloned from', async () => { const fork = makeRepo({ id: 'fork', upstream: { owner: 'acme', repo: 'widgets' } }) setRepos([fork]) @@ -104,6 +102,27 @@ describe('useRepoSlugIndex fork upstream matching', () => { expect(result.current.lookupSlug('acme/widgets')).toEqual([]) }) + it('drops the fork alias while its own origin is unresolved', async () => { + const fork = makeRepo({ id: 'fork', upstream: { owner: 'acme', repo: 'widgets' } }) + setRepos([fork]) + repoSlug.mockResolvedValue(null) + + expect(await lookup('acme/widgets')).toEqual([]) + }) + + it('lists a repo once when its upstream is its own origin', async () => { + const selfAliased = makeRepo({ id: 'self', upstream: { owner: 'acme', repo: 'widgets' } }) + setRepos([selfAliased]) + repoSlug.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + + const { result } = renderHook(() => useRepoSlugIndex()) + await waitFor(() => expect(result.current.ready).toBe(true)) + expect(result.current.lookupSlugMatches('acme/widgets')).toEqual({ + origin: [selfAliased], + upstream: [] + }) + }) + it('reports origin and upstream matches separately for selection filtering', async () => { const upstreamClone = makeRepo({ id: 'upstream', upstream: null }) const fork = makeRepo({ id: 'fork', upstream: { owner: 'SciPhi-AI', repo: 'R2R' } }) diff --git a/src/renderer/src/lib/repo-slug-index.ts b/src/renderer/src/lib/repo-slug-index.ts index ecca47c209b..457ea8651e6 100644 --- a/src/renderer/src/lib/repo-slug-index.ts +++ b/src/renderer/src/lib/repo-slug-index.ts @@ -29,10 +29,7 @@ import { type RepoSlugMatches, type SlugIndex } from './repo-slug-cache' -import { - githubHostFromIdentityKey, - githubRepoIdentityKey -} from '../../../shared/github-repository-identity-key' +import { githubRepoIdentityKey } from '../../../shared/github-repository-identity-key' export { lookupReposBySlugFromCache } from './repo-slug-cache' @@ -161,7 +158,7 @@ async function buildIndex( // clone has their personal fork as `origin`, so the origin-only index // dropped every row (#12647). `repo.upstream` is already resolved when the // repo is added, so this costs no extra IPC. - const upstreamKey = repoUpstreamIdentityKey(repo, githubHostFromIdentityKey(slug)) + const upstreamKey = repoUpstreamIdentityKey(repo, slug) if (upstreamKey && upstreamKey !== slug) { upstreamNext.set(upstreamKey, [...(upstreamNext.get(upstreamKey) ?? []), repo]) } diff --git a/src/shared/github-repository-identity-key.ts b/src/shared/github-repository-identity-key.ts index 891f652c6dd..ac42fcd8ad6 100644 --- a/src/shared/github-repository-identity-key.ts +++ b/src/shared/github-repository-identity-key.ts @@ -21,6 +21,7 @@ export function githubRepoIdentityKey(repo: { // Why: callers that only kept the key (not the identity it came from) still need // its host segment to scope a second, host-less identity into the same namespace. // `owner` and `repo` never contain `/`, so a three-segment key is host-qualified. +// `undefined` means github.com, so never pass a key that may be unresolved. export function githubHostFromIdentityKey(key: string | null | undefined): string | undefined { const segments = key?.split('/') ?? [] return segments.length === 3 ? segments[0] : undefined