fix(github-project): skip the fork alias when its own origin is unresolved

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.
This commit is contained in:
Jinwoo-H
2026-08-07 08:41:18 +09:00
committed by Wooseong Kim
parent 095292a4ad
commit acc76f533a
8 changed files with 97 additions and 32 deletions
@@ -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',
+11 -6
View File
@@ -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(
@@ -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(
+8 -2
View File
@@ -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)
+18 -14
View File
@@ -17,23 +17,27 @@ export type SlugIndex = Map<string, Repo[]>
* 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)
}
}
+21 -2
View File
@@ -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' } })
+2 -5
View File
@@ -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])
}
@@ -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