From a12bdb19d3097faf9221c22a464a511782a59b2a Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:44:03 +1100 Subject: [PATCH 1/2] fix(artifact-delivery): read moved and oversized Repositories A run of a Skill in a renamed or transferred Repository failed: GitHub answers the old name with a 301 to /repositories/, and every read treated that redirect as fatal. 229 registry Skills across 13 Repositories hit it, facebook/react among them. The read now follows that one hop on the API origin. The Resolution keeps the name the request used, because the released CLI checks the attested name against its request. A load trusts the Repository ID and reads by the current name. A tree response over the 8 MiB read limit threw, so the queue spent its whole retry ladder and then failed as SERVICE_UNAVAILABLE. It is now a value each caller answers. A named search splits a tree that one response cannot hold, so a small Skill in posthog/posthog or vercel/next.js resolves by name. --- .../server/utils/github-source.ts | 240 ++++++++++++---- .../unit/artifact-github-source-scale.test.ts | 259 ++++++++++++++++++ test/unit/artifact-named-skill.test.ts | 14 +- 3 files changed, 450 insertions(+), 63 deletions(-) create mode 100644 test/unit/artifact-github-source-scale.test.ts diff --git a/layers/artifact-delivery/server/utils/github-source.ts b/layers/artifact-delivery/server/utils/github-source.ts index 6909dcad..eafa41e4 100644 --- a/layers/artifact-delivery/server/utils/github-source.ts +++ b/layers/artifact-delivery/server/utils/github-source.ts @@ -22,6 +22,9 @@ const GITHUB_API_VERSION = '2026-03-10' const MAX_GITHUB_JSON_BYTES = 8 * 1024 * 1024 const MAX_TREE_ENTRIES = 2000 const MAX_TREE_REQUESTS = 128 +// One tree read can hold 8 MiB of JSON and the tree parsed from it. Two at a +// time keep a split walk well inside a Worker's 128 MiB. +const TREE_WALK_CONCURRENCY = 2 const MAX_SKILL_PATH_SEGMENTS = 64 // One invocation loads every blob once. With one repository read per load and // resolve, a handful of tree reads, the D1 state transitions and the R2 write, @@ -97,6 +100,12 @@ const blobResponseSchema = z.object({ type TreeEntry = z.infer +/** Where the GitHub API serves a Repository now. */ +interface RepositoryName { + owner: string + repository: string +} + interface ListedTree { _tag: 'listed' entries: TreeEntry[] @@ -168,17 +177,8 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi outcome._tag === 'rate-limited' ? rateLimitRejection(outcome.resetAt) : sourceReadRejection(outcome._tag, expectedVisibility) - const requestJson = async (step: GithubReadStep, path: string, schema: z.ZodType): Promise< - { _tag: 'ok', value: T } - | { _tag: 'not-found' } - | { _tag: 'access-denied' } - | { _tag: 'rate-limited', resetAt: number | null } - > => { - type Outcome - = | { _tag: 'ok', value: T } - | { _tag: 'not-found' } - | { _tag: 'access-denied' } - | { _tag: 'rate-limited', resetAt: number | null } + const requestJson = async (step: GithubReadStep, path: string, schema: z.ZodType): Promise> => { + type Outcome = ReadOutcome const headers = new Headers({ 'Accept': 'application/vnd.github+json', 'User-Agent': 'skilld.dev', @@ -188,11 +188,22 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi headers.set('Authorization', `Bearer ${options.token}`) // Every try requests this same URL, so a retry can never read another ref. const url = `${GITHUB_API}${path}` + const fetchOnce = (target: string) => fetchNoRedirect(options.fetch, target, { + headers, + signal: AbortSignal.timeout(GITHUB_READ_TRY_TIMEOUT_MS), + }) + // A renamed or transferred Repository answers its old name with a 301 to + // `/repositories/`. The read follows that one hop, so a run of the old + // name keeps working. Every other redirect stays fatal. + const fetchFollowingMove = async () => { + const fetched = await fetchOnce(url) + if (fetched._tag !== 'unexpected-redirect') + return fetched + const moved = movedRepositoryUrl(fetched.location) + return moved ? await fetchOnce(moved) : fetched + } const tryOnce = async (): Promise> => { - const sent = await fetchNoRedirect(options.fetch, url, { - headers, - signal: AbortSignal.timeout(GITHUB_READ_TRY_TIMEOUT_MS), - }).then( + const sent = await fetchFollowingMove().then( fetched => ({ _tag: 'sent' as const, fetched }), (error: unknown) => ({ _tag: 'threw' as const, error }), ) @@ -223,10 +234,13 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi status: response.status, } } - const body = await readBoundedJson(response, MAX_GITHUB_JSON_BYTES).then( - value => ({ _tag: 'json' as const, value }), - (error: unknown) => ({ _tag: 'unreadable' as const, error }), - ) + const body = await readBoundedJson(response, MAX_GITHUB_JSON_BYTES) + .catch((error: unknown) => ({ _tag: 'unreadable' as const, error })) + // A body over the limit is a fact about the Git object, so it settles + // as a value each caller can answer. It used to throw, which spent the + // whole queue retry ladder and then failed as SERVICE_UNAVAILABLE. + if (body._tag === 'too-large') + return { _tag: 'settled', value: { _tag: 'too-large' } } if (body._tag === 'unreadable') { const transient = !isBodyLimitError(body.error) return { @@ -312,14 +326,16 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi rootTreeSha: string, ): Promise => { const recursive = await getTree(owner, repository, rootTreeSha, true) - if (recursive._tag !== 'ok') + if (recursive._tag !== 'ok' && recursive._tag !== 'too-large') return readRejection(recursive) - if (!recursive.value.truncated) { + if (recursive._tag === 'ok' && !recursive.value.truncated) { if (recursive.value.tree.length > MAX_TREE_ENTRIES) return sourceLimitRejection(`The Skill folder has more than ${MAX_TREE_ENTRIES} entries.`) return { _tag: 'listed', entries: recursive.value.tree, truncated: false } } + // GitHub truncated the listing, or it passed the read limit. Both mean + // one response could not hold it, so walk one tree per directory. const entries: TreeEntry[] = [] const queue: Array<{ sha: string, prefix: string }> = [{ sha: rootTreeSha, prefix: '' }] let requests = 0 @@ -328,10 +344,10 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi return sourceLimitRejection(`The Skill folder needs more than ${MAX_TREE_REQUESTS} tree reads.`) const current = queue.shift()! const response = await getTree(owner, repository, current.sha, false) + if (response._tag === 'too-large' || (response._tag === 'ok' && response.value.truncated)) + return sourceLimitRejection('GitHub returned an incomplete Skill tree.') if (response._tag !== 'ok') return readRejection(response) - if (response.value.truncated) - return sourceLimitRejection('GitHub returned an incomplete Skill tree.') for (const entry of response.value.tree) { const path = current.prefix ? `${current.prefix}/${entry.path}` : entry.path const nested = { ...entry, path } @@ -381,6 +397,9 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi * * Copies of one Skill in several folders resolve to the canonical copy. * They used to fail as "More than one Skill matched". + * + * The registry answers a name it admitted before this search runs, so the + * search serves names it does not hold, such as a private Skill. */ const resolveNamedSkill = async ( owner: string, @@ -388,24 +407,85 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi treeSha: string, name: string, ): Promise => { - const tree = await getTree(owner, repository, treeSha, true) - if (tree._tag !== 'ok') - return readRejection(tree) - if (tree.value.truncated) { + const folders = await listSkillFolders(owner, repository, treeSha) + if (folders._tag === 'unlistable') { return reject( 'INVALID_SOURCE', 'The Repository is too large to find a Skill by name. Name the Skill by its path.', [name], ) } - const matches = tree.value.tree - .filter(entry => entry.type === 'blob' && isRegistrySkillPath(entry.path)) - .map(entry => entry.path === 'SKILL.md' ? '.' : entry.path.slice(0, -'/SKILL.md'.length)) + if (folders._tag === 'rejected') + return folders + const matches = folders.folders .filter(path => (path === '.' ? repository : path.split('/').at(-1)) === name) return canonicalSkillFolder(matches) ?? reject('SOURCE_NOT_FOUND', 'No Skill matched the requested name.', [name]) } + /** + * Every registry Skill folder in a tree, in as few reads as GitHub allows. + * + * One recursive read lists most Repositories. GitHub truncates a recursive + * listing past 100,000 entries or 7 MB, and a large one can pass the 8 MiB + * read limit: posthog/posthog, n8n-io/n8n, vercel/next.js. Each failed the + * search outright, whatever the Skill size. A tree that one response cannot + * hold now splits: one read lists its own level, and each folder in it gets + * its own recursive read, which can split again. `MAX_TREE_REQUESTS` bounds + * the walk. + */ + const listSkillFolders = async ( + owner: string, + repository: string, + rootTreeSha: string, + ): Promise<{ _tag: 'listed', folders: string[] } | { _tag: 'unlistable' } | SourceRejection> => { + type Visit + = | { _tag: 'listed', folders: string[], children: Array<{ sha: string, prefix: string }> } + | { _tag: 'unlistable' } + | SourceRejection + let requests = 0 + const read = (sha: string, recursive: boolean) => { + requests++ + return getTree(owner, repository, sha, recursive) + } + const visit = async ({ sha, prefix }: { sha: string, prefix: string }): Promise => { + const join = (path: string) => prefix ? `${prefix}/${path}` : path + const whole = await read(sha, true) + if (whole._tag === 'ok' && !whole.value.truncated) + return { _tag: 'listed', folders: skillFolders(whole.value.tree.map(entry => ({ ...entry, path: join(entry.path) }))), children: [] } + if (whole._tag !== 'ok' && whole._tag !== 'too-large') + return readRejection(whole) + const level = await read(sha, false) + if (level._tag === 'too-large' || (level._tag === 'ok' && level.value.truncated)) + return { _tag: 'unlistable' } + if (level._tag !== 'ok') + return readRejection(level) + const entries = level.value.tree.map(entry => ({ ...entry, path: join(entry.path) })) + return { + _tag: 'listed', + folders: skillFolders(entries), + children: entries.filter(entry => entry.type === 'tree').map(entry => ({ sha: entry.sha, prefix: entry.path })), + } + } + + const folders: string[] = [] + const pending = [{ sha: rootTreeSha, prefix: '' }] + while (pending.length > 0) { + // A split folder costs two reads, so a wave can need twice its size. + const wave = pending.splice(0, TREE_WALK_CONCURRENCY) + if (requests + wave.length * 2 > MAX_TREE_REQUESTS) + return { _tag: 'unlistable' } + const visited = await Promise.all(wave.map(visit)) + for (const result of visited) { + if (result._tag !== 'listed') + return result + folders.push(...result.folders) + pending.push(...result.children) + } + } + return { _tag: 'listed', folders } + } + return { async resolve(request) { const normalizedPath = request.selector.type === 'path' @@ -428,6 +508,8 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi ? reject('SOURCE_NOT_FOUND', 'The Repository was not found.', []) : reject('SOURCE_ACCESS_DENIED', 'GitHub denied access to the Repository.', []) } + if (repository._tag === 'too-large') + return readRejection(repository) if (repository.value.private !== (expectedVisibility === 'private')) { return expectedVisibility === 'private' ? reject('SOURCE_NOT_FOUND', 'The Repository was not found.', []) @@ -455,6 +537,8 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi return reject('SOURCE_NOT_FOUND', 'The requested Git reference was not found.', [request.ref?.value ?? repository.value.default_branch]) if (commit._tag === 'access-denied') return reject('SOURCE_ACCESS_DENIED', 'GitHub denied access to the requested Git reference.', []) + if (commit._tag === 'too-large') + return readRejection(commit) if (commit.value.sha !== requestedCommit) { return reject('INVALID_SOURCE', 'GitHub returned another commit identity.', [ requestedCommit, @@ -473,13 +557,18 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi if (typeof skillPath !== 'string') return skillPath + // A moved Repository keeps the name the request used. GitHub confirmed + // that the name leads to this Repository ID, and the released CLI + // checks the attested name against its own request. + const moved = !sameGithubName(request.owner, repository.value.owner.login) + || !sameGithubName(request.repository, repository.value.name) return { _tag: 'resolved', source: { provider: 'github', repositoryId: repository.value.id, - owner: repository.value.owner.login, - repository: repository.value.name, + owner: moved ? request.owner : repository.value.owner.login, + repository: moved ? request.repository : repository.value.name, visibility: expectedVisibility, commitSha: commit.value.sha, treeSha: commit.value.commit.tree.sha, @@ -503,19 +592,21 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi ? reject('SOURCE_NOT_FOUND', 'The Repository was not found.', []) : reject('SOURCE_ACCESS_DENIED', 'This Artifact source is no longer public.', []) } + if (repository._tag === 'too-large') + return readRejection(repository) if (repository.value.private !== (expectedVisibility === 'private')) return reject('SOURCE_ACCESS_DENIED', 'The Repository visibility changed.', []) - if ( - repository.value.id !== source.repositoryId - || repository.value.owner.login !== source.owner - || repository.value.name !== source.repository - ) { + // The Repository ID is the identity. A name can move while the build + // runs, or after a reused build recorded it, and the commit and tree + // digests still pin every byte. + if (repository.value.id !== source.repositoryId) return reject('INVALID_SOURCE', 'The Repository identity changed during Artifact creation.', []) - } - const skillTree = await findTreeAtPath(source.owner, source.repository, source.treeSha, source.skillPath) + // Every later read uses the current name, so none of them redirects. + const at: RepositoryName = { owner: repository.value.owner.login, repository: repository.value.name } + const skillTree = await findTreeAtPath(at.owner, at.repository, source.treeSha, source.skillPath) if (typeof skillTree !== 'string') return skillTree - const listed = await listTreeBounded(source.owner, source.repository, skillTree) + const listed = await listTreeBounded(at.owner, at.repository, skillTree) if (listed._tag !== 'listed') return listed const selected = selectArtifactEntries(listed.entries, source.skillPath) @@ -528,7 +619,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi totalBlobBytes: selected.entries.reduce((total, entry) => total + entry.size, 0), }) if (choice._tag === 'tarball') { - const extracted = await loadFromTarball(source, selected.entries) + const extracted = await loadFromTarball(source, at, selected.entries) if (extracted._tag === 'extracted') return { _tag: 'loaded', value: { source, files: extracted.files } } // The tarball is an optimisation, never an authority. Anything it got @@ -544,12 +635,13 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi findings: extracted.findings.slice(0, 20), }) } - return await loadFromBlobs(source, selected.entries) + return await loadFromBlobs(source, at, selected.entries) }, } async function loadFromTarball( source: ResolvedSource, + at: RepositoryName, entries: Array, ): Promise { const headers = new Headers({ @@ -563,7 +655,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi // redirects. The bytes it returns are verified against the tree digests, // which is what makes an unauthenticated byte host acceptable. const response = await options.fetch( - `${GITHUB_API}/repos/${encodeURIComponent(source.owner)}/${encodeURIComponent(source.repository)}/tarball/${source.commitSha}`, + `${GITHUB_API}/repos/${encodeURIComponent(at.owner)}/${encodeURIComponent(at.repository)}/tarball/${source.commitSha}`, { headers, redirect: 'follow', signal: AbortSignal.timeout(TARBALL_REQUEST_TIMEOUT_MS) }, ).catch((thrown: unknown) => { // A refused connection or the request timeout lands here. The build has @@ -591,6 +683,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi async function loadFromBlobs( source: ResolvedSource, + at: RepositoryName, entries: Array, ): Promise { const files: ArtifactSourceFile[] = [] @@ -599,7 +692,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const loaded = await Promise.all(batch.map(async (entry): Promise => { const response = await requestJson( 'blob', - `/repos/${encodeURIComponent(source.owner)}/${encodeURIComponent(source.repository)}/git/blobs/${entry.sha}`, + `/repos/${encodeURIComponent(at.owner)}/${encodeURIComponent(at.repository)}/git/blobs/${entry.sha}`, blobResponseSchema, ) if (response._tag !== 'ok') @@ -670,6 +763,18 @@ function artifactFileMode(treeMode: string): 420 | 493 { return treeMode === '100755' ? 493 : 420 } +/** The folder of every registry SKILL.md among tree entries, with `.` for the root. */ +function skillFolders(entries: TreeEntry[]): string[] { + return entries + .filter(entry => entry.type === 'blob' && isRegistrySkillPath(entry.path)) + .map(entry => entry.path === 'SKILL.md' ? '.' : entry.path.slice(0, -'/SKILL.md'.length)) +} + +/** GitHub owner and Repository names match without case. */ +function sameGithubName(left: string, right: string): boolean { + return left.toLowerCase() === right.toLowerCase() +} + function normalizeRequestedSkillPath(input: string): string | null { const path = input.normalize('NFC') if (path === '.') @@ -789,10 +894,17 @@ function comparePath(left: string, right: string): number { return left < right ? -1 : left > right ? 1 : 0 } -async function readBoundedJson(response: Response, maximumBytes: number): Promise { +async function readBoundedJson( + response: Response, + maximumBytes: number, +): Promise<{ _tag: 'json', value: unknown } | { _tag: 'too-large' }> { const declared = Number(response.headers.get('content-length')) - if (Number.isFinite(declared) && declared > maximumBytes) - throw new Error('GitHub response exceeded the byte limit') + if (Number.isFinite(declared) && declared > maximumBytes) { + await response.body?.cancel().catch(() => { + // The declared size already decided the outcome. A body that will not close changes nothing. + }) + return { _tag: 'too-large' } + } if (!response.body) throw new Error('GitHub returned an empty response') const reader = response.body.getReader() @@ -805,7 +917,7 @@ async function readBoundedJson(response: Response, maximumBytes: number): Promis size += next.value.byteLength if (size > maximumBytes) { await reader.cancel('response too large') - throw new Error('GitHub response exceeded the byte limit') + return { _tag: 'too-large' } } chunks.push(next.value) } @@ -815,11 +927,26 @@ async function readBoundedJson(response: Response, maximumBytes: number): Promis bytes.set(chunk, offset) offset += chunk.byteLength } - return JSON.parse(new TextDecoder().decode(bytes)) as unknown + return { _tag: 'json', value: JSON.parse(new TextDecoder().decode(bytes)) as unknown } +} + +/** + * The API URL a moved Repository answers with, or null for any other redirect. + * + * Only `/repositories/` on the API origin passes, so the token never + * leaves api.github.com and no redirect reads an arbitrary path. + */ +function movedRepositoryUrl(location: string | null): string | null { + if (!location) + return null + const url = URL.parse(location, GITHUB_API) + if (!url || url.origin !== GITHUB_API || url.username || url.password) + return null + return /^\/repositories\/[1-9]\d*(?:\/|$)/.test(url.pathname) ? url.href : null } function sourceReadRejection( - reason: 'not-found' | 'access-denied' | 'identity-mismatch', + reason: 'not-found' | 'access-denied' | 'identity-mismatch' | 'too-large', visibility: 'public' | 'private', ): SourceRejection { if (visibility === 'private' && (reason === 'not-found' || reason === 'access-denied')) @@ -828,6 +955,8 @@ function sourceReadRejection( return reject('SOURCE_NOT_FOUND', 'The Git object was not found.', []) if (reason === 'access-denied') return reject('SOURCE_ACCESS_DENIED', 'GitHub denied access to the Git object.', []) + if (reason === 'too-large') + return reject('INVALID_SOURCE', 'The GitHub response is larger than the read limit.', []) return reject('INVALID_SOURCE', 'GitHub returned another Git object identity.', []) } @@ -836,8 +965,17 @@ function sourceLimitRejection(summary: string, findings: string[] = []): SourceR } type FailedReadOutcome - = { _tag: 'not-found' | 'access-denied' | 'identity-mismatch' } + = { _tag: 'not-found' | 'access-denied' | 'identity-mismatch' | 'too-large' } + | { _tag: 'rate-limited', resetAt: number | null } + +/** One GitHub JSON read that reached an answer. A failed path to GitHub throws instead. */ +type ReadOutcome + = | { _tag: 'ok', value: T } + | { _tag: 'not-found' } + | { _tag: 'access-denied' } | { _tag: 'rate-limited', resetAt: number | null } + /** The body passed `MAX_GITHUB_JSON_BYTES`. A retry gets the same body. */ + | { _tag: 'too-large' } function epochHeader(headers: Headers, name: string): number | null { const raw = headers.get(name) diff --git a/test/unit/artifact-github-source-scale.test.ts b/test/unit/artifact-github-source-scale.test.ts new file mode 100644 index 00000000..ff336be7 --- /dev/null +++ b/test/unit/artifact-github-source-scale.test.ts @@ -0,0 +1,259 @@ +import { createHash } from 'node:crypto' +import { describe, expect, it, vi } from 'vitest' +import { createPublicGithubSourceClient } from '../../layers/artifact-delivery/server/utils/github-source' + +const commitSha = '0123456789abcdef0123456789abcdef01234567' +const rootTreeSha = '89abcdef0123456789abcdef0123456789abcdef' +const docsTreeSha = '4444444444444444444444444444444444444444' +const skillsTreeSha = '1111111111111111111111111111111111111111' +const skillTreeSha = '2222222222222222222222222222222222222222' +const skillText = '---\nname: demo\ndescription: Use this Skill for demo work.\n---\n' +const skillBlobSha = gitBlobSha(skillText) +const API = 'https://api.github.com' + +describe('a Repository tree larger than one GitHub response', () => { + // posthog/posthog, n8n-io/n8n, vercel/next.js and openshift/hypershift + // answer a recursive tree read with more than 8 MiB. + it('finds a named Skill when the recursive tree exceeds the read limit', async () => { + const fetchMock = splitRepositoryFetch({ root: 'oversized' }) + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.resolve(namedRequest('demo')) + + expect(result).toMatchObject({ _tag: 'resolved', source: { skillPath: 'skills/demo' } }) + }) + + it('finds a named Skill when GitHub truncates the recursive tree', async () => { + const fetchMock = splitRepositoryFetch({ root: 'truncated' }) + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.resolve(namedRequest('demo')) + + expect(result).toMatchObject({ _tag: 'resolved', source: { skillPath: 'skills/demo' } }) + }) + + it('asks for a path when one directory is too large to list', async () => { + const fetchMock = splitRepositoryFetch({ root: 'oversized', rootLevel: 'truncated' }) + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.resolve(namedRequest('demo')) + + expect(result).toMatchObject({ + _tag: 'rejected', + code: 'INVALID_SOURCE', + summary: 'The Repository is too large to find a Skill by name. Name the Skill by its path.', + }) + }) + + it('loads a small Skill folder whose recursive listing exceeds the read limit', async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url === `${API}/repos/skilld-dev/skills`) + return json(repository('skilld-dev', 'skills')) + if (url.endsWith(`/git/trees/${rootTreeSha}`)) + return json({ sha: rootTreeSha, tree: [tree('skills', skillsTreeSha)] }) + if (url.endsWith(`/git/trees/${skillsTreeSha}`)) + return json({ sha: skillsTreeSha, tree: [tree('demo', skillTreeSha)] }) + if (url.endsWith(`/git/trees/${skillTreeSha}?recursive=1`)) + return oversized() + if (url.endsWith(`/git/trees/${skillTreeSha}`)) + return json({ sha: skillTreeSha, tree: [blob('SKILL.md', skillBlobSha, skillText.length)] }) + if (url.endsWith(`/git/blobs/${skillBlobSha}`)) + return json(blobResponse(skillText, skillBlobSha)) + return json({}, 404) + }) + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.load(resolvedSource('skilld-dev', 'skills')) + + expect(result._tag).toBe('loaded') + }) +}) + +describe('a moved Repository', () => { + // facebook/react moved to react/react. GitHub answers the old name with a + // 301 to /repositories/. + it('resolves under the name the request used', async () => { + const fetchMock = movedRepositoryFetch() + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.resolve({ + provider: 'github', + owner: 'facebook', + repository: 'react', + selector: { type: 'path', path: 'skills/demo' }, + ref: { type: 'commit', value: commitSha }, + }) + + expect(result).toEqual({ + _tag: 'resolved', + source: { + provider: 'github', + repositoryId: 10270250, + owner: 'facebook', + repository: 'react', + visibility: 'public', + commitSha, + treeSha: rootTreeSha, + skillPath: 'skills/demo', + }, + }) + }) + + it('loads every Git object from the new name', async () => { + const fetchMock = movedRepositoryFetch() + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.load(resolvedSource('facebook', 'react', 10270250)) + + expect(result._tag).toBe('loaded') + const reads = fetchMock.mock.calls.map(call => String(call[0])) + expect(reads.filter(url => url.startsWith(`${API}/repos/facebook/react`))).toEqual([`${API}/repos/facebook/react`]) + }) + + it('rejects a moved name that now leads to another Repository', async () => { + const fetchMock = movedRepositoryFetch() + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.load(resolvedSource('facebook', 'react', 999)) + + expect(result).toMatchObject({ _tag: 'rejected', code: 'INVALID_SOURCE' }) + }) + + it('never follows a redirect off the GitHub API', async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url === `${API}/repos/facebook/react`) + return new Response(null, { status: 301, headers: { location: 'https://example.com/repositories/10270250' } }) + return json(repository('react', 'react', 10270250)) + }) + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + await expect(client.resolve({ + provider: 'github', + owner: 'facebook', + repository: 'react', + selector: { type: 'path', path: 'skills/demo' }, + ref: { type: 'commit', value: commitSha }, + })).rejects.toThrow('GitHub redirected 301') + expect(fetchMock.mock.calls.map(call => String(call[0]))).toEqual([`${API}/repos/facebook/react`]) + }) +}) + +/** + * A Repository whose root is `docs/` and `skills/demo/SKILL.md`. The root + * recursive read is the large one; each top-level folder lists in one read. + */ +function splitRepositoryFetch(options: { root: 'oversized' | 'truncated', rootLevel?: 'truncated' }) { + return vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url === `${API}/repos/skilld-dev/skills`) + return json(repository('skilld-dev', 'skills')) + if (url.endsWith('/git/ref/heads/main')) + return json({ ref: 'refs/heads/main', object: { type: 'commit', sha: commitSha } }) + if (url.endsWith(`/commits/${commitSha}`)) + return json({ sha: commitSha, commit: { tree: { sha: rootTreeSha } } }) + if (url.endsWith(`/git/trees/${rootTreeSha}?recursive=1`)) { + return options.root === 'oversized' + ? oversized() + : json({ sha: rootTreeSha, tree: [tree('docs', docsTreeSha)], truncated: true }) + } + if (url.endsWith(`/git/trees/${rootTreeSha}`)) { + return json({ + sha: rootTreeSha, + tree: [blob('README.md', skillBlobSha, 10), tree('docs', docsTreeSha), tree('skills', skillsTreeSha)], + truncated: options.rootLevel === 'truncated', + }) + } + if (url.endsWith(`/git/trees/${docsTreeSha}?recursive=1`)) + return json({ sha: docsTreeSha, tree: [blob('guide.md', skillBlobSha, 10)], truncated: false }) + if (url.endsWith(`/git/trees/${skillsTreeSha}?recursive=1`)) { + return json({ + sha: skillsTreeSha, + tree: [tree('demo', skillTreeSha), blob('demo/SKILL.md', skillBlobSha, skillText.length)], + truncated: false, + }) + } + return json({}, 404) + }) +} + +/** facebook/react answers 301; react/react serves every Git object. */ +function movedRepositoryFetch() { + return vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url.startsWith(`${API}/repos/facebook/react`)) + return new Response(null, { status: 301, headers: { location: `${API}/repositories/10270250` } }) + if (url === `${API}/repositories/10270250`) + return json(repository('react', 'react', 10270250)) + const moved = `${API}/repos/react/react` + if (url === `${moved}/commits/${commitSha}`) + return json({ sha: commitSha, commit: { tree: { sha: rootTreeSha } } }) + if (url === `${moved}/git/trees/${rootTreeSha}`) + return json({ sha: rootTreeSha, tree: [tree('skills', skillsTreeSha)] }) + if (url === `${moved}/git/trees/${skillsTreeSha}`) + return json({ sha: skillsTreeSha, tree: [tree('demo', skillTreeSha)] }) + if (url === `${moved}/git/trees/${skillTreeSha}?recursive=1`) + return json({ sha: skillTreeSha, tree: [blob('SKILL.md', skillBlobSha, skillText.length)], truncated: false }) + if (url === `${moved}/git/blobs/${skillBlobSha}`) + return json(blobResponse(skillText, skillBlobSha)) + return json({}, 404) + }) +} + +function namedRequest(name: string) { + return { + provider: 'github' as const, + owner: 'skilld-dev', + repository: 'skills', + selector: { type: 'named-skill' as const, name }, + } +} + +function resolvedSource(owner: string, name: string, repositoryId = 123) { + return { + provider: 'github' as const, + repositoryId, + owner, + repository: name, + visibility: 'public' as const, + commitSha, + treeSha: rootTreeSha, + skillPath: 'skills/demo', + } +} + +function repository(owner: string, name: string, id = 123) { + return { id, name, owner: { login: owner }, private: false, default_branch: 'main' } +} + +/** A tree response one byte past the 8 MiB read limit. */ +function oversized(): Response { + return new Response(`{"pad":"${'x'.repeat(8 * 1024 * 1024)}"}`, { + status: 200, + headers: { 'content-type': 'application/json' }, + }) +} + +function tree(path: string, sha: string) { + return { path, mode: '040000', type: 'tree', sha } +} + +function blob(path: string, sha: string, size: number) { + return { path, mode: '100644', type: 'blob', sha, size } +} + +function blobResponse(value: string, sha: string) { + return { sha, size: value.length, encoding: 'base64', content: btoa(value) } +} + +function gitBlobSha(value: string): string { + return createHash('sha1').update(`blob ${value.length}\0${value}`).digest('hex') +} + +function json(value: unknown, status = 200): Response { + return new Response(JSON.stringify(value), { + status, + headers: { 'content-type': 'application/json' }, + }) +} diff --git a/test/unit/artifact-named-skill.test.ts b/test/unit/artifact-named-skill.test.ts index aaf0c696..982c8fa3 100644 --- a/test/unit/artifact-named-skill.test.ts +++ b/test/unit/artifact-named-skill.test.ts @@ -57,19 +57,9 @@ describe('named Skill resolution', () => { expect(result).toMatchObject({ _tag: 'resolved', source: { skillPath: '.claude/skills/demo' } }) }) - - it('asks for a path when GitHub cannot list the whole Repository', async () => { - const result = await resolveNamed('demo', [blob('skills/demo/SKILL.md')], true) - - expect(result).toMatchObject({ - _tag: 'rejected', - code: 'INVALID_SOURCE', - summary: 'The Repository is too large to find a Skill by name. Name the Skill by its path.', - }) - }) }) -async function resolveNamed(name: string, entries: object[], truncated = false) { +async function resolveNamed(name: string, entries: object[]) { const fetchMock = vi.fn(async (input: RequestInfo | URL) => { const url = String(input) if (url.endsWith('/repos/skilld-dev/skills')) @@ -79,7 +69,7 @@ async function resolveNamed(name: string, entries: object[], truncated = false) if (url.endsWith(`/commits/${commitSha}`)) return json({ sha: commitSha, commit: { tree: { sha: rootTreeSha } } }) if (url.endsWith(`/git/trees/${rootTreeSha}?recursive=1`)) - return json({ sha: rootTreeSha, tree: entries, truncated }) + return json({ sha: rootTreeSha, tree: entries, truncated: false }) return json({}, 404) }) const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) From 2ca97b3a1a73b990370de144fafb6c62723ecb35 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:56:16 +1100 Subject: [PATCH 2/2] fix(artifact-delivery): cap the split search by name at 64 reads Anyone can send a name, and every tree read comes out of the site's GitHub quota. The split walk now stops as soon as the folders it knows about cannot fit the budget. posthog/posthog needs more than 128 reads, so it used to spend 128 before it asked for a path; it now stops after about 40. n8n-io/n8n, vercel/next.js and openshift/hypershift need 27 to 45 and still resolve. --- .../server/utils/github-source.ts | 20 +++++++++++---- .../unit/artifact-github-source-scale.test.ts | 25 +++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/layers/artifact-delivery/server/utils/github-source.ts b/layers/artifact-delivery/server/utils/github-source.ts index eafa41e4..94b3e97d 100644 --- a/layers/artifact-delivery/server/utils/github-source.ts +++ b/layers/artifact-delivery/server/utils/github-source.ts @@ -22,6 +22,13 @@ const GITHUB_API_VERSION = '2026-03-10' const MAX_GITHUB_JSON_BYTES = 8 * 1024 * 1024 const MAX_TREE_ENTRIES = 2000 const MAX_TREE_REQUESTS = 128 +/** + * The tree reads one search by name may spend. Every read comes out of the + * site's GitHub quota, and anyone can send a name. Measured 2026-10-07 on the + * split walk: n8n-io/n8n needs 27, vercel/next.js 37, openshift/hypershift 45. + * posthog/posthog needs more than 128 and stops after about 40. + */ +const MAX_NAME_SEARCH_TREE_READS = 64 // One tree read can hold 8 MiB of JSON and the tree parsed from it. Two at a // time keep a split walk well inside a Worker's 128 MiB. const TREE_WALK_CONCURRENCY = 2 @@ -431,8 +438,11 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi * read limit: posthog/posthog, n8n-io/n8n, vercel/next.js. Each failed the * search outright, whatever the Skill size. A tree that one response cannot * hold now splits: one read lists its own level, and each folder in it gets - * its own recursive read, which can split again. `MAX_TREE_REQUESTS` bounds - * the walk. + * its own recursive read, which can split again. + * + * `MAX_NAME_SEARCH_TREE_READS` bounds the walk. Each folder still to visit + * costs at least one read, so the walk stops as soon as the folders it + * knows about cannot fit, before it spends reads it cannot finish. */ const listSkillFolders = async ( owner: string, @@ -471,10 +481,10 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const folders: string[] = [] const pending = [{ sha: rootTreeSha, prefix: '' }] while (pending.length > 0) { - // A split folder costs two reads, so a wave can need twice its size. - const wave = pending.splice(0, TREE_WALK_CONCURRENCY) - if (requests + wave.length * 2 > MAX_TREE_REQUESTS) + // A split folder costs a second read, so the wave adds its own size once more. + if (requests + pending.length + Math.min(pending.length, TREE_WALK_CONCURRENCY) > MAX_NAME_SEARCH_TREE_READS) return { _tag: 'unlistable' } + const wave = pending.splice(0, TREE_WALK_CONCURRENCY) const visited = await Promise.all(wave.map(visit)) for (const result of visited) { if (result._tag !== 'listed') diff --git a/test/unit/artifact-github-source-scale.test.ts b/test/unit/artifact-github-source-scale.test.ts index ff336be7..fd5770e2 100644 --- a/test/unit/artifact-github-source-scale.test.ts +++ b/test/unit/artifact-github-source-scale.test.ts @@ -45,6 +45,31 @@ describe('a Repository tree larger than one GitHub response', () => { }) }) + it('stops before it spends reads the search cannot finish', async () => { + // posthog/posthog splits into more folders than one search may read. + const folders = Array.from({ length: 80 }, (_, index) => tree(`product-${index}`, index.toString(16).padStart(40, '5'))) + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url === `${API}/repos/skilld-dev/skills`) + return json(repository('skilld-dev', 'skills')) + if (url.endsWith('/git/ref/heads/main')) + return json({ ref: 'refs/heads/main', object: { type: 'commit', sha: commitSha } }) + if (url.endsWith(`/commits/${commitSha}`)) + return json({ sha: commitSha, commit: { tree: { sha: rootTreeSha } } }) + if (url.endsWith(`/git/trees/${rootTreeSha}?recursive=1`)) + return oversized() + if (url.endsWith(`/git/trees/${rootTreeSha}`)) + return json({ sha: rootTreeSha, tree: folders }) + return json({}, 404) + }) + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + + const result = await client.resolve(namedRequest('demo')) + + expect(result).toMatchObject({ _tag: 'rejected', code: 'INVALID_SOURCE' }) + expect(fetchMock.mock.calls.filter(call => String(call[0]).includes('/git/trees/'))).toHaveLength(2) + }) + it('loads a small Skill folder whose recursive listing exceeds the read limit', async () => { const fetchMock = vi.fn(async (input: RequestInfo | URL) => { const url = String(input)