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