From adb296c4628564e779a603f74d695f74262a2aa9 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:42:40 +1100 Subject: [PATCH 1/8] fix(artifact-delivery): keep healthy runs inside their limits A run polls its Resolution once a second, so one slow build spent the whole guest allowance of 60 requests a minute and failed with RATE_LIMITED. Polls now draw from their own allowance (ADR-0012). GitHub answers a secondary rate limit with 403 or 429 and Retry-After. A 403 failed the run as SOURCE_ACCESS_DENIED, and a 429 threw into the queue retry ladder. Both now fail as RATE_LIMITED with the named wait, and a rate-limited tarball no longer falls back to one read per blob. Every retryable failure now names retryAfterSeconds. The first queue retry waits 5 seconds instead of 60, inside the CLI's 60 second window. --- docs/adr/0012-resolution-poll-allowance.md | 32 ++++ docs/arch/README.md | 2 +- .../artifact-delivery/server/utils/build.ts | 15 +- .../server/utils/github-source.ts | 55 +++++- .../artifact-delivery/server/utils/queue.ts | 16 +- shared/server/api-rate-limit.ts | 22 ++- test/unit/api-rate-limit.test.ts | 36 +++- test/unit/artifact-rate-limit.test.ts | 175 +++++++++++++++++- worker-configuration.d.ts | 3 +- wrangler.jsonc | 4 + 10 files changed, 341 insertions(+), 19 deletions(-) create mode 100644 docs/adr/0012-resolution-poll-allowance.md diff --git a/docs/adr/0012-resolution-poll-allowance.md b/docs/adr/0012-resolution-poll-allowance.md new file mode 100644 index 00000000..6ef929e1 --- /dev/null +++ b/docs/adr/0012-resolution-poll-allowance.md @@ -0,0 +1,32 @@ +# ADR-0012: Resolution poll allowance + +Date: 2026-10-07 + +Amends [ADR-0008](0008-api-rate-limits.md). + +## Context + +ADR-0008 gives every v1 operation one shared allowance. +A guest gets 60 requests per 60 seconds, keyed by `CF-Connecting-IP`. + +`skilld run` polls `GET /api/v1/resolutions/{id}` at the rate the server names in `pollAfterMs`. +The server names 1000 ms. The CLI waits up to 60 seconds for a Resolution. +One build that takes a minute therefore costs about 60 polls. +That spends the whole guest allowance, so the run fails with `RATE_LIMITED`. +A second run from the same network fails the same way. + +## Decision + +A poll of one Resolution draws from a separate binding, `API_RESOLUTION_POLL_RATE_LIMIT`. +It allows 600 requests per 60 seconds. +It uses the ADR-0008 key: the verified account ID, or the guest IP. +Ten runs from one network can poll through a one-minute build. + +A poll is a `GET` of `/api/v1/resolutions/{id}`. +Every other v1 request keeps the ADR-0008 allowance, including the `POST` that starts a Resolution. +An exhausted poll allowance answers like an exhausted shared one: HTTP 429, `RATE_LIMITED`, and `Retry-After: 60`. + +## Consequences + +A poll is one indexed D1 read. 600 polls per minute is 10 reads per second for one key. +The starting `POST` stays in the shared allowance, so the number of builds one network can start does not change. diff --git a/docs/arch/README.md b/docs/arch/README.md index 8d5ce47d..96bc3b7d 100644 --- a/docs/arch/README.md +++ b/docs/arch/README.md @@ -45,7 +45,7 @@ descriptor (skilld-dev/skilld: packages/sdk/src/contract) └─ parity test test/unit/api-v1-parity.test.ts: one route file per operation ``` -- **Operation**: one `namespace.verb` entry in the contract, such as `skills.get`. Its ID never changes. Public handlers never receive caller identity. Rate-limit middleware verifies credentials only to select an allowance. Account operations reuse that identity. V1 responses bypass Workers Cache so every network request reaches the limiter. See [ADR-0008](../adr/0008-api-rate-limits.md). +- **Operation**: one `namespace.verb` entry in the contract, such as `skills.get`. Its ID never changes. Public handlers never receive caller identity. Rate-limit middleware verifies credentials only to select an allowance. Account operations reuse that identity. V1 responses bypass Workers Cache so every network request reaches the limiter. See [ADR-0008](../adr/0008-api-rate-limits.md). Resolution polls draw from their own allowance: [ADR-0012](../adr/0012-resolution-poll-allowance.md). - **defineApiOperation**: the shape of a `/api/v1` route: `{ operation, handler }`. The descriptor parses the input, checks the credential, and sets the status and cache headers. The handler only loads data. The answer passes the strict producer schema, so an unnamed field fails closed with a 500. - **Route file**: lives in the layer that owns the data, at the operation's own path: `GET /api/v1/skills/{owner}/{repository}/{name}` is `layers/registry/server/api/v1/skills/[owner]/[repository]/[name].get.ts`. It calls that layer's utilities, or reads the internal route in process. It never duplicates SQL. - **v1 presenter**: a pure function in `layers//server/presenters/-v1.ts`, typed `OperationResult`. diff --git a/layers/artifact-delivery/server/utils/build.ts b/layers/artifact-delivery/server/utils/build.ts index d07a4672..daea8253 100644 --- a/layers/artifact-delivery/server/utils/build.ts +++ b/layers/artifact-delivery/server/utils/build.ts @@ -662,6 +662,13 @@ async function failWithRejection( ) } +/** + * The wait a retryable failure names when its cause named none. GitHub asks + * for at least a minute after a secondary rate limit, and the build queue + * gives up only after several minutes of failed attempts. + */ +export const DEFAULT_RETRY_AFTER_SECONDS = 60 + export async function failResolution( dependencies: Pick, row: NonNullable>>, @@ -675,9 +682,13 @@ export async function failResolution( // The upstream reset header is an absolute epoch; error_retry_after means a // relative delay in seconds, like HTTP Retry-After. Store the delay a reader // can add to now, and never let a clock skew store zero or a negative one. - const errorRetryAfter = retryAtEpochSeconds === undefined + // A retryable failure whose cause named no time still names one, so the + // CLI never prints "may be retried" without saying when. + const errorRetryAfter = !retryable ? undefined - : Math.max(1, retryAtEpochSeconds - now) + : retryAtEpochSeconds === undefined + ? DEFAULT_RETRY_AFTER_SECONDS + : Math.max(1, retryAtEpochSeconds - now) const advanced = await transitionResolution(dependencies.db, row, 'failed', { errorCode: code, errorRetryable: retryable, diff --git a/layers/artifact-delivery/server/utils/github-source.ts b/layers/artifact-delivery/server/utils/github-source.ts index 6909dcad..2815cda9 100644 --- a/layers/artifact-delivery/server/utils/github-source.ts +++ b/layers/artifact-delivery/server/utils/github-source.ts @@ -156,6 +156,8 @@ interface GithubClientOptions { onReadFailure?: (failure: GithubReadFailure) => void sleep?: (milliseconds: number) => Promise random?: () => number + /** Unix seconds. A Retry-After delay becomes an absolute retry time against it. */ + now?: () => number } export function createPublicGithubSourceClient(options: GithubClientOptions): PublicGithubSourceClient { @@ -164,6 +166,7 @@ export function createPublicGithubSourceClient(options: GithubClientOptions): Pu export function createGithubSourceClient(options: GithubClientOptions): PublicGithubSourceClient { const expectedVisibility = options.visibility ?? 'public' + const now = options.now ?? (() => Math.floor(Date.now() / 1000)) const readRejection = (outcome: FailedReadOutcome) => outcome._tag === 'rate-limited' ? rateLimitRejection(outcome.resetAt) @@ -204,15 +207,15 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const response = fetched.response if (response.status === 404) return { _tag: 'settled', value: { _tag: 'not-found' } } - if (response.status === 401 || response.status === 403) { - // A spent quota is a fact about this minute, not about the Repository. - // It used to throw, which wrote nothing: the Resolution sat in its - // current state through the 60, 120, 240, 480 second delivery ladder and - // then failed as SERVICE_UNAVAILABLE with no reason on it. - if (response.status === 403 && response.headers.get('x-ratelimit-remaining') === '0') - return { _tag: 'settled', value: { _tag: 'rate-limited', resetAt: epochHeader(response.headers, 'x-ratelimit-reset') } } + // A spent quota is a fact about this minute, not about the Repository. + // It used to throw, which wrote nothing: the Resolution sat in its + // current state through the delivery retry ladder and then failed as + // SERVICE_UNAVAILABLE with no reason on it. + const limited = githubRateLimit(response, now()) + if (limited) + return { _tag: 'settled', value: limited } + if (response.status === 401 || response.status === 403) return { _tag: 'settled', value: { _tag: 'access-denied' } } - } if (!response.ok) { await response.body?.cancel().catch(() => { // The status already decided the outcome. A body that will not close changes nothing. @@ -531,6 +534,10 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const extracted = await loadFromTarball(source, selected.entries) if (extracted._tag === 'extracted') return { _tag: 'loaded', value: { source, files: extracted.files } } + // The blobs API reads one file per request against the same limit. + // Falling back would spend the most quota exactly when none is left. + if (extracted._tag === 'rate-limited') + return rateLimitRejection(extracted.resetAt) // The tarball is an optimisation, never an authority. Anything it got // wrong, including a file `.gitattributes export-ignore` removed from // the archive, falls through to the blobs API, which serves every blob @@ -551,7 +558,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi async function loadFromTarball( source: ResolvedSource, entries: Array, - ): Promise { + ): Promise { const headers = new Headers({ 'Accept': 'application/vnd.github+json', 'User-Agent': 'skilld.dev', @@ -572,6 +579,11 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi }) if ('_tag' in response) return { _tag: 'unusable', reason: 'unavailable', findings: [response.message] } + const limited = githubRateLimit(response, now()) + if (limited) { + await response.body?.cancel() + return limited + } if (!response.ok || !response.body) { await response.body?.cancel() return { _tag: 'unusable', reason: 'unavailable', findings: [`GitHub returned ${response.status}`] } @@ -847,6 +859,31 @@ function epochHeader(headers: Headers, name: string): number | null { return Number.isSafeInteger(value) && value > 0 ? value : null } +interface GithubRateLimit { + _tag: 'rate-limited' + /** Unix seconds GitHub allows the next request, when it said so. */ + resetAt: number | null +} + +/** + * GitHub's rate limit answer, or null for any other response. + * + * GitHub answers a spent primary quota with 403 or 429 and no remaining + * requests, and a secondary limit with 403 or 429 and Retry-After. A 429 is + * always a limit. A 403 with neither header denies access. + * https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api + */ +export function githubRateLimit(response: Pick, now: number): GithubRateLimit | null { + if (response.status !== 403 && response.status !== 429) + return null + const retryAfter = epochHeader(response.headers, 'retry-after') + if (retryAfter !== null) + return { _tag: 'rate-limited', resetAt: now + retryAfter } + if (response.headers.get('x-ratelimit-remaining') === '0') + return { _tag: 'rate-limited', resetAt: epochHeader(response.headers, 'x-ratelimit-reset') } + return response.status === 429 ? { _tag: 'rate-limited', resetAt: null } : null +} + function rateLimitRejection(resetAt: number | null): SourceRejection { const rejection = reject('RATE_LIMITED', 'GitHub refused the read: its rate limit is spent.', []) return resetAt === null ? rejection : { ...rejection, retryAfterSeconds: resetAt } diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 7c2466f0..185de39d 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -25,6 +25,17 @@ const artifactBuildMessageSchema = z.object({ resolutionId: z.string().uuid(), }).strict() +/** + * The wait before each retry of a build attempt that threw. A fifth failed + * attempt fails the Resolution as SERVICE_UNAVAILABLE. + * + * The CLI waits 60 seconds for a Resolution, so the first retry comes within + * that window: one transient GitHub or signer error no longer fails the run. + * It used to wait 60 seconds. The later waits still ride out a long GitHub + * network fault, such as the one on 2026-09-30. + */ +export const ARTIFACT_BUILD_RETRY_DELAYS_SECONDS: readonly number[] = [5, 30, 120, 480] + export async function enqueueArtifactBuild(env: Cloudflare.Env, resolutionId: string): Promise { await env.ARTIFACT_BUILD_QUEUE.send({ version: 1, resolutionId }) } @@ -59,8 +70,9 @@ export async function consumeArtifactBuildBatch( attempt: message.attempts, error: outcome.error instanceof Error ? outcome.error.message : String(outcome.error), })) - if (message.attempts < 5) { - message.retry({ delaySeconds: Math.min(3600, 60 * 2 ** Math.max(0, message.attempts - 1)) }) + const delaySeconds = ARTIFACT_BUILD_RETRY_DELAYS_SECONDS[message.attempts - 1] + if (delaySeconds !== undefined) { + message.retry({ delaySeconds }) continue } const row = await getResolution(dependencies.db, parsed.data.resolutionId) diff --git a/shared/server/api-rate-limit.ts b/shared/server/api-rate-limit.ts index 72051f19..ebc8b464 100644 --- a/shared/server/api-rate-limit.ts +++ b/shared/server/api-rate-limit.ts @@ -13,7 +13,10 @@ declare module 'h3' { } } -/** One allowance across v1 operations, keyed by verified account or Cloudflare's client IP. */ +/** + * One allowance across v1 operations, keyed by verified account or Cloudflare's client IP. + * Resolution polls draw from a separate allowance under the same key. + */ export function createApiRateLimitHandler(resolveUser: (event: H3Event) => Promise) { return defineEventHandler(async (event) => { const path = event.path.split('?')[0]! @@ -30,7 +33,9 @@ export function createApiRateLimitHandler(resolveUser: (event: H3Event) => Promi const { env, requestId } = event.context.platform // Do not trust X-Forwarded-For. Missing Cloudflare identity shares a conservative bucket. const key = user ? `account:${user.id}` : `guest:${getHeader(event, 'cf-connecting-ip') ?? 'unknown'}` - const limiter = user ? env.API_ACCOUNT_RATE_LIMIT : env.API_GUEST_RATE_LIMIT + const limiter = isResolutionPoll(event.method, path) + ? env.API_RESOLUTION_POLL_RATE_LIMIT + : user ? env.API_ACCOUNT_RATE_LIMIT : env.API_GUEST_RATE_LIMIT const { success } = await limiter.limit({ key }) if (success) return @@ -52,3 +57,16 @@ export function createApiRateLimitHandler(resolveUser: (event: H3Event) => Promi }) }) } + +const RESOLUTION_POLL_PATH = /^\/api\/v1\/resolutions\/[^/]+$/ + +/** + * A read of one Resolution while its build runs. + * + * The server sets the poll rate through `pollAfterMs`, so a poll is not a + * request the caller chose to make. A one-minute build costs sixty polls, + * which used to spend the whole guest allowance (ADR-0012). + */ +function isResolutionPoll(method: string, path: string): boolean { + return method === 'GET' && RESOLUTION_POLL_PATH.test(path) +} diff --git a/test/unit/api-rate-limit.test.ts b/test/unit/api-rate-limit.test.ts index 6741032b..7afa6f3c 100644 --- a/test/unit/api-rate-limit.test.ts +++ b/test/unit/api-rate-limit.test.ts @@ -5,12 +5,14 @@ import { createApiRateLimitHandler } from '../../shared/server/api-rate-limit' const guest = vi.fn() const account = vi.fn() +const poll = vi.fn() const resolveUser = vi.fn() +const RESOLUTION_ID = '0d6f6c2e-5b1a-4c3e-9f1d-2a7b8c9d0e1f' function serve() { const app = createApp() app.use(eventHandler((event) => { - event.context.platform = { requestId: 'request-test', env: { API_GUEST_RATE_LIMIT: { limit: guest }, API_ACCOUNT_RATE_LIMIT: { limit: account } } } as never + event.context.platform = { requestId: 'request-test', env: { API_GUEST_RATE_LIMIT: { limit: guest }, API_ACCOUNT_RATE_LIMIT: { limit: account }, API_RESOLUTION_POLL_RATE_LIMIT: { limit: poll } } } as never })) app.use(createApiRateLimitHandler(resolveUser)) app.use(eventHandler(() => ({ ok: true }))) @@ -22,6 +24,7 @@ describe('api rate limits', () => { beforeEach(() => { guest.mockReset().mockResolvedValue({ success: true }) account.mockReset().mockResolvedValue({ success: true }) + poll.mockReset().mockResolvedValue({ success: true }) resolveUser.mockReset().mockResolvedValue(null) }) @@ -93,4 +96,35 @@ describe('api rate limits', () => { expect(await response.json()).toMatchObject({ code: 'RATE_LIMITED' }) expect(guest).not.toHaveBeenCalled() }) + + // A run polls its Resolution once a second while it builds. Sixty polls used + // to spend the whole guest allowance, so one slow build failed the run with + // RATE_LIMITED, and so did a second run from the same network. + it('counts Resolution polls against their own allowance, not the shared one', async () => { + const fetch = serve() + for (let attempt = 0; attempt < 3; attempt++) + await fetch(`/api/v1/resolutions/${RESOLUTION_ID}`, { 'CF-Connecting-IP': '203.0.113.8' }) + expect(poll.mock.calls).toEqual(Array.from({ length: 3 }, () => [{ key: 'guest:203.0.113.8' }])) + expect(guest).not.toHaveBeenCalled() + }) + + it('keys an account poll by the account', async () => { + resolveUser.mockResolvedValue({ id: 7, login: 'octo' }) + await serve()(`/api/v1/resolutions/${RESOLUTION_ID}`, { Authorization: 'Bearer valid' }) + expect(poll).toHaveBeenCalledWith({ key: 'account:7' }) + expect(account).not.toHaveBeenCalled() + }) + + it('still counts the request that starts a Resolution against the shared allowance', async () => { + await serve()('/api/v1/resolutions', { 'CF-Connecting-IP': '203.0.113.8' }, 'POST') + expect(guest).toHaveBeenCalledWith({ key: 'guest:203.0.113.8' }) + expect(poll).not.toHaveBeenCalled() + }) + + it('limits polls once their own allowance is spent', async () => { + poll.mockResolvedValue({ success: false }) + const response = await serve()(`/api/v1/resolutions/${RESOLUTION_ID}`, { 'CF-Connecting-IP': '203.0.113.8' }) + expect(response.status).toBe(429) + expect(await response.json()).toMatchObject({ code: 'RATE_LIMITED' }) + }) }) diff --git a/test/unit/artifact-rate-limit.test.ts b/test/unit/artifact-rate-limit.test.ts index 824295be..133cec2e 100644 --- a/test/unit/artifact-rate-limit.test.ts +++ b/test/unit/artifact-rate-limit.test.ts @@ -1,6 +1,7 @@ import type { SourceRequest } from '../../layers/artifact-delivery/server/schemas/contracts' import type { ArtifactSigner } from '../../layers/artifact-delivery/server/utils/attestation' import type { ArtifactBuildDependencies } from '../../layers/artifact-delivery/server/utils/build' +import type { PublicGithubSourceClient } from '../../layers/artifact-delivery/server/utils/github-source' import type { TrustedRoot } from '../../layers/artifact-delivery/server/utils/trusted-root' import { describe, expect, it, vi } from 'vitest' import { processArtifactBuild } from '../../layers/artifact-delivery/server/utils/build' @@ -8,7 +9,8 @@ import { createPublicGithubSourceClient, isRetryableProblem, } from '../../layers/artifact-delivery/server/utils/github-source' -import { createResolution, resolutionRequestIdentity } from '../../layers/artifact-delivery/server/utils/state' +import { ARTIFACT_BUILD_QUEUE_NAME, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' +import { createResolution, getResolution, presentResolution, resolutionRequestIdentity } from '../../layers/artifact-delivery/server/utils/state' import { createSqliteD1 } from './helpers/d1-sqlite' const commitSha = '0123456789abcdef0123456789abcdef01234567' @@ -123,6 +125,97 @@ describe('gitHub rate limits as values', () => { }) }) +// GitHub answers a secondary rate limit with 403 or 429 and a Retry-After +// header, and a spent primary quota with 403 or 429 and no remaining requests. +// A 403 with Retry-After used to fail the run as SOURCE_ACCESS_DENIED, which +// nobody retries. A 429 used to throw, so the build sat through the queue +// retry ladder and spent quota on every attempt. +describe('gitHub secondary rate limits as values', () => { + const request: SourceRequest = { + provider: 'github', + owner: 'skilld-dev', + repository: 'skills', + selector: { type: 'path', path: 'skills/demo' }, + } + + it.each([403, 429])('rejects a %i with Retry-After as RATE_LIMITED after the named delay', async (status) => { + const client = createPublicGithubSourceClient({ + fetch: vi.fn(async () => new Response('{}', { + status, + headers: { 'retry-after': '30', 'x-ratelimit-remaining': '4321', 'content-type': 'application/json' }, + })) as unknown as typeof fetch, + now: () => NOW, + }) + + expect(await client.resolve(request)).toMatchObject({ + _tag: 'rejected', + code: 'RATE_LIMITED', + retryAfterSeconds: NOW + 30, + }) + }) + + it('rejects a 429 spent quota with its reset time', async () => { + const client = createPublicGithubSourceClient({ + fetch: vi.fn(async () => new Response('{}', { + status: 429, + headers: { 'x-ratelimit-remaining': '0', 'x-ratelimit-reset': String(RESET_AT), 'content-type': 'application/json' }, + })) as unknown as typeof fetch, + }) + + expect(await client.resolve(request)).toMatchObject({ + _tag: 'rejected', + code: 'RATE_LIMITED', + retryAfterSeconds: RESET_AT, + }) + }) + + it('stops at a rate-limited tarball instead of reading every blob', async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url.includes('/tarball/')) { + return new Response('{}', { + status: 403, + headers: { 'retry-after': '60', 'content-type': 'application/json' }, + }) + } + if (url.includes('/git/blobs/')) + return json({ sha: 'd'.repeat(40), size: 12, encoding: 'base64', content: btoa('# Demo skill') }) + if (url.endsWith('/repos/skilld-dev/skills')) + return json({ id: 123, name: 'skills', owner: { login: 'skilld-dev' }, private: false, default_branch: 'main' }) + if (url.endsWith('/git/trees/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa')) + return json({ sha: 'a'.repeat(40), tree: [{ path: 'skills', mode: '040000', type: 'tree', sha: 'b'.repeat(40) }] }) + if (url.endsWith('/git/trees/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb')) + return json({ sha: 'b'.repeat(40), tree: [{ path: 'demo', mode: '040000', type: 'tree', sha: 'c'.repeat(40) }] }) + if (url.endsWith('/git/trees/cccccccccccccccccccccccccccccccccccccccc?recursive=1')) { + return json({ + sha: 'c'.repeat(40), + truncated: false, + tree: [{ path: 'SKILL.md', mode: '100644', type: 'blob', sha: 'd'.repeat(40), size: 12 }], + }) + } + return json({}, 404) + }) + const client = createPublicGithubSourceClient({ + fetch: fetchMock as unknown as typeof fetch, + now: () => NOW, + }) + + const result = await client.load({ + provider: 'github', + repositoryId: 123, + owner: 'skilld-dev', + repository: 'skills', + visibility: 'public', + commitSha, + treeSha: 'a'.repeat(40), + skillPath: 'skills/demo', + }) + + expect(result).toMatchObject({ _tag: 'rejected', code: 'RATE_LIMITED', retryAfterSeconds: NOW + 60 }) + expect(fetchMock.mock.calls.some(([input]) => String(input).includes('/git/blobs/'))).toBe(false) + }) +}) + describe('which problems a caller may retry', () => { it.each([ ['RATE_LIMITED', true], @@ -170,6 +263,86 @@ describe('a spent quota persists a delay, not the reset epoch', () => { }) }) +describe('every retryable failure names its wait', () => { + it('names a wait when GitHub sent no reset', async () => { + const harness = await requestedResolution('rate-limit-no-reset-0001') + const dependencies = buildDependencies(harness.sqlite.db, createPublicGithubSourceClient({ + fetch: vi.fn(async () => new Response('{}', { status: 429 })) as unknown as typeof fetch, + })) + + await processArtifactBuild(dependencies, harness.id) + + const row = await getResolution(harness.sqlite.db, harness.id) + expect(presentResolution(row!)).toMatchObject({ state: 'failed', code: 'RATE_LIMITED', retryable: true, retryAfterSeconds: 60 }) + harness.sqlite.close() + }) + + it('names a wait when the build queue gives up', async () => { + const harness = await requestedResolution('queue-gives-up-0001') + const message = queueMessage(harness.id, 5) + + await consumeArtifactBuildBatch({} as Cloudflare.Env, queueBatch([message]), () => buildDependencies(harness.sqlite.db, githubDown())) + + const row = await getResolution(harness.sqlite.db, harness.id) + expect(presentResolution(row!)).toMatchObject({ state: 'failed', code: 'SERVICE_UNAVAILABLE', retryable: true, retryAfterSeconds: 60 }) + harness.sqlite.close() + }) +}) + +// The CLI waits 60 seconds for a Resolution. The first queue retry used to +// wait 60 seconds too, so one transient GitHub or signer error failed every +// run that met it, although the retry then built the Artifact. +describe('a failed build attempt retries inside the run deadline', () => { + it('retries the first failed attempt within ten seconds', async () => { + const harness = await requestedResolution('queue-first-retry-0001') + const message = queueMessage(harness.id, 1) + + await consumeArtifactBuildBatch({} as Cloudflare.Env, queueBatch([message]), () => buildDependencies(harness.sqlite.db, githubDown())) + + expect(message.retry).toHaveBeenCalledOnce() + expect(message.retry.mock.calls[0]![0].delaySeconds).toBeLessThanOrEqual(10) + harness.sqlite.close() + }) +}) + +async function requestedResolution(idempotencyKey: string) { + const sqlite = createSqliteD1(ARTIFACT_MIGRATIONS) + const identity = await resolutionRequestIdentity(resolutionRequest, idempotencyKey) + const created = await createResolution(sqlite.db, resolutionRequest, identity, NOW) + if (created._tag === 'idempotency-conflict') + throw new Error('Test Resolution conflicted') + return { sqlite, id: created.row.id } +} + +function buildDependencies(db: D1Database, github: PublicGithubSourceClient): ArtifactBuildDependencies { + return { + db, + github, + bucket: {} as R2Bucket, + signer: {} as ArtifactSigner, + trustedRoot: {} as TrustedRoot, + now: () => NOW, + } +} + +function githubDown(): PublicGithubSourceClient { + const fault = async () => Promise.reject(new Error('The operation was aborted due to timeout')) + return { resolve: vi.fn(fault), load: vi.fn(fault) } +} + +function queueMessage(resolutionId: string, attempts: number) { + return { + body: { version: 1, resolutionId }, + attempts, + ack: vi.fn(), + retry: vi.fn<(options: { delaySeconds: number }) => void>(), + } +} + +function queueBatch(messages: Array>) { + return { queue: ARTIFACT_BUILD_QUEUE_NAME, messages } as unknown as Parameters[1] +} + function rateLimited(): Response { return rateLimitedAt(RESET_AT) } diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 051b5bc3..3fb5ce73 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: 50fee44c1595f78f46cc0523374267df) +// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: a047a1748d71da3be1be11f140a75f7b) // Runtime types generated with workerd@1.20260828.1 2026-08-20 nodejs_compat interface __BaseEnv_Env { KV_CACHE: KVNamespace; @@ -17,6 +17,7 @@ interface __BaseEnv_Env { ARTIFACT_BUILD_QUEUE: Queue; API_GUEST_RATE_LIMIT: RateLimit; API_ACCOUNT_RATE_LIMIT: RateLimit; + API_RESOLUTION_POLL_RATE_LIMIT: RateLimit; TASK_SEARCH_RATE_LIMIT: RateLimit; AI: Ai; CF_VERSION_METADATA: WorkerVersionMetadata; diff --git a/wrangler.jsonc b/wrangler.jsonc index 521662a5..619fb32e 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -13,6 +13,10 @@ "ratelimits": [ { "name": "API_GUEST_RATE_LIMIT", "namespace_id": "2401001", "simple": { "limit": 60, "period": 60 } }, { "name": "API_ACCOUNT_RATE_LIMIT", "namespace_id": "2401002", "simple": { "limit": 600, "period": 60 } }, + // A run polls its Resolution once a second while it builds, at the rate the + // server names. Polls draw from this allowance, keyed like the two above, + // so ten runs from one network can wait out a one-minute build (ADR-0012). + { "name": "API_RESOLUTION_POLL_RATE_LIMIT", "namespace_id": "2401004", "simple": { "limit": 600, "period": 60 } }, // Task search runs a paid model. The longest period a rate limit binding // takes is 60 seconds; the daily ceiling is the spend budget in // runtimeConfig.taskSearch, across every visitor. From e2260b006c6d1945f384a7a415bae8f146d185ed Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:47:31 +1100 Subject: [PATCH 2/8] fix(artifact-delivery): read GitHub with a token kept for runs The hourly registry sync spends GITHUB_TOKEN to zero before each GitHub reset. On 2026-10-06, 13 runs failed RATE_LIMITED in those minutes. GitHub counts a personal token's quota per account, so builds now read with ARTIFACT_GITHUB_TOKEN when it is set, and with GITHUB_TOKEN until then. --- .dev.vars.example | 3 +++ layers/artifact-delivery/server/utils/queue.ts | 15 ++++++++++++++- test/unit/artifact-rate-limit.test.ts | 16 +++++++++++++++- worker-configuration.d.ts | 5 +++-- 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/.dev.vars.example b/.dev.vars.example index 06e4d999..063a60b9 100644 --- a/.dev.vars.example +++ b/.dev.vars.example @@ -11,6 +11,9 @@ NUXT_OG_IMAGE_SECRET= NUXT_IMAGE_PROXY_KEY= NUXT_AI_READY_RUNTIME_SYNC_SECRET= GITHUB_TOKEN= +# Public Artifact builds read GitHub with this token when it is set. Use a token +# from another GitHub account, so runs keep a quota the registry sync cannot spend. +ARTIFACT_GITHUB_TOKEN= ARTIFACT_PRIVATE_ACCESS_ENABLED=false GITHUB_APP_ID= GITHUB_APP_CLIENT_ID= diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 185de39d..2fe4417f 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -98,6 +98,19 @@ export function reportGithubReadFailure(failure: GithubReadFailure): void { })) } +/** + * The token public Artifact builds read GitHub with. + * + * GitHub counts a personal token's quota per account, and the registry sync + * spends `GITHUB_TOKEN` to zero before each hourly reset. Every run that needed + * GitHub in those minutes failed RATE_LIMITED. `ARTIFACT_GITHUB_TOKEN`, from + * another account, keeps a quota for runs. Until it is set, builds share + * `GITHUB_TOKEN`. + */ +export function artifactGithubToken(env: Partial>): string | undefined { + return env.ARTIFACT_GITHUB_TOKEN || env.GITHUB_TOKEN +} + export function createArtifactBuildDependencies(env: Cloudflare.Env): ArtifactBuildDependencies { const runtimeFetch = globalThis.fetch.bind(globalThis) const privateDependencies = privateArtifactAccessEnabled(env) @@ -105,7 +118,7 @@ export function createArtifactBuildDependencies(env: Cloudflare.Env): ArtifactBu : {} return { db: env.DB, - github: createPublicGithubSourceClient({ fetch: runtimeFetch, token: env.GITHUB_TOKEN, onReadFailure: reportGithubReadFailure }), + github: createPublicGithubSourceClient({ fetch: runtimeFetch, token: artifactGithubToken(env), onReadFailure: reportGithubReadFailure }), bucket: env.PUBLIC_ARTIFACTS, signer: createArtifactSigner(env.ARTIFACT_SIGNER), trustedRoot: parseTrustedRoot(env.ARTIFACT_TRUSTED_ROOT_JSON, Math.floor(Date.now() / 1000)), diff --git a/test/unit/artifact-rate-limit.test.ts b/test/unit/artifact-rate-limit.test.ts index 133cec2e..4d7437ed 100644 --- a/test/unit/artifact-rate-limit.test.ts +++ b/test/unit/artifact-rate-limit.test.ts @@ -9,7 +9,7 @@ import { createPublicGithubSourceClient, isRetryableProblem, } from '../../layers/artifact-delivery/server/utils/github-source' -import { ARTIFACT_BUILD_QUEUE_NAME, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' +import { ARTIFACT_BUILD_QUEUE_NAME, artifactGithubToken, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' import { createResolution, getResolution, presentResolution, resolutionRequestIdentity } from '../../layers/artifact-delivery/server/utils/state' import { createSqliteD1 } from './helpers/d1-sqlite' @@ -216,6 +216,20 @@ describe('gitHub secondary rate limits as values', () => { }) }) +// The hourly registry sync spends the site token to zero before each GitHub +// reset. On 2026-10-06, 13 runs failed RATE_LIMITED in those minutes. A token +// from another GitHub account carries its own quota, kept for runs. +describe('the token Artifact builds read GitHub with', () => { + it('prefers the token kept for Artifact builds', () => { + expect(artifactGithubToken({ ARTIFACT_GITHUB_TOKEN: 'runs', GITHUB_TOKEN: 'site' })).toBe('runs') + }) + + it('uses the site token while no Artifact token is set', () => { + expect(artifactGithubToken({ ARTIFACT_GITHUB_TOKEN: '', GITHUB_TOKEN: 'site' })).toBe('site') + expect(artifactGithubToken({ GITHUB_TOKEN: 'site' })).toBe('site') + }) +}) + describe('which problems a caller may retry', () => { it.each([ ['RATE_LIMITED', true], diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 3fb5ce73..dd96a883 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: a047a1748d71da3be1be11f140a75f7b) +// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: 44fdc49aa25ec9befef6b0cb85d9da7e) // Runtime types generated with workerd@1.20260828.1 2026-08-20 nodejs_compat interface __BaseEnv_Env { KV_CACHE: KVNamespace; @@ -37,6 +37,7 @@ interface __BaseEnv_Env { NUXT_IMAGE_PROXY_KEY: string; NUXT_AI_READY_RUNTIME_SYNC_SECRET: string; GITHUB_TOKEN: string; + ARTIFACT_GITHUB_TOKEN: string; ARTIFACT_PRIVATE_ACCESS_ENABLED: string; GITHUB_APP_ID: string; GITHUB_APP_CLIENT_ID: string; @@ -64,7 +65,7 @@ type StringifyValues> = { [Binding in keyof EnvType]: EnvType[Binding] extends string ? EnvType[Binding] : string; }; declare namespace NodeJS { - interface ProcessEnv extends StringifyValues> {} + interface ProcessEnv extends StringifyValues> {} } // Begin runtime types From efe99022f764807a9a1192259183c00de88b1820 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 02:04:28 +1100 Subject: [PATCH 3/8] feat(artifact-delivery): read GitHub as the read App for builds GitHub counts a personal token's quota per account. GITHUB_TOKEN is a personal token, so the registry sync, page views and the account owner's other tools spend the same bucket that builds need, and runs failed RATE_LIMITED in the minutes before each hourly reset. An App installation has its own bucket. Builds now mint an installation token for the read App and fall back to ARTIFACT_GITHUB_TOKEN, then GITHUB_TOKEN, with a reported reason when the App gives none. --- .dev.vars.example | 6 + docs/ops/artifact-delivery-rollout.md | 11 +- .../server/utils/github-app.ts | 90 ++++++-- .../server/utils/github-read-credential.ts | 214 ++++++++++++++++++ .../artifact-delivery/server/utils/queue.ts | 60 ++++- nuxt.config.ts | 3 + test/unit/artifact-github-read-app.test.ts | 204 +++++++++++++++++ test/unit/artifact-rate-limit.test.ts | 16 +- worker-configuration.d.ts | 7 +- 9 files changed, 566 insertions(+), 45 deletions(-) create mode 100644 layers/artifact-delivery/server/utils/github-read-credential.ts create mode 100644 test/unit/artifact-github-read-app.test.ts diff --git a/.dev.vars.example b/.dev.vars.example index 063a60b9..d5a4b14a 100644 --- a/.dev.vars.example +++ b/.dev.vars.example @@ -14,6 +14,12 @@ GITHUB_TOKEN= # Public Artifact builds read GitHub with this token when it is set. Use a token # from another GitHub account, so runs keep a quota the registry sync cannot spend. ARTIFACT_GITHUB_TOKEN= +# Public Artifact builds read GitHub as the read App: metadata read only, +# installed on skilld-dev. Its installation has a quota nothing else spends. +# The key is a PKCS #8 PEM. Without these, builds use the tokens above. +SKILLD_READ_APP_ID= +SKILLD_READ_APP_INSTALLATION_ID= +SKILLD_READ_APP_PRIVATE_KEY_PKCS8= ARTIFACT_PRIVATE_ACCESS_ENABLED=false GITHUB_APP_ID= GITHUB_APP_CLIENT_ID= diff --git a/docs/ops/artifact-delivery-rollout.md b/docs/ops/artifact-delivery-rollout.md index 3a1214bb..16e2ad09 100644 --- a/docs/ops/artifact-delivery-rollout.md +++ b/docs/ops/artifact-delivery-rollout.md @@ -113,7 +113,16 @@ Preserve these existing values: - `NUXT_TOKEN_KEY` - `GITHUB_TOKEN` -The existing `GITHUB_TOKEN` reads public [GitHub](https://github.com) sources. +The existing `GITHUB_TOKEN` reads public [GitHub](https://github.com) sources for the registry sync and `/gh` pages. + +Public Artifact builds read GitHub with a credential of their own, in this order: + +1. The read App `skilld-dev-registry-reads`: `SKILLD_READ_APP_ID`, `SKILLD_READ_APP_INSTALLATION_ID` and `SKILLD_READ_APP_PRIVATE_KEY_PKCS8` (a PKCS #8 PEM). It has metadata read permission only, and its installation has its own quota. +2. `ARTIFACT_GITHUB_TOKEN`. +3. `GITHUB_TOKEN`. + +If the App secrets are set but unusable, builds read with the next credential and emit an `artifact-github-credential` event with the reason. +Keep the read App apart from the `GITHUB_APP_*` secrets below. Those belong to private delivery. Changing `NUXT_TOKEN_KEY` breaks stored GitHub credentials. diff --git a/layers/artifact-delivery/server/utils/github-app.ts b/layers/artifact-delivery/server/utils/github-app.ts index 6fdfd17f..f6a7d541 100644 --- a/layers/artifact-delivery/server/utils/github-app.ts +++ b/layers/artifact-delivery/server/utils/github-app.ts @@ -491,22 +491,16 @@ export function createGithubAppClient(config: GithubAppConfig): GithubAppClient }, async createRepositoryToken(installationId, repositoryId) { - const jwt = await createGithubAppJwt(config) - const result = await request( - `/app/installations/${installationId}/access_tokens`, - jwt, - installationTokenSchema, - { - method: 'POST', - body: { - repository_ids: [repositoryId], - permissions: { contents: 'read', metadata: 'read' }, - }, + const result = await requestInstallationToken({ + fetch: config.fetch, + jwt: await createGithubAppJwt(config), + installationId, + body: { + repository_ids: [repositoryId], + permissions: { contents: 'read', metadata: 'read' }, }, - ) - return result._tag === 'ok' - ? { _tag: 'created', token: result.value.token, expiresAt: result.value.expires_at } - : result + }) + return result._tag === 'created' ? result : { _tag: 'not-found' } }, async userCanAccessRepository(userToken, installationId, repositoryId) { @@ -547,21 +541,81 @@ export async function verifyGithubWebhookSignature( return different === 0 } +export type InstallationTokenResult + = | { _tag: 'created', token: string, expiresAt: string } + /** GitHub answered 401, 403 or 404: the App, its key or the installation is gone. */ + | { _tag: 'refused', status: number } + +/** + * Exchange an App JWT for an installation access token. + * + * A refusal is a value. Any other failure, such as a 5xx or an unreadable + * answer, throws: it says nothing about the App. + */ +export async function requestInstallationToken(input: { + fetch: typeof globalThis.fetch + jwt: string + installationId: number + body: { repository_ids?: number[], permissions: Record } +}): Promise { + const fetched = await fetchNoRedirect(input.fetch, `${GITHUB_API}/app/installations/${input.installationId}/access_tokens`, { + method: 'POST', + headers: { + 'Accept': 'application/vnd.github+json', + 'Authorization': `Bearer ${input.jwt}`, + 'Content-Type': 'application/json', + 'User-Agent': 'skilld.dev', + 'X-GitHub-Api-Version': GITHUB_API_VERSION, + }, + body: JSON.stringify(input.body), + signal: AbortSignal.timeout(GITHUB_REQUEST_TIMEOUT_MS), + }) + if (fetched._tag === 'unexpected-redirect') + throw new Error(`GitHub App request redirected ${fetched.status}`) + const response = fetched.response + if (response.status === 401 || response.status === 403 || response.status === 404) { + await response.body?.cancel() + return { _tag: 'refused', status: response.status } + } + if (!response.ok) + throw new Error(`GitHub App request returned ${response.status}`) + const value = installationTokenSchema.safeParse(await readBoundedJson(response, MAX_GITHUB_TOKEN_RESPONSE_BYTES)) + if (!value.success) + throw new Error('GitHub App returned an invalid response') + return { _tag: 'created', token: value.data.token, expiresAt: value.data.expires_at } +} + async function createGithubAppJwt(config: GithubAppConfig): Promise { const privateKey = decodeCanonicalBase64Url(config.privateKeyPkcs8) if (!privateKey) throw new Error('GitHub App private key is malformed') - const issuedAt = config.now() - 60 + return await signGithubAppJwt({ issuer: config.clientId, privateKeyPkcs8: privateKey, now: config.now() }) +} + +/** + * An RS256 GitHub App JWT. `issuer` is the App's client ID or its App ID; + * GitHub accepts either. + * + * It is valid for ten minutes from a minute in the past, which absorbs clock + * drift between the Worker and GitHub. Ten minutes is GitHub's maximum. + */ +export async function signGithubAppJwt(input: { + issuer: string + privateKeyPkcs8: Uint8Array + /** Unix seconds. */ + now: number +}): Promise { + const issuedAt = input.now - 60 const header = bytesToBase64Url(new TextEncoder().encode(JSON.stringify({ alg: 'RS256', typ: 'JWT' }))) const payload = bytesToBase64Url(new TextEncoder().encode(JSON.stringify({ iat: issuedAt, exp: issuedAt + 600, - iss: config.clientId, + iss: input.issuer, }))) const unsigned = `${header}.${payload}` const imported = await crypto.subtle.importKey( 'pkcs8', - toArrayBuffer(privateKey), + toArrayBuffer(input.privateKeyPkcs8), { name: 'RSASSA-PKCS1-v1_5', hash: 'SHA-256' }, false, ['sign'], diff --git a/layers/artifact-delivery/server/utils/github-read-credential.ts b/layers/artifact-delivery/server/utils/github-read-credential.ts new file mode 100644 index 00000000..6398b471 --- /dev/null +++ b/layers/artifact-delivery/server/utils/github-read-credential.ts @@ -0,0 +1,214 @@ +import type { PublicGithubSourceClient } from './github-source' +import { requestInstallationToken, signGithubAppJwt } from './github-app' + +/** + * Renew an installation token this long before GitHub says it expires, so a + * build that starts with it never sees it lapse part way through. + */ +export const INSTALLATION_TOKEN_RENEW_SECONDS = 5 * 60 + +/** The read App: metadata read only, installed on skilld-dev. */ +export interface ReadAppConfig { + appId: number + installationId: number + privateKeyPkcs8: Uint8Array +} + +/** The token a build reads with when the App gives none, by its secret name. */ +export interface FallbackToken { + name: 'ARTIFACT_GITHUB_TOKEN' | 'GITHUB_TOKEN' + token: string +} + +/** + * Where public Artifact builds get their GitHub credential, parsed once from + * the Worker env. + * + * GitHub counts a personal token's quota per account, so every token of one + * account shares one bucket. An App installation has a bucket of its own. + */ +export type ArtifactGithubCredentialConfig + = | { _tag: 'app', app: ReadAppConfig, fallback: FallbackToken | null } + /** App secrets are set but unusable. Builds read with the fallback and report why. */ + | { _tag: 'app-misconfigured', reason: string, fallback: FallbackToken | null } + | { _tag: 'token', token: FallbackToken } + | { _tag: 'anonymous' } + +export interface ArtifactGithubCredentialEnv { + SKILLD_READ_APP_ID?: string + SKILLD_READ_APP_INSTALLATION_ID?: string + SKILLD_READ_APP_PRIVATE_KEY_PKCS8?: string + ARTIFACT_GITHUB_TOKEN?: string + GITHUB_TOKEN?: string +} + +/** Why a build read with a fallback token. It names secrets, never a value. */ +export interface ArtifactGithubCredentialReport { + outcome: 'app-misconfigured' | 'app-token-unavailable' + reason: string + fallback: FallbackToken['name'] | 'anonymous' +} + +interface CachedInstallationToken { + token: string + /** Unix seconds. */ + expiresAt: number +} + +/** + * Installation tokens kept between builds in one isolate. A token lives an + * hour; minting one per build would add a request and a signature to each. + */ +export interface InstallationTokenCache { + tokens: Map + pending: Map> +} + +export function createInstallationTokenCache(): InstallationTokenCache { + return { tokens: new Map(), pending: new Map() } +} + +export interface ArtifactGithubCredentialRuntime { + fetch: typeof globalThis.fetch + /** Unix seconds. */ + now: () => number + tokenCache: InstallationTokenCache + report: (event: ArtifactGithubCredentialReport) => void +} + +/** The credential for one GitHub read. Undefined reads anonymously. */ +export type ArtifactGithubCredential = () => Promise + +export function parseArtifactGithubCredentialConfig(env: ArtifactGithubCredentialEnv): ArtifactGithubCredentialConfig { + const fallback = fallbackToken(env) + const appSecrets = [env.SKILLD_READ_APP_ID, env.SKILLD_READ_APP_INSTALLATION_ID, env.SKILLD_READ_APP_PRIVATE_KEY_PKCS8] + if (appSecrets.every(value => !value?.trim())) + return fallback ? { _tag: 'token', token: fallback } : { _tag: 'anonymous' } + const misconfigured = (reason: string): ArtifactGithubCredentialConfig => ({ _tag: 'app-misconfigured', reason, fallback }) + const appId = positiveInteger(env.SKILLD_READ_APP_ID) + if (appId === null) + return misconfigured('SKILLD_READ_APP_ID is not a GitHub App ID') + const installationId = positiveInteger(env.SKILLD_READ_APP_INSTALLATION_ID) + if (installationId === null) + return misconfigured('SKILLD_READ_APP_INSTALLATION_ID is not an installation ID') + const privateKeyPkcs8 = pkcs8PemBytes(env.SKILLD_READ_APP_PRIVATE_KEY_PKCS8 ?? '') + if (!privateKeyPkcs8) + return misconfigured('SKILLD_READ_APP_PRIVATE_KEY_PKCS8 is not a PKCS #8 PEM private key') + return { _tag: 'app', app: { appId, installationId, privateKeyPkcs8 }, fallback } +} + +/** + * The GitHub credential public Artifact builds read with: the read App's + * installation token, then `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`. + * + * The registry sync and `/gh` page views keep `GITHUB_TOKEN`. When the App + * cannot give a token, the build still runs on the fallback and the reason is + * reported, so a broken key shows up as an event instead of as a spent quota. + */ +export function createArtifactGithubCredential( + config: ArtifactGithubCredentialConfig, + runtime: ArtifactGithubCredentialRuntime, +): ArtifactGithubCredential { + if (config._tag === 'anonymous') + return async () => undefined + if (config._tag === 'token') + return async () => config.token.token + const fallbackName = config.fallback?.name ?? 'anonymous' + if (config._tag === 'app-misconfigured') { + return async () => { + runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) + return config.fallback?.token + } + } + const app = config.app + const key = `${app.appId}:${app.installationId}` + return async () => { + const cached = runtime.tokenCache.tokens.get(key) + if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) + return cached.token + let pending = runtime.tokenCache.pending.get(key) + if (!pending) { + pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) + runtime.tokenCache.pending.set(key, pending) + } + const minted = await pending + if ('token' in minted) { + runtime.tokenCache.tokens.set(key, minted) + return minted.token + } + runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) + return config.fallback?.token + } +} + +/** + * A GitHub source client whose every read asks for the current credential, so + * a long-lived client never reads with an expired installation token. + */ +export function withGithubCredential( + credential: ArtifactGithubCredential, + create: (token: string | undefined) => PublicGithubSourceClient, +): PublicGithubSourceClient { + return { + resolve: async request => await create(await credential()).resolve(request), + load: async source => await create(await credential()).load(source), + } +} + +async function mintInstallationToken( + app: ReadAppConfig, + runtime: Pick, +): Promise { + const minted = await signGithubAppJwt({ issuer: String(app.appId), privateKeyPkcs8: app.privateKeyPkcs8, now: runtime.now() }) + .then(jwt => requestInstallationToken({ + fetch: runtime.fetch, + jwt, + installationId: app.installationId, + body: { permissions: { metadata: 'read' } }, + })) + // A network fault, a 5xx or a key WebCrypto rejects: the build reads with + // the fallback token, and the reason reaches the report. + .catch((error: unknown) => ({ _tag: 'failed' as const, reason: error instanceof Error ? error.message : String(error) })) + if (minted._tag === 'failed') + return { _tag: 'unavailable', reason: minted.reason.slice(0, 200) } + if (minted._tag === 'refused') + return { _tag: 'unavailable', reason: `GitHub refused the installation token (${minted.status})` } + const expiresAt = Math.floor(Date.parse(minted.expiresAt) / 1000) + return { token: minted.token, expiresAt } +} + +function fallbackToken(env: ArtifactGithubCredentialEnv): FallbackToken | null { + if (env.ARTIFACT_GITHUB_TOKEN?.trim()) + return { name: 'ARTIFACT_GITHUB_TOKEN', token: env.ARTIFACT_GITHUB_TOKEN.trim() } + if (env.GITHUB_TOKEN?.trim()) + return { name: 'GITHUB_TOKEN', token: env.GITHUB_TOKEN.trim() } + return null +} + +function positiveInteger(value: string | undefined): number | null { + const trimmed = value?.trim() ?? '' + if (!/^[1-9]\d*$/.test(trimmed)) + return null + const parsed = Number(trimmed) + return Number.isSafeInteger(parsed) ? parsed : null +} + +/** + * The DER bytes of a PKCS #8 PEM private key, or null. + * + * A secret pasted through a dashboard or a JSON file can hold its line breaks + * as the two characters `\n`, so both forms parse. A PKCS #1 key + * (`BEGIN RSA PRIVATE KEY`, as GitHub downloads it) does not: WebCrypto + * imports PKCS #8 only. Convert it with `openssl pkcs8 -topk8 -nocrypt`. + */ +function pkcs8PemBytes(value: string): Uint8Array | null { + const text = value.replaceAll('\\n', '\n').trim() + const match = /^-----BEGIN PRIVATE KEY-----([A-Z0-9+/=\s]+)-----END PRIVATE KEY-----$/i.exec(text) + if (!match) + return null + const base64 = match[1]!.replace(/\s+/g, '') + if (!/^[A-Z0-9+/]+={0,2}$/i.test(base64) || base64.length % 4 !== 0) + return null + const binary = atob(base64) + return Uint8Array.from(binary, character => character.charCodeAt(0)) +} diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 2fe4417f..cfb0badd 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -1,6 +1,11 @@ import type { QueueBatch } from '#cf-jobs/server' import type { ArtifactBuildDependencies } from './build' -import type { GithubReadFailure } from './github-source' +import type { + ArtifactGithubCredentialEnv, + ArtifactGithubCredentialReport, + ArtifactGithubCredentialRuntime, +} from './github-read-credential' +import type { GithubReadFailure, PublicGithubSourceClient } from './github-source' import { createWideEvent } from '@harlan-zw/nuxt-wide-events/standalone' import { z } from 'zod' import { emitOperationalEvent } from '#server/utils/operational-event' @@ -11,6 +16,12 @@ import { githubAppUserTokenDependenciesFromEnv, loadAccountGithubAppUserToken, } from './github-app' +import { + createArtifactGithubCredential, + createInstallationTokenCache, + parseArtifactGithubCredentialConfig, + withGithubCredential, +} from './github-read-credential' import { createGithubSourceClient, createPublicGithubSourceClient } from './github-source' import { createD1PrivateArtifactKeyProvider, privateArtifactWrappingKeysFromEnv } from './private-crypto' import { privateArtifactAccessEnabled } from './private-feature' @@ -99,16 +110,47 @@ export function reportGithubReadFailure(failure: GithubReadFailure): void { } /** - * The token public Artifact builds read GitHub with. + * One installation token cache per isolate. A token lives an hour, so it + * outlives the queue batch that minted it. + */ +const installationTokens = createInstallationTokenCache() + +/** + * The GitHub source public Artifact builds read with. * * GitHub counts a personal token's quota per account, and the registry sync - * spends `GITHUB_TOKEN` to zero before each hourly reset. Every run that needed - * GitHub in those minutes failed RATE_LIMITED. `ARTIFACT_GITHUB_TOKEN`, from - * another account, keeps a quota for runs. Until it is set, builds share - * `GITHUB_TOKEN`. + * and `/gh` page views spend `GITHUB_TOKEN` to zero before each hourly + * reset. Every run that needed GitHub in those minutes failed RATE_LIMITED. + * Builds read with the read App's installation token, whose bucket nothing + * else spends, then `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`. */ -export function artifactGithubToken(env: Partial>): string | undefined { - return env.ARTIFACT_GITHUB_TOKEN || env.GITHUB_TOKEN +export function createArtifactGithubSource( + env: ArtifactGithubCredentialEnv, + runtime: ArtifactGithubCredentialRuntime = defaultGithubSourceRuntime(), +): PublicGithubSourceClient { + const credential = createArtifactGithubCredential(parseArtifactGithubCredentialConfig(env), runtime) + return withGithubCredential( + credential, + token => createPublicGithubSourceClient({ fetch: runtime.fetch, token, onReadFailure: reportGithubReadFailure }), + ) +} + +function defaultGithubSourceRuntime(): ArtifactGithubCredentialRuntime { + return { + fetch: globalThis.fetch.bind(globalThis), + now: () => Math.floor(Date.now() / 1000), + tokenCache: installationTokens, + report: reportGithubCredential, + } +} + +function reportGithubCredential(event: ArtifactGithubCredentialReport): void { + emitOperationalEvent(createWideEvent({ + 'operation': 'artifact-github-credential', + 'outcome': event.outcome, + 'reason': event.reason, + 'github.credential': event.fallback, + })) } export function createArtifactBuildDependencies(env: Cloudflare.Env): ArtifactBuildDependencies { @@ -118,7 +160,7 @@ export function createArtifactBuildDependencies(env: Cloudflare.Env): ArtifactBu : {} return { db: env.DB, - github: createPublicGithubSourceClient({ fetch: runtimeFetch, token: artifactGithubToken(env), onReadFailure: reportGithubReadFailure }), + github: createArtifactGithubSource(env), bucket: env.PUBLIC_ARTIFACTS, signer: createArtifactSigner(env.ARTIFACT_SIGNER), trustedRoot: parseTrustedRoot(env.ARTIFACT_TRUSTED_ROOT_JSON, Math.floor(Date.now() / 1000)), diff --git a/nuxt.config.ts b/nuxt.config.ts index 2b45c2e2..78fa4bb3 100644 --- a/nuxt.config.ts +++ b/nuxt.config.ts @@ -142,6 +142,9 @@ export default defineNuxtConfig({ 'error.count', 'failed.count', 'failed.tasks', + // `artifact-github-credential`: which fallback token a build read GitHub + // with after the read App gave none. + 'github.credential', 'github.endpoint', 'github.step', 'item.count', diff --git a/test/unit/artifact-github-read-app.test.ts b/test/unit/artifact-github-read-app.test.ts new file mode 100644 index 00000000..95f3a5d4 --- /dev/null +++ b/test/unit/artifact-github-read-app.test.ts @@ -0,0 +1,204 @@ +import type { SourceRequest } from '../../layers/artifact-delivery/server/schemas/contracts' +import type { ArtifactGithubCredentialReport } from '../../layers/artifact-delivery/server/utils/github-read-credential' +import { describe, expect, it } from 'vitest' +import { createInstallationTokenCache } from '../../layers/artifact-delivery/server/utils/github-read-credential' +import { createArtifactGithubSource } from '../../layers/artifact-delivery/server/utils/queue' + +// Builds shared the site token with the hourly registry sync and every `/gh` +// page view, so a run failed RATE_LIMITED whenever the sync spent it. The read +// App's installation has its own bucket of 5,000 requests an hour. +const APP_ID = '5212127' +const INSTALLATION_ID = '168540695' +const NOW = 1_791_300_000 +const request: SourceRequest = { + provider: 'github', + owner: 'skilld-dev', + repository: 'skills', + selector: { type: 'path', path: 'skills/demo' }, +} + +describe('the GitHub credential public Artifact builds read with', () => { + it('reads with the read App installation token when the App secrets are set', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600 }) + const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github)) + + await source.resolve(request) + + expect(github.mints).toHaveLength(1) + expect(github.mints[0]!.url).toBe(`https://api.github.com/app/installations/${INSTALLATION_ID}/access_tokens`) + expect(github.mints[0]!.body).toEqual({ permissions: { metadata: 'read' } }) + const claims = await verifiedJwtClaims(github.mints[0]!.authorization, keys.publicKey) + expect(claims.iss).toBe(APP_ID) + expect(claims.exp - claims.iat).toBeLessThanOrEqual(600) + expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1']) + }) + + it('reuses one installation token until five minutes before it expires', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600 }) + const clock = { now: NOW } + const cache = createInstallationTokenCache() + const read = () => createArtifactGithubSource(appEnv(keys.pem), runtime(github, { cache, now: () => clock.now })).resolve(request) + + await read() + clock.now = NOW + 3600 - 301 + await read() + expect(github.mints).toHaveLength(1) + + clock.now = NOW + 3600 - 300 + await read() + expect(github.mints).toHaveLength(2) + expect(github.readAuthorizations).toEqual([ + 'Bearer ghs_installation_1', + 'Bearer ghs_installation_1', + 'Bearer ghs_installation_2', + ]) + }) + + it('mints once for reads that start together', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600 }) + const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github)) + + await Promise.all([source.resolve(request), source.resolve(request), source.resolve(request)]) + + expect(github.mints).toHaveLength(1) + }) + + it('accepts a PEM whose line breaks were stored as \\n', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600 }) + const source = createArtifactGithubSource(appEnv(keys.pem.replaceAll('\n', '\\n')), runtime(github)) + + await source.resolve(request) + + expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1']) + }) + + it('falls back to ARTIFACT_GITHUB_TOKEN, then GITHUB_TOKEN, without the App secrets', async () => { + const both = fakeGithub({ expiresAt: NOW + 3600 }) + await createArtifactGithubSource({ ARTIFACT_GITHUB_TOKEN: 'runs', GITHUB_TOKEN: 'site' }, runtime(both)).resolve(request) + const siteOnly = fakeGithub({ expiresAt: NOW + 3600 }) + await createArtifactGithubSource({ ARTIFACT_GITHUB_TOKEN: '', GITHUB_TOKEN: 'site' }, runtime(siteOnly)).resolve(request) + + expect(both.mints).toHaveLength(0) + expect(both.readAuthorizations).toEqual(['Bearer runs']) + expect(siteOnly.readAuthorizations).toEqual(['Bearer site']) + }) + + it('reads with the fallback token, and reports why, when GitHub refuses the installation token', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600, mintStatus: 401 }) + const reports: ArtifactGithubCredentialReport[] = [] + const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })) + + await source.resolve(request) + + expect(github.readAuthorizations).toEqual(['Bearer runs']) + expect(reports).toEqual([{ outcome: 'app-token-unavailable', reason: 'GitHub refused the installation token (401)', fallback: 'ARTIFACT_GITHUB_TOKEN' }]) + }) + + it('reports an unreadable App key and reads with the fallback token', async () => { + const github = fakeGithub({ expiresAt: NOW + 3600 }) + const reports: ArtifactGithubCredentialReport[] = [] + const env = { ...appEnv('-----BEGIN RSA PRIVATE KEY-----\nAAAA\n-----END RSA PRIVATE KEY-----'), ARTIFACT_GITHUB_TOKEN: '' } + const source = createArtifactGithubSource(env, runtime(github, { report: event => reports.push(event) })) + + await source.resolve(request) + + expect(github.mints).toHaveLength(0) + expect(github.readAuthorizations).toEqual(['Bearer site']) + expect(reports).toEqual([{ outcome: 'app-misconfigured', reason: 'SKILLD_READ_APP_PRIVATE_KEY_PKCS8 is not a PKCS #8 PEM private key', fallback: 'GITHUB_TOKEN' }]) + }) + + it('never puts a token or the key in a report', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600, mintStatus: 500 }) + const reports: ArtifactGithubCredentialReport[] = [] + await createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })).resolve(request) + + const text = JSON.stringify(reports) + expect(text).not.toContain('runs') + expect(text).not.toContain('site') + expect(text).not.toContain('PRIVATE KEY') + expect(reports).toHaveLength(1) + }) +}) + +interface FakeGithub { + fetch: typeof fetch + mints: Array<{ url: string, authorization: string, body: unknown }> + readAuthorizations: Array +} + +function fakeGithub(options: { expiresAt: number, mintStatus?: number }): FakeGithub { + const mints: FakeGithub['mints'] = [] + const readAuthorizations: FakeGithub['readAuthorizations'] = [] + const fetcher = async (input: RequestInfo | URL, init?: RequestInit): Promise => { + const url = String(input) + const headers = new Headers(init?.headers) + if (url.includes('/access_tokens')) { + mints.push({ url, authorization: headers.get('authorization') ?? '', body: JSON.parse(String(init?.body)) }) + if (options.mintStatus) + return new Response('{}', { status: options.mintStatus }) + return Response.json({ token: `ghs_installation_${mints.length}`, expires_at: new Date(options.expiresAt * 1000).toISOString() }, { status: 201 }) + } + readAuthorizations.push(headers.get('authorization')) + return Response.json({ message: 'Not Found' }, { status: 404 }) + } + return { fetch: fetcher as typeof fetch, mints, readAuthorizations } +} + +function runtime( + github: FakeGithub, + overrides: Partial<{ cache: ReturnType, now: () => number, report: (event: ArtifactGithubCredentialReport) => void }> = {}, +) { + return { + fetch: github.fetch, + now: overrides.now ?? (() => NOW), + tokenCache: overrides.cache ?? createInstallationTokenCache(), + report: overrides.report ?? (() => {}), + } +} + +function appEnv(pem: string) { + return { + SKILLD_READ_APP_ID: APP_ID, + SKILLD_READ_APP_INSTALLATION_ID: INSTALLATION_ID, + SKILLD_READ_APP_PRIVATE_KEY_PKCS8: pem, + ARTIFACT_GITHUB_TOKEN: 'runs', + GITHUB_TOKEN: 'site', + } +} + +async function appKeys(): Promise<{ pem: string, publicKey: CryptoKey }> { + const pair = await crypto.subtle.generateKey( + { name: 'RSASSA-PKCS1-v1_5', modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: 'SHA-256' }, + true, + ['sign', 'verify'], + ) as CryptoKeyPair + const der = new Uint8Array(await crypto.subtle.exportKey('pkcs8', pair.privateKey) as ArrayBuffer) + const base64 = btoa(String.fromCharCode(...der)).match(/.{1,64}/g)!.join('\n') + return { pem: `-----BEGIN PRIVATE KEY-----\n${base64}\n-----END PRIVATE KEY-----\n`, publicKey: pair.publicKey } +} + +async function verifiedJwtClaims(authorization: string, publicKey: CryptoKey): Promise<{ iss: string, iat: number, exp: number }> { + const jwt = authorization.replace(/^Bearer /, '') + const [header, payload, signature] = jwt.split('.') as [string, string, string] + const valid = await crypto.subtle.verify( + 'RSASSA-PKCS1-v1_5', + publicKey, + base64UrlBytes(signature), + new TextEncoder().encode(`${header}.${payload}`), + ) + expect(valid).toBe(true) + expect(JSON.parse(new TextDecoder().decode(base64UrlBytes(header)))).toEqual({ alg: 'RS256', typ: 'JWT' }) + return JSON.parse(new TextDecoder().decode(base64UrlBytes(payload))) +} + +function base64UrlBytes(value: string): Uint8Array { + const normalized = value.replaceAll('-', '+').replaceAll('_', '/') + const binary = atob(normalized + '='.repeat((4 - normalized.length % 4) % 4)) + return Uint8Array.from(binary, character => character.charCodeAt(0)) +} diff --git a/test/unit/artifact-rate-limit.test.ts b/test/unit/artifact-rate-limit.test.ts index 4d7437ed..133cec2e 100644 --- a/test/unit/artifact-rate-limit.test.ts +++ b/test/unit/artifact-rate-limit.test.ts @@ -9,7 +9,7 @@ import { createPublicGithubSourceClient, isRetryableProblem, } from '../../layers/artifact-delivery/server/utils/github-source' -import { ARTIFACT_BUILD_QUEUE_NAME, artifactGithubToken, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' +import { ARTIFACT_BUILD_QUEUE_NAME, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' import { createResolution, getResolution, presentResolution, resolutionRequestIdentity } from '../../layers/artifact-delivery/server/utils/state' import { createSqliteD1 } from './helpers/d1-sqlite' @@ -216,20 +216,6 @@ describe('gitHub secondary rate limits as values', () => { }) }) -// The hourly registry sync spends the site token to zero before each GitHub -// reset. On 2026-10-06, 13 runs failed RATE_LIMITED in those minutes. A token -// from another GitHub account carries its own quota, kept for runs. -describe('the token Artifact builds read GitHub with', () => { - it('prefers the token kept for Artifact builds', () => { - expect(artifactGithubToken({ ARTIFACT_GITHUB_TOKEN: 'runs', GITHUB_TOKEN: 'site' })).toBe('runs') - }) - - it('uses the site token while no Artifact token is set', () => { - expect(artifactGithubToken({ ARTIFACT_GITHUB_TOKEN: '', GITHUB_TOKEN: 'site' })).toBe('site') - expect(artifactGithubToken({ GITHUB_TOKEN: 'site' })).toBe('site') - }) -}) - describe('which problems a caller may retry', () => { it.each([ ['RATE_LIMITED', true], diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index dd96a883..cafb6af1 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: 44fdc49aa25ec9befef6b0cb85d9da7e) +// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: 91ae1b6a0c6d3e279dd2c18ecea4e0c4) // Runtime types generated with workerd@1.20260828.1 2026-08-20 nodejs_compat interface __BaseEnv_Env { KV_CACHE: KVNamespace; @@ -38,6 +38,9 @@ interface __BaseEnv_Env { NUXT_AI_READY_RUNTIME_SYNC_SECRET: string; GITHUB_TOKEN: string; ARTIFACT_GITHUB_TOKEN: string; + SKILLD_READ_APP_ID: string; + SKILLD_READ_APP_INSTALLATION_ID: string; + SKILLD_READ_APP_PRIVATE_KEY_PKCS8: string; ARTIFACT_PRIVATE_ACCESS_ENABLED: string; GITHUB_APP_ID: string; GITHUB_APP_CLIENT_ID: string; @@ -65,7 +68,7 @@ type StringifyValues> = { [Binding in keyof EnvType]: EnvType[Binding] extends string ? EnvType[Binding] : string; }; declare namespace NodeJS { - interface ProcessEnv extends StringifyValues> {} + interface ProcessEnv extends StringifyValues> {} } // Begin runtime types From bbcc3a5b41b24685eff80c23bca2a257779df8d4 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 02:19:41 +1100 Subject: [PATCH 4/8] feat(artifact-delivery): read with the requester's token at a spent quota When every shared GitHub bucket is spent, a signed-in run still has its own: the account's GitHub sign-in token reads public Repositories at that account's quota. The build retries only the refused read with it, and only for the account that asked. The requester row lives while the build runs. The build deletes it when it settles, account deletion removes it, and the daily purge removes one a dead build left behind. --- .../server/api/v1/resolutions/index.post.ts | 1 + .../artifact-delivery/server/utils/build.ts | 15 +- .../artifact-delivery/server/utils/queue.ts | 18 ++ .../server/utils/request-resolution.ts | 19 +- .../server/utils/requester-github.ts | 117 ++++++++++++ .../identity/server/utils/account-deletion.ts | 1 + .../identity/server/utils/data-retention.ts | 9 + .../0143_artifact_resolution_requesters.sql | 13 ++ test/unit/account-deletion.test.ts | 8 + test/unit/artifact-build-reuse.test.ts | 1 + test/unit/artifact-delivery.test.ts | 1 + test/unit/artifact-rate-limit.test.ts | 1 + test/unit/artifact-requester-github.test.ts | 177 ++++++++++++++++++ test/unit/skill-run-sweep.test.ts | 1 + 14 files changed, 379 insertions(+), 3 deletions(-) create mode 100644 layers/artifact-delivery/server/utils/requester-github.ts create mode 100644 migrations/0143_artifact_resolution_requesters.sql create mode 100644 test/unit/artifact-requester-github.test.ts diff --git a/layers/artifact-delivery/server/api/v1/resolutions/index.post.ts b/layers/artifact-delivery/server/api/v1/resolutions/index.post.ts index 631a6983..fdbc3bff 100644 --- a/layers/artifact-delivery/server/api/v1/resolutions/index.post.ts +++ b/layers/artifact-delivery/server/api/v1/resolutions/index.post.ts @@ -49,6 +49,7 @@ export default withArtifactProblems(defineApiHandler({ repositoryId: privateAccess.repositoryId, } : { visibility: 'public' }, + requesterAccountId: user?.id ?? null, }) if (result._tag === 'idempotency-conflict') { throw createError({ diff --git a/layers/artifact-delivery/server/utils/build.ts b/layers/artifact-delivery/server/utils/build.ts index daea8253..79ca3b75 100644 --- a/layers/artifact-delivery/server/utils/build.ts +++ b/layers/artifact-delivery/server/utils/build.ts @@ -29,6 +29,7 @@ import { import { checkArtifactSource, checksBlockArtifact, checksPermitSigning } from './checks' import { canonicalJson, digestHex } from './encoding' import { isRetryableProblem } from './github-source' +import { withRequesterFallback } from './requester-github' import { ARTIFACT_POLICY_VERSION, findReadyPublicBuild, @@ -43,6 +44,12 @@ export interface ArtifactBuildDependencies { db: D1Database github: PublicGithubSourceClient privateGithub?: (row: ResolutionRow) => Promise + /** + * The GitHub client of the signed-in account that asked for a public + * Resolution, or null. A build uses it only for a read the shared + * credential's quota refused, and only for that account's own Resolution. + */ + requesterGithub?: (row: ResolutionRow) => Promise bucket: R2Bucket privateArtifacts?: { put: (input: { @@ -703,8 +710,12 @@ async function githubForResolution( dependencies: ArtifactBuildDependencies, row: ResolutionRow, ): Promise { - if (row.visibility === 'public') - return dependencies.github + if (row.visibility === 'public') { + const requesterGithub = dependencies.requesterGithub + return requesterGithub + ? withRequesterFallback(dependencies.github, () => requesterGithub(row)) + : dependencies.github + } if (!dependencies.privateGithub) { return { _tag: 'rejected', diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index cfb0badd..5540b406 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -26,6 +26,7 @@ import { createGithubSourceClient, createPublicGithubSourceClient } from './gith import { createD1PrivateArtifactKeyProvider, privateArtifactWrappingKeysFromEnv } from './private-crypto' import { privateArtifactAccessEnabled } from './private-feature' import { putPrivateArtifact } from './private-storage' +import { createRequesterGithubSource, forgetResolutionRequester } from './requester-github' import { getResolution } from './state' import { parseTrustedRoot } from './trusted-root' @@ -70,6 +71,10 @@ export async function consumeArtifactBuildBatch( .then(value => ({ _tag: 'ok' as const, value })) .catch(error => ({ _tag: 'error' as const, error })) if (outcome._tag === 'ok') { + // A settled build never reads GitHub again, so it no longer needs to + // know who asked. A superseded one leaves that to the build that won. + if (outcome.value._tag !== 'superseded') + await forgetResolutionRequester(dependencies.db, parsed.data.resolutionId) message.ack() continue } @@ -89,6 +94,7 @@ export async function consumeArtifactBuildBatch( const row = await getResolution(dependencies.db, parsed.data.resolutionId) if (row) await failResolution(dependencies, row, 'SERVICE_UNAVAILABLE', true) + await forgetResolutionRequester(dependencies.db, parsed.data.resolutionId) message.ack() } } @@ -161,6 +167,18 @@ export function createArtifactBuildDependencies(env: Cloudflare.Env): ArtifactBu return { db: env.DB, github: createArtifactGithubSource(env), + requesterGithub: createRequesterGithubSource({ + db: env.DB, + tokenKey: () => env.NUXT_TOKEN_KEY, + fetch: runtimeFetch, + now: () => Math.floor(Date.now() / 1000), + onReadFailure: reportGithubReadFailure, + onUnusableToken: reason => emitOperationalEvent(createWideEvent({ + operation: 'artifact-github-credential', + outcome: 'requester-token-unusable', + reason, + })), + }), bucket: env.PUBLIC_ARTIFACTS, signer: createArtifactSigner(env.ARTIFACT_SIGNER), trustedRoot: parseTrustedRoot(env.ARTIFACT_TRUSTED_ROOT_JSON, Math.floor(Date.now() / 1000)), diff --git a/layers/artifact-delivery/server/utils/request-resolution.ts b/layers/artifact-delivery/server/utils/request-resolution.ts index d9556bb6..7a775eb2 100644 --- a/layers/artifact-delivery/server/utils/request-resolution.ts +++ b/layers/artifact-delivery/server/utils/request-resolution.ts @@ -3,6 +3,7 @@ import type { SourceRequest } from '../schemas/contracts' import type { FetchAdmittedSkillIdentity } from './admitted-identity' import type { CreateResolutionResult } from './state' import { admittedSourceRequest } from './admitted-identity' +import { recordResolutionRequester } from './requester-github' import { createResolution, resolutionRequestIdentity } from './state' export interface ResolutionRequestDependencies { @@ -48,7 +49,16 @@ export function fetchAdmittedSkillIdentity(context?: H3EventContext): FetchAdmit */ export async function requestResolution( dependencies: ResolutionRequestDependencies, - input: { source: SourceRequest, idempotencyKey: string, access: ResolutionAccess }, + input: { + source: SourceRequest + idempotencyKey: string + access: ResolutionAccess + /** + * The signed-in account that sent a public request, or null. Its build may + * read GitHub with that account's own token once the shared quota is spent. + */ + requesterAccountId?: number | null + }, ): Promise { const accountId = input.access.visibility === 'private' ? input.access.accountId : undefined const identity = await resolutionRequestIdentity(input.source, input.idempotencyKey, accountId) @@ -56,6 +66,13 @@ export async function requestResolution( ? input.source : await admittedSourceRequest(input.source, dependencies.lookupAdmitted) const result = await createResolution(dependencies.db, source, identity, dependencies.now(), input.access) + if (result._tag === 'created' && input.access.visibility === 'public' && input.requesterAccountId) { + await recordResolutionRequester(dependencies.db, { + resolutionId: result.row.id, + accountId: input.requesterAccountId, + now: dependencies.now(), + }) + } if (result._tag !== 'idempotency-conflict' && result.row.state === 'requested') await dependencies.enqueue(result.row.id) return result diff --git a/layers/artifact-delivery/server/utils/requester-github.ts b/layers/artifact-delivery/server/utils/requester-github.ts new file mode 100644 index 00000000..e82c2319 --- /dev/null +++ b/layers/artifact-delivery/server/utils/requester-github.ts @@ -0,0 +1,117 @@ +import type { GithubReadFailure, PublicGithubSourceClient } from './github-source' +import type { ResolutionRow } from './state' +import { decryptToken } from '#layers/identity/server/utils/crypto' +import { createPublicGithubSourceClient } from './github-source' + +/** + * Record the signed-in account that asked for a public Resolution. + * + * Only that Resolution's build reads it, and only after the shared GitHub + * credentials are rate limited. The row goes when the build settles. + */ +export async function recordResolutionRequester( + db: D1Database, + input: { resolutionId: string, accountId: number, now: number }, +): Promise { + await db.prepare( + `INSERT OR IGNORE INTO artifact_resolution_requesters (resolution_id, account_id, created_at) + VALUES (?1, ?2, ?3)`, + ).bind(input.resolutionId, input.accountId, input.now).run() +} + +/** Forget who asked for a Resolution. Its build has settled. */ +export async function forgetResolutionRequester(db: D1Database, resolutionId: string): Promise { + await db.prepare(`DELETE FROM artifact_resolution_requesters WHERE resolution_id = ?1`).bind(resolutionId).run() +} + +export interface RequesterGithubDependencies { + db: D1Database + /** + * `NUXT_TOKEN_KEY`, which encrypts the stored GitHub OAuth tokens. It is read + * only when a requester's token is needed. + */ + tokenKey: () => string + fetch: typeof globalThis.fetch + /** Unix seconds. */ + now: () => number + onReadFailure?: (failure: GithubReadFailure) => void + /** Called when a stored token cannot be used. It never receives the token. */ + onUnusableToken?: (reason: string) => void +} + +/** + * The GitHub client of the account that asked for one public Resolution, or + * null when nobody signed in asked for it, or the account holds no usable + * GitHub token. + * + * The token is the account's GitHub sign-in token. Its scopes, `read:user` and + * `user:email`, read public Repositories at that account's own quota, and no + * private one. The public client also refuses any Repository GitHub reports + * as private, so a build read with it can only produce public content. + */ +export function createRequesterGithubSource( + dependencies: RequesterGithubDependencies, +): (row: ResolutionRow) => Promise { + return async (row) => { + if (row.visibility !== 'public') + return null + const stored = await dependencies.db.prepare( + `SELECT u.github_token_encrypted, u.github_token_expires_at + FROM artifact_resolution_requesters r + JOIN users u ON u.id = r.account_id + WHERE r.resolution_id = ?1 + LIMIT 1`, + ).bind(row.id).first<{ github_token_encrypted: string | null, github_token_expires_at: number | null }>() + if (!stored?.github_token_encrypted) + return null + if (stored.github_token_expires_at !== null && stored.github_token_expires_at <= dependencies.now() + 60) + return null + const token = await decryptToken(stored.github_token_encrypted, dependencies.tokenKey()).then( + value => ({ _tag: 'token' as const, value }), + (error: unknown) => ({ _tag: 'unreadable' as const, reason: error instanceof Error ? error.message : String(error) }), + ) + if (token._tag === 'unreadable') { + dependencies.onUnusableToken?.(`The stored GitHub token did not decrypt: ${token.reason}`.slice(0, 200)) + return null + } + return createPublicGithubSourceClient({ + fetch: dependencies.fetch, + token: token.value, + onReadFailure: dependencies.onReadFailure, + }) + } +} + +/** + * A client that reads with the shared credential, and repeats a read the + * shared quota refused with the requester's own client. + * + * Every other answer, success or rejection, stays the shared one, so a + * requester's token is spent only when nothing else could read. + */ +export function withRequesterFallback( + shared: PublicGithubSourceClient, + requester: () => Promise, +): PublicGithubSourceClient { + let own: Promise | null = null + const ownClient = () => { + own ??= requester() + return own + } + return { + async resolve(request) { + const first = await shared.resolve(request) + if (first._tag !== 'rejected' || first.code !== 'RATE_LIMITED') + return first + const client = await ownClient() + return client ? await client.resolve(request) : first + }, + async load(source) { + const first = await shared.load(source) + if (first._tag !== 'rejected' || first.code !== 'RATE_LIMITED') + return first + const client = await ownClient() + return client ? await client.load(source) : first + }, + } +} diff --git a/layers/identity/server/utils/account-deletion.ts b/layers/identity/server/utils/account-deletion.ts index 06e82f0c..488a2f59 100644 --- a/layers/identity/server/utils/account-deletion.ts +++ b/layers/identity/server/utils/account-deletion.ts @@ -27,6 +27,7 @@ const ACCOUNT_ROW_DELETES = [ `DELETE FROM digest_runs WHERE user_id = ?1`, `DELETE FROM weekly_runs WHERE user_id = ?1`, `DELETE FROM email_preference_events WHERE user_id = ?1`, + `DELETE FROM artifact_resolution_requesters WHERE account_id = ?1`, `DELETE FROM artifact_download_grants WHERE account_id = ?1`, `DELETE FROM private_artifact_attestations WHERE account_id = ?1`, `DELETE FROM private_artifacts WHERE account_id = ?1`, diff --git a/layers/identity/server/utils/data-retention.ts b/layers/identity/server/utils/data-retention.ts index 6e5241eb..cc795f30 100644 --- a/layers/identity/server/utils/data-retention.ts +++ b/layers/identity/server/utils/data-retention.ts @@ -14,6 +14,11 @@ export const RETENTION_SECONDS = { cliSignIn: DAY_SECONDS, /** Delivery history and preference changes support three months of reporting. */ emailHistory: 90 * DAY_SECONDS, + /** + * The account that asked for a public Artifact build. The build deletes it + * when it settles; this removes one a dead build left behind. + */ + runRequester: DAY_SECONDS, } as const export interface RetentionPurgeResult { @@ -23,6 +28,7 @@ export interface RetentionPurgeResult { digestRuns: number weeklyRuns: number emailPreferenceEvents: number + runRequesters: number } /** @@ -40,6 +46,7 @@ export async function purgeRetainedPersonalData( const endedTokenCutoff = now - RETENTION_SECONDS.endedCliToken const signInCutoff = now - RETENTION_SECONDS.cliSignIn const historyCutoff = now - RETENTION_SECONDS.emailHistory + const requesterCutoff = now - RETENTION_SECONDS.runRequester const results = await db.batch([ db.prepare( @@ -62,6 +69,7 @@ export async function purgeRetainedPersonalData( ).bind(historyCutoff), db.prepare(`DELETE FROM weekly_runs WHERE window_end <= ?1`).bind(historyCutoff), db.prepare(`DELETE FROM email_preference_events WHERE occurred_at <= ?1`).bind(historyCutoff), + db.prepare(`DELETE FROM artifact_resolution_requesters WHERE created_at <= ?1`).bind(requesterCutoff), ]) const changes = results.map(result => result.meta.changes) @@ -72,5 +80,6 @@ export async function purgeRetainedPersonalData( digestRuns: changes[3] ?? 0, weeklyRuns: changes[4] ?? 0, emailPreferenceEvents: changes[5] ?? 0, + runRequesters: changes[6] ?? 0, } } diff --git a/migrations/0143_artifact_resolution_requesters.sql b/migrations/0143_artifact_resolution_requesters.sql new file mode 100644 index 00000000..246075cd --- /dev/null +++ b/migrations/0143_artifact_resolution_requesters.sql @@ -0,0 +1,13 @@ +-- The signed-in account that asked for a public Resolution, kept only while +-- its build runs. When the shared GitHub credentials are rate limited, the +-- build reads GitHub with this account's own token, for this build only. +-- The build deletes the row when it settles; the daily personal data purge +-- removes any row a dead build left behind. +CREATE TABLE artifact_resolution_requesters ( + resolution_id TEXT PRIMARY KEY REFERENCES artifact_resolutions(id) ON DELETE CASCADE, + account_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + created_at INTEGER NOT NULL +); + +CREATE INDEX idx_artifact_resolution_requesters_created_at + ON artifact_resolution_requesters(created_at); diff --git a/test/unit/account-deletion.test.ts b/test/unit/account-deletion.test.ts index 66827b02..e6fc9094 100644 --- a/test/unit/account-deletion.test.ts +++ b/test/unit/account-deletion.test.ts @@ -190,6 +190,14 @@ function seedAccount(raw: DatabaseSync, account: Account, githubToken: string | id, installationId, ) + run( + `INSERT INTO artifact_resolutions ( + id, request_fingerprint, state, requested_owner, requested_repository, selector_type, selector_value, visibility + ) VALUES (?, ?, 'resolving', 'acme', 'skills', 'named-skill', 'tdd', 'public')`, + `public-run-${login}`, + `public-fingerprint-${login}`, + ) + run(`INSERT INTO artifact_resolution_requesters (resolution_id, account_id, created_at) VALUES (?, ?, ?)`, `public-run-${login}`, id, NOW) run(`INSERT INTO artifact_check_results (resolution_id, name, version, outcome, required) VALUES (?, 'skill-structure', '1', 'pass', 1)`, resolutionId) run( `INSERT INTO artifacts (id, content_sha256, content_bytes, format, r2_key) VALUES (?, ?, 128, 'skilld-tar-v1', ?)`, diff --git a/test/unit/artifact-build-reuse.test.ts b/test/unit/artifact-build-reuse.test.ts index 5dc57596..2f711843 100644 --- a/test/unit/artifact-build-reuse.test.ts +++ b/test/unit/artifact-build-reuse.test.ts @@ -43,6 +43,7 @@ const MIGRATIONS = [ 'migrations/0112_private_artifact_keys.sql', 'migrations/0122_artifact_resolution_retry_after.sql', 'migrations/0131_artifact_build_reuse.sql', + 'migrations/0143_artifact_resolution_requesters.sql', ] const COMMIT = '0123456789abcdef0123456789abcdef01234567' const NEXT_COMMIT = 'fedcba9876543210fedcba9876543210fedcba98' diff --git a/test/unit/artifact-delivery.test.ts b/test/unit/artifact-delivery.test.ts index 03069179..379a51c5 100644 --- a/test/unit/artifact-delivery.test.ts +++ b/test/unit/artifact-delivery.test.ts @@ -41,6 +41,7 @@ const ARTIFACT_MIGRATIONS = [ 'migrations/0111_github_app_delivery.sql', 'migrations/0112_private_artifact_keys.sql', 'migrations/0122_artifact_resolution_retry_after.sql', + 'migrations/0143_artifact_resolution_requesters.sql', ] const sourceRequest: SourceRequest = { provider: 'github', diff --git a/test/unit/artifact-rate-limit.test.ts b/test/unit/artifact-rate-limit.test.ts index 133cec2e..5d1ffe98 100644 --- a/test/unit/artifact-rate-limit.test.ts +++ b/test/unit/artifact-rate-limit.test.ts @@ -22,6 +22,7 @@ const ARTIFACT_MIGRATIONS = [ 'migrations/0111_github_app_delivery.sql', 'migrations/0112_private_artifact_keys.sql', 'migrations/0122_artifact_resolution_retry_after.sql', + 'migrations/0143_artifact_resolution_requesters.sql', ] const resolutionRequest: SourceRequest = { provider: 'github', diff --git a/test/unit/artifact-requester-github.test.ts b/test/unit/artifact-requester-github.test.ts new file mode 100644 index 00000000..3ef2c0a6 --- /dev/null +++ b/test/unit/artifact-requester-github.test.ts @@ -0,0 +1,177 @@ +import type { SourceRequest } from '../../layers/artifact-delivery/server/schemas/contracts' +import type { ArtifactSigner } from '../../layers/artifact-delivery/server/utils/attestation' +import type { ArtifactBuildDependencies } from '../../layers/artifact-delivery/server/utils/build' +import type { PublicGithubSourceClient, SourceRejection } from '../../layers/artifact-delivery/server/utils/github-source' +import type { TrustedRoot } from '../../layers/artifact-delivery/server/utils/trusted-root' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { processArtifactBuild } from '../../layers/artifact-delivery/server/utils/build' +import { ARTIFACT_BUILD_QUEUE_NAME, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' +import { requestResolution } from '../../layers/artifact-delivery/server/utils/request-resolution' +import { createRequesterGithubSource } from '../../layers/artifact-delivery/server/utils/requester-github' +import { getResolution } from '../../layers/artifact-delivery/server/utils/state' +import { encryptToken } from '../../layers/identity/server/utils/crypto' +import { purgeRetainedPersonalData } from '../../layers/identity/server/utils/data-retention' +import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' + +// The shared GitHub buckets run out each hour. A signed-in run can still read +// GitHub with its own account's token, which has its own quota, for its own +// build only. +const NOW = 1_791_300_000 +const RESET_AT = NOW + 600 +const TOKEN_KEY = btoa(String.fromCharCode(...new Uint8Array(32).fill(7))) +const ALICE = 9101 +const BOB = 9102 +const request: SourceRequest = { + provider: 'github', + owner: 'skilld-dev', + repository: 'skills', + selector: { type: 'path', path: 'skills/demo' }, +} + +describe('a signed-in run when the shared GitHub credential is rate limited', () => { + let fixture: ReturnType + + beforeEach(async () => { + fixture = createSqliteD1(allMigrations()) + const insert = fixture.raw.prepare( + `INSERT INTO users (id, github_id, login, github_token_encrypted, created_at, last_login_at) + VALUES (?, ?, ?, ?, ?, ?)`, + ) + insert.run(ALICE, 900_001, 'alice', await encryptToken('gho_alice', TOKEN_KEY), NOW, NOW) + insert.run(BOB, 900_002, 'bob', await encryptToken('gho_bob', TOKEN_KEY), NOW, NOW) + }) + + afterEach(() => fixture.close()) + + it('reads GitHub with the requester\'s own token', async () => { + const github = userGithub() + const id = await requested('alice-run-0000001', ALICE) + + await processArtifactBuild(dependencies(github), id) + + expect(github.authorizations).toContain('Bearer gho_alice') + expect(github.authorizations).not.toContain('Bearer gho_bob') + // The requester's read reached GitHub, which has no such Repository. + expect(await getResolution(fixture.db, id)).toMatchObject({ state: 'failed', error_code: 'SOURCE_NOT_FOUND' }) + }) + + it('keeps RATE_LIMITED for an anonymous run, and reads with no user token', async () => { + const github = userGithub() + const id = await requested('anonymous-run-001', null) + + await processArtifactBuild(dependencies(github), id) + + expect(github.authorizations).toEqual([]) + expect(await getResolution(fixture.db, id)).toMatchObject({ state: 'failed', error_code: 'RATE_LIMITED' }) + }) + + it('never reads one user\'s build with another user\'s token', async () => { + const github = userGithub() + await requested('alice-run-0000002', ALICE) + const bobRun = await requested('bob-run-000000001', BOB) + const anonymousRun = await requested('anonymous-run-002', null) + + await processArtifactBuild(dependencies(github), bobRun) + await processArtifactBuild(dependencies(github), anonymousRun) + + expect(github.authorizations.length).toBeGreaterThan(0) + expect(github.authorizations.every(value => value === 'Bearer gho_bob')).toBe(true) + }) + + it('keeps the shared credential while it has quota', async () => { + const github = userGithub() + const shared = sharedGithub({ code: 'SOURCE_NOT_FOUND' }) + const id = await requested('alice-run-0000003', ALICE) + + await processArtifactBuild(dependencies(github, shared), id) + + expect(github.authorizations).toEqual([]) + expect(shared.resolve).toHaveBeenCalledOnce() + }) + + it('reports RATE_LIMITED when the requester has no stored GitHub token', async () => { + fixture.raw.prepare(`UPDATE users SET github_token_encrypted = NULL WHERE id = ?`).run(ALICE) + const github = userGithub() + const id = await requested('alice-run-0000004', ALICE) + + await processArtifactBuild(dependencies(github), id) + + expect(github.authorizations).toEqual([]) + expect(await getResolution(fixture.db, id)).toMatchObject({ state: 'failed', error_code: 'RATE_LIMITED' }) + }) + + it('forgets who asked once the build settles', async () => { + const github = userGithub() + const id = await requested('alice-run-0000005', ALICE) + const message = { id: 'message-1', attempts: 1, body: { version: 1, resolutionId: id }, ack: vi.fn(), retry: vi.fn() } + + await consumeArtifactBuildBatch( + {} as Cloudflare.Env, + { queue: ARTIFACT_BUILD_QUEUE_NAME, messages: [message] } as unknown as Parameters[1], + () => dependencies(github), + ) + + expect(message.ack).toHaveBeenCalledOnce() + expect(requesters()).toEqual([]) + }) + + it('purges a requester a dead build left behind after a day', async () => { + const stale = await requested('alice-run-0000006', ALICE) + fixture.raw.prepare(`UPDATE artifact_resolution_requesters SET created_at = ? WHERE resolution_id = ?`).run(NOW - 86_401, stale) + const fresh = await requested('alice-run-0000007', ALICE) + + await purgeRetainedPersonalData(fixture.db, NOW) + + expect(requesters()).toEqual([fresh]) + }) + + function requesters(): unknown[] { + return fixture.raw.prepare(`SELECT resolution_id FROM artifact_resolution_requesters`).all().map(row => Object.values(row)[0]) + } + + async function requested(idempotencyKey: string, requesterAccountId: number | null): Promise { + const result = await requestResolution({ + db: fixture.db, + lookupAdmitted: async () => null, + enqueue: async () => {}, + now: () => NOW, + }, { source: request, idempotencyKey, access: { visibility: 'public' }, requesterAccountId }) + if (result._tag === 'idempotency-conflict') + throw new Error('Test Resolution conflicted') + return result.row.id + } + + function dependencies(github: UserGithub, shared: PublicGithubSourceClient = sharedGithub({ code: 'RATE_LIMITED' })): ArtifactBuildDependencies { + return { + db: fixture.db, + github: shared, + requesterGithub: createRequesterGithubSource({ db: fixture.db, tokenKey: () => TOKEN_KEY, fetch: github.fetch, now: () => NOW }), + bucket: {} as R2Bucket, + signer: {} as ArtifactSigner, + trustedRoot: {} as TrustedRoot, + now: () => NOW, + } + } +}) + +function sharedGithub(rejection: { code: SourceRejection['code'] }): PublicGithubSourceClient & { resolve: ReturnType } { + const rejected: SourceRejection = rejection.code === 'RATE_LIMITED' + ? { _tag: 'rejected', code: 'RATE_LIMITED', summary: 'GitHub refused the read: its rate limit is spent.', findings: [], retryAfterSeconds: RESET_AT } + : { _tag: 'rejected', code: rejection.code, summary: 'The Repository was not found.', findings: [] } + return { resolve: vi.fn(async () => rejected), load: vi.fn(async () => rejected) } +} + +interface UserGithub { + fetch: typeof fetch + authorizations: Array +} + +/** GitHub as a user token sees it: it has quota, and no such Repository. */ +function userGithub(): UserGithub { + const authorizations: UserGithub['authorizations'] = [] + const fetcher = async (_input: RequestInfo | URL, init?: RequestInit) => { + authorizations.push(new Headers(init?.headers).get('authorization')) + return Response.json({ message: 'Not Found' }, { status: 404 }) + } + return { fetch: fetcher as typeof fetch, authorizations } +} diff --git a/test/unit/skill-run-sweep.test.ts b/test/unit/skill-run-sweep.test.ts index bd99c8a8..3d5d8874 100644 --- a/test/unit/skill-run-sweep.test.ts +++ b/test/unit/skill-run-sweep.test.ts @@ -15,6 +15,7 @@ const MIGRATIONS = [ 'migrations/0112_private_artifact_keys.sql', 'migrations/0122_artifact_resolution_retry_after.sql', 'migrations/0142_artifact_run_checks.sql', + 'migrations/0143_artifact_resolution_requesters.sql', ] const NOW = 1_791_000_000 const archify: RunnableSkill = { owner: 'tt-a1i', repository: 'archify', name: 'archify' } From f22f16356d3a46eab9e4e7206b38fe7af84e7832 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 02:19:46 +1100 Subject: [PATCH 5/8] fix(registry): spend less GitHub quota on the sync and page views GITHUB_TOKEN is a personal token, so the registry shares its hourly quota with every other tool on that account. Measured 2026-10-06: about 1,650 of 1,900 daily sync jobs only advanced the freshness cursor, about 630 /gh views a day re-asked GitHub for a repository it had already said was missing, and the owner and commit reads went out anonymous and failed. The hourly task now reads candidate summaries in GraphQL batches of 100 and records unchanged repositories in place. Sync and page tree reads go by tree SHA. The repository page caches 404s and tree scans at the edge, answers 404 for a private repository, and falls back to ungh.cc metadata when GitHub refuses. Owner and commit reads authenticate through the ETag client, so a repeat read answers 304 for free. Sync pauses name the bucket. --- .../registry/server/api/orgs/[owner].get.ts | 52 ++-- .../server/api/repos/[owner]/[repo].get.ts | 129 +++++++--- .../server/api/skill-related/[...slug].get.ts | 35 +-- .../server/api/skills/featured.get.ts | 48 ++-- .../server/tasks/sync-github-skills.ts | 45 +++- layers/registry/server/utils/github-client.ts | 237 +++++++++++++----- .../server/utils/github-sync-control.ts | 18 +- layers/registry/server/utils/sync-prefetch.ts | 144 +++++++++++ layers/registry/server/utils/sync-repo.ts | 116 +++++---- layers/registry/server/utils/ungh-repo.ts | 72 ++++++ server/jobs/registry/repo-maintenance.ts | 2 + test/unit/github-conditional-read.test.ts | 43 ++++ test/unit/github-repo-summary-batch.test.ts | 99 ++++++++ test/unit/github-sync-control.test.ts | 16 ++ test/unit/repo-source-profile-quota.test.ts | 109 ++++++++ test/unit/repo-source-profile-timeout.test.ts | 3 + test/unit/skill-detail-d1-cost.test.ts | 6 +- test/unit/sync-prefetch.test.ts | 113 +++++++++ test/unit/sync-repo-correctness.test.ts | 2 +- test/unit/sync-repo-freshness.test.ts | 1 + 20 files changed, 1067 insertions(+), 223 deletions(-) create mode 100644 layers/registry/server/utils/sync-prefetch.ts create mode 100644 layers/registry/server/utils/ungh-repo.ts create mode 100644 test/unit/github-conditional-read.test.ts create mode 100644 test/unit/github-repo-summary-batch.test.ts create mode 100644 test/unit/repo-source-profile-quota.test.ts create mode 100644 test/unit/sync-prefetch.test.ts diff --git a/layers/registry/server/api/orgs/[owner].get.ts b/layers/registry/server/api/orgs/[owner].get.ts index 751aaf13..c9b26af2 100644 --- a/layers/registry/server/api/orgs/[owner].get.ts +++ b/layers/registry/server/api/orgs/[owner].get.ts @@ -1,11 +1,12 @@ import type { H3Event } from 'h3' import type { TagPayload } from '../../jobs/generate-tags' +import type { GithubBindings } from '../../utils/github-client' import type { RegistrySkill } from '../../utils/skills-registry' import { getDB } from '#server/utils/db' import { cached, readCache, writeCache } from '#shared/server/cache' import { officialRepos } from '../../data/official-repos' import { TAG_BY_SLUG } from '../../jobs/taxonomy' -import { GITHUB_PAGE_READ_TIMEOUT_MS } from '../../utils/github-client' +import { getGithubJson, GITHUB_PAGE_READ_TIMEOUT_MS, resolveGithubBindings } from '../../utils/github-client' import { resolveRepoSourceIdentitiesForOwner } from '../../utils/repo-source-identity' import { getGeneratedBatch } from '../../utils/skill-generated' import { querySkills } from '../../utils/skills-registry' @@ -68,19 +69,27 @@ const ORG_PROFILE_CACHE_STALE_TTL = 60 * 5 const officialOwners = new Set(officialRepos.map(r => r.owner)) const kindByOwner = new Map(officialRepos.map(r => [r.owner, r.kind])) -async function fetchAndStoreOwner(owner: string, db: D1Database): Promise { - const res = await fetch(`https://api.github.com/users/${owner}`, { - signal: AbortSignal.timeout(GITHUB_PAGE_READ_TIMEOUT_MS), - headers: { - 'User-Agent': 'skilld.dev', - 'Accept': 'application/vnd.github+json', - }, - }).catch(() => { - emitOperationalEvent(createWideEvent({ operation: 'org-profile-fetch', outcome: 'failed' })) - return null - }) +interface GithubOwnerResponse { + name?: string + bio?: string + blog?: string + location?: string + followers?: number + public_repos?: number + type?: string +} + +async function fetchAndStoreOwner(owner: string, db: D1Database, bindings: GithubBindings): Promise { + // Authenticated and conditional: an unchanged profile answers 304 from + // the ETag cache for free. Anonymous, this read shared the Worker IP's 60 + // requests an hour with every other Cloudflare tenant, and failed. + const res = await getGithubJson(`/users/${encodeURIComponent(owner)}`, bindings, { timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS }) + .catch(() => { + emitOperationalEvent(createWideEvent({ operation: 'org-profile-fetch', outcome: 'failed' })) + return null + }) - if (!res || !res.ok) { + if (!res?.data) { if (res?.status === 404) { await db .prepare( @@ -93,16 +102,7 @@ async function fetchAndStoreOwner(owner: string, db: D1Database): Promise { +async function loadOwner(owner: string, db: D1Database, bindings: GithubBindings): Promise { const cached = await db .prepare( `SELECT kind, name, bio, blog, location, followers, last_synced_at, sync_status @@ -145,7 +145,7 @@ async function loadOwner(owner: string, db: D1Database): Promise { emitOperationalEvent(createWideEvent({ operation, @@ -31,6 +48,44 @@ function githubReadFailed(operation: string, error: unknown): FetchOutcome }): SkillFileScan { + return { + status: tree.truncated ? 'truncated' : 'ok', + skillFiles: tree.tree + .filter(entry => entry.type === 'blob' && isRegistrySkillPath(entry.path)) + .map(entry => entry.path) + .sort(), + } +} + +function profileFrom(meta: RepoMeta, scan: SkillFileScan | null): RepoSourceProfile { + const skillFiles = scan?.skillFiles ?? [] + return { + owner: meta.owner.login, + repo: meta.name, + description: meta.description, + githubUrl: meta.html_url || `https://github.com/${meta.owner.login}/${meta.name}`, + defaultBranch: meta.default_branch, + stars: meta.stargazers_count, + forks: meta.forks_count, + pushedAt: meta.pushed_at, + createdAt: meta.created_at, + archived: Boolean(meta.archived), + fork: Boolean(meta.fork), + skillFileScanStatus: scan?.status ?? 'unavailable', + skillFileCount: skillFiles.length, + skillFiles, + } +} + export default defineCachedEventHandler(async (event) => { const ownerParam = getRouterParam(event, 'owner') const repoParam = getRouterParam(event, 'repo') @@ -39,55 +94,61 @@ export default defineCachedEventHandler(async (event) => { const owner = ownerParam.toLowerCase() const repo = repoParam.toLowerCase() + const edgeCache = useStorage('edge-cache') + if (await readCache(edgeCache, missingRepoKey(owner, repo))) + throw createError({ statusCode: 404, message: 'Repository not found' }) + const db = getDB(event) - const source = await resolveRepoSourceIdentity(db, { owner, repo }) + const row = await db + .prepare(`SELECT source_owner, source_repo, last_tree_sha FROM repos WHERE owner = ? AND repo = ?`) + .bind(owner, repo) + .first<{ source_owner: string | null, source_repo: string | null, last_tree_sha: string | null }>() + const source = resolveRepoSourceIdentityFromRow({ owner, repo }, row) const bindings = resolveGithubBindings(event.context.platform.env) // A read that timed out or dropped is an outage, not a missing repository. const repoRes = await getRepo(source.owner, source.repo, bindings, { timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS }) .catch((error: unknown) => githubReadFailed('repo-source-profile-repo', error)) - if (repoRes.status === 404) + // The token can read private repositories, and this page is public, so a + // private one answers exactly like one that does not exist. + if (repoRes.status === 404 || repoRes.data?.private) { + await writeCache(edgeCache, missingRepoKey(owner, repo), true, { ttl: MISSING_REPO_TTL_SECONDS }) throw createError({ statusCode: 404, message: 'Repository not found' }) + } if (!repoRes.data) { emitOperationalEvent(createWideEvent({ 'operation': 'repo-source-profile', 'outcome': 'unavailable', 'upstream.status': repoRes.status || null, })) + // Metadata from ungh.cc keeps the page whole while the shared quota is + // spent. The Skill file list needs the tree, which only GitHub serves. + if (githubRefused(repoRes)) { + const fallback = await readUnghRepoMeta(source.owner, source.repo) + if (fallback._tag === 'found') + return profileFrom(fallback.meta, null) + emitOperationalEvent(createWideEvent({ operation: 'repo-source-profile-ungh', outcome: 'unavailable', reason: fallback.reason })) + } return buildUnavailableRepoSourceProfile(owner, repo) satisfies RepoSourceProfile } const meta = repoRes.data - const repoOwner = meta.owner.login - const repoName = meta.name - const treeRes = await getTree(repoOwner, repoName, meta.default_branch, bindings, { timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS }) - .catch((error: unknown) => githubReadFailed('repo-source-profile-tree', error)) - const skillFiles = (treeRes.data?.tree ?? []) - .filter(entry => entry.type === 'blob' && isRegistrySkillPath(entry.path)) - .map(entry => entry.path) - .sort() - const skillFileScanStatus = !treeRes.data - ? 'unavailable' - : treeRes.data.truncated - ? 'truncated' - : 'ok' + // The sync records the tree it last read. Read at that SHA, the listing + // never changes, so one scan serves every later view. A repository the + // registry has not synced is read at its default branch. + const treeSha = row?.last_tree_sha ?? null + const cachedScan = treeSha + ? await readCache(edgeCache, treeScanKey(meta.owner.login, meta.name, treeSha)) + : null + if (cachedScan) + return profileFrom(meta, cachedScan) satisfies RepoSourceProfile - return { - owner: repoOwner, - repo: repoName, - description: meta.description, - githubUrl: meta.html_url || `https://github.com/${repoOwner}/${repoName}`, - defaultBranch: meta.default_branch, - stars: meta.stargazers_count, - forks: meta.forks_count, - pushedAt: meta.pushed_at, - createdAt: meta.created_at, - archived: Boolean(meta.archived), - fork: Boolean(meta.fork), - skillFileScanStatus, - skillFileCount: skillFiles.length, - skillFiles, - } satisfies RepoSourceProfile + const treeRes = await getTree(meta.owner.login, meta.name, treeSha ?? meta.default_branch, bindings, { timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS }) + .catch((error: unknown) => githubReadFailed('repo-source-profile-tree', error)) + const scan = treeRes.data ? scanTree(treeRes.data) : null + if (scan && treeSha) + await writeCache(edgeCache, treeScanKey(meta.owner.login, meta.name, treeSha), scan, { ttl: TREE_SCAN_TTL_SECONDS }) + return profileFrom(meta, scan) satisfies RepoSourceProfile }, { maxAge: 60 * 15, swr: true, diff --git a/layers/registry/server/api/skill-related/[...slug].get.ts b/layers/registry/server/api/skill-related/[...slug].get.ts index 4125397b..63163716 100644 --- a/layers/registry/server/api/skill-related/[...slug].get.ts +++ b/layers/registry/server/api/skill-related/[...slug].get.ts @@ -1,12 +1,13 @@ import type { H3Event } from 'h3' import type { EmbeddingNeighbor } from '../../jobs/generate-embeddings' +import type { GithubBindings } from '../../utils/github-client' import type { CoOccurrenceNeighbor } from '../../utils/skill-co-occurrence' import type { SkillCommitSourceRow } from '../../utils/skill-commit-source' import type { CachedRelated } from '../../utils/skill-related' import { readCache, readThroughCache, writeCache } from '#shared/server/cache' import { defineApiHandler } from '#shared/server/handler' import { getEmbeddingNeighbors } from '../../jobs/generate-embeddings' -import { GITHUB_PAGE_READ_TIMEOUT_MS } from '../../utils/github-client' +import { getGithubJson, GITHUB_PAGE_READ_TIMEOUT_MS, resolveGithubBindings } from '../../utils/github-client' import { getCoOccurrenceNeighbors } from '../../utils/skill-co-occurrence' import { skillCommitSourceFromRow } from '../../utils/skill-commit-source' import { @@ -76,7 +77,7 @@ export default defineApiHandler({ const [commits, related, coOccurrenceNeighbors, embeddingNeighbors] = await Promise.all([ source - ? getSkillCommits(source.owner, source.repo, source.path) + ? getSkillCommits(source.owner, source.repo, source.path, resolveGithubBindings(platform.env)) : Promise.resolve([]), findRelatedSkills(event, { owner: skill.owner, repo: skill.repo, excludeName: skill.name, limit: 6 }), getCoOccurrenceNeighbors(platform.db, skill.name), @@ -166,26 +167,32 @@ interface GhCommitResponse { author: { login: string, avatar_url: string } | null } -async function getSkillCommits(owner: string, repo: string, path: string): Promise { +async function getSkillCommits(owner: string, repo: string, path: string, bindings: GithubBindings): Promise { const cacheKey = `skills:commits:v2:${owner}/${repo}:${path}` const cached = await readCache(useStorage('edge-cache'), cacheKey) if (cached) return cached - const data = await $fetch(`https://api.github.com/repos/${owner}/${repo}/commits`, { - query: { path, per_page: 5 }, - headers: { 'Accept': 'application/vnd.github+json', 'User-Agent': 'skilld.dev' }, - // ofetch retries a GET once on its own. The commit list is optional - // decoration, so a slow GitHub costs the list, never the page. - retry: 0, - timeout: GITHUB_PAGE_READ_TIMEOUT_MS, - }).catch(() => { - emitOperationalEvent(createWideEvent({ operation: 'skill-related-commits-fetch', outcome: 'failed' })) + // Authenticated and conditional: an unchanged history answers 304 from the + // ETag cache for free. Anonymous, this read shared the Worker IP's 60 + // requests an hour with every other Cloudflare tenant, and about 330 a day + // failed (2026-10-06). The commit list is optional decoration, so a slow + // GitHub costs the list, never the page. + const query = new URLSearchParams({ path, per_page: '5' }) + const response = await getGithubJson( + `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits?${query.toString()}`, + bindings, + { timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS }, + ).catch((error: unknown) => { + emitOperationalEvent(createWideEvent({ operation: 'skill-related-commits-fetch', outcome: 'failed', reason: error instanceof Error ? error.message : String(error) })) return null }) - - if (!Array.isArray(data)) + const data = response?.data + if (!Array.isArray(data)) { + if (response) + emitOperationalEvent(createWideEvent({ 'operation': 'skill-related-commits-fetch', 'outcome': 'failed', 'upstream.status': response.status })) return [] + } const commits: SkillCommit[] = data.map(c => ({ sha: c.sha, diff --git a/layers/registry/server/api/skills/featured.get.ts b/layers/registry/server/api/skills/featured.get.ts index 9c914068..433e6c6c 100644 --- a/layers/registry/server/api/skills/featured.get.ts +++ b/layers/registry/server/api/skills/featured.get.ts @@ -1,8 +1,9 @@ import type { TagPayload } from '../../jobs/generate-tags' +import type { GithubBindings } from '../../utils/github-client' import { defineApiHandler } from '#shared/server/handler' import { officialRepos } from '../../data/official-repos' import { FeaturedSkillsQuery } from '../../schemas/featured-query' -import { GITHUB_PAGE_READ_TIMEOUT_MS } from '../../utils/github-client' +import { getGithubJson, GITHUB_PAGE_READ_TIMEOUT_MS, resolveGithubBindings } from '../../utils/github-client' import { getGeneratedBatch } from '../../utils/skill-generated' import { getFeaturedOfficialSections, getTopReposByCount, getTopReposByStars } from '../../utils/skills-registry' @@ -15,27 +16,30 @@ interface OwnerProfileRow { sync_status: string | null } -async function fetchAndStoreOwner(owner: string, db: D1Database): Promise { - const res = await fetch(`https://api.github.com/users/${owner}`, { - signal: AbortSignal.timeout(GITHUB_PAGE_READ_TIMEOUT_MS), - headers: { 'User-Agent': 'skilld.dev', 'Accept': 'application/vnd.github+json' }, - }).catch(() => { - emitOperationalEvent(createWideEvent({ operation: 'featured-owner-fetch', outcome: 'failed' })) - return null - }) +interface GithubOwnerResponse { + name?: string + bio?: string + blog?: string + location?: string + followers?: number + public_repos?: number + type?: string +} - if (!res?.ok) +async function fetchAndStoreOwner(owner: string, db: D1Database, bindings: GithubBindings): Promise { + // Authenticated and conditional: an unchanged profile answers 304 from + // the ETag cache for free. Anonymous, this read shared the Worker IP's 60 + // requests an hour with every other Cloudflare tenant, and failed. + const res = await getGithubJson(`/users/${encodeURIComponent(owner)}`, bindings, { timeoutMs: GITHUB_PAGE_READ_TIMEOUT_MS }) + .catch(() => { + emitOperationalEvent(createWideEvent({ operation: 'featured-owner-fetch', outcome: 'failed' })) + return null + }) + + if (!res?.data) return null - const data = await res.json() as { - name?: string - bio?: string - blog?: string - location?: string - followers?: number - public_repos?: number - type?: string - } + const data = res.data const kind = data.type === 'Organization' ? 'org' : 'user' await db.prepare( @@ -66,7 +70,7 @@ async function fetchAndStoreOwner(owner: string, db: D1Database): Promise> { +async function loadOwnerProfiles(owners: string[], db: D1Database, bindings: GithubBindings): Promise> { const profiles = new Map() const now = Math.floor(Date.now() / 1000) @@ -84,7 +88,7 @@ async function loadOwnerProfiles(owners: string[], db: D1Database): Promise s.owner), platform.db) + const profileMap = await loadOwnerProfiles(devSections.map(s => s.owner), platform.db, resolveGithubBindings(platform.env)) const enriched = sections.map(enrichSkills) const enrichedDevs = devSections.map((section) => { const profile = profileMap.get(section.owner) diff --git a/layers/registry/server/tasks/sync-github-skills.ts b/layers/registry/server/tasks/sync-github-skills.ts index 403f0717..48c6d06a 100644 --- a/layers/registry/server/tasks/sync-github-skills.ts +++ b/layers/registry/server/tasks/sync-github-skills.ts @@ -8,6 +8,8 @@ import { SUBSCRIBED_REPO_STALE_SECONDS, } from '~~/server/utils/sync-thresholds' import { observedSchedulePolicy } from '#shared/schedule-policy' +import { resolveGithubBindings } from '../utils/github-client' +import { githubSyncPermit } from '../utils/github-sync-control' import { appendRepoReverificationCandidates, DISCOVERY_SYNC_CANDIDATES_SQL, @@ -18,6 +20,7 @@ import { STAGE_HISTORICAL_DISCOVERY_CANDIDATES_SQL, SUBSCRIBED_SYNC_CANDIDATES_SQL, } from '../utils/sync-candidates' +import { prefetchUnchangedRepos } from '../utils/sync-prefetch' const MAX_REPOS_PER_RUN = 250 const MAX_HISTORICAL_CANDIDATES_PER_RUN = 250 @@ -106,26 +109,47 @@ export default defineScheduledTask({ { limit: MAX_REPOS_PER_RUN, maxReverified: MAX_REVERIFIED_REPOS_PER_RUN }, ) - if (queue.length === 0) { + const reverified = queue.length - ordered.length + const candidates = queue.map(({ owner, repo, ownerVerified }) => ({ + owner, + repo, + ownerVerified, + claimDiscovery: discoveryKeys.has(`${owner}/${repo}`), + })) + // While the sync is paused, every job would release itself unread, so + // a prefetch would only spend the quota the pause is saving. + const permit = await githubSyncPermit(db, now) + const prefetch = permit._tag === 'paused' + ? { _tag: 'unread' as const, queue: candidates, reason: 'sync-paused' } + : await prefetchUnchangedRepos({ db, bindings: resolveGithubBindings(env), now }, candidates) + .catch((error: unknown) => { + // The jobs read every repository themselves, as before the + // prefetch existed, so the run still completes. + emitOperationalEvent(createWideEvent({ + operation: 'sync-github-skills-prefetch', + outcome: 'failed', + reason: error instanceof Error ? error.message : String(error), + })) + return { _tag: 'unread' as const, queue: candidates, reason: 'prefetch-threw' } + }) + const prefetched = prefetch._tag === 'prefetched' + ? { unchanged: prefetch.unchanged, graphqlRequests: prefetch.requests } + : { unread: prefetch.reason } + + if (prefetch.queue.length === 0) { await reportJobRun(db, 'sync-github-skills', { cron: CRON, status: 'ok', durationMs: Date.now() - startedAt, }) - return { result: { queued: 0, deferred, stagedHistorical, reverified: 0 } } + return { result: { queued: 0, deferred, stagedHistorical, reverified, prefetched } } } const batch = await createRegistryJobBatch( env as Cloudflare.Env & Record, { name: `registry-sync:${now}`, - jobs: queue.map(({ owner, repo, ownerVerified }) => ({ - operation: 'sync', - owner, - repo, - ownerVerified, - claimDiscovery: discoveryKeys.has(`${owner}/${repo}`), - })), + jobs: prefetch.queue.map(candidate => ({ operation: 'sync' as const, ...candidate })), }, ) const dispatchFailed = batch.dispatched.filter(result => result.status !== 'sent') @@ -145,7 +169,8 @@ export default defineScheduledTask({ queued: batch.jobIds.length, deferred, stagedHistorical, - reverified: queue.length - ordered.length, + reverified, + prefetched, dispatchFailed: dispatchFailed.length, }, } diff --git a/layers/registry/server/utils/github-client.ts b/layers/registry/server/utils/github-client.ts index 565dcc78..851ee513 100644 --- a/layers/registry/server/utils/github-client.ts +++ b/layers/registry/server/utils/github-client.ts @@ -101,6 +101,12 @@ export interface RateLimitInfo { remaining: number limit: number reset: number + /** + * The bucket GitHub counted the request against: `core` for REST and + * `graphql` for GraphQL. The two are separate quotas. A pause reason that + * said only "163 requests remaining" could not tell which one ran out. + */ + resource: string | null } export interface FetchOutcome { @@ -149,6 +155,7 @@ function parseRateLimit(headers: Headers): RateLimitInfo | null { remaining: Number(remaining), limit: Number(headers.get('x-ratelimit-limit') ?? 0), reset: Number(headers.get('x-ratelimit-reset') ?? 0), + resource: headers.get('x-ratelimit-resource'), } } @@ -197,6 +204,26 @@ async function ghRequest( return { status: res.status, data: body, rateLimit, notModified: false } } +/** + * One authenticated, conditional GET of an API path, such as + * `/users/nuxt`. A 304 answers from the KV ETag cache and costs no quota. + */ +export async function getGithubJson( + path: `/${string}`, + bindings: GithubBindings, + options?: GithubReadOptions, +): Promise> { + return ghRequest(`${API_BASE}${path}`, bindings, readInit(options)) +} + +/** + * Whether GitHub refused a read for want of quota or capacity, as opposed to + * answering it. A timeout or dropped connection reads as status 0. + */ +export function githubRefused(outcome: Pick, 'status'>): boolean { + return outcome.status === 0 || outcome.status === 403 || outcome.status === 429 || outcome.status >= 500 +} + export async function getRepo( owner: string, repo: string, @@ -206,31 +233,52 @@ export async function getRepo( return ghRequest(`${API_BASE}/repos/${owner}/${repo}`, bindings, readInit(options)) } -interface RepoSummaryGqlResponse { - repository: { +interface RepoSummaryGql { + name: string + nameWithOwner: string + url: string + owner: { login: string } + description: string | null + stargazerCount: number + forkCount: number + pushedAt: string + createdAt: string + isArchived: boolean + isFork: boolean + defaultBranchRef: { name: string - nameWithOwner: string - url: string - owner: { login: string } - description: string | null - stargazerCount: number - forkCount: number - pushedAt: string - createdAt: string - isArchived: boolean - isFork: boolean - defaultBranchRef: { - name: string - target: { oid: string, tree: { oid: string } } | null - } | null + target: { oid: string, tree: { oid: string } } | null } | null } +const REPO_SUMMARY_FIELDS = `name nameWithOwner url owner{login} + description stargazerCount forkCount pushedAt createdAt isArchived isFork + defaultBranchRef{name target{... on Commit{oid tree{oid}}}}` + export interface RepoSummary { meta: RepoMeta headTreeSha: string | null } +function repoSummaryFromGql(r: RepoSummaryGql): RepoSummary { + const branch = r.defaultBranchRef?.name || 'main' + const meta: RepoMeta = { + name: r.name, + full_name: r.nameWithOwner, + html_url: r.url, + owner: { login: r.owner.login }, + default_branch: branch, + description: r.description, + stargazers_count: r.stargazerCount, + forks_count: r.forkCount, + pushed_at: r.pushedAt, + created_at: r.createdAt, + archived: r.isArchived, + fork: r.isFork, + } + return { meta, headTreeSha: r.defaultBranchRef?.target?.tree.oid ?? null } +} + /** * GraphQL-backed combo of getRepo + head-tree-SHA. One subrequest replaces * the REST getRepo (1) and lets sync-repo skip the REST getTree call (1) @@ -248,59 +296,92 @@ export async function getRepoSummary( ): Promise> { const query = `query($owner:String!,$repo:String!){ repository(owner:$owner,name:$repo){ - name nameWithOwner url owner{login} - description stargazerCount forkCount pushedAt createdAt isArchived isFork - defaultBranchRef{name target{... on Commit{oid tree{oid}}}} + ${REPO_SUMMARY_FIELDS} } }` - const headers = new Headers() - headers.set('Accept', 'application/vnd.github+json') - headers.set('Content-Type', 'application/json') - headers.set('User-Agent', 'skilld.dev') - if (bindings.GITHUB_TOKEN) - headers.set('Authorization', `Bearer ${bindings.GITHUB_TOKEN}`) - - const res = await fetch(GRAPHQL_URL, { - method: 'POST', - headers, - body: JSON.stringify({ query, variables: { owner, repo } }), - }) - const rateLimit = parseRateLimit(res.headers) - if (!res.ok) - return { status: res.status, data: null, rateLimit, notModified: false } - - const body = await res.json() as { data?: RepoSummaryGqlResponse, errors?: Array<{ type?: string, message?: string }> } + const out = await gqlRequest<{ repository: RepoSummaryGql | null }>(query, { owner, repo }, bindings) + if (out._tag === 'failed') + return { status: out.status, data: null, rateLimit: out.rateLimit, notModified: false } // GraphQL surfaces NOT_FOUND in errors with HTTP 200. Map it to 404 so // callers preserve their existing rate-limit / broken-repo branching. - if (body.errors?.length) { - const notFound = body.errors.some(e => e.type === 'NOT_FOUND') - return { status: notFound ? 404 : 502, data: null, rateLimit, notModified: false } + if (out.errors.length) { + const notFound = out.errors.some(e => e.type === 'NOT_FOUND') + return { status: notFound ? 404 : 502, data: null, rateLimit: out.rateLimit, notModified: false } } - const r = body.data?.repository + const r = out.data?.repository if (!r) - return { status: 404, data: null, rateLimit, notModified: false } + return { status: 404, data: null, rateLimit: out.rateLimit, notModified: false } + return { status: 200, data: repoSummaryFromGql(r), rateLimit: out.rateLimit, notModified: false } +} - const branch = r.defaultBranchRef?.name || 'main' - const meta: RepoMeta = { - name: r.name, - full_name: r.nameWithOwner, - html_url: r.url, - owner: { login: r.owner.login }, - default_branch: branch, - description: r.description, - stargazers_count: r.stargazerCount, - forks_count: r.forkCount, - pushed_at: r.pushedAt, - created_at: r.createdAt, - archived: r.isArchived, - fork: r.isFork, +/** + * Repositories one batched summary query reads. GitHub prices a query by the + * connections it asks for, and a summary asks for none, so one query of 100 + * costs the same single point as one query of 1. 100 is GitHub's ceiling for + * nodes per connection, and keeps the query under 15 KB. + */ +export const REPO_SUMMARY_BATCH_SIZE = 100 + +export interface RepoSummaryRequest { + owner: string + repo: string +} + +export type RepoSummaryBatchOutcome + = | { + _tag: 'read' + /** + * One entry per request, in request order. Null means GitHub gave no + * summary for that alias, such as NOT_FOUND. The caller leaves those to + * the per-repository sync, which owns the missing and renamed verdicts. + */ + summaries: Array + rateLimit: RateLimitInfo | null + /** GraphQL requests sent. */ + requests: number } - return { - status: 200, - data: { meta, headTreeSha: r.defaultBranchRef?.target?.tree.oid ?? null }, - rateLimit, - notModified: false, + | { _tag: 'failed', status: number, rateLimit: RateLimitInfo | null, requests: number } + +/** + * The summary of many repositories in aliased GraphQL queries of up to + * {@link REPO_SUMMARY_BATCH_SIZE}. + * + * The hourly sync used to send one summary query per repository from its own + * queue job. About 87% of those found the tree unchanged (1,649 of 1,900 jobs + * on 2026-10-06), so the job and its query did nothing but advance the + * freshness cursor. + */ +export async function getRepoSummariesBatch( + requests: RepoSummaryRequest[], + bindings: GithubBindings, +): Promise { + const summaries: Array = [] + let rateLimit: RateLimitInfo | null = null + let sent = 0 + for (const batch of chunk(requests, REPO_SUMMARY_BATCH_SIZE)) { + const varDecls = batch.flatMap((_, i) => [`$o${i}:String!`, `$n${i}:String!`]) + const aliases = batch.map((_, i) => `r${i}:repository(owner:$o${i},name:$n${i}){${REPO_SUMMARY_FIELDS}}`).join(' ') + const variables: Record = {} + batch.forEach((request, i) => { + variables[`o${i}`] = request.owner + variables[`n${i}`] = request.repo + }) + sent++ + const out = await gqlRequest>(`query(${varDecls.join(',')}){${aliases}}`, variables, bindings) + rateLimit = out.rateLimit ?? rateLimit + if (out._tag === 'failed') + return { _tag: 'failed', status: out.status, rateLimit, requests: sent } + // A NOT_FOUND names one alias and leaves the others whole. Any other + // error type, such as RATE_LIMITED, says nothing trustworthy about the + // batch, so it fails as a whole and the per-repository sync takes over. + if (out.errors.some(error => error.type !== 'NOT_FOUND')) + return { _tag: 'failed', status: 502, rateLimit, requests: sent } + for (let i = 0; i < batch.length; i++) { + const r = out.data?.[`r${i}`] + summaries.push(r ? repoSummaryFromGql(r) : null) + } } + return { _tag: 'read', summaries, rateLimit, requests: sent } } export async function getTree( @@ -341,11 +422,18 @@ export async function getCommits( ) } -async function gqlPost( +interface GqlError { type?: string, message?: string } + +type GqlRequestOutcome + = | { _tag: 'answered', data: T | null, errors: GqlError[], rateLimit: RateLimitInfo | null } + | { _tag: 'failed', status: number, rateLimit: RateLimitInfo | null } + +/** One GraphQL POST. Partial data and per-alias errors come back together. */ +async function gqlRequest( query: string, variables: Record, bindings: GithubBindings, -): Promise<{ status: number, data: T | null, rateLimit: RateLimitInfo | null }> { +): Promise> { const headers = new Headers() headers.set('Accept', 'application/vnd.github+json') headers.set('Content-Type', 'application/json') @@ -359,21 +447,32 @@ async function gqlPost( }) const rateLimit = parseRateLimit(res.headers) if (!res.ok) - return { status: res.status, data: null, rateLimit } + return { _tag: 'failed', status: res.status, rateLimit } // A gateway can answer 200 with a truncated or non-JSON body. Parsing that // eagerly threw `Unexpected end of JSON input` out of the client and reached // the sync summary as an opaque reason with no status attached. const body = await res.json().catch(() => { emitOperationalEvent(createWideEvent({ operation: 'github-graphql-parse', outcome: 'invalid-response' })) return null - }) as { data?: T, errors?: Array<{ type?: string, message?: string }> } | null + }) as { data?: T, errors?: GqlError[] } | null if (!body) - return { status: 502, data: null, rateLimit } - if (body.errors?.length) { - const notFound = body.errors.some(e => e.type === 'NOT_FOUND') - return { status: notFound ? 404 : 502, data: null, rateLimit } + return { _tag: 'failed', status: 502, rateLimit } + return { _tag: 'answered', data: body.data ?? null, errors: body.errors ?? [], rateLimit } +} + +async function gqlPost( + query: string, + variables: Record, + bindings: GithubBindings, +): Promise<{ status: number, data: T | null, rateLimit: RateLimitInfo | null }> { + const out = await gqlRequest(query, variables, bindings) + if (out._tag === 'failed') + return { status: out.status, data: null, rateLimit: out.rateLimit } + if (out.errors.length) { + const notFound = out.errors.some(e => e.type === 'NOT_FOUND') + return { status: notFound ? 404 : 502, data: null, rateLimit: out.rateLimit } } - return { status: 200, data: body.data ?? null, rateLimit } + return { status: 200, data: out.data, rateLimit: out.rateLimit } } export interface BlobBatchOutcome extends FetchOutcome> { diff --git a/layers/registry/server/utils/github-sync-control.ts b/layers/registry/server/utils/github-sync-control.ts index 8e28ff28..3ee37942 100644 --- a/layers/registry/server/utils/github-sync-control.ts +++ b/layers/registry/server/utils/github-sync-control.ts @@ -57,11 +57,24 @@ export function githubRatePauseUntil( return Math.min(now + 12 * 60 * 60, Math.max(fallback, (resetAt ?? 0) + 5)) } +/** + * Requests the sync leaves unspent in a GitHub bucket before it pauses. + * + * `GITHUB_TOKEN` is a personal token, and GitHub counts its quota per + * account, so every tool on that account draws on the same bucket. The sync + * spends about 10 REST requests and 130 GraphQL points an hour, and page + * views about 50 REST requests at most. 200 covers page views for the rest + * of any hour once the sync stops. + */ +export const GITHUB_SYNC_RESERVE = 200 + export function githubSyncPauseDecision(input: { owner: string repo: string now: number remaining?: number + /** The bucket `remaining` counts, from `x-ratelimit-resource`. */ + resource?: string resetAt?: number rateLimited: boolean unauthorized: boolean @@ -73,11 +86,12 @@ export function githubSyncPauseDecision(input: { reason: `${input.owner}/${input.repo}: GitHub credential rejected`, } } - if (!input.rateLimited && (input.remaining == null || input.remaining >= 200)) + if (!input.rateLimited && (input.remaining == null || input.remaining >= GITHUB_SYNC_RESERVE)) return { _tag: 'continue' } + const bucket = input.resource ? `${input.resource} ` : '' return { _tag: 'pause', pauseUntil: githubRatePauseUntil(input.now, input.resetAt), - reason: `${input.owner}/${input.repo}: ${input.rateLimited ? 'rate limited' : `${input.remaining} requests remaining`}`, + reason: `${input.owner}/${input.repo}: ${input.rateLimited ? 'rate limited' : `${input.remaining} ${bucket}requests remaining`}`, } } diff --git a/layers/registry/server/utils/sync-prefetch.ts b/layers/registry/server/utils/sync-prefetch.ts new file mode 100644 index 00000000..feaf7981 --- /dev/null +++ b/layers/registry/server/utils/sync-prefetch.ts @@ -0,0 +1,144 @@ +/// + +import type { GithubBindings, RepoSummary } from './github-client' +import type { ExistingRepo } from './sync-repo' +import { getRepoSummariesBatch } from './github-client' +import { resolveRepoSourceIdentityFromRow } from './repo-source-identity' +import { repoUnchangedStatements, unchangedRepoStatus } from './sync-repo' + +/** One repository the hourly sync is about to queue. */ +export interface SyncCandidate { + owner: string + repo: string + ownerVerified: boolean + claimDiscovery: boolean +} + +export type SyncPrefetchResult + = | { + _tag: 'prefetched' + /** Candidates that still need a sync job. */ + queue: SyncCandidate[] + /** Repositories recorded as unchanged without a job. */ + unchanged: number + /** GraphQL requests the prefetch sent. */ + requests: number + } + | { + _tag: 'unread' + /** Every candidate: the per-repository sync reads each one itself. */ + queue: SyncCandidate[] + reason: string + } + +export interface SyncPrefetchDependencies { + db: D1Database + bindings: GithubBindings + /** Unix seconds. */ + now: number +} + +/** A sync job that touched its progress row this recently may still hold it. */ +const PROGRESS_ACTIVE_SECONDS = 30 * 60 + +/** Repositories whose unchanged writes share one D1 batch. */ +const WRITE_BATCH_REPOSITORIES = 25 + +interface PrefetchRow extends ExistingRepo { + owner: string + repo: string + has_skills: number + in_progress: number +} + +/** + * Read every eligible candidate's summary in batched GraphQL queries, record + * the unchanged ones in place, and return the candidates that still need a job. + * + * Each candidate used to get a queue job whose first act was one GraphQL + * summary read. 1,649 of 1,900 such jobs on 2026-10-06 found the tree + * unchanged and only advanced the freshness cursor. The cursor writes here + * are the same statements `syncRepo` runs on that path. + * + * A discovery claim, a job still holding the repository's progress row, a + * Skill-less repository and any repository GitHub gave no summary for keep + * their job: `syncRepo` owns those verdicts. + */ +export async function prefetchUnchangedRepos( + dependencies: SyncPrefetchDependencies, + candidates: SyncCandidate[], +): Promise { + const eligible = candidates.filter(candidate => !candidate.claimDiscovery) + if (eligible.length === 0) + return { _tag: 'prefetched', queue: candidates, unchanged: 0, requests: 0 } + + const rows = await loadPrefetchRows(dependencies.db, eligible, dependencies.now) + const requests = eligible.map(candidate => resolveRepoSourceIdentityFromRow(candidate, rows.get(repoKey(candidate)))) + const batch = await getRepoSummariesBatch(requests, dependencies.bindings) + if (batch._tag === 'failed') + return { _tag: 'unread', queue: candidates, reason: `graphql-${batch.status}` } + + const unchanged = new Set() + const writes: D1PreparedStatement[][] = [] + eligible.forEach((candidate, index) => { + const row = rows.get(repoKey(candidate)) + const summary = batch.summaries[index] + if (!row || !summary || row.in_progress === 1 || !isUnchanged(row, summary)) + return + unchanged.add(repoKey(candidate)) + writes.push(repoUnchangedStatements(dependencies.db, { + owner: candidate.owner, + repo: candidate.repo, + meta: summary.meta, + checkedAt: dependencies.now, + ownerVerified: candidate.ownerVerified, + })) + }) + for (let offset = 0; offset < writes.length; offset += WRITE_BATCH_REPOSITORIES) + await dependencies.db.batch(writes.slice(offset, offset + WRITE_BATCH_REPOSITORIES).flat()) + + return { + _tag: 'prefetched', + queue: candidates.filter(candidate => !unchanged.has(repoKey(candidate))), + unchanged: unchanged.size, + requests: batch.requests, + } +} + +function isUnchanged(row: PrefetchRow, summary: RepoSummary): boolean { + const pushedAt = Date.parse(summary.meta.pushed_at) + return unchangedRepoStatus({ + existing: row, + hasAdmittedSkills: row.has_skills === 1, + headTreeSha: summary.headTreeSha, + repoPushedAt: Number.isFinite(pushedAt) ? Math.floor(pushedAt / 1000) : null, + }) !== null +} + +function repoKey(identity: { owner: string, repo: string }): string { + return `${identity.owner}/${identity.repo}` +} + +/** One D1 read for every candidate: a JSON array binds as a single parameter. */ +async function loadPrefetchRows( + db: D1Database, + candidates: SyncCandidate[], + now: number, +): Promise> { + const pairs = JSON.stringify(candidates.map(candidate => [candidate.owner, candidate.repo])) + const result = await db.prepare( + `WITH candidates(owner, repo) AS ( + SELECT json_extract(value, '$[0]'), json_extract(value, '$[1]') + FROM json_each(?1) + ) + SELECT r.owner, r.repo, r.last_tree_sha, r.pushed_at, r.source_owner, r.source_repo, + EXISTS (SELECT 1 FROM skills s WHERE s.owner = r.owner AND s.repo = r.repo) AS has_skills, + EXISTS ( + SELECT 1 FROM repo_sync_progress p + WHERE p.owner = r.owner AND p.repo = r.repo AND p.updated_at > ?2 + ) AS in_progress + FROM candidates c + JOIN repos r ON r.owner = c.owner AND r.repo = c.repo`, + ).bind(pairs, now - PROGRESS_ACTIVE_SECONDS).all() + return new Map((result.results ?? []).map(row => [repoKey(row), row])) +} diff --git a/layers/registry/server/utils/sync-repo.ts b/layers/registry/server/utils/sync-repo.ts index f7844afa..72df3bfa 100644 --- a/layers/registry/server/utils/sync-repo.ts +++ b/layers/registry/server/utils/sync-repo.ts @@ -25,6 +25,8 @@ export interface SyncRepoStats { revisionsInserted: number activityEmitted: number rateLimitRemaining?: number + /** The GitHub bucket `rateLimitRemaining` counts: `core` or `graphql`. */ + rateLimitResource?: string rateLimitResetAt?: number continuation?: SyncRepoContinuation } @@ -52,7 +54,7 @@ interface ExistingSkillAssets { assets: string } -interface ExistingRepo { +export interface ExistingRepo { last_tree_sha: string | null pushed_at: number | null source_owner: string | null @@ -358,7 +360,7 @@ export async function refreshRepoAssets( SET source_owner = ?, source_repo = ? WHERE owner = ? AND repo = ? `).bind(sourceOwner, sourceRepo, owner, repo).run() - const treeRes = await getTree(sourceOwner, sourceRepo, branch, bindings) + const treeRes = await getTree(sourceOwner, sourceRepo, repoRes.data.headTreeSha ?? branch, bindings) logRateLimit(`asset-backfill tree ${owner}/${repo}`, treeRes.rateLimit) const rateLimitRemaining = Math.min( rate.rateLimitRemaining ?? Number.POSITIVE_INFINITY, @@ -516,18 +518,62 @@ async function repoHasAdmittedSkills(db: D1Database, owner: string, repo: string return row?.admitted === 1 } -async function markUnchangedOwnerVerified( +/** + * Why a repository needs no content sync this hour, or null when it does. + * + * Only a repository with admitted Skills qualifies: a Skill-less candidate is + * still being discovered, so its tree must be read whatever the cursor says. + */ +export function unchangedRepoStatus(input: { + existing: ExistingRepo | null + hasAdmittedSkills: boolean + headTreeSha: string | null + repoPushedAt: number | null +}): 'skipped-tree-sha' | 'skipped-pushed-at' | null { + const { existing, hasAdmittedSkills, headTreeSha, repoPushedAt } = input + if (!hasAdmittedSkills || !existing?.last_tree_sha) + return null + if (headTreeSha && existing.last_tree_sha === headTreeSha) + return 'skipped-tree-sha' + if (existing.pushed_at != null && repoPushedAt != null && existing.pushed_at >= repoPushedAt) + return 'skipped-pushed-at' + return null +} + +/** + * The writes that record an unchanged repository: fresh metadata, the + * freshness cursor, the day's star count and, for an owner-verified repository, + * the verified flag on every Skill. + */ +export function repoUnchangedStatements( db: D1Database, - owner: string, - repo: string, - meta: RepoMeta, - pushedAt: number | null, - checkedAt: number, -): Promise { - await db.batch([ + input: { + owner: string + repo: string + meta: RepoMeta + checkedAt: number + ownerVerified: boolean + }, +): D1PreparedStatement[] { + const { owner, repo, meta, checkedAt } = input + const pushedAt = epoch(meta.pushed_at) + const statements = [ markRepoSummaryCheckedStatement(db, owner, repo, meta, pushedAt, checkedAt), clearRepoMissingSkillsStatement(db, owner, repo, checkedAt), ...repoStarObservationStatements(db, owner, repo, meta.stargazers_count ?? 0, checkedAt), + ] + return input.ownerVerified + ? [...statements, ...ownerVerifiedStatements(db, owner, repo, checkedAt)] + : statements +} + +function ownerVerifiedStatements( + db: D1Database, + owner: string, + repo: string, + checkedAt: number, +): D1PreparedStatement[] { + return [ db.prepare( `UPDATE skills SET owner_verified = 1 @@ -542,7 +588,7 @@ async function markUnchangedOwnerVerified( queued_at = excluded.queued_at, attempts = 0`, ).bind(checkedAt, owner, repo), - ]) + ] } /** @@ -726,12 +772,13 @@ export async function syncRepo( revisionsInserted: 0, activityEmitted: 0, } - const trackRateLimit = (rateLimit: { remaining: number, reset: number } | null): void => { + const trackRateLimit = (rateLimit: { remaining: number, reset: number, resource: string | null } | null): void => { if (!rateLimit) return - stats.rateLimitRemaining = stats.rateLimitRemaining == null - ? rateLimit.remaining - : Math.min(stats.rateLimitRemaining, rateLimit.remaining) + if (stats.rateLimitRemaining == null || rateLimit.remaining < stats.rateLimitRemaining) { + stats.rateLimitRemaining = rateLimit.remaining + stats.rateLimitResource = rateLimit.resource ?? undefined + } stats.rateLimitResetAt = Math.max(stats.rateLimitResetAt ?? 0, rateLimit.reset) } @@ -775,39 +822,24 @@ export async function syncRepo( const hasAdmittedSkills = await repoHasAdmittedSkills(db, owner, repo) const markUnchanged = async (status: 'skipped-tree-sha' | 'skipped-pushed-at'): Promise => { - if (opts.ownerVerified) { - await markUnchangedOwnerVerified(db, owner, repo, meta, repoPushedAt, checkedAt) - stats.status = 'verified-only' - return stats - } - await db.batch([ - markRepoSummaryCheckedStatement(db, owner, repo, meta, repoPushedAt, checkedAt), - clearRepoMissingSkillsStatement(db, owner, repo, checkedAt), - ...repoStarObservationStatements(db, owner, repo, meta.stargazers_count ?? 0, checkedAt), - ]) - stats.status = status + await db.batch(repoUnchangedStatements(db, { owner, repo, meta, checkedAt, ownerVerified: opts.ownerVerified === true })) + stats.status = opts.ownerVerified ? 'verified-only' : status return stats } // GraphQL gave us the head tree SHA in the same request. If it matches // our cached value, the repo is unchanged and we skip the REST getTree // call entirely. Skill-less candidates deliberately bypass this cursor. - if (!opts.continuation && !opts.forceContent && hasAdmittedSkills && existingRepo?.last_tree_sha && headTreeSha && existingRepo.last_tree_sha === headTreeSha) - return markUnchanged('skipped-tree-sha') - - if ( - !opts.continuation - && !opts.forceContent - && hasAdmittedSkills - && existingRepo?.pushed_at != null - && existingRepo.last_tree_sha != null - && repoPushedAt != null - && existingRepo.pushed_at >= repoPushedAt - ) { - return markUnchanged('skipped-pushed-at') - } - - const treeRes = await getTree(sourceOwner, sourceRepo, branch, bindings) + const unchanged = opts.continuation || opts.forceContent + ? null + : unchangedRepoStatus({ existing: existingRepo, hasAdmittedSkills, headTreeSha, repoPushedAt }) + if (unchanged) + return markUnchanged(unchanged) + + // The head tree SHA names the exact tree the summary saw. A branch name + // can move between the two reads, and a SHA URL is immutable, so its ETag + // cache entry answers 304 for as long as the tree stands. + const treeRes = await getTree(sourceOwner, sourceRepo, headTreeSha ?? branch, bindings) logRateLimit(`tree ${owner}/${repo}`, treeRes.rateLimit) trackRateLimit(treeRes.rateLimit) diff --git a/layers/registry/server/utils/ungh-repo.ts b/layers/registry/server/utils/ungh-repo.ts new file mode 100644 index 00000000..2673261f --- /dev/null +++ b/layers/registry/server/utils/ungh-repo.ts @@ -0,0 +1,72 @@ +import type { RepoMeta } from './github-client' +import { z } from 'zod' +import { GITHUB_PAGE_READ_TIMEOUT_MS } from './github-client' + +const unghRepoSchema = z.object({ + repo: z.object({ + name: z.string().min(1), + repo: z.string().regex(/^[^/]+\/[^/]+$/), + description: z.string().nullable().optional(), + createdAt: z.string(), + pushedAt: z.string(), + stars: z.number().int().nonnegative(), + forks: z.number().int().nonnegative(), + defaultBranch: z.string().min(1), + }), +}) + +export type UnghRepoResult + = | { _tag: 'found', meta: RepoMeta } + | { _tag: 'unavailable', reason: string } + +/** + * Public repository metadata from ungh.cc, the unjs GitHub proxy that caches + * anonymous reads. + * + * A page-view fallback for when `GITHUB_TOKEN` is spent or GitHub is down. It + * answers metadata only. It never serves a tree or file content, and nothing + * built or signed reads it. ungh.cc knows no archived or fork flag, so both + * read false. Its 404 is no verdict about GitHub, so it reads as unavailable. + */ +export async function readUnghRepoMeta( + owner: string, + repo: string, + fetcher: typeof fetch = fetch, +): Promise { + const response = await fetcher(`https://ungh.cc/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}`, { + headers: { 'User-Agent': 'skilld.dev', 'Accept': 'application/json' }, + signal: AbortSignal.timeout(GITHUB_PAGE_READ_TIMEOUT_MS), + }).catch((error: unknown) => ({ _tag: 'threw' as const, reason: error instanceof Error ? error.message : String(error) })) + if ('_tag' in response) + return { _tag: 'unavailable', reason: response.reason } + if (!response.ok) { + await response.body?.cancel() + return { _tag: 'unavailable', reason: `ungh ${response.status}` } + } + const parsed = unghRepoSchema.safeParse(await response.json().catch(() => { + // A body that is not JSON fails the schema below, which reports it. + return null + })) + if (!parsed.success) + return { _tag: 'unavailable', reason: 'ungh invalid response' } + const { repo: found } = parsed.data + const [login] = found.repo.split('/') as [string, string] + return { + _tag: 'found', + meta: { + name: found.name, + full_name: found.repo, + html_url: `https://github.com/${found.repo}`, + owner: { login }, + default_branch: found.defaultBranch, + description: found.description ?? null, + stargazers_count: found.stars, + forks_count: found.forks, + pushed_at: found.pushedAt, + created_at: found.createdAt, + archived: false, + fork: false, + private: false, + }, + } +} diff --git a/server/jobs/registry/repo-maintenance.ts b/server/jobs/registry/repo-maintenance.ts index dc8d16b9..1559f297 100644 --- a/server/jobs/registry/repo-maintenance.ts +++ b/server/jobs/registry/repo-maintenance.ts @@ -190,6 +190,7 @@ async function applyRateGuard( repo: string now: number remaining?: number + resource?: string resetAt?: number rateLimited: boolean unauthorized: boolean @@ -333,6 +334,7 @@ export async function handleRegistryRepoJob( repo: payload.repo, now, remaining: stats.rateLimitRemaining, + resource: stats.rateLimitResource, resetAt: stats.rateLimitResetAt, rateLimited: stats.status === 'rate-limited', unauthorized: stats.status === 'unauthorized', diff --git a/test/unit/github-conditional-read.test.ts b/test/unit/github-conditional-read.test.ts new file mode 100644 index 00000000..1734cab7 --- /dev/null +++ b/test/unit/github-conditional-read.test.ts @@ -0,0 +1,43 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { getGithubJson } from '../../layers/registry/server/utils/github-client' + +/** A KV namespace over a Map: the ETag cache the client writes and reads. */ +function memoryKv(): KVNamespace { + const values = new Map() + return { + get: async (key: string, type?: string) => { + const value = values.get(key) + return value === undefined ? null : type === 'json' ? JSON.parse(value) : value + }, + put: async (key: string, value: string) => { + values.set(key, value) + }, + } as unknown as KVNamespace +} + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('getGithubJson', () => { + it('asks again with the stored ETag and answers a 304 from the cache', async () => { + const seen: Array<{ authorization: string | null, ifNoneMatch: string | null }> = [] + vi.stubGlobal('fetch', vi.fn(async (_url: string, init: RequestInit) => { + const headers = new Headers(init.headers) + seen.push({ authorization: headers.get('authorization'), ifNoneMatch: headers.get('if-none-match') }) + return headers.get('if-none-match') === '"v1"' + ? new Response(null, { status: 304 }) + : new Response(JSON.stringify({ login: 'nuxt', type: 'Organization' }), { status: 200, headers: { etag: '"v1"' } }) + })) + const bindings = { GITHUB_TOKEN: 'registry-token', KV_CACHE: memoryKv() } + + await getGithubJson('/users/nuxt', bindings) + const repeat = await getGithubJson<{ type: string }>('/users/nuxt', bindings) + + expect(repeat).toMatchObject({ status: 304, notModified: true, data: { type: 'Organization' } }) + expect(seen).toEqual([ + { authorization: 'Bearer registry-token', ifNoneMatch: null }, + { authorization: 'Bearer registry-token', ifNoneMatch: '"v1"' }, + ]) + }) +}) diff --git a/test/unit/github-repo-summary-batch.test.ts b/test/unit/github-repo-summary-batch.test.ts new file mode 100644 index 00000000..e300de3b --- /dev/null +++ b/test/unit/github-repo-summary-batch.test.ts @@ -0,0 +1,99 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { getRepoSummariesBatch } from '../../layers/registry/server/utils/github-client' + +const bindings = { GITHUB_TOKEN: 'test-token' } + +interface GraphqlBody { + query: string + variables: Record +} + +function repository(owner: string, name: string, tree: string) { + return { + name, + nameWithOwner: `${owner}/${name}`, + url: `https://github.com/${owner}/${name}`, + owner: { login: owner }, + description: null, + stargazerCount: 3, + forkCount: 1, + pushedAt: '2026-10-01T00:00:00Z', + createdAt: '2025-01-01T00:00:00Z', + isArchived: false, + isFork: false, + defaultBranchRef: { name: 'main', target: { oid: 'c'.repeat(40), tree: { oid: tree } } }, + } +} + +/** Answers every alias, except owners named `gone`, which GitHub reports NOT_FOUND. */ +function graphqlAnswer(_input: unknown, init?: RequestInit): Response { + const body = JSON.parse(String(init?.body)) as GraphqlBody + const data: Record = {} + const errors: unknown[] = [] + for (const key of Object.keys(body.variables).filter(name => name.startsWith('o'))) { + const index = key.slice(1) + const owner = body.variables[key]! + const name = body.variables[`n${index}`]! + if (owner === 'gone') { + data[`r${index}`] = null + errors.push({ type: 'NOT_FOUND', path: [`r${index}`], message: 'Could not resolve' }) + } + else { + data[`r${index}`] = repository(owner, name, `tree-${owner}-${name}`) + } + } + return new Response(JSON.stringify({ data, errors: errors.length ? errors : undefined }), { + status: 200, + headers: { 'x-ratelimit-remaining': '4990', 'x-ratelimit-resource': 'graphql' }, + }) +} + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('getRepoSummariesBatch', () => { + it('reads 230 repositories in three queries, in request order', async () => { + const fetchMock = vi.fn(graphqlAnswer) + vi.stubGlobal('fetch', fetchMock) + const requests = Array.from({ length: 230 }, (_, i) => ({ owner: 'acme', repo: `repo-${i}` })) + + const result = await getRepoSummariesBatch(requests, bindings) + + expect(fetchMock).toHaveBeenCalledTimes(3) + expect(result._tag).toBe('read') + if (result._tag !== 'read') + return + expect(result.summaries).toHaveLength(230) + expect(result.summaries[229]?.headTreeSha).toBe('tree-acme-repo-229') + expect(result.summaries[0]?.meta.full_name).toBe('acme/repo-0') + expect(result.rateLimit?.resource).toBe('graphql') + }) + + it('leaves a repository GitHub cannot find empty and keeps the rest', async () => { + vi.stubGlobal('fetch', vi.fn(graphqlAnswer)) + + const result = await getRepoSummariesBatch([ + { owner: 'acme', repo: 'one' }, + { owner: 'gone', repo: 'two' }, + { owner: 'acme', repo: 'three' }, + ], bindings) + + expect(result._tag === 'read' && result.summaries.map(summary => summary?.headTreeSha ?? null)).toEqual([ + 'tree-acme-one', + null, + 'tree-acme-three', + ]) + }) + + it('fails the whole batch on an error that is not NOT_FOUND', async () => { + vi.stubGlobal('fetch', vi.fn(async () => new Response(JSON.stringify({ + data: null, + errors: [{ type: 'RATE_LIMITED', message: 'API rate limit exceeded' }], + }), { status: 200 }))) + + const result = await getRepoSummariesBatch([{ owner: 'acme', repo: 'one' }], bindings) + + expect(result).toMatchObject({ _tag: 'failed', status: 502 }) + }) +}) diff --git a/test/unit/github-sync-control.test.ts b/test/unit/github-sync-control.test.ts index 42cc26f3..114ecded 100644 --- a/test/unit/github-sync-control.test.ts +++ b/test/unit/github-sync-control.test.ts @@ -41,4 +41,20 @@ describe('github sync pause decision', () => { remaining: 200, })).toEqual({ _tag: 'continue' }) }) + + it('names the GitHub bucket that ran low', () => { + expect(githubSyncPauseDecision({ + now: 1_000, + owner: 'nuxt', + repo: 'ui', + unauthorized: false, + rateLimited: false, + remaining: 163, + resource: 'graphql', + resetAt: 2_000, + })).toMatchObject({ + _tag: 'pause', + reason: 'nuxt/ui: 163 graphql requests remaining', + }) + }) }) diff --git a/test/unit/repo-source-profile-quota.test.ts b/test/unit/repo-source-profile-quota.test.ts new file mode 100644 index 00000000..c1597be9 --- /dev/null +++ b/test/unit/repo-source-profile-quota.test.ts @@ -0,0 +1,109 @@ +import type { H3Event } from 'h3' +import type { SqliteD1 } from './helpers/d1-sqlite' +import { createStorage } from 'unstorage' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' + +const route = { owner: 'acme', repo: 'skills' } +let edgeCache = createStorage() +let harness: SqliteD1 + +function stubRuntime(): void { + vi.stubGlobal('defineCachedEventHandler', (handler: unknown) => handler) + vi.stubGlobal('createError', (input: Record) => Object.assign(new Error(String(input.message)), input)) + vi.stubGlobal('getRouterParam', (_event: unknown, key: 'owner' | 'repo') => route[key]) + vi.stubGlobal('useStorage', () => edgeCache) +} + +const TREE_SHA = 'a'.repeat(40) + +function repoMeta(overrides: Record = {}) { + return { + name: 'skills', + full_name: 'acme/skills', + html_url: 'https://github.com/acme/skills', + owner: { login: 'acme' }, + default_branch: 'main', + description: 'Skills', + stargazers_count: 12, + forks_count: 3, + pushed_at: '2026-10-01T00:00:00Z', + created_at: '2025-01-01T00:00:00Z', + private: false, + ...overrides, + } +} + +function json(body: unknown, init: ResponseInit = {}): Response { + return new Response(JSON.stringify(body), { status: 200, ...init }) +} + +async function profile(): Promise> { + const handler = (await import('../../layers/registry/server/api/repos/[owner]/[repo].get')).default as (event: H3Event) => Promise> + return handler({ context: { platform: { db: harness.db, env: { GITHUB_TOKEN: 'token' } } } } as unknown as H3Event) +} + +function calls(fetchMock: ReturnType, fragment: string): number { + return fetchMock.mock.calls.filter(([input]) => String(input).includes(fragment)).length +} + +beforeEach(() => { + edgeCache = createStorage() + harness = createSqliteD1(allMigrations()) + route.repo = 'skills' + stubRuntime() +}) + +// The global wide event stubs come from the setup file, so this file never +// calls `vi.unstubAllGlobals()`: each test replaces the stubs it needs. +afterEach(() => { + harness.close() +}) + +describe('repository source profile GitHub spend', () => { + it('answers 404 for a private repository and never lists its tree', async () => { + const fetchMock = vi.fn(async () => json(repoMeta({ private: true }))) + vi.stubGlobal('fetch', fetchMock) + + await expect(profile()).rejects.toMatchObject({ statusCode: 404 }) + expect(calls(fetchMock, '/git/trees/')).toBe(0) + }) + + it('remembers a repository GitHub cannot find, so a repeat view reads nothing', async () => { + route.repo = 'gone' + const fetchMock = vi.fn(async () => json({ message: 'Not Found' }, { status: 404 })) + vi.stubGlobal('fetch', fetchMock) + + await expect(profile()).rejects.toMatchObject({ statusCode: 404 }) + await expect(profile()).rejects.toMatchObject({ statusCode: 404 }) + + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it('reads repository metadata from ungh.cc when GitHub refuses with a spent quota', async () => { + const fetchMock = vi.fn(async (input: unknown) => String(input).startsWith('https://ungh.cc/') + ? json({ repo: { id: 1, name: 'skills', repo: 'acme/skills', description: 'From ungh', createdAt: '2025-01-01T00:00:00Z', pushedAt: '2026-10-01T00:00:00Z', stars: 99, forks: 4, defaultBranch: 'trunk' } }) + : json({ message: 'API rate limit exceeded' }, { status: 403, headers: { 'x-ratelimit-remaining': '0' } })) + vi.stubGlobal('fetch', fetchMock) + + const body = await profile() + + expect(body).toMatchObject({ owner: 'acme', repo: 'skills', stars: 99, description: 'From ungh', defaultBranch: 'trunk', skillFileScanStatus: 'unavailable' }) + expect(calls(fetchMock, '/git/trees/')).toBe(0) + }) + + it('lists a registry repository at its synced tree, once per tree', async () => { + harness.raw.prepare(`INSERT INTO repos (owner, repo, last_tree_sha) VALUES ('acme', 'skills', ?)`).run(TREE_SHA) + const fetchMock = vi.fn(async (input: unknown) => String(input).includes('/git/trees/') + ? json({ sha: TREE_SHA, tree: [{ path: 'skills/one/SKILL.md', type: 'blob', sha: 'b'.repeat(40) }] }) + : json(repoMeta())) + vi.stubGlobal('fetch', fetchMock) + + const first = await profile() + const second = await profile() + + expect(first).toMatchObject({ skillFileScanStatus: 'ok', skillFiles: ['skills/one/SKILL.md'] }) + expect(second).toMatchObject({ skillFiles: ['skills/one/SKILL.md'] }) + expect(fetchMock.mock.calls.filter(([input]) => String(input).includes(`/git/trees/${TREE_SHA}`))).toHaveLength(1) + }) +}) diff --git a/test/unit/repo-source-profile-timeout.test.ts b/test/unit/repo-source-profile-timeout.test.ts index 1e6f4baf..4458ecf6 100644 --- a/test/unit/repo-source-profile-timeout.test.ts +++ b/test/unit/repo-source-profile-timeout.test.ts @@ -1,16 +1,19 @@ import type { H3Event } from 'h3' +import { createStorage } from 'unstorage' import { afterEach, describe, expect, it, vi } from 'vitest' import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' vi.stubGlobal('defineCachedEventHandler', (handler: unknown) => handler) vi.stubGlobal('createError', (input: Record) => Object.assign(new Error(String(input.message)), input)) vi.stubGlobal('getRouterParam', (_event: unknown, key: string) => (key === 'owner' ? 'skilld-dev' : 'skills')) +vi.stubGlobal('useStorage', () => createStorage()) afterEach(() => { vi.unstubAllGlobals() vi.stubGlobal('defineCachedEventHandler', (handler: unknown) => handler) vi.stubGlobal('createError', (input: Record) => Object.assign(new Error(String(input.message)), input)) vi.stubGlobal('getRouterParam', (_event: unknown, key: string) => (key === 'owner' ? 'skilld-dev' : 'skills')) + vi.stubGlobal('useStorage', () => createStorage()) }) async function profile() { diff --git a/test/unit/skill-detail-d1-cost.test.ts b/test/unit/skill-detail-d1-cost.test.ts index 152ef08e..eee0d577 100644 --- a/test/unit/skill-detail-d1-cost.test.ts +++ b/test/unit/skill-detail-d1-cost.test.ts @@ -236,17 +236,17 @@ describe('findSkill', () => { describe('uncached skill related render', () => { it('reads D1 at most three times and keeps the commit source', async () => { seed({ maximumQueries: 3 }) - upstreamFetch.mockImplementationOnce(async () => [{ + rawFetch.mockImplementationOnce(async () => new Response(JSON.stringify([{ sha: 'abcdef1234567', html_url: 'https://github.com/c/abcdef1', commit: { message: 'edit alpha', author: { name: 'Eric', date: '2026-09-01T00:00:00Z' } }, author: null, - }] as never) + }]))) const handler = (await import('../../layers/registry/server/api/skill-related/[...slug].get')).default as (event: H3Event) => Promise> const body = await handler({ context: { platform: { db: harness.db, env: {} } }, node: { req: { headers: {} } } } as unknown as H3Event) - expect(upstreamFetch).toHaveBeenCalledWith(`https://api.github.com/repos/${OWNER}/${REPO}/commits`, expect.objectContaining({ query: { path: 'skills/alpha/SKILL.md', per_page: 5 } })) + expect(String(rawFetch.mock.calls[0]?.[0])).toBe(`https://api.github.com/repos/${OWNER}/${REPO}/commits?path=skills%2Falpha%2FSKILL.md&per_page=5`) expect(body.commits.map((commit: { shortSha: string }) => commit.shortSha)).toEqual(['abcdef1']) expect(body.relatedRepoSkills.map((skill: { name: string }) => skill.name)).toEqual(['beta']) }) diff --git a/test/unit/sync-prefetch.test.ts b/test/unit/sync-prefetch.test.ts new file mode 100644 index 00000000..a8d0f640 --- /dev/null +++ b/test/unit/sync-prefetch.test.ts @@ -0,0 +1,113 @@ +import type { SqliteD1 } from './helpers/d1-sqlite' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { prefetchUnchangedRepos } from '../../layers/registry/server/utils/sync-prefetch' +import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' + +const NOW = 1_791_300_000 +const bindings = { GITHUB_TOKEN: 'test-token' } + +/** The head tree GitHub reports for each repository. */ +const heads: Record = { + 'acme/unchanged': 'tree-same', + 'acme/changed': 'tree-new', + 'acme/fresh': 'tree-fresh', + 'acme/busy': 'tree-busy', + 'acme/discovered': 'tree-discovered', +} + +function graphqlAnswer(_input: unknown, init?: RequestInit): Response { + const body = JSON.parse(String(init?.body)) as { variables: Record } + const data: Record = {} + const errors: unknown[] = [] + for (const key of Object.keys(body.variables).filter(name => name.startsWith('o'))) { + const index = key.slice(1) + const fullName = `${body.variables[key]}/${body.variables[`n${index}`]}` + const tree = heads[fullName] + if (!tree) { + data[`r${index}`] = null + errors.push({ type: 'NOT_FOUND', path: [`r${index}`] }) + continue + } + const [owner, name] = fullName.split('/') as [string, string] + data[`r${index}`] = { + name, + nameWithOwner: fullName, + url: `https://github.com/${fullName}`, + owner: { login: owner }, + description: 'Skills', + stargazerCount: 77, + forkCount: 2, + pushedAt: '2026-10-01T00:00:00Z', + createdAt: '2025-01-01T00:00:00Z', + isArchived: false, + isFork: false, + defaultBranchRef: { name: 'main', target: { oid: 'c'.repeat(40), tree: { oid: tree } } }, + } + } + return new Response(JSON.stringify({ data, errors: errors.length ? errors : undefined }), { status: 200 }) +} + +function candidate(repo: string, claimDiscovery = false, owner = 'acme') { + return { owner, repo, ownerVerified: false, claimDiscovery } +} + +describe('prefetchUnchangedRepos', () => { + let harness: SqliteD1 + + beforeEach(() => { + harness = createSqliteD1(allMigrations()) + for (const [repo, tree] of [['unchanged', 'tree-same'], ['changed', 'tree-old'], ['fresh', null], ['busy', 'tree-busy'], ['discovered', 'tree-discovered']] as const) { + harness.raw.prepare(`INSERT INTO repos (owner, repo, last_tree_sha, pushed_at, stars) VALUES ('acme', ?, ?, 100, 1)`).run(repo, tree) + if (repo !== 'fresh') + harness.raw.prepare(`INSERT INTO skills (owner, repo, name, display_name, slug) VALUES ('acme', ?, 'one', 'One', 'one')`).run(repo) + } + harness.raw.prepare(`INSERT INTO repo_sync_progress (owner, repo, job_id, tree_sha, checked_at, next_offset, updated_at) VALUES ('acme', 'busy', 'job-1', 'tree-busy', ?, 250, ?)`).run(NOW - 60, NOW - 60) + }) + + afterEach(() => { + harness.close() + vi.unstubAllGlobals() + }) + + it('records an unchanged repository in place and queues only the ones that need a job', async () => { + const fetchMock = vi.fn(graphqlAnswer) + vi.stubGlobal('fetch', fetchMock) + + const result = await prefetchUnchangedRepos({ db: harness.db, bindings, now: NOW }, [ + candidate('unchanged'), + candidate('changed'), + candidate('fresh'), + candidate('busy'), + candidate('discovered', true), + candidate('missing', false, 'gone'), + ]) + + expect(fetchMock).toHaveBeenCalledTimes(1) + expect(result.queue.map(item => `${item.owner}/${item.repo}`)).toEqual([ + 'acme/changed', + 'acme/fresh', + 'acme/busy', + 'acme/discovered', + 'gone/missing', + ]) + expect(result).toMatchObject({ _tag: 'prefetched', unchanged: 1 }) + expect(harness.raw.prepare(`SELECT stars, repo_meta_synced_at FROM repos WHERE owner = 'acme' AND repo = 'unchanged'`).get()) + .toEqual({ stars: 77, repo_meta_synced_at: NOW }) + expect(harness.raw.prepare(`SELECT repo_meta_synced_at FROM repos WHERE owner = 'acme' AND repo = 'changed'`).get()) + .toEqual({ repo_meta_synced_at: null }) + }) + + it('queues every candidate when GitHub does not answer the batch', async () => { + vi.stubGlobal('fetch', vi.fn(async () => new Response('bad gateway', { status: 502 }))) + + const result = await prefetchUnchangedRepos({ db: harness.db, bindings, now: NOW }, [ + candidate('unchanged'), + candidate('changed'), + ]) + + expect(result).toMatchObject({ _tag: 'unread', reason: 'graphql-502' }) + expect(result.queue).toHaveLength(2) + expect(harness.raw.prepare(`SELECT repo_meta_synced_at FROM repos WHERE owner = 'acme' AND repo = 'unchanged'`).get()) + .toEqual({ repo_meta_synced_at: null }) + }) +}) diff --git a/test/unit/sync-repo-correctness.test.ts b/test/unit/sync-repo-correctness.test.ts index cb5edc7d..42aa1138 100644 --- a/test/unit/sync-repo-correctness.test.ts +++ b/test/unit/sync-repo-correctness.test.ts @@ -136,7 +136,7 @@ describe('syncRepo content acknowledgement', () => { const result = await syncRepo('steipete', 'clawdis', {}, db, { ownerVerified: true }) expect(result).toMatchObject({ owner: 'steipete', repo: 'clawdis', status: 'indexed' }) - expect(github.getTree).toHaveBeenCalledWith('openclaw', 'openclaw', 'main', {}) + expect(github.getTree).toHaveBeenCalledWith('openclaw', 'openclaw', 'new-tree', {}) expect(github.getBlobsBatch).toHaveBeenCalledWith( 'openclaw', 'openclaw', diff --git a/test/unit/sync-repo-freshness.test.ts b/test/unit/sync-repo-freshness.test.ts index 966c06e2..8771d110 100644 --- a/test/unit/sync-repo-freshness.test.ts +++ b/test/unit/sync-repo-freshness.test.ts @@ -215,6 +215,7 @@ describe('syncRepo freshness cursor', () => { FROM repos WHERE owner = 'acme' AND repo = 'skills' `).get() + expect(github.getTree).toHaveBeenCalledWith('acme', 'skills', 'moved-tree', {}) expect(result.status).toBe('failed') expect(result.reason).toBe('tree_truncated') expect(row).toEqual({ From 450a61198deab98a6ab2d0d9459a8e46911dc883 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 02:21:09 +1100 Subject: [PATCH 6/8] docs(registry): state what the sync job count measured --- layers/registry/server/utils/github-client.ts | 5 ++--- layers/registry/server/utils/sync-prefetch.ts | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/layers/registry/server/utils/github-client.ts b/layers/registry/server/utils/github-client.ts index 851ee513..16b5a8a4 100644 --- a/layers/registry/server/utils/github-client.ts +++ b/layers/registry/server/utils/github-client.ts @@ -347,9 +347,8 @@ export type RepoSummaryBatchOutcome * {@link REPO_SUMMARY_BATCH_SIZE}. * * The hourly sync used to send one summary query per repository from its own - * queue job. About 87% of those found the tree unchanged (1,649 of 1,900 jobs - * on 2026-10-06), so the job and its query did nothing but advance the - * freshness cursor. + * queue job. On 2026-10-06, 1,649 of 1,900 such jobs wrote no Skill row, and + * a job that finds its tree unchanged only advances the freshness cursor. */ export async function getRepoSummariesBatch( requests: RepoSummaryRequest[], diff --git a/layers/registry/server/utils/sync-prefetch.ts b/layers/registry/server/utils/sync-prefetch.ts index feaf7981..ec59cf2a 100644 --- a/layers/registry/server/utils/sync-prefetch.ts +++ b/layers/registry/server/utils/sync-prefetch.ts @@ -56,9 +56,9 @@ interface PrefetchRow extends ExistingRepo { * the unchanged ones in place, and return the candidates that still need a job. * * Each candidate used to get a queue job whose first act was one GraphQL - * summary read. 1,649 of 1,900 such jobs on 2026-10-06 found the tree - * unchanged and only advanced the freshness cursor. The cursor writes here - * are the same statements `syncRepo` runs on that path. + * summary read. On 2026-10-06, 1,649 of 1,900 such jobs wrote no Skill row. + * A job that finds its tree unchanged only advances the freshness cursor, and + * the writes here are the same statements `syncRepo` runs on that path. * * A discovery claim, a job still holding the repository's progress row, a * Skill-less repository and any repository GitHub gave no summary for keep From 9c9948443c70b6154522c899bc941bee3ca5eaa0 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 02:46:24 +1100 Subject: [PATCH 7/8] fix(artifact-delivery): read with the fallback token where GitHub denies the App neondatabase/agent-skills is public and a personal token reads it, but GitHub answers the read App 403. Every run of its Skills failed SOURCE_ACCESS_DENIED after builds moved to the App. A read the App was denied now repeats once with ARTIFACT_GITHUB_TOKEN or GITHUB_TOKEN, and the denial is reported as an artifact-github-credential event. --- .../server/utils/github-read-credential.ts | 93 +++++++++++++------ .../artifact-delivery/server/utils/queue.ts | 1 + test/unit/artifact-github-read-app.test.ts | 34 ++++++- 3 files changed, 99 insertions(+), 29 deletions(-) diff --git a/layers/artifact-delivery/server/utils/github-read-credential.ts b/layers/artifact-delivery/server/utils/github-read-credential.ts index 6398b471..897a19a9 100644 --- a/layers/artifact-delivery/server/utils/github-read-credential.ts +++ b/layers/artifact-delivery/server/utils/github-read-credential.ts @@ -1,4 +1,4 @@ -import type { PublicGithubSourceClient } from './github-source' +import type { LoadSourceResult, PublicGithubSourceClient, ResolveSourceResult } from './github-source' import { requestInstallationToken, signGithubAppJwt } from './github-app' /** @@ -44,7 +44,7 @@ export interface ArtifactGithubCredentialEnv { /** Why a build read with a fallback token. It names secrets, never a value. */ export interface ArtifactGithubCredentialReport { - outcome: 'app-misconfigured' | 'app-token-unavailable' + outcome: 'app-misconfigured' | 'app-token-unavailable' | 'app-denied' reason: string fallback: FallbackToken['name'] | 'anonymous' } @@ -76,8 +76,18 @@ export interface ArtifactGithubCredentialRuntime { report: (event: ArtifactGithubCredentialReport) => void } -/** The credential for one GitHub read. Undefined reads anonymously. */ -export type ArtifactGithubCredential = () => Promise +/** The credential for one GitHub read. An undefined token reads anonymously. */ +export interface GithubReadCredential { + token: string | undefined + /** The read App's installation token, which a Repository can deny. */ + isApp: boolean +} + +export interface ArtifactGithubCredential { + current: () => Promise + /** The token after the read App, or null. */ + fallback: FallbackToken | null +} export function parseArtifactGithubCredentialConfig(env: ArtifactGithubCredentialEnv): ArtifactGithubCredentialConfig { const fallback = fallbackToken(env) @@ -110,48 +120,77 @@ export function createArtifactGithubCredential( runtime: ArtifactGithubCredentialRuntime, ): ArtifactGithubCredential { if (config._tag === 'anonymous') - return async () => undefined + return { current: async () => ({ token: undefined, isApp: false }), fallback: null } if (config._tag === 'token') - return async () => config.token.token + return { current: async () => ({ token: config.token.token, isApp: false }), fallback: null } const fallbackName = config.fallback?.name ?? 'anonymous' + const fallback = { token: config.fallback?.token, isApp: false } if (config._tag === 'app-misconfigured') { - return async () => { - runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) - return config.fallback?.token + return { + current: async () => { + runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) + return fallback + }, + fallback: null, } } const app = config.app const key = `${app.appId}:${app.installationId}` - return async () => { - const cached = runtime.tokenCache.tokens.get(key) - if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) - return cached.token - let pending = runtime.tokenCache.pending.get(key) - if (!pending) { - pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) - runtime.tokenCache.pending.set(key, pending) - } - const minted = await pending - if ('token' in minted) { - runtime.tokenCache.tokens.set(key, minted) - return minted.token - } - runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) - return config.fallback?.token + return { + current: async () => { + const cached = runtime.tokenCache.tokens.get(key) + if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) + return { token: cached.token, isApp: true } + let pending = runtime.tokenCache.pending.get(key) + if (!pending) { + pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) + runtime.tokenCache.pending.set(key, pending) + } + const minted = await pending + if ('token' in minted) { + runtime.tokenCache.tokens.set(key, minted) + return { token: minted.token, isApp: true } + } + runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) + return fallback + }, + fallback: config.fallback, } } /** * A GitHub source client whose every read asks for the current credential, so * a long-lived client never reads with an expired installation token. + * + * Some organizations deny the read App on a public Repository that a personal + * token reads: `neondatabase/agent-skills` answered SOURCE_ACCESS_DENIED to + * every run on 2026-10-06. A read the App was denied repeats once with the + * fallback token, and the denial is reported. */ export function withGithubCredential( credential: ArtifactGithubCredential, create: (token: string | undefined) => PublicGithubSourceClient, + report: (event: ArtifactGithubCredentialReport) => void, ): PublicGithubSourceClient { + const read = async ( + repository: { owner: string, repository: string }, + run: (client: PublicGithubSourceClient) => Promise, + ): Promise => { + const current = await credential.current() + const first = await run(create(current.token)) + const fallback = credential.fallback + if (!current.isApp || !fallback || first._tag !== 'rejected' || first.code !== 'SOURCE_ACCESS_DENIED') + return first + report({ + outcome: 'app-denied', + reason: `GitHub denied the read App for ${repository.owner}/${repository.repository}`, + fallback: fallback.name, + }) + return await run(create(fallback.token)) + } return { - resolve: async request => await create(await credential()).resolve(request), - load: async source => await create(await credential()).load(source), + resolve: async request => await read(request, client => client.resolve(request)), + load: async source => await read(source, client => client.load(source)), } } diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index aad0a5a4..dbef1478 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -183,6 +183,7 @@ export function createArtifactGithubSource( return withGithubCredential( credential, token => createPublicGithubSourceClient({ fetch: runtime.fetch, token, onReadFailure: reportGithubReadFailure, cache }), + runtime.report, ) } diff --git a/test/unit/artifact-github-read-app.test.ts b/test/unit/artifact-github-read-app.test.ts index 95f3a5d4..5977eab0 100644 --- a/test/unit/artifact-github-read-app.test.ts +++ b/test/unit/artifact-github-read-app.test.ts @@ -124,6 +124,31 @@ describe('the GitHub credential public Artifact builds read with', () => { expect(text).not.toContain('PRIVATE KEY') expect(reports).toHaveLength(1) }) + + it('repeats a read the Repository denied the read App with the fallback token, and reports it', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600, deniesApp: true }) + const reports: ArtifactGithubCredentialReport[] = [] + const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })) + + const result = await source.resolve(request) + + expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1', 'Bearer runs']) + // The fallback read reached GitHub, which has no such Repository. + expect(result).toMatchObject({ _tag: 'rejected', code: 'SOURCE_NOT_FOUND' }) + expect(reports).toEqual([{ outcome: 'app-denied', reason: 'GitHub denied the read App for skilld-dev/skills', fallback: 'ARTIFACT_GITHUB_TOKEN' }]) + }) + + it('keeps the denial when no token follows the read App', async () => { + const keys = await appKeys() + const github = fakeGithub({ expiresAt: NOW + 3600, deniesApp: true }) + const env = { ...appEnv(keys.pem), ARTIFACT_GITHUB_TOKEN: '', GITHUB_TOKEN: '' } + + const result = await createArtifactGithubSource(env, runtime(github)).resolve(request) + + expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1']) + expect(result).toMatchObject({ _tag: 'rejected', code: 'SOURCE_ACCESS_DENIED' }) + }) }) interface FakeGithub { @@ -132,7 +157,7 @@ interface FakeGithub { readAuthorizations: Array } -function fakeGithub(options: { expiresAt: number, mintStatus?: number }): FakeGithub { +function fakeGithub(options: { expiresAt: number, mintStatus?: number, deniesApp?: boolean }): FakeGithub { const mints: FakeGithub['mints'] = [] const readAuthorizations: FakeGithub['readAuthorizations'] = [] const fetcher = async (input: RequestInfo | URL, init?: RequestInit): Promise => { @@ -144,7 +169,12 @@ function fakeGithub(options: { expiresAt: number, mintStatus?: number }): FakeGi return new Response('{}', { status: options.mintStatus }) return Response.json({ token: `ghs_installation_${mints.length}`, expires_at: new Date(options.expiresAt * 1000).toISOString() }, { status: 201 }) } - readAuthorizations.push(headers.get('authorization')) + const authorization = headers.get('authorization') + readAuthorizations.push(authorization) + // Some organizations answer the read App 403 on a public Repository that + // a personal token reads: neondatabase/agent-skills on 2026-10-06. + if (options.deniesApp && authorization?.startsWith('Bearer ghs_')) + return Response.json({ message: 'Resource not accessible by integration' }, { status: 403 }) return Response.json({ message: 'Not Found' }, { status: 404 }) } return { fetch: fetcher as typeof fetch, mints, readAuthorizations } From 19d3120a90ab07b5e7cf75fbe82a9ee84fb28577 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 03:18:19 +1100 Subject: [PATCH 8/8] feat(github): read GitHub as the App everywhere, tokens as fallback GITHUB_TOKEN is a personal token, so the registry sync, page views and mention checks spent the same bucket as its owner's tools and CI. Every Worker GitHub read now uses the read App installation token, shared with builds through one isolate cache. GITHUB_TOKEN is read only for a read GitHub denied the App, and each use is reported. Without it, a denied read fails for that Repository only: the sync no longer counts a denial as a spent quota, and a build answers SOURCE_ACCESS_DENIED unless the signed-in requester's token can read it. --- .dev.vars.example | 14 +- docs/ops/artifact-delivery-rollout.md | 14 +- .../server/utils/github-app.ts | 86 +--- .../server/utils/github-read-credential.ts | 235 +---------- .../artifact-delivery/server/utils/queue.ts | 34 +- .../server/utils/requester-github.ts | 17 +- .../server/utils/daily-health-check.ts | 4 +- layers/registry/server/utils/github-client.ts | 179 ++++++-- .../server/utils/github-rate-limited.ts | 10 + layers/registry/server/utils/sync-repo.ts | 19 +- shared/server/github-app-credential.ts | 393 ++++++++++++++++++ test/unit/artifact-github-read-app.test.ts | 20 +- test/unit/artifact-requester-github.test.ts | 11 + .../registry-github-app-credential.test.ts | 195 +++++++++ 14 files changed, 843 insertions(+), 388 deletions(-) create mode 100644 layers/registry/server/utils/github-rate-limited.ts create mode 100644 shared/server/github-app-credential.ts create mode 100644 test/unit/registry-github-app-credential.test.ts diff --git a/.dev.vars.example b/.dev.vars.example index d5a4b14a..953dad2b 100644 --- a/.dev.vars.example +++ b/.dev.vars.example @@ -10,13 +10,17 @@ NUXT_OG_IMAGE_SECRET= # Signs same-origin image proxy addresses. Keep it separate from NUXT_TOKEN_KEY. NUXT_IMAGE_PROXY_KEY= NUXT_AI_READY_RUNTIME_SYNC_SECRET= +# Every Worker GitHub read uses the read App (SKILLD_READ_APP_*): metadata read +# only, installed on skilld-dev, with a quota nothing else spends. The key is a +# PKCS #8 PEM. Without the App secrets, as in local development, reads use +# GITHUB_TOKEN. +# +# With the App set, GITHUB_TOKEN is a fallback only: it repeats a read GitHub +# denied the App, such as an organization that restricts Apps. Builds try +# ARTIFACT_GITHUB_TOKEN first. Each fallback read is logged as an app-denied event. +# Both may stay empty. GITHUB_TOKEN= -# Public Artifact builds read GitHub with this token when it is set. Use a token -# from another GitHub account, so runs keep a quota the registry sync cannot spend. ARTIFACT_GITHUB_TOKEN= -# Public Artifact builds read GitHub as the read App: metadata read only, -# installed on skilld-dev. Its installation has a quota nothing else spends. -# The key is a PKCS #8 PEM. Without these, builds use the tokens above. SKILLD_READ_APP_ID= SKILLD_READ_APP_INSTALLATION_ID= SKILLD_READ_APP_PRIVATE_KEY_PKCS8= diff --git a/docs/ops/artifact-delivery-rollout.md b/docs/ops/artifact-delivery-rollout.md index 16e2ad09..eddf17dc 100644 --- a/docs/ops/artifact-delivery-rollout.md +++ b/docs/ops/artifact-delivery-rollout.md @@ -111,17 +111,17 @@ Preserve these existing values: - `NUXT_OAUTH_GITHUB_CLIENT_ID` - `NUXT_OAUTH_GITHUB_CLIENT_SECRET` - `NUXT_TOKEN_KEY` -- `GITHUB_TOKEN` -The existing `GITHUB_TOKEN` reads public [GitHub](https://github.com) sources for the registry sync and `/gh` pages. +Every Worker GitHub read uses the read App `skilld-dev-registry-reads`: the registry sync, repository maintenance, mention checks, `/gh` pages and public Artifact builds. Set `SKILLD_READ_APP_ID`, `SKILLD_READ_APP_INSTALLATION_ID` and `SKILLD_READ_APP_PRIVATE_KEY_PKCS8` (a PKCS #8 PEM). The App has metadata read permission only, and its installation has its own quota. -Public Artifact builds read GitHub with a credential of their own, in this order: +Personal tokens are fallbacks only. They repeat a read GitHub denied the App, such as an organization that restricts Apps: -1. The read App `skilld-dev-registry-reads`: `SKILLD_READ_APP_ID`, `SKILLD_READ_APP_INSTALLATION_ID` and `SKILLD_READ_APP_PRIVATE_KEY_PKCS8` (a PKCS #8 PEM). It has metadata read permission only, and its installation has its own quota. -2. `ARTIFACT_GITHUB_TOKEN`. -3. `GITHUB_TOKEN`. +1. Builds try `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`, then the token of the signed-in account that asked for the run. +2. Every other caller tries `GITHUB_TOKEN`. -If the App secrets are set but unusable, builds read with the next credential and emit an `artifact-github-credential` event with the reason. +Each denial emits an `app-denied` event: `github-credential` for registry reads and `artifact-github-credential` for builds. The `github.credential` field names the fallback, or `none`. Without a fallback, the read fails for that Repository only: a build answers `SOURCE_ACCESS_DENIED`, and the sync records a failure without pausing. + +If the App secrets are set but unusable, reads use the fallback and emit an event with the reason. Keep the read App apart from the `GITHUB_APP_*` secrets below. Those belong to private delivery. Changing `NUXT_TOKEN_KEY` breaks stored GitHub credentials. diff --git a/layers/artifact-delivery/server/utils/github-app.ts b/layers/artifact-delivery/server/utils/github-app.ts index f6a7d541..b8b7d695 100644 --- a/layers/artifact-delivery/server/utils/github-app.ts +++ b/layers/artifact-delivery/server/utils/github-app.ts @@ -1,5 +1,6 @@ import { z } from 'zod' import { decryptToken, encryptToken } from '#layers/identity/server/utils/crypto' +import { requestInstallationToken, signGithubAppJwt } from '#shared/server/github-app-credential' import { base64ToBytes, bytesToBase64Url } from './encoding' import { fetchNoRedirect } from './fetch-no-redirect' @@ -48,11 +49,6 @@ const githubUserSchema = z.object({ login: z.string().min(1).max(100), }).passthrough() -const installationTokenSchema = z.object({ - token: z.string().min(1).max(2048), - expires_at: z.string().datetime(), -}).passthrough() - const refreshedUserTokenSchema = z.object({ access_token: z.string().min(1).max(2048), expires_in: z.number().int().positive(), @@ -541,50 +537,6 @@ export async function verifyGithubWebhookSignature( return different === 0 } -export type InstallationTokenResult - = | { _tag: 'created', token: string, expiresAt: string } - /** GitHub answered 401, 403 or 404: the App, its key or the installation is gone. */ - | { _tag: 'refused', status: number } - -/** - * Exchange an App JWT for an installation access token. - * - * A refusal is a value. Any other failure, such as a 5xx or an unreadable - * answer, throws: it says nothing about the App. - */ -export async function requestInstallationToken(input: { - fetch: typeof globalThis.fetch - jwt: string - installationId: number - body: { repository_ids?: number[], permissions: Record } -}): Promise { - const fetched = await fetchNoRedirect(input.fetch, `${GITHUB_API}/app/installations/${input.installationId}/access_tokens`, { - method: 'POST', - headers: { - 'Accept': 'application/vnd.github+json', - 'Authorization': `Bearer ${input.jwt}`, - 'Content-Type': 'application/json', - 'User-Agent': 'skilld.dev', - 'X-GitHub-Api-Version': GITHUB_API_VERSION, - }, - body: JSON.stringify(input.body), - signal: AbortSignal.timeout(GITHUB_REQUEST_TIMEOUT_MS), - }) - if (fetched._tag === 'unexpected-redirect') - throw new Error(`GitHub App request redirected ${fetched.status}`) - const response = fetched.response - if (response.status === 401 || response.status === 403 || response.status === 404) { - await response.body?.cancel() - return { _tag: 'refused', status: response.status } - } - if (!response.ok) - throw new Error(`GitHub App request returned ${response.status}`) - const value = installationTokenSchema.safeParse(await readBoundedJson(response, MAX_GITHUB_TOKEN_RESPONSE_BYTES)) - if (!value.success) - throw new Error('GitHub App returned an invalid response') - return { _tag: 'created', token: value.data.token, expiresAt: value.data.expires_at } -} - async function createGithubAppJwt(config: GithubAppConfig): Promise { const privateKey = decodeCanonicalBase64Url(config.privateKeyPkcs8) if (!privateKey) @@ -592,42 +544,6 @@ async function createGithubAppJwt(config: GithubAppConfig): Promise { return await signGithubAppJwt({ issuer: config.clientId, privateKeyPkcs8: privateKey, now: config.now() }) } -/** - * An RS256 GitHub App JWT. `issuer` is the App's client ID or its App ID; - * GitHub accepts either. - * - * It is valid for ten minutes from a minute in the past, which absorbs clock - * drift between the Worker and GitHub. Ten minutes is GitHub's maximum. - */ -export async function signGithubAppJwt(input: { - issuer: string - privateKeyPkcs8: Uint8Array - /** Unix seconds. */ - now: number -}): Promise { - const issuedAt = input.now - 60 - const header = bytesToBase64Url(new TextEncoder().encode(JSON.stringify({ alg: 'RS256', typ: 'JWT' }))) - const payload = bytesToBase64Url(new TextEncoder().encode(JSON.stringify({ - iat: issuedAt, - exp: issuedAt + 600, - iss: input.issuer, - }))) - const unsigned = `${header}.${payload}` - const imported = await crypto.subtle.importKey( - 'pkcs8', - toArrayBuffer(input.privateKeyPkcs8), - { name: 'RSASSA-PKCS1-v1_5', hash: 'SHA-256' }, - false, - ['sign'], - ) - const signature = await crypto.subtle.sign( - 'RSASSA-PKCS1-v1_5', - imported, - new TextEncoder().encode(unsigned), - ) - return `${unsigned}.${bytesToBase64Url(new Uint8Array(signature))}` -} - async function readBoundedJson(response: Response, maximumBytes: number): Promise { const declared = Number(response.headers.get('content-length')) if (Number.isFinite(declared) && declared > maximumBytes) diff --git a/layers/artifact-delivery/server/utils/github-read-credential.ts b/layers/artifact-delivery/server/utils/github-read-credential.ts index 897a19a9..73bcc3cb 100644 --- a/layers/artifact-delivery/server/utils/github-read-credential.ts +++ b/layers/artifact-delivery/server/utils/github-read-credential.ts @@ -1,161 +1,13 @@ +import type { GithubCredential, GithubCredentialConfig, GithubCredentialEnv, GithubCredentialReport } from '#shared/server/github-app-credential' import type { LoadSourceResult, PublicGithubSourceClient, ResolveSourceResult } from './github-source' -import { requestInstallationToken, signGithubAppJwt } from './github-app' +import { parseGithubCredentialConfig } from '#shared/server/github-app-credential' /** - * Renew an installation token this long before GitHub says it expires, so a - * build that starts with it never sees it lapse part way through. - */ -export const INSTALLATION_TOKEN_RENEW_SECONDS = 5 * 60 - -/** The read App: metadata read only, installed on skilld-dev. */ -export interface ReadAppConfig { - appId: number - installationId: number - privateKeyPkcs8: Uint8Array -} - -/** The token a build reads with when the App gives none, by its secret name. */ -export interface FallbackToken { - name: 'ARTIFACT_GITHUB_TOKEN' | 'GITHUB_TOKEN' - token: string -} - -/** - * Where public Artifact builds get their GitHub credential, parsed once from - * the Worker env. - * - * GitHub counts a personal token's quota per account, so every token of one - * account shares one bucket. An App installation has a bucket of its own. - */ -export type ArtifactGithubCredentialConfig - = | { _tag: 'app', app: ReadAppConfig, fallback: FallbackToken | null } - /** App secrets are set but unusable. Builds read with the fallback and report why. */ - | { _tag: 'app-misconfigured', reason: string, fallback: FallbackToken | null } - | { _tag: 'token', token: FallbackToken } - | { _tag: 'anonymous' } - -export interface ArtifactGithubCredentialEnv { - SKILLD_READ_APP_ID?: string - SKILLD_READ_APP_INSTALLATION_ID?: string - SKILLD_READ_APP_PRIVATE_KEY_PKCS8?: string - ARTIFACT_GITHUB_TOKEN?: string - GITHUB_TOKEN?: string -} - -/** Why a build read with a fallback token. It names secrets, never a value. */ -export interface ArtifactGithubCredentialReport { - outcome: 'app-misconfigured' | 'app-token-unavailable' | 'app-denied' - reason: string - fallback: FallbackToken['name'] | 'anonymous' -} - -interface CachedInstallationToken { - token: string - /** Unix seconds. */ - expiresAt: number -} - -/** - * Installation tokens kept between builds in one isolate. A token lives an - * hour; minting one per build would add a request and a signature to each. - */ -export interface InstallationTokenCache { - tokens: Map - pending: Map> -} - -export function createInstallationTokenCache(): InstallationTokenCache { - return { tokens: new Map(), pending: new Map() } -} - -export interface ArtifactGithubCredentialRuntime { - fetch: typeof globalThis.fetch - /** Unix seconds. */ - now: () => number - tokenCache: InstallationTokenCache - report: (event: ArtifactGithubCredentialReport) => void -} - -/** The credential for one GitHub read. An undefined token reads anonymously. */ -export interface GithubReadCredential { - token: string | undefined - /** The read App's installation token, which a Repository can deny. */ - isApp: boolean -} - -export interface ArtifactGithubCredential { - current: () => Promise - /** The token after the read App, or null. */ - fallback: FallbackToken | null -} - -export function parseArtifactGithubCredentialConfig(env: ArtifactGithubCredentialEnv): ArtifactGithubCredentialConfig { - const fallback = fallbackToken(env) - const appSecrets = [env.SKILLD_READ_APP_ID, env.SKILLD_READ_APP_INSTALLATION_ID, env.SKILLD_READ_APP_PRIVATE_KEY_PKCS8] - if (appSecrets.every(value => !value?.trim())) - return fallback ? { _tag: 'token', token: fallback } : { _tag: 'anonymous' } - const misconfigured = (reason: string): ArtifactGithubCredentialConfig => ({ _tag: 'app-misconfigured', reason, fallback }) - const appId = positiveInteger(env.SKILLD_READ_APP_ID) - if (appId === null) - return misconfigured('SKILLD_READ_APP_ID is not a GitHub App ID') - const installationId = positiveInteger(env.SKILLD_READ_APP_INSTALLATION_ID) - if (installationId === null) - return misconfigured('SKILLD_READ_APP_INSTALLATION_ID is not an installation ID') - const privateKeyPkcs8 = pkcs8PemBytes(env.SKILLD_READ_APP_PRIVATE_KEY_PKCS8 ?? '') - if (!privateKeyPkcs8) - return misconfigured('SKILLD_READ_APP_PRIVATE_KEY_PKCS8 is not a PKCS #8 PEM private key') - return { _tag: 'app', app: { appId, installationId, privateKeyPkcs8 }, fallback } -} - -/** - * The GitHub credential public Artifact builds read with: the read App's + * Where public Artifact builds get their GitHub credential: the read App's * installation token, then `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`. - * - * The registry sync and `/gh` page views keep `GITHUB_TOKEN`. When the App - * cannot give a token, the build still runs on the fallback and the reason is - * reported, so a broken key shows up as an event instead of as a spent quota. */ -export function createArtifactGithubCredential( - config: ArtifactGithubCredentialConfig, - runtime: ArtifactGithubCredentialRuntime, -): ArtifactGithubCredential { - if (config._tag === 'anonymous') - return { current: async () => ({ token: undefined, isApp: false }), fallback: null } - if (config._tag === 'token') - return { current: async () => ({ token: config.token.token, isApp: false }), fallback: null } - const fallbackName = config.fallback?.name ?? 'anonymous' - const fallback = { token: config.fallback?.token, isApp: false } - if (config._tag === 'app-misconfigured') { - return { - current: async () => { - runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) - return fallback - }, - fallback: null, - } - } - const app = config.app - const key = `${app.appId}:${app.installationId}` - return { - current: async () => { - const cached = runtime.tokenCache.tokens.get(key) - if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) - return { token: cached.token, isApp: true } - let pending = runtime.tokenCache.pending.get(key) - if (!pending) { - pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) - runtime.tokenCache.pending.set(key, pending) - } - const minted = await pending - if ('token' in minted) { - runtime.tokenCache.tokens.set(key, minted) - return { token: minted.token, isApp: true } - } - runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) - return fallback - }, - fallback: config.fallback, - } +export function parseArtifactGithubCredentialConfig(env: GithubCredentialEnv): GithubCredentialConfig { + return parseGithubCredentialConfig(env, ['ARTIFACT_GITHUB_TOKEN', 'GITHUB_TOKEN']) } /** @@ -165,12 +17,12 @@ export function createArtifactGithubCredential( * Some organizations deny the read App on a public Repository that a personal * token reads: `neondatabase/agent-skills` answered SOURCE_ACCESS_DENIED to * every run on 2026-10-06. A read the App was denied repeats once with the - * fallback token, and the denial is reported. + * fallback token. Every denial is reported, with or without a fallback. */ export function withGithubCredential( - credential: ArtifactGithubCredential, + credential: GithubCredential, create: (token: string | undefined) => PublicGithubSourceClient, - report: (event: ArtifactGithubCredentialReport) => void, + report: (event: GithubCredentialReport) => void, ): PublicGithubSourceClient { const read = async ( repository: { owner: string, repository: string }, @@ -178,76 +30,23 @@ export function withGithubCredential( ): Promise => { const current = await credential.current() const first = await run(create(current.token)) - const fallback = credential.fallback - if (!current.isApp || !fallback || first._tag !== 'rejected' || first.code !== 'SOURCE_ACCESS_DENIED') + if (!current.isApp || first._tag !== 'rejected' || first.code !== 'SOURCE_ACCESS_DENIED') return first + const fallback = credential.fallback report({ outcome: 'app-denied', reason: `GitHub denied the read App for ${repository.owner}/${repository.repository}`, - fallback: fallback.name, + fallback: fallback?.name ?? 'none', }) - return await run(create(fallback.token)) + if (fallback) + return await run(create(fallback.token)) + return { + ...first, + summary: 'GitHub denies the skilld.dev read App access to this Repository, and no fallback token is set.', + } } return { resolve: async request => await read(request, client => client.resolve(request)), load: async source => await read(source, client => client.load(source)), } } - -async function mintInstallationToken( - app: ReadAppConfig, - runtime: Pick, -): Promise { - const minted = await signGithubAppJwt({ issuer: String(app.appId), privateKeyPkcs8: app.privateKeyPkcs8, now: runtime.now() }) - .then(jwt => requestInstallationToken({ - fetch: runtime.fetch, - jwt, - installationId: app.installationId, - body: { permissions: { metadata: 'read' } }, - })) - // A network fault, a 5xx or a key WebCrypto rejects: the build reads with - // the fallback token, and the reason reaches the report. - .catch((error: unknown) => ({ _tag: 'failed' as const, reason: error instanceof Error ? error.message : String(error) })) - if (minted._tag === 'failed') - return { _tag: 'unavailable', reason: minted.reason.slice(0, 200) } - if (minted._tag === 'refused') - return { _tag: 'unavailable', reason: `GitHub refused the installation token (${minted.status})` } - const expiresAt = Math.floor(Date.parse(minted.expiresAt) / 1000) - return { token: minted.token, expiresAt } -} - -function fallbackToken(env: ArtifactGithubCredentialEnv): FallbackToken | null { - if (env.ARTIFACT_GITHUB_TOKEN?.trim()) - return { name: 'ARTIFACT_GITHUB_TOKEN', token: env.ARTIFACT_GITHUB_TOKEN.trim() } - if (env.GITHUB_TOKEN?.trim()) - return { name: 'GITHUB_TOKEN', token: env.GITHUB_TOKEN.trim() } - return null -} - -function positiveInteger(value: string | undefined): number | null { - const trimmed = value?.trim() ?? '' - if (!/^[1-9]\d*$/.test(trimmed)) - return null - const parsed = Number(trimmed) - return Number.isSafeInteger(parsed) ? parsed : null -} - -/** - * The DER bytes of a PKCS #8 PEM private key, or null. - * - * A secret pasted through a dashboard or a JSON file can hold its line breaks - * as the two characters `\n`, so both forms parse. A PKCS #1 key - * (`BEGIN RSA PRIVATE KEY`, as GitHub downloads it) does not: WebCrypto - * imports PKCS #8 only. Convert it with `openssl pkcs8 -topk8 -nocrypt`. - */ -function pkcs8PemBytes(value: string): Uint8Array | null { - const text = value.replaceAll('\\n', '\n').trim() - const match = /^-----BEGIN PRIVATE KEY-----([A-Z0-9+/=\s]+)-----END PRIVATE KEY-----$/i.exec(text) - if (!match) - return null - const base64 = match[1]!.replace(/\s+/g, '') - if (!/^[A-Z0-9+/]+={0,2}$/i.test(base64) || base64.length % 4 !== 0) - return null - const binary = atob(base64) - return Uint8Array.from(binary, character => character.charCodeAt(0)) -} diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 074f3e9c..55fd516e 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -1,14 +1,11 @@ import type { QueueBatch } from '#cf-jobs/server' +import type { GithubCredentialEnv, GithubCredentialReport, GithubCredentialRuntime } from '#shared/server/github-app-credential' import type { ArtifactBuildDependencies } from './build' -import type { - ArtifactGithubCredentialEnv, - ArtifactGithubCredentialReport, - ArtifactGithubCredentialRuntime, -} from './github-read-credential' import type { GithubObjectCache, GithubReadFailure, PublicGithubSourceClient } from './github-source' import { createWideEvent } from '@harlan-zw/nuxt-wide-events/standalone' import { z } from 'zod' import { emitOperationalEvent } from '#server/utils/operational-event' +import { createGithubCredential, isolateInstallationTokenCache } from '#shared/server/github-app-credential' import { createArtifactSigner } from './attestation' import { failResolution, processArtifactBuild } from './build' import { @@ -16,12 +13,7 @@ import { githubAppUserTokenDependenciesFromEnv, loadAccountGithubAppUserToken, } from './github-app' -import { - createArtifactGithubCredential, - createInstallationTokenCache, - parseArtifactGithubCredentialConfig, - withGithubCredential, -} from './github-read-credential' +import { parseArtifactGithubCredentialConfig, withGithubCredential } from './github-read-credential' import { createGithubSourceClient, createPublicGithubSourceClient } from './github-source' import { createD1PrivateArtifactKeyProvider, privateArtifactWrappingKeysFromEnv } from './private-crypto' import { privateArtifactAccessEnabled } from './private-feature' @@ -165,12 +157,6 @@ export function createKvGithubObjectCache(kv: KVNamespace): GithubObjectCache { } } -/** - * One installation token cache per isolate. A token lives an hour, so it - * outlives the queue batch that minted it. - */ -const installationTokens = createInstallationTokenCache() - /** * The GitHub source public Artifact builds read with. * @@ -181,11 +167,11 @@ const installationTokens = createInstallationTokenCache() * else spends, then `ARTIFACT_GITHUB_TOKEN`, then `GITHUB_TOKEN`. */ export function createArtifactGithubSource( - env: ArtifactGithubCredentialEnv, - runtime: ArtifactGithubCredentialRuntime = defaultGithubSourceRuntime(), + env: GithubCredentialEnv, + runtime: GithubCredentialRuntime = defaultGithubSourceRuntime(), cache?: GithubObjectCache, ): PublicGithubSourceClient { - const credential = createArtifactGithubCredential(parseArtifactGithubCredentialConfig(env), runtime) + const credential = createGithubCredential(parseArtifactGithubCredentialConfig(env), runtime) return withGithubCredential( credential, token => createPublicGithubSourceClient({ fetch: runtime.fetch, token, onReadFailure: reportGithubReadFailure, cache }), @@ -193,16 +179,18 @@ export function createArtifactGithubSource( ) } -function defaultGithubSourceRuntime(): ArtifactGithubCredentialRuntime { +function defaultGithubSourceRuntime(): GithubCredentialRuntime { return { fetch: globalThis.fetch.bind(globalThis), now: () => Math.floor(Date.now() / 1000), - tokenCache: installationTokens, + // A token lives an hour, so it outlives the queue batch that minted it, + // and the registry reads in this isolate share it. + tokenCache: isolateInstallationTokenCache(), report: reportGithubCredential, } } -function reportGithubCredential(event: ArtifactGithubCredentialReport): void { +function reportGithubCredential(event: GithubCredentialReport): void { emitOperationalEvent(createWideEvent({ 'operation': 'artifact-github-credential', 'outcome': event.outcome, diff --git a/layers/artifact-delivery/server/utils/requester-github.ts b/layers/artifact-delivery/server/utils/requester-github.ts index dcd91ba7..745bfdd0 100644 --- a/layers/artifact-delivery/server/utils/requester-github.ts +++ b/layers/artifact-delivery/server/utils/requester-github.ts @@ -1,4 +1,4 @@ -import type { GithubObjectCache, GithubReadFailure, PublicGithubSourceClient } from './github-source' +import type { GithubObjectCache, GithubReadFailure, LoadSourceResult, PublicGithubSourceClient, ResolveSourceResult } from './github-source' import type { ResolutionRow } from './state' import { decryptToken } from '#layers/identity/server/utils/crypto' import { createPublicGithubSourceClient } from './github-source' @@ -86,8 +86,10 @@ export function createRequesterGithubSource( } /** - * A client that reads with the shared credential, and repeats a read the - * shared quota refused with the requester's own client. + * A client that reads with the shared credential, and repeats with the + * requester's own client a read the shared credentials could not make: a spent + * quota, or a Repository whose organization denies the read App when no + * fallback token is set. * * Every other answer, success or rejection, stays the shared one, so a * requester's token is spent only when nothing else could read. @@ -104,17 +106,22 @@ export function withRequesterFallback( return { async resolve(request) { const first = await shared.resolve(request) - if (first._tag !== 'rejected' || first.code !== 'RATE_LIMITED') + if (!sharedCouldNotRead(first)) return first const client = await ownClient() return client ? await client.resolve(request) : first }, async load(source) { const first = await shared.load(source) - if (first._tag !== 'rejected' || first.code !== 'RATE_LIMITED') + if (!sharedCouldNotRead(first)) return first const client = await ownClient() return client ? await client.load(source) : first }, } } + +/** A spent quota, or a denial no shared credential could get past. */ +function sharedCouldNotRead(result: ResolveSourceResult | LoadSourceResult): boolean { + return result._tag === 'rejected' && (result.code === 'RATE_LIMITED' || result.code === 'SOURCE_ACCESS_DENIED') +} diff --git a/layers/identity/server/utils/daily-health-check.ts b/layers/identity/server/utils/daily-health-check.ts index 45434f85..c393969c 100644 --- a/layers/identity/server/utils/daily-health-check.ts +++ b/layers/identity/server/utils/daily-health-check.ts @@ -605,9 +605,9 @@ export function evaluateDailyHealthStatus( // while rotating is still routine rather than an incident. const token = summary.credentials.githubToken if (token._tag === 'expired') - red.push(`GITHUB_TOKEN expired ${plural(Math.abs(token.daysRemaining), 'day')} ago. Rotate it: sync is down until you do.`) + red.push(`GITHUB_TOKEN expired ${plural(Math.abs(token.daysRemaining), 'day')} ago. Repositories that deny the read App cannot sync or build until you rotate or delete it.`) else if (token._tag === 'expiring') - amber.push(`GITHUB_TOKEN expires in ${plural(token.daysRemaining, 'day')}. Rotate it before sync starts failing.`) + amber.push(`GITHUB_TOKEN expires in ${plural(token.daysRemaining, 'day')}. Rotate or delete it: it reads only Repositories that deny the read App.`) // Report every finding, ordered by severity, rather than only the winning // tier. Returning `red` alone meant one loud red hid every amber underneath diff --git a/layers/registry/server/utils/github-client.ts b/layers/registry/server/utils/github-client.ts index 16b5a8a4..9293fb1c 100644 --- a/layers/registry/server/utils/github-client.ts +++ b/layers/registry/server/utils/github-client.ts @@ -1,5 +1,18 @@ /// +import type { + GithubCredential, + GithubCredentialReport, + GithubReadCredential, + InstallationTokenCache, +} from '#shared/server/github-app-credential' +import { + createGithubCredential, + githubDeniedApp, + isolateInstallationTokenCache, + parseGithubCredentialConfig, +} from '#shared/server/github-app-credential' + const API_BASE = 'https://api.github.com' const GRAPHQL_URL = 'https://api.github.com/graphql' @@ -39,10 +52,29 @@ function readInit(options: GithubReadOptions | undefined): RequestInit | undefin export interface GithubBindings { KV_CACHE?: KVNamespace + /** + * The credential every Worker read asks for: the read App's installation + * token, with `GITHUB_TOKEN` only for a read GitHub denied the App. + */ + credential?: GithubCredential + /** A static token, for scripts and tests that pass no `credential`. */ GITHUB_TOKEN?: string + /** Receives each read GitHub denied the App. Defaults to a wide event. */ + reportCredential?: (event: GithubCredentialReport) => void } -type GithubBindingSource = Partial> +type GithubBindingSource = Partial> + +/** What a test or a caller with its own clock passes in place of the isolate defaults. */ +export interface GithubBindingRuntime { + tokenCache: InstallationTokenCache + /** Unix seconds. */ + now: () => number + report: (event: GithubCredentialReport) => void +} /** * Resolve GitHub bindings from a Cloudflare env (Worker runtime) or process.env (local dev). @@ -50,14 +82,62 @@ type GithubBindingSource = Partial = {}, +): GithubBindings { const env = cloudflareEnv ?? {} + const report = runtime.report ?? reportGithubCredential + const config = parseGithubCredentialConfig({ ...env, GITHUB_TOKEN: env.GITHUB_TOKEN ?? process.env.GITHUB_TOKEN }, ['GITHUB_TOKEN']) return { KV_CACHE: env.KV_CACHE, - GITHUB_TOKEN: env.GITHUB_TOKEN ?? process.env.GITHUB_TOKEN, + credential: createGithubCredential(config, { + // Late-bound, so a test that stubs the global fetch reaches the mint too. + fetch: (input, init) => fetch(input, init), + now: runtime.now ?? (() => Math.floor(Date.now() / 1000)), + tokenCache: runtime.tokenCache ?? isolateInstallationTokenCache(), + report, + }), + reportCredential: report, } } +function reportGithubCredential(event: GithubCredentialReport): void { + emitOperationalEvent(createWideEvent({ + 'operation': 'github-credential', + 'outcome': event.outcome, + 'reason': event.reason, + 'github.credential': event.fallback, + })) +} + +async function readCredential(bindings: GithubBindings): Promise { + return bindings.credential ? await bindings.credential.current() : { token: bindings.GITHUB_TOKEN, isApp: false } +} + +/** + * Report a read GitHub denied the App, and answer the token to repeat it + * with, or null when there is none. Every use of the fallback is reported. + */ +function fallbackAfterDenial(bindings: GithubBindings, label: string): string | null { + const fallback = bindings.credential?.fallback ?? null + ;(bindings.reportCredential ?? reportGithubCredential)({ + outcome: 'app-denied', + reason: `GitHub denied the read App for ${label}`, + fallback: fallback?.name ?? 'none', + }) + return fallback?.token ?? null +} + +function withAuthorization(headers: Headers, token: string | undefined): Headers { + const authorized = new Headers(headers) + if (token) + authorized.set('Authorization', `Bearer ${token}`) + else + authorized.delete('Authorization') + return authorized +} + export interface RepoMeta { name: string full_name: string @@ -114,6 +194,11 @@ export interface FetchOutcome { data: T | null rateLimit: RateLimitInfo | null notModified: boolean + /** + * GitHub denied the read App this read, and no fallback token could repeat + * it. The status is 403, but no quota is spent. + */ + denied?: true } /** @@ -168,8 +253,6 @@ async function ghRequest( headers.set('Accept', 'application/vnd.github+json') headers.set('User-Agent', 'skilld.dev') headers.set('X-GitHub-Api-Version', '2022-11-28') - if (bindings.GITHUB_TOKEN) - headers.set('Authorization', `Bearer ${bindings.GITHUB_TOKEN}`) const cacheKey = etagKey(url) let cached: CachedEntry | null = null @@ -182,7 +265,15 @@ async function ghRequest( headers.set('If-None-Match', cached.etag) } - const res = await fetch(url, { ...init, headers }) + const send = (token: string | undefined) => fetch(url, { ...init, headers: withAuthorization(headers, token) }) + const credential = await readCredential(bindings) + let res = await send(credential.token) + if (credential.isApp && await githubDeniedApp(res)) { + const fallback = fallbackAfterDenial(bindings, new URL(url).pathname) + if (fallback === null) + return { status: 403, data: null, rateLimit: parseRateLimit(res.headers), notModified: false, denied: true } + res = await send(fallback) + } const rateLimit = parseRateLimit(res.headers) if (res.status === 304 && cached) { @@ -299,9 +390,12 @@ export async function getRepoSummary( ${REPO_SUMMARY_FIELDS} } }` - const out = await gqlRequest<{ repository: RepoSummaryGql | null }>(query, { owner, repo }, bindings) - if (out._tag === 'failed') - return { status: out.status, data: null, rateLimit: out.rateLimit, notModified: false } + const out = await gqlRequest<{ repository: RepoSummaryGql | null }>(query, { owner, repo }, bindings, { label: `${owner}/${repo}`, retryForbidden: true }) + if (out._tag === 'failed') { + return out.denied + ? { status: out.status, data: null, rateLimit: out.rateLimit, notModified: false, denied: true } + : { status: out.status, data: null, rateLimit: out.rateLimit, notModified: false } + } // GraphQL surfaces NOT_FOUND in errors with HTTP 200. Map it to 404 so // callers preserve their existing rate-limit / broken-repo branching. if (out.errors.length) { @@ -366,14 +460,16 @@ export async function getRepoSummariesBatch( variables[`n${i}`] = request.repo }) sent++ - const out = await gqlRequest>(`query(${varDecls.join(',')}){${aliases}}`, variables, bindings) + const out = await gqlRequest>(`query(${varDecls.join(',')}){${aliases}}`, variables, bindings, { label: 'a summary batch', retryForbidden: false }) rateLimit = out.rateLimit ?? rateLimit if (out._tag === 'failed') return { _tag: 'failed', status: out.status, rateLimit, requests: sent } // A NOT_FOUND names one alias and leaves the others whole. Any other // error type, such as RATE_LIMITED, says nothing trustworthy about the // batch, so it fails as a whole and the per-repository sync takes over. - if (out.errors.some(error => error.type !== 'NOT_FOUND')) + // A FORBIDDEN alias is a Repository GitHub denies the read App. It reads + // as null here, and the per-repository sync repeats it with the fallback. + if (out.errors.some(error => error.type !== 'NOT_FOUND' && error.type !== 'FORBIDDEN')) return { _tag: 'failed', status: 502, rateLimit, requests: sent } for (let i = 0; i < batch.length; i++) { const r = out.data?.[`r${i}`] @@ -425,28 +521,57 @@ interface GqlError { type?: string, message?: string } type GqlRequestOutcome = | { _tag: 'answered', data: T | null, errors: GqlError[], rateLimit: RateLimitInfo | null } - | { _tag: 'failed', status: number, rateLimit: RateLimitInfo | null } + | { _tag: 'failed', status: number, rateLimit: RateLimitInfo | null, denied?: true } -/** One GraphQL POST. Partial data and per-alias errors come back together. */ +/** + * One GraphQL POST. Partial data and per-alias errors come back together. + * + * A query about one Repository that GitHub denied the read App, with a 403 + * or a FORBIDDEN error, repeats once with the fallback token. A batch over + * many Repositories passes `retryForbidden: false`: one denied alias says + * nothing about the others, and the per-repository sync repeats it. + */ async function gqlRequest( query: string, variables: Record, bindings: GithubBindings, + options: { label: string, retryForbidden: boolean }, +): Promise> { + const credential = await readCredential(bindings) + const first = await gqlSend(query, variables, credential.token, credential.isApp) + const deniedApp = first._tag === 'failed' + ? first.denied === true + : options.retryForbidden && first.errors.some(error => error.type === 'FORBIDDEN') + if (!credential.isApp || !deniedApp) + return first + const fallback = fallbackAfterDenial(bindings, options.label) + if (fallback === null) + return { _tag: 'failed', status: 403, rateLimit: first.rateLimit, denied: true } + return await gqlSend(query, variables, fallback, false) +} + +async function gqlSend( + query: string, + variables: Record, + token: string | undefined, + asApp: boolean, ): Promise> { const headers = new Headers() headers.set('Accept', 'application/vnd.github+json') headers.set('Content-Type', 'application/json') headers.set('User-Agent', 'skilld.dev') - if (bindings.GITHUB_TOKEN) - headers.set('Authorization', `Bearer ${bindings.GITHUB_TOKEN}`) + if (token) + headers.set('Authorization', `Bearer ${token}`) const res = await fetch(GRAPHQL_URL, { method: 'POST', headers, body: JSON.stringify({ query, variables }), }) const rateLimit = parseRateLimit(res.headers) - if (!res.ok) - return { _tag: 'failed', status: res.status, rateLimit } + if (!res.ok) { + const denied = asApp && await githubDeniedApp(res) + return denied ? { _tag: 'failed', status: res.status, rateLimit, denied: true } : { _tag: 'failed', status: res.status, rateLimit } + } // A gateway can answer 200 with a truncated or non-JSON body. Parsing that // eagerly threw `Unexpected end of JSON input` out of the client and reached // the sync summary as an opaque reason with no status attached. @@ -463,10 +588,11 @@ async function gqlPost( query: string, variables: Record, bindings: GithubBindings, -): Promise<{ status: number, data: T | null, rateLimit: RateLimitInfo | null }> { - const out = await gqlRequest(query, variables, bindings) + label: string, +): Promise<{ status: number, data: T | null, rateLimit: RateLimitInfo | null, denied?: true }> { + const out = await gqlRequest(query, variables, bindings, { label, retryForbidden: true }) if (out._tag === 'failed') - return { status: out.status, data: null, rateLimit: out.rateLimit } + return out.denied ? { status: out.status, data: null, rateLimit: out.rateLimit, denied: true } : { status: out.status, data: null, rateLimit: out.rateLimit } if (out.errors.length) { const notFound = out.errors.some(e => e.type === 'NOT_FOUND') return { status: notFound ? 404 : 502, data: null, rateLimit: out.rateLimit } @@ -518,12 +644,12 @@ export async function getBlobsBatch( variables[`expr${i}`] = `${branch}:${p}` }) - const out = await gqlPost<{ repository: Record | null }>(query, variables, bindings) + const out = await gqlPost<{ repository: Record | null }>(query, variables, bindings, `${owner}/${repo}`) rateLimit = out.rateLimit ?? rateLimit // A partial map would look like a repo that lost files, and the caller // would delete skills it simply failed to read. Fail the whole batch. if (!out.data?.repository) - return { status: out.status, data: null, unreadable, rateLimit, notModified: false } + return { status: out.status, data: null, unreadable, rateLimit, notModified: false, ...(out.denied ? { denied: true as const } : {}) } batch.forEach((p, i) => { const blob = out.data!.repository![`b${i}`] if (typeof blob?.text === 'string') @@ -571,11 +697,11 @@ export async function getCommitsBatch( variables[`path${i}`] = p }) - const out = await gqlPost(query, variables, bindings) + const out = await gqlPost(query, variables, bindings, `${owner}/${repo}`) rateLimit = out.rateLimit ?? rateLimit const target = out.data?.repository?.defaultBranchRef?.target if (!target) - return { status: out.status, data: null, rateLimit, notModified: false } + return { status: out.status, data: null, rateLimit, notModified: false, ...(out.denied ? { denied: true as const } : {}) } batch.forEach((p, i) => { const nodes = target[`h${i}`]?.nodes ?? [] map.set(p, nodes.map(c => ({ @@ -605,8 +731,9 @@ export async function getRawFile( ): Promise { const headers = new Headers() headers.set('User-Agent', 'skilld.dev') - if (bindings.GITHUB_TOKEN) - headers.set('Authorization', `Bearer ${bindings.GITHUB_TOKEN}`) + const credential = await readCredential(bindings) + if (credential.token) + headers.set('Authorization', `Bearer ${credential.token}`) const res = await fetch(`${RAW_BASE}/${owner}/${repo}/${ref}/${path}`, { headers }) if (!res.ok) return null diff --git a/layers/registry/server/utils/github-rate-limited.ts b/layers/registry/server/utils/github-rate-limited.ts new file mode 100644 index 00000000..8439a62b --- /dev/null +++ b/layers/registry/server/utils/github-rate-limited.ts @@ -0,0 +1,10 @@ +/** + * Whether GitHub refused a read for want of quota. + * + * A read GitHub denied the read App also answers 403, but it is a fact about + * one Repository: `denied` marks it, and it never pauses the sync. It lives + * apart from the GitHub client, which sync tests replace whole. + */ +export function githubRateLimited(outcome: { status: number, denied?: true }): boolean { + return (outcome.status === 403 || outcome.status === 429) && !outcome.denied +} diff --git a/layers/registry/server/utils/sync-repo.ts b/layers/registry/server/utils/sync-repo.ts index 359d32f4..dc44917d 100644 --- a/layers/registry/server/utils/sync-repo.ts +++ b/layers/registry/server/utils/sync-repo.ts @@ -5,6 +5,7 @@ import type { SkillTrustTier } from './skill-trust' import { canonicalSkillPaths, isRegistrySkillPath, isSkilldCacheSkill } from '#shared/skill-path' import { isCategoryPinned } from '../data/clusters' import { getBlobsBatch, getCommitsBatch, getRepoSummary, getTree, logRateLimit } from './github-client' +import { githubRateLimited } from './github-rate-limited' import { repoStarObservationStatements } from './repo-history' import { resolveRepoSourceIdentityFromRow } from './repo-source-identity' import { skillContentSha256 } from './skill-content-hash' @@ -345,8 +346,8 @@ export async function refreshRepoAssets( owner, repo, reason: `repo fetch ${repoRes.status}`, - retryable: repoRes.status === 403 || repoRes.status === 429 || repoRes.status >= 500, - rateLimited: repoRes.status === 403 || repoRes.status === 429, + retryable: githubRateLimited(repoRes) || repoRes.status >= 500, + rateLimited: githubRateLimited(repoRes), unauthorized: repoRes.status === 401, ...rate, } @@ -377,8 +378,8 @@ export async function refreshRepoAssets( owner, repo, reason: `tree fetch ${treeRes.status}`, - retryable: treeRes.status === 403 || treeRes.status === 429 || treeRes.status >= 500, - rateLimited: treeRes.status === 403 || treeRes.status === 429, + retryable: githubRateLimited(treeRes) || treeRes.status >= 500, + rateLimited: githubRateLimited(treeRes), unauthorized: treeRes.status === 401, ...finalRate, } @@ -797,7 +798,7 @@ export async function syncRepo( return stats } - if (repoRes.status === 403 || repoRes.status === 429) { + if (githubRateLimited(repoRes)) { stats.status = 'rate-limited' stats.reason = `rate-limited (${repoRes.rateLimit?.remaining ?? '?'} remaining)` return stats @@ -807,7 +808,7 @@ export async function syncRepo( if (repoRes.status === 404 || repoRes.status === 410) await markRepoMissing(db, owner, repo, nowSec()) stats.status = 'failed' - stats.reason = `repo fetch ${repoRes.status}` + stats.reason = repoRes.denied ? 'github denied the read App (403)' : `repo fetch ${repoRes.status}` return stats } @@ -844,7 +845,7 @@ export async function syncRepo( trackRateLimit(treeRes.rateLimit) if (!treeRes.data) { - stats.status = treeRes.status === 403 || treeRes.status === 429 ? 'rate-limited' : 'failed' + stats.status = githubRateLimited(treeRes) ? 'rate-limited' : 'failed' stats.reason = `tree_fetch_failed:${treeRes.status}` return stats } @@ -1110,7 +1111,7 @@ export async function syncRepo( logRateLimit(`blobs ${owner}/${repo}`, blobsRes.rateLimit) trackRateLimit(blobsRes.rateLimit) if (!blobsRes.data) { - stats.status = blobsRes.status === 403 || blobsRes.status === 429 ? 'rate-limited' : 'failed' + stats.status = githubRateLimited(blobsRes) ? 'rate-limited' : 'failed' stats.reason = `blob_batch_failed:${blobsRes.status}` return stats } @@ -1252,7 +1253,7 @@ export async function syncRepo( logRateLimit(`commits-batch ${owner}/${repo}`, commitsRes.rateLimit) trackRateLimit(commitsRes.rateLimit) if (!commitsRes.data) { - stats.status = commitsRes.status === 403 || commitsRes.status === 429 ? 'rate-limited' : 'failed' + stats.status = githubRateLimited(commitsRes) ? 'rate-limited' : 'failed' stats.reason = `commit_batch_failed:${commitsRes.status}` return stats } diff --git a/shared/server/github-app-credential.ts b/shared/server/github-app-credential.ts new file mode 100644 index 00000000..03dd0973 --- /dev/null +++ b/shared/server/github-app-credential.ts @@ -0,0 +1,393 @@ +import { z } from 'zod' + +/** + * The read App's GitHub credential, shared by every Worker caller. + * + * GitHub counts a personal token's quota per account, so every token of one + * account shares one bucket, and the account owner's own tools spend it too. + * The read App (`skilld-dev-registry-reads`, metadata read only) has an + * installation with a bucket of its own. A personal token remains only as the + * fallback for a Repository whose organization denies the App. + */ + +const GITHUB_API = 'https://api.github.com' +const GITHUB_API_VERSION = '2026-03-10' +const GITHUB_REQUEST_TIMEOUT_MS = 15_000 +const MAX_TOKEN_RESPONSE_BYTES = 64 * 1024 + +/** + * Renew an installation token this long before GitHub says it expires, so a + * read that starts with it never sees it lapse part way through. + */ +export const INSTALLATION_TOKEN_RENEW_SECONDS = 5 * 60 + +/** The read App: metadata read only, installed on skilld-dev. */ +export interface ReadAppConfig { + appId: number + installationId: number + privateKeyPkcs8: Uint8Array +} + +/** A personal token a caller may fall back to, by its secret name. */ +export interface FallbackToken { + name: 'ARTIFACT_GITHUB_TOKEN' | 'GITHUB_TOKEN' + token: string +} + +/** Where a caller's GitHub credential comes from, parsed once from the Worker env. */ +export type GithubCredentialConfig + = | { _tag: 'app', app: ReadAppConfig, fallback: FallbackToken | null } + /** App secrets are set but unusable. Reads use the fallback and report why. */ + | { _tag: 'app-misconfigured', reason: string, fallback: FallbackToken | null } + /** No App secret is set, as in local development and tests. */ + | { _tag: 'token', token: FallbackToken } + | { _tag: 'anonymous' } + +export interface GithubCredentialEnv { + SKILLD_READ_APP_ID?: string + SKILLD_READ_APP_INSTALLATION_ID?: string + SKILLD_READ_APP_PRIVATE_KEY_PKCS8?: string + ARTIFACT_GITHUB_TOKEN?: string + GITHUB_TOKEN?: string +} + +/** + * Why a read used a token other than the App's, or found none. It names + * secrets and paths, never a token value. + */ +export interface GithubCredentialReport { + outcome: 'app-misconfigured' | 'app-token-unavailable' | 'app-denied' + reason: string + fallback: FallbackToken['name'] | 'anonymous' | 'none' +} + +interface CachedInstallationToken { + token: string + /** Unix seconds. */ + expiresAt: number +} + +/** + * Installation tokens kept between reads in one isolate. A token lives an + * hour; minting one per read would add a request and a signature to each. + */ +export interface InstallationTokenCache { + tokens: Map + pending: Map> +} + +export function createInstallationTokenCache(): InstallationTokenCache { + return { tokens: new Map(), pending: new Map() } +} + +let isolateTokenCache: InstallationTokenCache | null = null + +/** + * The one installation token cache of this isolate, so the registry and the + * build queue share a token instead of minting one each. + */ +export function isolateInstallationTokenCache(): InstallationTokenCache { + isolateTokenCache ??= createInstallationTokenCache() + return isolateTokenCache +} + +export interface GithubCredentialRuntime { + fetch: typeof globalThis.fetch + /** Unix seconds. */ + now: () => number + tokenCache: InstallationTokenCache + report: (event: GithubCredentialReport) => void +} + +/** The credential for one GitHub read. An undefined token reads anonymously. */ +export interface GithubReadCredential { + token: string | undefined + /** The read App's installation token, which a Repository can deny. */ + isApp: boolean +} + +export interface GithubCredential { + current: () => Promise + /** The token for a read GitHub denied the App, or null. */ + fallback: FallbackToken | null +} + +/** + * Parse the read App secrets and the caller's fallback tokens, in order. + * + * With no App secret at all, the first fallback token becomes the primary + * credential, which is what local development runs on. + */ +export function parseGithubCredentialConfig( + env: GithubCredentialEnv, + fallbackNames: ReadonlyArray, +): GithubCredentialConfig { + const fallback = firstToken(env, fallbackNames) + const appSecrets = [env.SKILLD_READ_APP_ID, env.SKILLD_READ_APP_INSTALLATION_ID, env.SKILLD_READ_APP_PRIVATE_KEY_PKCS8] + if (appSecrets.every(value => !value?.trim())) + return fallback ? { _tag: 'token', token: fallback } : { _tag: 'anonymous' } + const misconfigured = (reason: string): GithubCredentialConfig => ({ _tag: 'app-misconfigured', reason, fallback }) + const appId = positiveInteger(env.SKILLD_READ_APP_ID) + if (appId === null) + return misconfigured('SKILLD_READ_APP_ID is not a GitHub App ID') + const installationId = positiveInteger(env.SKILLD_READ_APP_INSTALLATION_ID) + if (installationId === null) + return misconfigured('SKILLD_READ_APP_INSTALLATION_ID is not an installation ID') + const privateKeyPkcs8 = pkcs8PemBytes(env.SKILLD_READ_APP_PRIVATE_KEY_PKCS8 ?? '') + if (!privateKeyPkcs8) + return misconfigured('SKILLD_READ_APP_PRIVATE_KEY_PKCS8 is not a PKCS #8 PEM private key') + return { _tag: 'app', app: { appId, installationId, privateKeyPkcs8 }, fallback } +} + +/** + * The GitHub credential a caller reads with: the read App's installation + * token, or the fallback when the App gives none. + * + * When the App cannot give a token, the read still runs on the fallback and + * the reason is reported, so a broken key shows up as an event instead of as + * a spent quota. + */ +export function createGithubCredential( + config: GithubCredentialConfig, + runtime: GithubCredentialRuntime, +): GithubCredential { + if (config._tag === 'anonymous') + return { current: async () => ({ token: undefined, isApp: false }), fallback: null } + if (config._tag === 'token') + return { current: async () => ({ token: config.token.token, isApp: false }), fallback: null } + const fallbackName = config.fallback?.name ?? 'anonymous' + const fallback = { token: config.fallback?.token, isApp: false } + if (config._tag === 'app-misconfigured') { + return { + current: async () => { + runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) + return fallback + }, + fallback: null, + } + } + const app = config.app + const key = `${app.appId}:${app.installationId}` + return { + current: async () => { + const cached = runtime.tokenCache.tokens.get(key) + if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) + return { token: cached.token, isApp: true } + let pending = runtime.tokenCache.pending.get(key) + if (!pending) { + pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) + runtime.tokenCache.pending.set(key, pending) + } + const minted = await pending + if ('token' in minted) { + runtime.tokenCache.tokens.set(key, minted) + return { token: minted.token, isApp: true } + } + runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) + return fallback + }, + fallback: config.fallback, + } +} + +/** + * Whether GitHub refused a read because it denies the App, as opposed to a + * spent quota. Organizations that restrict Apps answer 403 with quota left, + * and no rate limit message. `neondatabase` did on 2026-10-06. + * + * It reads the body of a 403, so pass a clone or a response whose body is + * not needed. + */ +export async function githubDeniedApp(response: Response): Promise { + if (response.status !== 403) + return false + if (response.headers.get('x-ratelimit-remaining') === '0' || response.headers.get('retry-after')) + return false + // A 403 body that will not read names no rate limit either, so it counts + // as a denial. The status already says the read failed. + const text = await response.text().catch(() => '') + return !/rate limit/i.test(text) +} + +const installationTokenSchema = z.object({ + token: z.string().min(1).max(2048), + expires_at: z.string().datetime(), +}).passthrough() + +export type InstallationTokenResult + = | { _tag: 'created', token: string, expiresAt: string } + /** GitHub answered 401, 403 or 404: the App, its key or the installation is gone. */ + | { _tag: 'refused', status: number } + +/** + * Exchange an App JWT for an installation access token. + * + * A refusal is a value. Any other failure, such as a 5xx or an unreadable + * answer, throws: it says nothing about the App. + */ +export async function requestInstallationToken(input: { + fetch: typeof globalThis.fetch + jwt: string + installationId: number + body: { repository_ids?: number[], permissions: Record } +}): Promise { + const response = await input.fetch(`${GITHUB_API}/app/installations/${input.installationId}/access_tokens`, { + method: 'POST', + headers: { + 'Accept': 'application/vnd.github+json', + 'Authorization': `Bearer ${input.jwt}`, + 'Content-Type': 'application/json', + 'User-Agent': 'skilld.dev', + 'X-GitHub-Api-Version': GITHUB_API_VERSION, + }, + body: JSON.stringify(input.body), + // workerd accepts only `follow` and `manual`. A redirect here is an error. + redirect: 'manual', + signal: AbortSignal.timeout(GITHUB_REQUEST_TIMEOUT_MS), + }) + if (response.status >= 300 && response.status < 400) { + await response.body?.cancel() + throw new Error(`GitHub App request redirected ${response.status}`) + } + if (response.status === 401 || response.status === 403 || response.status === 404) { + await response.body?.cancel() + return { _tag: 'refused', status: response.status } + } + if (!response.ok) + throw new Error(`GitHub App request returned ${response.status}`) + const value = installationTokenSchema.safeParse(await readBoundedJson(response, MAX_TOKEN_RESPONSE_BYTES)) + if (!value.success) + throw new Error('GitHub App returned an invalid response') + return { _tag: 'created', token: value.data.token, expiresAt: value.data.expires_at } +} + +/** + * An RS256 GitHub App JWT. `issuer` is the App's client ID or its App ID; + * GitHub accepts either. + * + * It is valid for ten minutes from a minute in the past, which absorbs clock + * drift between the Worker and GitHub. Ten minutes is GitHub's maximum. + */ +export async function signGithubAppJwt(input: { + issuer: string + privateKeyPkcs8: Uint8Array + /** Unix seconds. */ + now: number +}): Promise { + const issuedAt = input.now - 60 + const header = base64Url(new TextEncoder().encode(JSON.stringify({ alg: 'RS256', typ: 'JWT' }))) + const payload = base64Url(new TextEncoder().encode(JSON.stringify({ + iat: issuedAt, + exp: issuedAt + 600, + iss: input.issuer, + }))) + const unsigned = `${header}.${payload}` + const imported = await crypto.subtle.importKey( + 'pkcs8', + Uint8Array.from(input.privateKeyPkcs8).buffer, + { name: 'RSASSA-PKCS1-v1_5', hash: 'SHA-256' }, + false, + ['sign'], + ) + const signature = await crypto.subtle.sign( + 'RSASSA-PKCS1-v1_5', + imported, + new TextEncoder().encode(unsigned), + ) + return `${unsigned}.${base64Url(new Uint8Array(signature))}` +} + +async function mintInstallationToken( + app: ReadAppConfig, + runtime: Pick, +): Promise { + const minted = await signGithubAppJwt({ issuer: String(app.appId), privateKeyPkcs8: app.privateKeyPkcs8, now: runtime.now() }) + .then(jwt => requestInstallationToken({ + fetch: runtime.fetch, + jwt, + installationId: app.installationId, + body: { permissions: { metadata: 'read' } }, + })) + // A network fault, a 5xx or a key WebCrypto rejects: the read uses the + // fallback token, and the reason reaches the report. + .catch((error: unknown) => ({ _tag: 'failed' as const, reason: error instanceof Error ? error.message : String(error) })) + if (minted._tag === 'failed') + return { _tag: 'unavailable', reason: minted.reason.slice(0, 200) } + if (minted._tag === 'refused') + return { _tag: 'unavailable', reason: `GitHub refused the installation token (${minted.status})` } + const expiresAt = Math.floor(Date.parse(minted.expiresAt) / 1000) + return { token: minted.token, expiresAt } +} + +function firstToken(env: GithubCredentialEnv, names: ReadonlyArray): FallbackToken | null { + for (const name of names) { + const token = env[name]?.trim() + if (token) + return { name, token } + } + return null +} + +function positiveInteger(value: string | undefined): number | null { + const trimmed = value?.trim() ?? '' + if (!/^[1-9]\d*$/.test(trimmed)) + return null + const parsed = Number(trimmed) + return Number.isSafeInteger(parsed) ? parsed : null +} + +/** + * The DER bytes of a PKCS #8 PEM private key, or null. + * + * A secret pasted through a dashboard or a JSON file can hold its line breaks + * as the two characters `\n`, so both forms parse. A PKCS #1 key + * (`BEGIN RSA PRIVATE KEY`, as GitHub downloads it) does not: WebCrypto + * imports PKCS #8 only. Convert it with `openssl pkcs8 -topk8 -nocrypt`. + */ +function pkcs8PemBytes(value: string): Uint8Array | null { + const text = value.replaceAll('\\n', '\n').trim() + const match = /^-----BEGIN PRIVATE KEY-----([A-Z0-9+/=\s]+)-----END PRIVATE KEY-----$/i.exec(text) + if (!match) + return null + const base64 = match[1]!.replace(/\s+/g, '') + if (!/^[A-Z0-9+/]+={0,2}$/i.test(base64) || base64.length % 4 !== 0) + return null + const binary = atob(base64) + return Uint8Array.from(binary, character => character.charCodeAt(0)) +} + +function base64Url(bytes: Uint8Array): string { + let binary = '' + for (let offset = 0; offset < bytes.length; offset += 0x8000) + binary += String.fromCharCode(...bytes.subarray(offset, offset + 0x8000)) + return btoa(binary).replaceAll('+', '-').replaceAll('/', '_').replace(/=+$/, '') +} + +async function readBoundedJson(response: Response, maximumBytes: number): Promise { + const declared = Number(response.headers.get('content-length')) + if (Number.isFinite(declared) && declared > maximumBytes) + throw new Error('GitHub App response exceeded the byte limit') + if (!response.body) + throw new Error('GitHub App returned an empty response') + const reader = response.body.getReader() + const chunks: Uint8Array[] = [] + let size = 0 + while (true) { + const next = await reader.read() + if (next.done) + break + size += next.value.byteLength + if (size > maximumBytes) { + await reader.cancel('response too large') + throw new Error('GitHub App response exceeded the byte limit') + } + chunks.push(next.value) + } + const bytes = new Uint8Array(size) + let offset = 0 + for (const chunk of chunks) { + bytes.set(chunk, offset) + offset += chunk.byteLength + } + return JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(bytes)) as unknown +} diff --git a/test/unit/artifact-github-read-app.test.ts b/test/unit/artifact-github-read-app.test.ts index 5977eab0..bcdf950f 100644 --- a/test/unit/artifact-github-read-app.test.ts +++ b/test/unit/artifact-github-read-app.test.ts @@ -1,8 +1,8 @@ import type { SourceRequest } from '../../layers/artifact-delivery/server/schemas/contracts' -import type { ArtifactGithubCredentialReport } from '../../layers/artifact-delivery/server/utils/github-read-credential' +import type { GithubCredentialReport } from '../../shared/server/github-app-credential' import { describe, expect, it } from 'vitest' -import { createInstallationTokenCache } from '../../layers/artifact-delivery/server/utils/github-read-credential' import { createArtifactGithubSource } from '../../layers/artifact-delivery/server/utils/queue' +import { createInstallationTokenCache } from '../../shared/server/github-app-credential' // Builds shared the site token with the hourly registry sync and every `/gh` // page view, so a run failed RATE_LIMITED whenever the sync spent it. The read @@ -90,7 +90,7 @@ describe('the GitHub credential public Artifact builds read with', () => { it('reads with the fallback token, and reports why, when GitHub refuses the installation token', async () => { const keys = await appKeys() const github = fakeGithub({ expiresAt: NOW + 3600, mintStatus: 401 }) - const reports: ArtifactGithubCredentialReport[] = [] + const reports: GithubCredentialReport[] = [] const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })) await source.resolve(request) @@ -101,7 +101,7 @@ describe('the GitHub credential public Artifact builds read with', () => { it('reports an unreadable App key and reads with the fallback token', async () => { const github = fakeGithub({ expiresAt: NOW + 3600 }) - const reports: ArtifactGithubCredentialReport[] = [] + const reports: GithubCredentialReport[] = [] const env = { ...appEnv('-----BEGIN RSA PRIVATE KEY-----\nAAAA\n-----END RSA PRIVATE KEY-----'), ARTIFACT_GITHUB_TOKEN: '' } const source = createArtifactGithubSource(env, runtime(github, { report: event => reports.push(event) })) @@ -115,7 +115,7 @@ describe('the GitHub credential public Artifact builds read with', () => { it('never puts a token or the key in a report', async () => { const keys = await appKeys() const github = fakeGithub({ expiresAt: NOW + 3600, mintStatus: 500 }) - const reports: ArtifactGithubCredentialReport[] = [] + const reports: GithubCredentialReport[] = [] await createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })).resolve(request) const text = JSON.stringify(reports) @@ -128,7 +128,7 @@ describe('the GitHub credential public Artifact builds read with', () => { it('repeats a read the Repository denied the read App with the fallback token, and reports it', async () => { const keys = await appKeys() const github = fakeGithub({ expiresAt: NOW + 3600, deniesApp: true }) - const reports: ArtifactGithubCredentialReport[] = [] + const reports: GithubCredentialReport[] = [] const source = createArtifactGithubSource(appEnv(keys.pem), runtime(github, { report: event => reports.push(event) })) const result = await source.resolve(request) @@ -147,7 +147,11 @@ describe('the GitHub credential public Artifact builds read with', () => { const result = await createArtifactGithubSource(env, runtime(github)).resolve(request) expect(github.readAuthorizations).toEqual(['Bearer ghs_installation_1']) - expect(result).toMatchObject({ _tag: 'rejected', code: 'SOURCE_ACCESS_DENIED' }) + expect(result).toMatchObject({ + _tag: 'rejected', + code: 'SOURCE_ACCESS_DENIED', + summary: 'GitHub denies the skilld.dev read App access to this Repository, and no fallback token is set.', + }) }) }) @@ -182,7 +186,7 @@ function fakeGithub(options: { expiresAt: number, mintStatus?: number, deniesApp function runtime( github: FakeGithub, - overrides: Partial<{ cache: ReturnType, now: () => number, report: (event: ArtifactGithubCredentialReport) => void }> = {}, + overrides: Partial<{ cache: ReturnType, now: () => number, report: (event: GithubCredentialReport) => void }> = {}, ) { return { fetch: github.fetch, diff --git a/test/unit/artifact-requester-github.test.ts b/test/unit/artifact-requester-github.test.ts index 3ef2c0a6..b4fa30af 100644 --- a/test/unit/artifact-requester-github.test.ts +++ b/test/unit/artifact-requester-github.test.ts @@ -89,6 +89,17 @@ describe('a signed-in run when the shared GitHub credential is rate limited', () expect(shared.resolve).toHaveBeenCalledOnce() }) + it('reads with the requester\'s token when GitHub denies the read App and no fallback token is set', async () => { + const github = userGithub() + const shared = sharedGithub({ code: 'SOURCE_ACCESS_DENIED' }) + const id = await requested('alice-run-0000008', ALICE) + + await processArtifactBuild(dependencies(github, shared), id) + + expect(github.authorizations).toContain('Bearer gho_alice') + expect(await getResolution(fixture.db, id)).toMatchObject({ state: 'failed', error_code: 'SOURCE_NOT_FOUND' }) + }) + it('reports RATE_LIMITED when the requester has no stored GitHub token', async () => { fixture.raw.prepare(`UPDATE users SET github_token_encrypted = NULL WHERE id = ?`).run(ALICE) const github = userGithub() diff --git a/test/unit/registry-github-app-credential.test.ts b/test/unit/registry-github-app-credential.test.ts new file mode 100644 index 00000000..dc659c77 --- /dev/null +++ b/test/unit/registry-github-app-credential.test.ts @@ -0,0 +1,195 @@ +import type { GithubCredentialReport } from '../../shared/server/github-app-credential' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + getRepo, + getRepoSummariesBatch, + getRepoSummary, + resolveGithubBindings, +} from '../../layers/registry/server/utils/github-client' +import { githubRateLimited } from '../../layers/registry/server/utils/github-rate-limited' +import { createInstallationTokenCache } from '../../shared/server/github-app-credential' + +// Every Worker GitHub read used `GITHUB_TOKEN`, a personal token whose quota +// the owner's own tools and CI runners also spend. The read App's +// installation has a bucket of its own. `GITHUB_TOKEN` is now read only where +// GitHub denies the App, such as an organization that restricts Apps. +const APP_ID = '5212127' +const INSTALLATION_ID = '168540695' +const NOW = 1_791_300_000 + +afterEach(() => { + vi.unstubAllGlobals() +}) + +describe('registry GitHub reads with the read App', () => { + it('reads with the installation token, not GITHUB_TOKEN', async () => { + const github = await stubGithub({}) + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime()) + + const repo = await getRepo('nuxt', 'ui', bindings) + + expect(repo.status).toBe(200) + expect(github.mints).toBe(1) + expect(github.reads).toEqual(['Bearer ghs_app']) + }) + + it('repeats a read GitHub denied the App with GITHUB_TOKEN, and reports it', async () => { + const github = await stubGithub({ deniesApp: true }) + const reports: GithubCredentialReport[] = [] + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime(reports)) + + const repo = await getRepo('neondatabase', 'agent-skills', bindings) + + expect(repo.status).toBe(200) + expect(github.reads).toEqual(['Bearer ghs_app', 'Bearer site']) + expect(reports).toEqual([{ + outcome: 'app-denied', + reason: 'GitHub denied the read App for /repos/neondatabase/agent-skills', + fallback: 'GITHUB_TOKEN', + }]) + }) + + it('answers a denied read as denied, not rate limited, with no GITHUB_TOKEN', async () => { + const github = await stubGithub({ deniesApp: true }) + const reports: GithubCredentialReport[] = [] + const bindings = resolveGithubBindings(await appEnv({}), runtime(reports)) + + const repo = await getRepo('neondatabase', 'agent-skills', bindings) + + expect(github.reads).toEqual(['Bearer ghs_app']) + expect(repo).toMatchObject({ status: 403, denied: true, data: null }) + // The sync pauses every repository for a spent quota. A denial names one. + expect(githubRateLimited(repo)).toBe(false) + expect(reports).toEqual([{ + outcome: 'app-denied', + reason: 'GitHub denied the read App for /repos/neondatabase/agent-skills', + fallback: 'none', + }]) + }) + + it('keeps a secondary rate limit a rate limit, with no second read', async () => { + const github = await stubGithub({ secondaryLimit: true }) + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime()) + + const repo = await getRepo('nuxt', 'ui', bindings) + + expect(github.reads).toEqual(['Bearer ghs_app']) + expect(githubRateLimited(repo)).toBe(true) + }) + + it('repeats a GraphQL summary GitHub answered FORBIDDEN with GITHUB_TOKEN', async () => { + const github = await stubGithub({ deniesApp: true }) + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime()) + + const summary = await getRepoSummary('neondatabase', 'agent-skills', bindings) + + expect(summary.status).toBe(200) + expect(github.reads).toEqual(['Bearer ghs_app', 'Bearer site']) + }) + + it('leaves a FORBIDDEN alias to the per-repository sync and keeps the rest of the batch', async () => { + const github = await stubGithub({ deniesApp: true }) + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime()) + + const batch = await getRepoSummariesBatch([ + { owner: 'nuxt', repo: 'ui' }, + { owner: 'neondatabase', repo: 'agent-skills' }, + ], bindings) + + expect(batch).toMatchObject({ _tag: 'read', summaries: [{ meta: { full_name: 'nuxt/ui' } }, null] }) + expect(github.reads).toEqual(['Bearer ghs_app']) + }) + + it('reads with GITHUB_TOKEN when no App secret is set, as in local development', async () => { + const github = await stubGithub({}) + const bindings = resolveGithubBindings({ GITHUB_TOKEN: 'local' }, runtime()) + + await getRepo('nuxt', 'ui', bindings) + + expect(github.mints).toBe(0) + expect(github.reads).toEqual(['Bearer local']) + }) +}) + +function runtime(reports: GithubCredentialReport[] = []) { + return { tokenCache: createInstallationTokenCache(), now: () => NOW, report: (event: GithubCredentialReport) => reports.push(event) } +} + +async function appEnv(tokens: { GITHUB_TOKEN?: string }) { + const pair = await crypto.subtle.generateKey( + { name: 'RSASSA-PKCS1-v1_5', modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: 'SHA-256' }, + true, + ['sign', 'verify'], + ) as CryptoKeyPair + const der = new Uint8Array(await crypto.subtle.exportKey('pkcs8', pair.privateKey) as ArrayBuffer) + const pem = `-----BEGIN PRIVATE KEY-----\n${btoa(String.fromCharCode(...der))}\n-----END PRIVATE KEY-----\n` + return { + SKILLD_READ_APP_ID: APP_ID, + SKILLD_READ_APP_INSTALLATION_ID: INSTALLATION_ID, + SKILLD_READ_APP_PRIVATE_KEY_PKCS8: pem, + ...tokens, + } +} + +/** + * GitHub as the App and a personal token see it. `neondatabase` denies the + * App on every read, as it did on 2026-10-06, and answers a personal token. + */ +async function stubGithub(options: { deniesApp?: boolean, secondaryLimit?: boolean }) { + const state = { mints: 0, reads: [] as Array } + vi.stubGlobal('fetch', vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input) + const headers = new Headers(init?.headers) + if (url.endsWith(`/app/installations/${INSTALLATION_ID}/access_tokens`)) { + state.mints++ + return Response.json({ token: 'ghs_app', expires_at: new Date((NOW + 3600) * 1000).toISOString() }, { status: 201 }) + } + const authorization = headers.get('authorization') + state.reads.push(authorization) + const asApp = authorization === 'Bearer ghs_app' + if (options.secondaryLimit) + return Response.json({ message: 'You have exceeded a secondary rate limit. Please wait a few minutes before you try again.' }, { status: 403 }) + if (url.endsWith('/graphql')) { + const body = JSON.parse(String(init?.body)) as { variables: Record } + const owners = Object.entries(body.variables).filter(([key]) => key === 'owner' || /^o\d+$/.test(key)) + const denied = options.deniesApp && asApp && owners.some(([, owner]) => owner === 'neondatabase') + const repository = (owner: string, name: string) => ({ + name, + nameWithOwner: `${owner}/${name}`, + url: `https://github.com/${owner}/${name}`, + owner: { login: owner }, + description: null, + stargazerCount: 1, + forkCount: 0, + pushedAt: '2026-10-01T00:00:00Z', + createdAt: '2026-01-01T00:00:00Z', + isArchived: false, + isFork: false, + defaultBranchRef: { name: 'main', target: { oid: 'a'.repeat(40), tree: { oid: 'b'.repeat(40) } } }, + }) + if ('owner' in body.variables) { + return denied + ? Response.json({ data: { repository: null }, errors: [{ type: 'FORBIDDEN', message: 'Resource not accessible by integration' }] }) + : Response.json({ data: { repository: repository(body.variables.owner!, body.variables.repo!) } }) + } + const data: Record = {} + const errors: Array<{ type: string, path: string[] }> = [] + for (const [key, owner] of owners) { + const alias = `r${key.slice(1)}` + const name = body.variables[`n${key.slice(1)}`]! + if (owner === 'neondatabase' && denied) { + data[alias] = null + errors.push({ type: 'FORBIDDEN', path: [alias] }) + } + else { + data[alias] = repository(owner, name) + } + } + return Response.json(errors.length ? { data, errors } : { data }) + } + if (options.deniesApp && asApp && url.includes('/neondatabase/')) + return Response.json({ message: 'Resource not accessible by integration' }, { status: 403, headers: { 'x-ratelimit-remaining': '4990' } }) + return Response.json({ name: 'repo', full_name: 'owner/repo', owner: { login: 'owner' }, default_branch: 'main' }, { status: 200 }) + })) + return state +}