diff --git a/layers/registry/server/utils/github-client.ts b/layers/registry/server/utils/github-client.ts index f017b9bd..ef097657 100644 --- a/layers/registry/server/utils/github-client.ts +++ b/layers/registry/server/utils/github-client.ts @@ -6,6 +6,7 @@ import type { GithubCredentialReport, InstallationTokenCache, } from '#shared/server/github-app-credential' +import { z } from 'zod' import { createGithubCredential, GITHUB_RATE_LIMIT_MESSAGE, @@ -425,6 +426,44 @@ export async function getRepoSummary( owner: string, repo: string, bindings: GithubBindings, + repositoryId?: number | null, +): Promise> { + const summary = await readRepoSummaryByName(owner, repo, bindings) + if (repositoryId == null || summary.data?.repositoryId === repositoryId) + return summary + if (!summary.data && summary.status !== 404 && summary.status !== 410) + return summary + + // An old name can disappear or belong to another Repository. Resolve the + // stored ID before reading content or treating the original as missing. + const identity = await ghRequest(`${API_BASE}/repositories/${repositoryId}`, bindings) + if (!identity.data) + return { ...identity, data: null } + const parsed = recoveredRepositorySchema.safeParse(identity.data) + if (!parsed.success || parsed.data.id !== repositoryId) + return { ...identity, status: 502, data: null } + if (parsed.data.private) + return { ...identity, status: 404, data: null } + + const recovered = await readRepoSummaryByName(parsed.data.owner.login, parsed.data.name, bindings) + // A second rename or name reuse can race the lookup. Never accept a + // different ID, and never recurse into another recovery attempt. + return recovered.data && recovered.data.repositoryId !== repositoryId + ? { ...recovered, status: 502, data: null } + : recovered +} + +const recoveredRepositorySchema = z.object({ + id: z.number().int().positive(), + private: z.boolean(), + owner: z.object({ login: z.string().regex(/^[\w-]+$/) }), + name: z.string().regex(/^[\w.-]+$/), +}) + +async function readRepoSummaryByName( + owner: string, + repo: string, + bindings: GithubBindings, ): Promise> { const query = `query($owner:String!,$repo:String!){ repository(owner:$owner,name:$repo){ diff --git a/layers/registry/server/utils/repository-purpose-effect.ts b/layers/registry/server/utils/repository-purpose-effect.ts index 9ae0c344..40bcabb5 100644 --- a/layers/registry/server/utils/repository-purpose-effect.ts +++ b/layers/registry/server/utils/repository-purpose-effect.ts @@ -48,8 +48,9 @@ export async function listRepositoryPurposeCandidates(db: D1Database, now: numbe export async function readRepositoryPurposeEvidence( input: RepositoryIdentity, bindings: GithubBindings, + repositoryId?: number | null, ): Promise { - const summary = await getRepoSummary(input.owner, input.repo, bindings) + const summary = await getRepoSummary(input.owner, input.repo, bindings, repositoryId) if (summary.status === 404 || summary.status === 410) return { _tag: 'source_missing', status: summary.status } if (!summary.data?.headCommitSha || !summary.data.headTreeSha || summary.data.meta.private) @@ -100,10 +101,13 @@ export async function persistRepositoryPurpose(db: D1Database, finding: Reposito export async function refreshRepositoryPurpose(deps: { db: D1Database - readEvidence: (input: RepositoryIdentity) => Promise + readEvidence: (input: RepositoryIdentity, repositoryId?: number | null) => Promise judge: RepositoryPurposeJudge }, input: RepositoryIdentity, now: number): Promise { - const evidence = await deps.readEvidence(input) + const existing = await deps.db.prepare('SELECT repository_id FROM repos WHERE owner=? AND repo=?') + .bind(input.owner, input.repo) + .first<{ repository_id: number | null }>() + const evidence = await deps.readEvidence(input, existing?.repository_id) if ('_tag' in evidence) { await markRepoMissing(deps.db, input.owner, input.repo, now) return evidence diff --git a/layers/registry/server/utils/sync-repo.ts b/layers/registry/server/utils/sync-repo.ts index 691ee700..f71d2719 100644 --- a/layers/registry/server/utils/sync-repo.ts +++ b/layers/registry/server/utils/sync-repo.ts @@ -354,7 +354,7 @@ export async function refreshRepoAssets( ): Promise { const existingRepo = await loadExistingRepo(db, owner, repo) const requestSource = resolveRepoSourceIdentityFromRow({ owner, repo }, existingRepo) - const repoRes = await getRepoSummary(requestSource.owner, requestSource.repo, bindings) + const repoRes = await getRepoSummary(requestSource.owner, requestSource.repo, bindings, existingRepo?.repository_id) logRateLimit(`asset-backfill repo ${owner}/${repo}`, repoRes.rateLimit) const rate: Pick = { ...(repoRes.rateLimit ? { rateLimitRemaining: repoRes.rateLimit.remaining, rateLimitResetAt: repoRes.rateLimit.reset } : {}), @@ -834,7 +834,7 @@ export async function syncRepo( const requestedRepoRow = await loadExistingRepo(db, requestedOwner, requestedRepo) const requestSource = resolveRepoSourceIdentityFromRow(requested, requestedRepoRow) - const repoRes = await getRepoSummary(requestSource.owner, requestSource.repo, bindings) + const repoRes = await getRepoSummary(requestSource.owner, requestSource.repo, bindings, requestedRepoRow?.repository_id) logRateLimit(`repo ${requestedOwner}/${requestedRepo}`, repoRes.rateLimit) trackRateLimit(repoRes.rateLimit) diff --git a/server/jobs/registry/repo-maintenance.ts b/server/jobs/registry/repo-maintenance.ts index ee380957..92fee814 100644 --- a/server/jobs/registry/repo-maintenance.ts +++ b/server/jobs/registry/repo-maintenance.ts @@ -311,7 +311,7 @@ export async function handleRegistryRepoJob( throw new Error('Repository purpose AI binding is missing.') return refreshRepositoryPurpose({ db: ctx.db, - readEvidence: input => readRepositoryPurposeEvidence(input, bindings), + readEvidence: (input, repositoryId) => readRepositoryPurposeEvidence(input, bindings, repositoryId), judge: state => ctx.env.AI.run(REPOSITORY_PURPOSE_MODEL, { state, questions: repositoryPurposeQuestions }), }, payload, now) }) diff --git a/server/jobs/registry/repository-purpose.ts b/server/jobs/registry/repository-purpose.ts index 74c1e376..cc8f8218 100644 --- a/server/jobs/registry/repository-purpose.ts +++ b/server/jobs/registry/repository-purpose.ts @@ -18,7 +18,7 @@ export default defineJob({ const bindings = resolveGithubBindings(ctx.env) const result = await refreshRepositoryPurpose({ db: ctx.db, - readEvidence: identity => readRepositoryPurposeEvidence(identity, bindings), + readEvidence: (identity, repositoryId) => readRepositoryPurposeEvidence(identity, bindings, repositoryId), judge: state => ctx.env.AI.run(REPOSITORY_PURPOSE_MODEL, { state, questions: repositoryPurposeQuestions }), }, input, Math.floor(Date.now() / 1000)) if (result._tag === 'source_missing') { diff --git a/test/unit/github-repository-recovery.test.ts b/test/unit/github-repository-recovery.test.ts new file mode 100644 index 00000000..f99f8914 --- /dev/null +++ b/test/unit/github-repository-recovery.test.ts @@ -0,0 +1,143 @@ +import type { SqliteD1 } from './helpers/d1-sqlite' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { getRepoSummary } from '../../layers/registry/server/utils/github-client' +import { readRepositoryPurposeEvidence, refreshRepositoryPurpose } from '../../layers/registry/server/utils/repository-purpose-effect' +import { syncRepo } from '../../layers/registry/server/utils/sync-repo' +import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' + +const COMMIT = 'a'.repeat(40) + +function summary(id: number, owner = 'acme', name = 'skills') { + return Response.json({ data: { repository: { + databaseId: id, + isPrivate: false, + name, + nameWithOwner: `${owner}/${name}`, + url: `https://github.com/${owner}/${name}`, + owner: { login: owner }, + description: 'Skills', + stargazerCount: 1, + forkCount: 0, + pushedAt: '2026-10-01T00:00:00Z', + createdAt: '2025-01-01T00:00:00Z', + isArchived: false, + isFork: false, + defaultBranchRef: { name: 'main', target: { oid: COMMIT, tree: { oid: 'tree' } } }, + } } }) +} + +function movedRepository(overrides = {}) { + return { id: 111, private: false, owner: { login: 'new-owner' }, name: 'renamed', ...overrides } +} + +describe('repository recovery by stored ID', () => { + let d1: SqliteD1 | null = null + afterEach(() => { + vi.unstubAllGlobals() + d1?.close() + d1 = null + }) + + it('classifies the stored Repository after its name disappears without quarantining it', async () => { + d1 = createSqliteD1(allMigrations()) + d1.raw.exec(`INSERT INTO repos(owner,repo,repository_id,repo_skill_count) VALUES ('acme','skills',111,1)`) + const fetch = vi.fn().mockResolvedValueOnce(new Response(null, { status: 404 })).mockResolvedValueOnce(Response.json(movedRepository())).mockResolvedValueOnce(summary(111, 'new-owner', 'renamed')).mockResolvedValueOnce(Response.json({ sha: 'tree', truncated: false, tree: [] })) + vi.stubGlobal('fetch', fetch) + + const result = await refreshRepositoryPurpose({ + db: d1.db, + readEvidence: (identity, repositoryId) => readRepositoryPurposeEvidence(identity, {}, repositoryId), + judge: async () => ({ model: 'jev-1.13.0', answers: { purpose: { + type: 'choice', + choice: 'software', + confidence: 1, + probabilities: { 'software': 1, 'directory': 0, 'skill-pack': 0, 'mirror': 0, 'uncertain': 0 }, + } } }), + }, { owner: 'acme', repo: 'skills' }, 1000) + + expect(result).toMatchObject({ _tag: 'classified', sourceCommit: COMMIT }) + expect(d1.raw.prepare('SELECT broken_since FROM repos').get()).toEqual({ broken_since: null }) + expect(d1.raw.prepare('SELECT source_commit FROM repository_purpose').get()).toEqual({ source_commit: COMMIT }) + }) + + it.each([404, 200])('moves stored Skills after recovering a name that answers %s', async (status) => { + d1 = createSqliteD1(allMigrations()) + d1.raw.exec(` + INSERT INTO repos (owner,repo,repository_id,last_tree_sha,repo_skill_count) + VALUES ('acme','skills',111,'tree',1); + INSERT INTO skills (owner,repo,name,display_name,slug,description,current_sha,source_resolved) + VALUES ('acme','skills','one','One','acme/one','A Skill','blob',1); + `) + const fetch = vi.fn().mockResolvedValueOnce(status === 200 ? summary(222) : new Response(null, { status })).mockResolvedValueOnce(Response.json(movedRepository())).mockResolvedValueOnce(summary(111, 'new-owner', 'renamed')) + vi.stubGlobal('fetch', fetch) + + const result = await syncRepo('acme', 'skills', {}, d1.db) + + expect(result).toMatchObject({ status: 'skipped-tree-sha', movedTo: { owner: 'new-owner', repo: 'renamed' } }) + expect(d1.raw.prepare('SELECT owner,repo,repository_id FROM repos').all()) + .toEqual([{ owner: 'new-owner', repo: 'renamed', repository_id: 111 }]) + expect(d1.raw.prepare('SELECT owner,repo,name,current_sha FROM skills').all()) + .toEqual([{ owner: 'new-owner', repo: 'renamed', name: 'one', current_sha: 'blob' }]) + expect(d1.raw.prepare('SELECT owner,repo,target_owner,target_repo FROM repo_aliases').all()) + .toEqual([{ owner: 'acme', repo: 'skills', target_owner: 'new-owner', target_repo: 'renamed' }]) + }) + + it.each([404, 410, 200])('recovers the stored Repository when the old name answers %s', async (status) => { + const fetch = vi.fn() + .mockResolvedValueOnce(status === 200 ? summary(222) : new Response(null, { status })) + .mockResolvedValueOnce(Response.json(movedRepository())) + .mockResolvedValueOnce(summary(111, 'new-owner', 'renamed')) + vi.stubGlobal('fetch', fetch) + + const result = await getRepoSummary('acme', 'skills', {}, 111) + + expect(result).toMatchObject({ status: 200, data: { repositoryId: 111, meta: { full_name: 'new-owner/renamed' } } }) + expect(fetch.mock.calls[1]?.[0]).toBe('https://api.github.com/repositories/111') + expect(JSON.parse(fetch.mock.calls[2]?.[1].body).variables).toEqual({ owner: 'new-owner', repo: 'renamed' }) + }) + + it.each([401, 403, 429, 502])('preserves temporary or credential failure %s without ID lookup', async (status) => { + const fetch = vi.fn().mockResolvedValue(new Response(null, { status })) + vi.stubGlobal('fetch', fetch) + + expect(await getRepoSummary('acme', 'skills', {}, 111)).toMatchObject({ status, data: null }) + expect(fetch).toHaveBeenCalledTimes(1) + }) + + it.each([undefined, null, 111])('avoids extra reads when the stored ID is %s', async (storedId) => { + const fetch = vi.fn().mockResolvedValue(summary(111)) + vi.stubGlobal('fetch', fetch) + + expect(await getRepoSummary('acme', 'skills', {}, storedId)).toMatchObject({ status: 200, data: { repositoryId: 111 } }) + expect(fetch).toHaveBeenCalledTimes(1) + }) + + it.each([404, 503])('preserves an ID lookup failure %s instead of indexing the replacement', async (status) => { + const fetch = vi.fn().mockResolvedValueOnce(summary(222)).mockResolvedValueOnce(new Response(null, { status })) + vi.stubGlobal('fetch', fetch) + + expect(await getRepoSummary('acme', 'skills', {}, 111)).toMatchObject({ status, data: null }) + expect(fetch).toHaveBeenCalledTimes(2) + }) + + it.each([ + { value: movedRepository({ private: true }), status: 404 }, + { value: movedRepository({ private: undefined }), status: 502 }, + { value: movedRepository({ id: 222 }), status: 502 }, + { value: movedRepository({ owner: null }), status: 502 }, + ])('refuses an unsafe ID lookup answer with status $status', async ({ value, status }) => { + const fetch = vi.fn().mockResolvedValueOnce(summary(222)).mockResolvedValueOnce(Response.json(value)) + vi.stubGlobal('fetch', fetch) + + expect(await getRepoSummary('acme', 'skills', {}, 111)).toMatchObject({ status, data: null }) + expect(fetch).toHaveBeenCalledTimes(2) + }) + + it('fails closed when the recovered name is reused before its summary read', async () => { + const fetch = vi.fn().mockResolvedValueOnce(summary(222)).mockResolvedValueOnce(Response.json(movedRepository())).mockResolvedValueOnce(summary(333, 'new-owner', 'renamed')) + vi.stubGlobal('fetch', fetch) + + expect(await getRepoSummary('acme', 'skills', {}, 111)).toMatchObject({ status: 502, data: null }) + expect(fetch).toHaveBeenCalledTimes(3) + }) +}) diff --git a/test/unit/repository-move.test.ts b/test/unit/repository-move.test.ts index fb938673..d87c6d84 100644 --- a/test/unit/repository-move.test.ts +++ b/test/unit/repository-move.test.ts @@ -106,9 +106,10 @@ describe('sync follows a moved Repository by its ID', () => { }) it('moves the Repository and its Skills to the new name and keeps going there', async () => { + exec(`UPDATE repos SET repository_id = ${REACT_ID} WHERE owner = 'facebook' AND repo = 'react'`) const stats = await syncRepo('facebook', 'react', {}, d1.db) - expect(github.getRepoSummary).toHaveBeenCalledWith('react', 'react', {}) + expect(github.getRepoSummary).toHaveBeenCalledWith('react', 'react', {}, REACT_ID) expect(stats.movedTo).toEqual({ owner: 'react', repo: 'react' }) expect(stats.status).toBe('skipped-tree-sha') expect(rows(`SELECT owner, repo, repository_id, source_owner, source_repo, stars, last_tree_sha FROM repos`)).toEqual([