From 9d65ff993a8f91f42d91ff9dec08759493c82ba2 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:14:09 +1100 Subject: [PATCH 1/4] fix(artifact-delivery): find a named Skill in any Repository size A name search read the whole Repository tree under the 2,000 entry limit for one Skill folder, so every Repository over that size failed with INVALID_SOURCE. Copies of one Skill in Agent folders or plugin mirrors failed as ambiguous. The search now reads one recursive tree and picks the canonical copy: visible folders first, then fewest segments, then code unit order. Size rejections now name the Skill folder, the size, and the limit, and say when a root Skill counts the whole Repository. --- .../server/utils/github-source.ts | 93 ++++++++++++---- shared/skill-path.ts | 36 ++++++ test/unit/artifact-named-skill.test.ts | 103 ++++++++++++++++++ test/unit/artifact-source-limits.test.ts | 53 ++++++++- 4 files changed, 263 insertions(+), 22 deletions(-) create mode 100644 test/unit/artifact-named-skill.test.ts diff --git a/layers/artifact-delivery/server/utils/github-source.ts b/layers/artifact-delivery/server/utils/github-source.ts index b6edb147..6909dcad 100644 --- a/layers/artifact-delivery/server/utils/github-source.ts +++ b/layers/artifact-delivery/server/utils/github-source.ts @@ -2,7 +2,7 @@ import type { ProblemCode, ResolvedSource, SourceRequest } from '../schemas/cont import type { GithubReadTry } from './github-read' import type { TarballExtraction } from './tarball-source' import { z } from 'zod' -import { isRegistrySkillPath } from '#shared/skill-path' +import { canonicalSkillFolder, isRegistrySkillPath } from '#shared/skill-path' import { base64ToBytes, gitBlobShaHex } from './encoding' import { fetchNoRedirect } from './fetch-no-redirect' import { @@ -316,7 +316,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi return readRejection(recursive) if (!recursive.value.truncated) { if (recursive.value.tree.length > MAX_TREE_ENTRIES) - return sourceLimitRejection(`The Skill has more than ${MAX_TREE_ENTRIES} source entries.`) + return sourceLimitRejection(`The Skill folder has more than ${MAX_TREE_ENTRIES} entries.`) return { _tag: 'listed', entries: recursive.value.tree, truncated: false } } @@ -325,7 +325,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi let requests = 0 while (queue.length > 0) { if (++requests > MAX_TREE_REQUESTS) - return sourceLimitRejection(`The Skill needs more than ${MAX_TREE_REQUESTS} tree reads.`) + return sourceLimitRejection(`The Skill folder needs more than ${MAX_TREE_REQUESTS} tree reads.`) const current = queue.shift()! const response = await getTree(owner, repository, current.sha, false) if (response._tag !== 'ok') @@ -337,7 +337,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const nested = { ...entry, path } entries.push(nested) if (entries.length > MAX_TREE_ENTRIES) - return sourceLimitRejection(`The Skill has more than ${MAX_TREE_ENTRIES} source entries.`) + return sourceLimitRejection(`The Skill folder has more than ${MAX_TREE_ENTRIES} entries.`) if (entry.type === 'tree') queue.push({ sha: entry.sha, prefix: path }) } @@ -368,24 +368,42 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi return currentSha } + /** + * The folder of the Skill a name selects, searched across the whole + * Repository. + * + * The search reads one recursive tree and loads nothing, so the Skill + * limits do not apply to it. It used to read the tree through + * `listTreeBounded`, whose 2,000 entry ceiling is a limit for one Skill: + * every Repository larger than that failed with INVALID_SOURCE, whatever + * the Skill size. Measured 2026-10-07: github/awesome-copilot (3,958 + * entries), garrytan/gstack (3,398), heygen-com/hyperframes (10,342). + * + * Copies of one Skill in several folders resolve to the canonical copy. + * They used to fail as "More than one Skill matched". + */ const resolveNamedSkill = async ( owner: string, repository: string, treeSha: string, name: string, ): Promise => { - const tree = await listTreeBounded(owner, repository, treeSha) - if (tree._tag !== 'listed') - return tree - const matches = tree.entries + const tree = await getTree(owner, repository, treeSha, true) + if (tree._tag !== 'ok') + return readRejection(tree) + if (tree.value.truncated) { + return reject( + 'INVALID_SOURCE', + 'The Repository is too large to find a Skill by name. Name the Skill by its path.', + [name], + ) + } + const matches = tree.value.tree .filter(entry => entry.type === 'blob' && isRegistrySkillPath(entry.path)) .map(entry => entry.path === 'SKILL.md' ? '.' : entry.path.slice(0, -'/SKILL.md'.length)) .filter(path => (path === '.' ? repository : path.split('/').at(-1)) === name) - if (matches.length === 0) - return reject('SOURCE_NOT_FOUND', 'No Skill matched the requested name.', [name]) - if (matches.length > 1) - return reject('INVALID_SOURCE', 'More than one Skill matched the requested name.', matches.slice(0, 100)) - return matches[0]! + return canonicalSkillFolder(matches) + ?? reject('SOURCE_NOT_FOUND', 'No Skill matched the requested name.', [name]) } return { @@ -500,7 +518,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const listed = await listTreeBounded(source.owner, source.repository, skillTree) if (listed._tag !== 'listed') return listed - const selected = selectArtifactEntries(listed.entries) + const selected = selectArtifactEntries(listed.entries, source.skillPath) if (selected._tag === 'rejected') return selected @@ -664,7 +682,14 @@ function normalizeRequestedSkillPath(input: string): string | null { return path } -function selectArtifactEntries(entries: TreeEntry[]): { _tag: 'selected', entries: Array } | SourceRejection { +/** + * The blobs one Skill folder packs, or the rule that refuses them. + * + * Every limit counts the Skill folder only. A Skill at the Repository root has + * the whole Repository as its folder, so each size message says so: a README + * image then counts against the Skill. + */ +function selectArtifactEntries(entries: TreeEntry[], skillPath: string): { _tag: 'selected', entries: Array } | SourceRejection { const findings: string[] = [] const identities = new Map() const blobs: Array = [] @@ -695,22 +720,50 @@ function selectArtifactEntries(entries: TreeEntry[]): { _tag: 'selected', entrie return reject('INVALID_SOURCE', 'The Skill source layout was rejected.', findings.slice(0, 100)) if (!blobs.some(entry => entry.path === 'SKILL.md')) return reject('INVALID_SOURCE', 'The Skill directory has no SKILL.md file.', ['SKILL.md']) + const rootNote = skillPath === '.' + ? ' The Skill folder is the Repository root, so every file in the Repository counts.' + : '' if (blobs.length > MAX_ARTIFACT_FILES) - return sourceLimitRejection(`The Skill has more than ${MAX_ARTIFACT_FILES} files.`) + return sourceLimitRejection(`${skillFolderLabel(skillPath)} has ${blobs.length} files. The limit is ${MAX_ARTIFACT_FILES}.`) const oversized = blobs.filter(entry => entry.size > MAX_FILE_BYTES) - if (oversized.length > 0) - return sourceLimitRejection(`A Skill file exceeds ${MAX_FILE_BYTES} bytes.`, oversized.map(entry => entry.path)) + if (oversized.length > 0) { + return sourceLimitRejection( + `The file \`${oversized[0]!.path}\` is ${mebibytes(oversized[0]!.size)}. The limit for one file is ${mebibytes(MAX_FILE_BYTES)}.${rootNote}`, + oversized.map(entry => `${entry.path}: ${entry.size.toLocaleString('en-US')} bytes`), + ) + } // The signer checks `content_bytes` against this same ceiling, and // `content_bytes` is the packed archive rather than the sum of the blobs. A // Skill that cleared a source-total check could therefore be refused after // the R2 write, with the signer's error instead of a named rejection. Guard // the number the signer will actually see. const projectedBytes = projectedUstarBytes(blobs.map(entry => entry.size)) - if (projectedBytes > MAX_ARTIFACT_BYTES) - return sourceLimitRejection(`The packaged Skill would exceed ${MAX_ARTIFACT_BYTES} bytes.`) + if (projectedBytes > MAX_ARTIFACT_BYTES) { + return sourceLimitRejection( + `${skillFolderLabel(skillPath)} packs to ${mebibytes(projectedBytes)}. The limit is ${mebibytes(MAX_ARTIFACT_BYTES)}.${rootNote}`, + largestFirst(blobs).map(entry => `${entry.path}: ${entry.size.toLocaleString('en-US')} bytes`), + ) + } return { _tag: 'selected', entries: blobs.sort((a, b) => comparePath(a.path, b.path)) } } +function skillFolderLabel(skillPath: string): string { + return skillPath === '.' ? 'The Skill folder' : `The Skill folder \`${skillPath}\`` +} + +/** + * Bytes as mebibytes, rounded up to two decimals so a size over a limit never + * prints equal to it. A whole number prints without decimals. + */ +function mebibytes(bytes: number): string { + const value = Math.ceil(bytes / 1024 / 1024 * 100) / 100 + return `${Number.isInteger(value) ? value : value.toFixed(2)} MiB` +} + +function largestFirst(entries: Array): Array { + return [...entries].sort((a, b) => b.size - a.size || comparePath(a.path, b.path)) +} + function isSafeArtifactPath(path: string): boolean { if (!path || path.length > 1024 || path.startsWith('/') || path.endsWith('/') || path.includes('\\') || path.includes('\0')) return false diff --git a/shared/skill-path.ts b/shared/skill-path.ts index 430bc9d8..307fd21f 100644 --- a/shared/skill-path.ts +++ b/shared/skill-path.ts @@ -41,6 +41,42 @@ export function isRegistrySkillPath(path: string): boolean { .some(segment => NON_SKILL_FOLDERS.has(segment.toLowerCase())) } +/** + * The canonical copy among Skill folders that share one name, or null for none. + * + * Repositories copy one Skill into many places: Agent folders such as + * `.claude/skills/x` and `.cursor/skills/x`, and plugin mirrors such as + * `plugins/x/skills/x`. Measured 2026-10-07: pbakaus/impeccable has 20 copies, + * freshtechbro/claudedesignskills has 4. Each copy is the same Skill to its + * author, so a name must pick one copy, the same one every time. + * + * Folders are directories relative to the Repository root, with `.` for the + * root. The order is a total order, so the input order never matters: + * + * 1. A folder with no hidden segment wins. Hidden folders are Agent targets. + * 2. Fewer segments win. The root has none. + * 3. Code unit order decides the rest. + */ +export function canonicalSkillFolder(folders: readonly string[]): string | null { + const ranked = [...new Set(folders)].sort(compareSkillFolders) + return ranked[0] ?? null +} + +function compareSkillFolders(left: string, right: string): number { + const leftSegments = left === '.' ? [] : left.split('/') + const rightSegments = right === '.' ? [] : right.split('/') + const hidden = Number(leftSegments.some(isHiddenSegment)) - Number(rightSegments.some(isHiddenSegment)) + if (hidden !== 0) + return hidden + if (leftSegments.length !== rightSegments.length) + return leftSegments.length - rightSegments.length + return left < right ? -1 : left > right ? 1 : 0 +} + +function isHiddenSegment(segment: string): boolean { + return segment.startsWith('.') +} + /** * A Markdown link into `.skilld/`, the local cache folder skilld v2 made. * diff --git a/test/unit/artifact-named-skill.test.ts b/test/unit/artifact-named-skill.test.ts new file mode 100644 index 00000000..aaf0c696 --- /dev/null +++ b/test/unit/artifact-named-skill.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it, vi } from 'vitest' +import { createPublicGithubSourceClient } from '../../layers/artifact-delivery/server/utils/github-source' + +const commitSha = '0123456789abcdef0123456789abcdef01234567' +const rootTreeSha = '89abcdef0123456789abcdef0123456789abcdef' +const blobSha = '3333333333333333333333333333333333333333' + +describe('named Skill resolution', () => { + it('finds a named Skill in a Repository with more entries than one Skill may hold', async () => { + // garrytan/gstack has 3,398 tree entries and one design-html Skill. + const filler = Array.from({ length: 2_500 }, (_, index) => blob(`docs/page-${index}.md`)) + const result = await resolveNamed('demo', [...filler, blob('skills/demo/SKILL.md')]) + + expect(result).toMatchObject({ _tag: 'resolved', source: { skillPath: 'skills/demo' } }) + }) + + it('picks the visible copy when Agent folders mirror the Skill', async () => { + // pbakaus/impeccable ships the same Skill in 20 Agent folders. + const result = await resolveNamed('demo', [ + blob('.claude/skills/demo/SKILL.md'), + blob('.cursor/skills/demo/SKILL.md'), + blob('plugins/bundle/skills/demo/SKILL.md'), + blob('skills/demo/SKILL.md'), + ]) + + expect(result).toMatchObject({ _tag: 'resolved', source: { skillPath: 'skills/demo' } }) + }) + + it('picks the root Skill over a plugin mirror of it', async () => { + // zarazhangrui/frontend-slides keeps one copy at the root and one under plugins/. + const result = await resolveNamed('skills', [ + blob('SKILL.md'), + blob('plugins/skills/skills/skills/SKILL.md'), + ]) + + expect(result).toMatchObject({ _tag: 'resolved', source: { skillPath: '.' } }) + }) + + it('picks the same copy whatever order the tree lists them in', async () => { + const copies = [ + blob('.cursor/skills/demo/SKILL.md'), + blob('cursor-plugin/skills/demo/SKILL.md'), + blob('plugin/skills/demo/SKILL.md'), + ] + const forward = await resolveNamed('demo', copies) + const backward = await resolveNamed('demo', [...copies].reverse()) + + expect(forward).toMatchObject({ _tag: 'resolved', source: { skillPath: 'cursor-plugin/skills/demo' } }) + expect(backward).toMatchObject({ _tag: 'resolved', source: { skillPath: 'cursor-plugin/skills/demo' } }) + }) + + it('picks a hidden copy when every copy is hidden', async () => { + const result = await resolveNamed('demo', [ + blob('.cursor/skills/demo/SKILL.md'), + blob('.claude/skills/demo/SKILL.md'), + ]) + + expect(result).toMatchObject({ _tag: 'resolved', source: { skillPath: '.claude/skills/demo' } }) + }) + + it('asks for a path when GitHub cannot list the whole Repository', async () => { + const result = await resolveNamed('demo', [blob('skills/demo/SKILL.md')], true) + + expect(result).toMatchObject({ + _tag: 'rejected', + code: 'INVALID_SOURCE', + summary: 'The Repository is too large to find a Skill by name. Name the Skill by its path.', + }) + }) +}) + +async function resolveNamed(name: string, entries: object[], truncated = false) { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + 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/ref/heads/main')) + return json({ ref: 'refs/heads/main', object: { type: 'commit', sha: commitSha } }) + if (url.endsWith(`/commits/${commitSha}`)) + return json({ sha: commitSha, commit: { tree: { sha: rootTreeSha } } }) + if (url.endsWith(`/git/trees/${rootTreeSha}?recursive=1`)) + return json({ sha: rootTreeSha, tree: entries, truncated }) + return json({}, 404) + }) + const client = createPublicGithubSourceClient({ fetch: fetchMock as typeof fetch }) + return await client.resolve({ + provider: 'github', + owner: 'skilld-dev', + repository: 'skills', + selector: { type: 'named-skill', name }, + }) +} + +function blob(path: string) { + return { path, mode: '100644', type: 'blob', sha: blobSha, size: 10 } +} + +function json(value: unknown, status = 200): Response { + return new Response(JSON.stringify(value), { + status, + headers: { 'content-type': 'application/json' }, + }) +} diff --git a/test/unit/artifact-source-limits.test.ts b/test/unit/artifact-source-limits.test.ts index f5adc5a9..842c3469 100644 --- a/test/unit/artifact-source-limits.test.ts +++ b/test/unit/artifact-source-limits.test.ts @@ -35,7 +35,7 @@ describe('artifact source size guards', () => { expect(loaded).toMatchObject({ _tag: 'rejected', code: 'INVALID_SOURCE', - summary: 'The Skill has more than 900 files.', + summary: 'The Skill folder `skills/demo` has 901 files. The limit is 900.', }) }) @@ -54,7 +54,45 @@ describe('artifact source size guards', () => { expect(loaded).toMatchObject({ _tag: 'rejected', code: 'INVALID_SOURCE', - summary: 'The packaged Skill would exceed 10485760 bytes.', + summary: 'The Skill folder `skills/demo` packs to 10.11 MiB. The limit is 10 MiB.', + }) + }) + + it('names the file over the one-file limit and its size', async () => { + // agiwhitelist/auteur keeps a 2,153,066 byte README image beside its SKILL.md. + const entries = [ + blob('SKILL.md', skillBlobSha, skillText.length), + blob('assets/readme/hero-anim.webp', skillBlobSha, 2_153_066), + ] + const client = createPublicGithubSourceClient({ + fetch: skillTreeFetch(entries) as unknown as typeof fetch, + }) + + const loaded = await client.load(resolvedSource()) + + expect(loaded).toMatchObject({ + _tag: 'rejected', + code: 'INVALID_SOURCE', + summary: 'The file `assets/readme/hero-anim.webp` is 2.06 MiB. The limit for one file is 2 MiB.', + findings: ['assets/readme/hero-anim.webp: 2,153,066 bytes'], + }) + }) + + it('says that a root Skill counts every file in the Repository', async () => { + const entries = [ + blob('SKILL.md', skillBlobSha, skillText.length), + blob('docs/assets/interactive-motion.gif', skillBlobSha, 2_479_001), + ] + const client = createPublicGithubSourceClient({ + fetch: rootTreeFetch(entries) as unknown as typeof fetch, + }) + + const loaded = await client.load({ ...resolvedSource(), skillPath: '.' }) + + expect(loaded).toMatchObject({ + _tag: 'rejected', + code: 'INVALID_SOURCE', + summary: 'The file `docs/assets/interactive-motion.gif` is 2.37 MiB. The limit for one file is 2 MiB. The Skill folder is the Repository root, so every file in the Repository counts.', }) }) }) @@ -103,6 +141,17 @@ function skillTreeFetch(entries: object[]) { }) } +function rootTreeFetch(entries: object[]) { + return vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url.endsWith('/repos/skilld-dev/skills')) + return json(publicRepository()) + if (url.endsWith(`/git/trees/${rootTreeSha}?recursive=1`)) + return json({ sha: rootTreeSha, tree: entries, truncated: false }) + return json({}, 404) + }) +} + function resolvedSource() { return { provider: 'github' as const, From 0569486562c27ae062235a7fe8bb95c9ea590a62 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:14:33 +1100 Subject: [PATCH 2/4] fix(artifact-delivery): warn on spec findings, carry retry wait The registry admits a Skill by its folder, so a frontmatter name that differs from the folder, a long description, or a missing field never changes which Skill runs. These findings blocked delivery of Skills the registry lists, such as leonxlnx/taste-skill. The check now reports them as a non-required warning with the exact values, and the policy version moves so no older attestation is reused. Safety checks stay required. A failed Resolution now carries retryAfterSeconds when the failure is retryable. skilld 3.2.0 and later read it. Production rate limits cluster at the top of the hour with waits of 4 to 288 seconds, so a client can wait instead of failing. --- .../server/schemas/contracts.ts | 2 + .../artifact-delivery/server/utils/checks.ts | 42 ++++++--- .../artifact-delivery/server/utils/state.ts | 8 +- test/unit/artifact-delivery.test.ts | 5 +- test/unit/artifact-signer.test.ts | 2 +- test/unit/artifact-spec-check.test.ts | 94 +++++++++++++++++++ test/unit/private-artifact-delivery.test.ts | 2 +- 7 files changed, 135 insertions(+), 20 deletions(-) create mode 100644 test/unit/artifact-spec-check.test.ts diff --git a/layers/artifact-delivery/server/schemas/contracts.ts b/layers/artifact-delivery/server/schemas/contracts.ts index 0755f8cc..42c0b07c 100644 --- a/layers/artifact-delivery/server/schemas/contracts.ts +++ b/layers/artifact-delivery/server/schemas/contracts.ts @@ -140,6 +140,8 @@ export const resolutionFailedSchema = z.object({ resolutionId: resolutionIdSchema, code: problemCodeSchema, retryable: z.boolean(), + /** Seconds until the same request can work. skilld 3.2.0 and later read it. */ + retryAfterSeconds: z.number().int().positive().optional(), }).strict() export const resolutionRevokedSchema = z.object({ diff --git a/layers/artifact-delivery/server/utils/checks.ts b/layers/artifact-delivery/server/utils/checks.ts index d3e40f08..37181be1 100644 --- a/layers/artifact-delivery/server/utils/checks.ts +++ b/layers/artifact-delivery/server/utils/checks.ts @@ -2,14 +2,21 @@ import type { ArtifactFile, CheckResult, ResolvedSource } from '../schemas/contr import type { ArtifactSourceFile } from './github-source' import { digestHex } from './encoding' -const AGENT_SKILLS_CHECK_VERSION = '2026-08-20' +/** + * 2026-10-07: the check became advisory. It still reports every finding, but + * no finding blocks delivery. The registry admits a Skill by its folder, so a + * frontmatter name, a long description or a missing field never changes which + * Skill runs. Safety checks stay required. + */ +const AGENT_SKILLS_CHECK_VERSION = '2026-10-07' +const MAX_DESCRIPTION_CHARACTERS = 1024 const PATH_POLICY_VERSION = '1' const CREDENTIAL_MATERIAL_VERSION = '1' const EXECUTABLE_FILES_VERSION = '1' const CURRENT_ARTIFACT_CHECKS = new Map([ ['path-policy', { version: PATH_POLICY_VERSION, required: true }], - ['agent-skills-spec', { version: AGENT_SKILLS_CHECK_VERSION, required: true }], + ['agent-skills-spec', { version: AGENT_SKILLS_CHECK_VERSION, required: false }], ['credential-material', { version: CREDENTIAL_MATERIAL_VERSION, required: true }], ['executable-files', { version: EXECUTABLE_FILES_VERSION, required: false }], ]) @@ -68,12 +75,16 @@ export async function checkArtifactSource( const directoryName = source.skillPath === '.' ? source.repository : source.skillPath.split('/').at(-1)! - if (!frontmatter.name || !/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(frontmatter.name) || frontmatter.name.length > 64) - specFindings.push('The Skill name must use 1 to 64 lowercase letters, numbers, or single hyphens.') + if (!frontmatter.name) + specFindings.push('The frontmatter has no name.') + else if (!/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(frontmatter.name) || frontmatter.name.length > 64) + specFindings.push(`The frontmatter name \`${frontmatter.name.slice(0, 100)}\` must use 1 to 64 lowercase letters, numbers, or single hyphens.`) else if (frontmatter.name !== directoryName) - specFindings.push('The Skill name must match its directory name.') - if (!frontmatter.description || frontmatter.description.length > 1024) - specFindings.push('The Skill description must contain 1 to 1024 characters.') + specFindings.push(`The frontmatter name \`${frontmatter.name}\` does not match the folder \`${directoryName}\`.`) + if (!frontmatter.description) + specFindings.push('The frontmatter has no description.') + else if (frontmatter.description.length > MAX_DESCRIPTION_CHARACTERS) + specFindings.push(`The description has ${frontmatter.description.length.toLocaleString('en-US')} characters. The limit is ${MAX_DESCRIPTION_CHARACTERS.toLocaleString('en-US')}.`) } } } @@ -81,8 +92,8 @@ export async function checkArtifactSource( ? { name: 'agent-skills-spec', version: AGENT_SKILLS_CHECK_VERSION, - outcome: 'fail', - required: true, + outcome: 'warn', + required: false, summary: 'The Skill does not match the Agent Skills specification.', findings: specFindings, } @@ -90,7 +101,7 @@ export async function checkArtifactSource( name: 'agent-skills-spec', version: AGENT_SKILLS_CHECK_VERSION, outcome: 'pass', - required: true, + required: false, } const credentialFindings: string[] = [] @@ -190,11 +201,14 @@ function readSkillFrontmatter(raw: string): { name?: string, description?: strin if (key !== 'name' && key !== 'description') continue const value = line.slice(separator + 1).trim() - if (value === '>' || value === '|') { - const block: string[] = [] + // A block scalar (`>`, `|-`, `>+2`) or a plain scalar that continues on + // indented lines. Reading `>-` as the value reported a two-character + // description for Skills whose description is over a thousand. + if (/^[>|][+-]?\d*$/.test(value) || value === '') { + const continued: string[] = [] while (index + 1 < lines.length && /^\s+/.test(lines[index + 1]!)) - block.push(lines[++index]!.trim()) - values[key] = value === '>' ? block.join(' ').trim() : block.join('\n').trim() + continued.push(lines[++index]!.trim()) + values[key] = continued.join(value.startsWith('|') ? '\n' : ' ').trim() } else { values[key] = value.replace(/^(['"])([\s\S]*)\1$/, '$2').trim() diff --git a/layers/artifact-delivery/server/utils/state.ts b/layers/artifact-delivery/server/utils/state.ts index dc268731..84bfba56 100644 --- a/layers/artifact-delivery/server/utils/state.ts +++ b/layers/artifact-delivery/server/utils/state.ts @@ -23,7 +23,7 @@ import { canonicalJson, digestHex } from './encoding' * commit produces. A ready build signed under another version is never reused, * so the bump also makes every commit load from GitHub once more. */ -export const ARTIFACT_POLICY_VERSION = '2026-08-20.1' +export const ARTIFACT_POLICY_VERSION = '2026-10-07.1' export const ACTIVE_BUILD_STATES = [ 'requested', @@ -79,6 +79,7 @@ const resolutionRowSchema = z.object({ encryption_key_id: z.string().nullable(), error_code: z.string().nullable(), error_retryable: z.union([z.literal(0), z.literal(1)]).nullable(), + error_retry_after: z.number().int().positive().nullable().optional(), created_at: z.number().int(), updated_at: z.number().int(), }) @@ -268,8 +269,8 @@ export async function transitionResolution( * - `pinned`: the request names a commit, and no GitHub read has happened. It * matches the commit, the owner and Repository name that GitHub reported for * the ready build, and the Skill path. A Skill name matches only a build - * requested by that same name, because only a resolve by name proved that - * exactly one Skill in the commit has it. + * requested by that same name, because a resolve by name picks one + * canonical folder of that name in the commit, the same one every time. */ export type ReadyBuildLookup = { _tag: 'resolved', source: ResolvedSource } @@ -416,6 +417,7 @@ export function presentResolution(row: ResolutionRow): ResolutionResponse { resolutionId: row.id, code: problemCodeSchema.parse(row.error_code), retryable: row.error_retryable === 1, + ...(row.error_retryable === 1 && row.error_retry_after ? { retryAfterSeconds: row.error_retry_after } : {}), } } if (row.state === 'revoked') { diff --git a/test/unit/artifact-delivery.test.ts b/test/unit/artifact-delivery.test.ts index e40d3f9f..03069179 100644 --- a/test/unit/artifact-delivery.test.ts +++ b/test/unit/artifact-delivery.test.ts @@ -173,7 +173,7 @@ describe('public Artifact delivery', () => { it('blocks failed checks before storage or signing', async () => { const harness = await createBuildHarness([{ ...validFiles[0]!, - bytes: new TextEncoder().encode('No frontmatter'), + bytes: new TextEncoder().encode('---\nname: demo\ndescription: Demo.\n---\n-----BEGIN PRIVATE KEY-----\n'), }]) const result = await processArtifactBuild(harness.dependencies, harness.resolutionId) @@ -238,11 +238,14 @@ describe('public Artifact delivery', () => { error_retry_after: 2_842_800, }) const failed = presentResolution((await getResolution(harness.dependencies.db, harness.resolutionId))!) + // skilld 3.2.0 and later read the wait, so a client can retry when the + // quota returns instead of guessing. expect(failed).toEqual({ state: 'failed', resolutionId: harness.resolutionId, code: 'RATE_LIMITED', retryable: true, + retryAfterSeconds: 2_842_800, }) harness.close() }) diff --git a/test/unit/artifact-signer.test.ts b/test/unit/artifact-signer.test.ts index f9c7519d..b9ba0905 100644 --- a/test/unit/artifact-signer.test.ts +++ b/test/unit/artifact-signer.test.ts @@ -257,7 +257,7 @@ async function createSignerFixture(options: SignerFixtureOptions = {}) { : artifactR2Key(contentSha256) const checks = [ { name: 'path-policy', version: '1', outcome: options.checkOutcome ?? 'pass', required: true }, - { name: 'agent-skills-spec', version: '2026-08-20', outcome: 'pass', required: true }, + { name: 'agent-skills-spec', version: '2026-10-07', outcome: 'pass', required: false }, { name: 'credential-material', version: '1', outcome: 'pass', required: true }, { name: 'executable-files', version: '1', outcome: 'pass', required: false }, ] as const diff --git a/test/unit/artifact-spec-check.test.ts b/test/unit/artifact-spec-check.test.ts new file mode 100644 index 00000000..748a1e3c --- /dev/null +++ b/test/unit/artifact-spec-check.test.ts @@ -0,0 +1,94 @@ +import type { ResolvedSource } from '../../layers/artifact-delivery/server/schemas/contracts' +import type { ArtifactSourceFile } from '../../layers/artifact-delivery/server/utils/github-source' +import { describe, expect, it } from 'vitest' +import { + checkArtifactSource, + checksBlockArtifact, + checksPermitSigning, +} from '../../layers/artifact-delivery/server/utils/checks' + +describe('agent Skills specification check', () => { + it('lets a Skill run when its frontmatter name differs from its folder', async () => { + // leonxlnx/taste-skill: folder `taste-skill`, frontmatter `design-taste-frontend`. + const checked = await check('skills/taste-skill', skillFile('---\nname: design-taste-frontend\ndescription: Anti-slop frontend design.\n---\n')) + + expect(checksBlockArtifact(checked.checkResults)).toBe(false) + expect(checksPermitSigning(checked.checkResults)).toBe(true) + expect(specResult(checked.checkResults)).toMatchObject({ + outcome: 'warn', + required: false, + findings: ['The frontmatter name `design-taste-frontend` does not match the folder `taste-skill`.'], + }) + }) + + it('lets a Skill run when its description is over the specification limit', async () => { + // jakubantalik/transitions.dev: a description longer than 1,024 characters. + const description = 'x'.repeat(1_342) + const checked = await check('skills/transitions-dev', skillFile(`---\nname: transitions-dev\ndescription: ${description}\n---\n`)) + + expect(checksPermitSigning(checked.checkResults)).toBe(true) + expect(specResult(checked.checkResults)).toMatchObject({ + outcome: 'warn', + findings: ['The description has 1,342 characters. The limit is 1,024.'], + }) + }) + + it('reads a folded block description to its full length', async () => { + // unclecatvn/agent-skills/flow-diagram writes its description as `>` block lines. + const line = 'y'.repeat(99) + const block = Array.from({ length: 11 }).fill(` ${line}`).join('\n') + const checked = await check('skills/flow-diagram', skillFile(`---\nname: flow-diagram\ndescription: >-\n${block}\n---\n`)) + + expect(specResult(checked.checkResults)).toMatchObject({ + outcome: 'warn', + findings: ['The description has 1,099 characters. The limit is 1,024.'], + }) + }) + + it('lets a Skill run when its frontmatter has no name', async () => { + const checked = await check('skills/demo', skillFile('---\ndescription: Use this Skill for demo work.\n---\n')) + + expect(checksPermitSigning(checked.checkResults)).toBe(true) + expect(specResult(checked.checkResults)).toMatchObject({ + outcome: 'warn', + findings: ['The frontmatter has no name.'], + }) + }) + + it('passes a Skill that matches the specification', async () => { + const checked = await check('skills/demo', skillFile('---\r\nname: demo\r\ndescription: Use this Skill for demo work.\r\n---\r\n')) + + expect(specResult(checked.checkResults)).toMatchObject({ outcome: 'pass', required: false }) + }) + + it('still blocks a Skill that contains private key material', async () => { + const checked = await check('skills/demo', skillFile('---\nname: demo\ndescription: Demo.\n---\n-----BEGIN PRIVATE KEY-----\n')) + + expect(checksBlockArtifact(checked.checkResults)).toBe(true) + }) +}) + +async function check(skillPath: string, file: ArtifactSourceFile) { + return await checkArtifactSource(source(skillPath), [file]) +} + +function specResult(results: Awaited>['checkResults']) { + return results.find(result => result.name === 'agent-skills-spec') +} + +function skillFile(text: string): ArtifactSourceFile { + return { path: 'SKILL.md', mode: 420, bytes: new TextEncoder().encode(text), gitBlobSha: 'a'.repeat(40) } +} + +function source(skillPath: string): ResolvedSource { + return { + provider: 'github', + repositoryId: 1, + owner: 'acme', + repository: 'skills', + visibility: 'public', + commitSha: '0'.repeat(40), + treeSha: '1'.repeat(40), + skillPath, + } +} diff --git a/test/unit/private-artifact-delivery.test.ts b/test/unit/private-artifact-delivery.test.ts index 5c01b689..75463c75 100644 --- a/test/unit/private-artifact-delivery.test.ts +++ b/test/unit/private-artifact-delivery.test.ts @@ -556,7 +556,7 @@ function insertUser(db: import('better-sqlite3').Database, id: number, githubId: function requiredChecks() { return [ { name: 'path-policy', version: '1', outcome: 'pass' as const, required: true }, - { name: 'agent-skills-spec', version: '2026-08-20', outcome: 'pass' as const, required: true }, + { name: 'agent-skills-spec', version: '2026-10-07', outcome: 'pass' as const, required: false }, { name: 'credential-material', version: '1', outcome: 'pass' as const, required: true }, { name: 'executable-files', version: '1', outcome: 'pass' as const, required: false }, ] From f3cbdec6cea6f963a71664ac054ee4434d87f6a1 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:14:41 +1100 Subject: [PATCH 3/4] fix(artifact-delivery): run a named Skill by its registry identity skilld run OWNER/REPO/NAME searched the Repository tree for the name, so delivery could disagree with the registry: it failed on copies the registry had already resolved, and it delivered the branch head while the Skill page named another commit. A public name the registry admitted now resolves the admitted folder at the commit the Skill page names. The registry answers over HTTP. A name it does not hold keeps the tree search. A reference the request names still wins. --- .../server/api/v1/resolutions/index.post.ts | 27 ++-- .../server/utils/admitted-identity.ts | 51 ++++++++ .../server/utils/request-resolution.ts | 62 +++++++++ .../server/api/skills/run-identity.get.ts | 34 +++++ .../server/presenters/skill-run-identity.ts | 13 ++ .../server/utils/skill-run-identity.ts | 62 +++++++++ test/unit/skill-run-identity.test.ts | 118 ++++++++++++++++++ 7 files changed, 351 insertions(+), 16 deletions(-) create mode 100644 layers/artifact-delivery/server/utils/admitted-identity.ts create mode 100644 layers/artifact-delivery/server/utils/request-resolution.ts create mode 100644 layers/registry/server/api/skills/run-identity.get.ts create mode 100644 layers/registry/server/presenters/skill-run-identity.ts create mode 100644 layers/registry/server/utils/skill-run-identity.ts create mode 100644 test/unit/skill-run-identity.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 3a5ad670..631a6983 100644 --- a/layers/artifact-delivery/server/api/v1/resolutions/index.post.ts +++ b/layers/artifact-delivery/server/api/v1/resolutions/index.post.ts @@ -10,11 +10,8 @@ import { withArtifactProblems } from '../../../utils/artifact-problem' import { findPrivateRepositoryAccess } from '../../../utils/private-access' import { privateArtifactAccessEnabled } from '../../../utils/private-feature' import { enqueueArtifactBuild } from '../../../utils/queue' +import { fetchAdmittedSkillIdentity, requestResolution } from '../../../utils/request-resolution' import { setSkillPageUrlHeader } from '../../../utils/skill-page' -import { - createResolution, - resolutionRequestIdentity, -} from '../../../utils/state' export default withArtifactProblems(defineApiHandler({ schema: createResolutionRequestSchema, @@ -36,14 +33,15 @@ export default withArtifactProblems(defineApiHandler({ body.source.repository, ) : { _tag: 'not-found' as const } - const accountId = privateAccess._tag === 'allowed' ? user!.id : undefined - const identity = await resolutionRequestIdentity(body.source, idempotencyKey, accountId) - const result = await createResolution( - platform.db, - body.source, - identity, - Math.floor(Date.now() / 1000), - privateAccess._tag === 'allowed' + const result = await requestResolution({ + db: platform.db, + lookupAdmitted: fetchAdmittedSkillIdentity(event.context), + enqueue: resolutionId => enqueueArtifactBuild(platform.env, resolutionId), + now: () => Math.floor(Date.now() / 1000), + }, { + source: body.source, + idempotencyKey, + access: privateAccess._tag === 'allowed' ? { visibility: 'private', accountId: privateAccess.accountId, @@ -51,7 +49,7 @@ export default withArtifactProblems(defineApiHandler({ repositoryId: privateAccess.repositoryId, } : { visibility: 'public' }, - ) + }) if (result._tag === 'idempotency-conflict') { throw createError({ statusCode: 409, @@ -80,9 +78,6 @@ export default withArtifactProblems(defineApiHandler({ data: { code: problemCodeSchema.parse(result.row.error_code) }, }) } - if (result.row.state === 'requested') - await enqueueArtifactBuild(platform.env, result.row.id) - const response = presentArtifactResolution(result.row) setResponseStatus(event, response.state === 'pending' ? 202 : 200) setHeader(event, 'cache-control', 'private, no-store') diff --git a/layers/artifact-delivery/server/utils/admitted-identity.ts b/layers/artifact-delivery/server/utils/admitted-identity.ts new file mode 100644 index 00000000..08dbe4a2 --- /dev/null +++ b/layers/artifact-delivery/server/utils/admitted-identity.ts @@ -0,0 +1,51 @@ +import type { SourceRequest } from '../schemas/contracts' + +export interface AdmittedSkillIdentity { + /** The admitted Skill folder inside the Repository, or `.` for the root. */ + skillPath: string + /** The commit the Skill page names, or null when the registry has none. */ + commitSha: string | null +} + +/** Ask the registry for the identity it admitted under one name; null when it holds none. */ +export type FetchAdmittedSkillIdentity = (skill: { + owner: string + repository: string + name: string +}) => Promise + +/** + * The source a public request resolves: the registry's admitted identity for + * a name it holds, and the request itself otherwise. + * + * The registry names one Skill per name, and the Skill page shows its folder + * and its source commit. A run of that name resolves exactly that folder at + * exactly that commit, so nothing in the Repository can disagree with it: + * copies in Agent folders, a frontmatter name, or the size of the tree. + * + * A reference the request names wins over the admitted commit, because the + * person asked for it. A name the registry does not hold, such as a private + * Skill, keeps the tree search by name. + */ +export async function admittedSourceRequest( + source: SourceRequest, + lookup: FetchAdmittedSkillIdentity, +): Promise { + if (source.selector.type !== 'named-skill') + return source + const identity = await lookup({ + owner: source.owner, + repository: source.repository, + name: source.selector.name, + }) + if (!identity) + return source + const ref = source.ref ?? (identity.commitSha ? { type: 'commit' as const, value: identity.commitSha } : undefined) + return { + provider: source.provider, + owner: source.owner, + repository: source.repository, + selector: { type: 'path', path: identity.skillPath }, + ...(ref ? { ref } : {}), + } +} diff --git a/layers/artifact-delivery/server/utils/request-resolution.ts b/layers/artifact-delivery/server/utils/request-resolution.ts new file mode 100644 index 00000000..d9556bb6 --- /dev/null +++ b/layers/artifact-delivery/server/utils/request-resolution.ts @@ -0,0 +1,62 @@ +import type { H3EventContext } from 'h3' +import type { SourceRequest } from '../schemas/contracts' +import type { FetchAdmittedSkillIdentity } from './admitted-identity' +import type { CreateResolutionResult } from './state' +import { admittedSourceRequest } from './admitted-identity' +import { createResolution, resolutionRequestIdentity } from './state' + +export interface ResolutionRequestDependencies { + db: D1Database + /** Ask the registry for the identity it admitted under a public name. */ + lookupAdmitted: FetchAdmittedSkillIdentity + enqueue: (resolutionId: string) => Promise + now: () => number +} + +export type ResolutionAccess + = | { visibility: 'public' } + | { + visibility: 'private' + accountId: number + installationId: number + repositoryId: number + } + +/** + * {@link FetchAdmittedSkillIdentity} wired to the registry layer over HTTP, + * the only way one layer reads another (ADR-0001). A request passes its event + * context so the internal call reuses its platform; a scheduled task passes + * none, and the platform plugin resolves the Worker env itself. + */ +export function fetchAdmittedSkillIdentity(context?: H3EventContext): FetchAdmittedSkillIdentity { + return async ({ owner, repository, name }) => { + // A named object passes `context`, which NitroFetchOptions does not declare. + const options = { query: { owner, repo: repository, name }, ...(context ? { context } : {}) } + const answer = await $fetch<{ skillPath: string | null, commitSha: string | null }>('/api/skills/run-identity', options) + return answer.skillPath ? { skillPath: answer.skillPath, commitSha: answer.commitSha } : null + } +} + +/** + * Create or replay one Resolution and queue its build. + * + * `POST /api/v1/resolutions` and the run sweep both call this, so the sweep + * checks exactly what `skilld run` gets. The fingerprint names the request as + * the client sent it, so a replay with the same Idempotency-Key still matches + * after the registry moves. A public name the registry admitted resolves its + * admitted folder and commit; a private request never asks the registry. + */ +export async function requestResolution( + dependencies: ResolutionRequestDependencies, + input: { source: SourceRequest, idempotencyKey: string, access: ResolutionAccess }, +): Promise { + const accountId = input.access.visibility === 'private' ? input.access.accountId : undefined + const identity = await resolutionRequestIdentity(input.source, input.idempotencyKey, accountId) + const source = input.access.visibility === 'private' + ? input.source + : await admittedSourceRequest(input.source, dependencies.lookupAdmitted) + const result = await createResolution(dependencies.db, source, identity, dependencies.now(), input.access) + if (result._tag !== 'idempotency-conflict' && result.row.state === 'requested') + await dependencies.enqueue(result.row.id) + return result +} diff --git a/layers/registry/server/api/skills/run-identity.get.ts b/layers/registry/server/api/skills/run-identity.get.ts new file mode 100644 index 00000000..54f0e05c --- /dev/null +++ b/layers/registry/server/api/skills/run-identity.get.ts @@ -0,0 +1,34 @@ +import { setHeader } from 'h3' +import { z } from 'zod' +import { defineApiHandler } from '#shared/server/handler' +import { presentSkillRunIdentity } from '../../presenters/skill-run-identity' +import { findSkillRunIdentity } from '../../utils/skill-run-identity' + +const RunIdentityQuery = z.object({ + owner: z.string().min(1).max(39).regex(/^[a-z0-9-]+$/i), + repo: z.string().min(1).max(100).regex(/^[\w.-]+$/), + name: z.string().min(1).max(64), +}).strict() + +const RunIdentityResponse = z.object({ + skillPath: z.string().min(1).nullable(), + commitSha: z.string().regex(/^[a-f0-9]{40}$/).nullable(), +}).strict() + +/** + * The admitted folder and source commit of one registry Skill, or nulls. + * + * Artifact delivery reads this over HTTP, so `skilld run OWNER/REPO/NAME` + * resolves the identity the registry admitted and the Skill page shows. Not + * cached: a stale answer would deliver a commit the page no longer names, and + * the lookup is one indexed read per Resolution request. + */ +export default defineApiHandler({ + schema: RunIdentityQuery, + response: RunIdentityResponse, + handler: async ({ event, body, platform }) => { + setHeader(event, 'cache-control', 'private, no-store') + return await findSkillRunIdentity(platform.db, { owner: body.owner, repository: body.repo, name: body.name }) + }, + presenter: presentSkillRunIdentity, +}) diff --git a/layers/registry/server/presenters/skill-run-identity.ts b/layers/registry/server/presenters/skill-run-identity.ts new file mode 100644 index 00000000..038afc03 --- /dev/null +++ b/layers/registry/server/presenters/skill-run-identity.ts @@ -0,0 +1,13 @@ +import type { SkillRunIdentity } from '../utils/skill-run-identity' + +export interface SkillRunIdentityDto { + skillPath: string | null + commitSha: string | null +} + +export function presentSkillRunIdentity(identity: SkillRunIdentity | null): SkillRunIdentityDto { + return { + skillPath: identity?.skillPath ?? null, + commitSha: identity?.commitSha ?? null, + } +} diff --git a/layers/registry/server/utils/skill-run-identity.ts b/layers/registry/server/utils/skill-run-identity.ts new file mode 100644 index 00000000..fe8313c8 --- /dev/null +++ b/layers/registry/server/utils/skill-run-identity.ts @@ -0,0 +1,62 @@ +export interface SkillRunIdentityQuery { + owner: string + repository: string + /** The registry Skill name: the folder name, or the Repository name at the root. */ + name: string +} + +export interface SkillRunIdentity { + /** The admitted Skill folder inside the Repository, or `.` for the root. */ + skillPath: string + /** The commit the Skill page names as its source, or null before the first revision. */ + commitSha: string | null +} + +interface IdentityRow { + rendered_skill_path: string + commit_sha: string | null +} + +/** + * The admitted identity that `skilld run OWNER/REPOSITORY/NAME` resolves. + * + * The registry admits one folder per name and renders the SKILL.md the Skill + * page shows. Delivery used to search the Repository tree for the name + * instead, so it could disagree with the registry: copies in Agent folders + * failed as ambiguous, and Repositories over 2,000 entries failed outright. + * The commit is the source commit the Skill page names, so a run delivers the + * SKILL.md bytes the page shows. + * + * Owner and Repository match without case, like `findSkillPagePath`, because + * the registry keeps the case it first admitted. A row whose case matches + * exactly wins. A row with no rendered path, or whose source is gone, has no + * identity to run. + */ +export async function findSkillRunIdentity( + db: D1Database, + query: SkillRunIdentityQuery, +): Promise { + const row = await db + .prepare(` + SELECT s.rendered_skill_path, + (SELECT sr.sha FROM skill_revisions sr + WHERE sr.owner = s.owner AND sr.repo = s.repo AND sr.name = s.name + ORDER BY sr.modified_at DESC LIMIT 1) AS commit_sha + FROM skills s + WHERE s.owner = ?1 COLLATE NOCASE AND s.repo = ?2 COLLATE NOCASE AND s.name = ?3 + AND s.source_resolved = 1 AND s.rendered_skill_path IS NOT NULL + ORDER BY (s.owner = ?1 AND s.repo = ?2) DESC + LIMIT 1`) + .bind(query.owner, query.repository, query.name) + .first() + if (!row) + return null + return { + skillPath: skillFolder(row.rendered_skill_path), + commitSha: row.commit_sha, + } +} + +function skillFolder(renderedPath: string): string { + return renderedPath === 'SKILL.md' ? '.' : renderedPath.replace(/\/SKILL\.md$/, '') +} diff --git a/test/unit/skill-run-identity.test.ts b/test/unit/skill-run-identity.test.ts new file mode 100644 index 00000000..d1b18dd9 --- /dev/null +++ b/test/unit/skill-run-identity.test.ts @@ -0,0 +1,118 @@ +// @vitest-environment node +import type { SourceRequest } from '../../layers/artifact-delivery/server/schemas/contracts' +import type { SqliteD1 } from './helpers/d1-sqlite' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { admittedSourceRequest } from '../../layers/artifact-delivery/server/utils/admitted-identity' +import { findSkillRunIdentity } from '../../layers/registry/server/utils/skill-run-identity' +import { createSqliteD1 } from './helpers/d1-sqlite' + +const OLDER = '1111111111111111111111111111111111111111' +const NEWER = '2222222222222222222222222222222222222222' + +let harness: SqliteD1 | null = null +function db() { + if (!harness) { + harness = createSqliteD1([]) + harness.raw.exec(`CREATE TABLE skills ( + owner TEXT, repo TEXT, name TEXT, + rendered_skill_path TEXT, source_resolved INTEGER DEFAULT 1 + ); + CREATE TABLE skill_revisions ( + owner TEXT, repo TEXT, name TEXT, sha TEXT, modified_at INTEGER + )`) + } + return harness +} +function addSkill(owner: string, repo: string, name: string, path: string | null, resolved = 1) { + db().raw.prepare('INSERT INTO skills VALUES (?, ?, ?, ?, ?)').run(owner, repo, name, path, resolved) +} +function addRevision(owner: string, repo: string, name: string, sha: string, modifiedAt: number) { + db().raw.prepare('INSERT INTO skill_revisions VALUES (?, ?, ?, ?, ?)').run(owner, repo, name, sha, modifiedAt) +} +afterEach(() => { + harness?.close() + harness = null +}) + +describe('findSkillRunIdentity', () => { + it('answers the admitted folder and the commit the Skill page names', async () => { + // pbakaus/impeccable: 20 copies, the registry admitted one. + addSkill('pbakaus', 'impeccable', 'impeccable', 'cursor-plugin/skills/impeccable/SKILL.md') + addRevision('pbakaus', 'impeccable', 'impeccable', OLDER, 100) + addRevision('pbakaus', 'impeccable', 'impeccable', NEWER, 200) + + expect(await findSkillRunIdentity(db().db, { owner: 'pbakaus', repository: 'impeccable', name: 'impeccable' })) + .toEqual({ skillPath: 'cursor-plugin/skills/impeccable', commitSha: NEWER }) + }) + + it('answers the root folder for a root Skill', async () => { + addSkill('zarazhangrui', 'frontend-slides', 'frontend-slides', 'SKILL.md') + + expect(await findSkillRunIdentity(db().db, { owner: 'zarazhangrui', repository: 'frontend-slides', name: 'frontend-slides' })) + .toEqual({ skillPath: '.', commitSha: null }) + }) + + it('matches the owner and repository whatever their case', async () => { + addSkill('leonxlnx', 'taste-skill', 'taste-skill', 'skills/taste-skill/SKILL.md') + + expect(await findSkillRunIdentity(db().db, { owner: 'Leonxlnx', repository: 'Taste-Skill', name: 'taste-skill' })) + .toEqual({ skillPath: 'skills/taste-skill', commitSha: null }) + }) + + it('answers null for a Skill the registry does not hold, has not rendered, or lost', async () => { + addSkill('acme', 'pair', 'unrendered', null) + addSkill('acme', 'pair', 'gone', 'gone/SKILL.md', 0) + + expect(await findSkillRunIdentity(db().db, { owner: 'acme', repository: 'pair', name: 'other' })).toBeNull() + expect(await findSkillRunIdentity(db().db, { owner: 'acme', repository: 'pair', name: 'unrendered' })).toBeNull() + expect(await findSkillRunIdentity(db().db, { owner: 'acme', repository: 'pair', name: 'gone' })).toBeNull() + }) +}) + +describe('admittedSourceRequest', () => { + const named: SourceRequest = { + provider: 'github', + owner: 'leonxlnx', + repository: 'taste-skill', + selector: { type: 'named-skill', name: 'taste-skill' }, + } + + it('resolves an admitted name by its folder at the commit the Skill page names', async () => { + const lookup = vi.fn(async () => ({ skillPath: 'skills/taste-skill', commitSha: NEWER })) + + expect(await admittedSourceRequest(named, lookup)).toEqual({ + provider: 'github', + owner: 'leonxlnx', + repository: 'taste-skill', + selector: { type: 'path', path: 'skills/taste-skill' }, + ref: { type: 'commit', value: NEWER }, + }) + expect(lookup).toHaveBeenCalledWith({ owner: 'leonxlnx', repository: 'taste-skill', name: 'taste-skill' }) + }) + + it('keeps a reference the request names', async () => { + const lookup = vi.fn(async () => ({ skillPath: 'skills/taste-skill', commitSha: NEWER })) + const request: SourceRequest = { ...named, ref: { type: 'branch', value: 'next' } } + + expect(await admittedSourceRequest(request, lookup)).toMatchObject({ + selector: { type: 'path', path: 'skills/taste-skill' }, + ref: { type: 'branch', value: 'next' }, + }) + }) + + it('resolves the default branch when the registry knows no commit', async () => { + const resolved = await admittedSourceRequest(named, async () => ({ skillPath: '.', commitSha: null })) + + expect(resolved).toEqual({ ...named, selector: { type: 'path', path: '.' } }) + expect(resolved).not.toHaveProperty('ref') + }) + + it('leaves a name the registry does not hold, and a path, as requested', async () => { + const lookup = vi.fn(async () => null) + const path: SourceRequest = { ...named, selector: { type: 'path', path: 'skills/taste-skill' } } + + expect(await admittedSourceRequest(named, lookup)).toBe(named) + expect(await admittedSourceRequest(path, lookup)).toBe(path) + expect(lookup).toHaveBeenCalledTimes(1) + }) +}) From 72753e0d9bafd66d7f32eb1d24aac7076fc4ff98 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 01:16:58 +1100 Subject: [PATCH 4/4] feat(artifact-delivery): sweep indexed Skills through skilld run Nothing noticed when a listed Skill stopped running: each failure surfaced only when a person copied its run command. The sweep-skill-runs task sends the Resolution request the CLI sends for 25 indexed Skills every 15 minutes, records how each settles in artifact_run_checks, and fails its run when a Skill starts failing for a reason a retry cannot change. The cadence watchdog alerts on that failed run. --- docs/arch/cron.md | 3 +- .../server/tasks/sweep-skill-runs.ts | 108 ++++++++ .../server/utils/run-sweep.ts | 260 ++++++++++++++++++ .../server/api/skills/runnable.get.ts | 18 ++ .../server/presenters/runnable-skills.ts | 7 + migrations/0142_artifact_run_checks.sql | 30 ++ scripts/lib/scheduled-task-source.ts | 1 + shared/schedule-policy.ts | 3 + test/unit/skill-run-sweep.test.ts | 146 ++++++++++ 9 files changed, 575 insertions(+), 1 deletion(-) create mode 100644 layers/artifact-delivery/server/tasks/sweep-skill-runs.ts create mode 100644 layers/artifact-delivery/server/utils/run-sweep.ts create mode 100644 layers/registry/server/api/skills/runnable.get.ts create mode 100644 layers/registry/server/presenters/runnable-skills.ts create mode 100644 migrations/0142_artifact_run_checks.sql create mode 100644 test/unit/skill-run-sweep.test.ts diff --git a/docs/arch/cron.md b/docs/arch/cron.md index 848f2eab..d89f3e13 100644 --- a/docs/arch/cron.md +++ b/docs/arch/cron.md @@ -2,7 +2,7 @@ Generated from literal `defineScheduledTask` declarations. Run `pnpm cron:docs` after schedule changes. -Tasks: 22. Unique Cloudflare triggers: 14. +Tasks: 23. Unique Cloudflare triggers: 15. | Task | Cron | Source | | --- | --- | --- | @@ -23,6 +23,7 @@ Tasks: 22. Unique Cloudflare triggers: 14. | `scheduled-cadence-watchdog` | `*/5 * * * *` | `server/tasks/scheduled-cadence-watchdog.ts` | | `send-digests` | `0 9 1 * *` | `layers/identity/server/tasks/send-digests.ts` | | `send-weekly` | `0 9 * * MON` | `layers/identity/server/tasks/send-weekly.ts` | +| `sweep-skill-runs` | `7,22,37,52 * * * *` | `layers/artifact-delivery/server/tasks/sweep-skill-runs.ts` | | `sync-bsky-mentions` | `17 */2 * * *` | `layers/registry/server/tasks/sync-bsky-mentions.ts` | | `sync-github-skills` | `0 * * * *` | `layers/registry/server/tasks/sync-github-skills.ts` | | `sync-reviewed-skill-repos` | `*/5 * * * *` | `layers/registry/server/tasks/sync-reviewed-skill-repos.ts` | diff --git a/layers/artifact-delivery/server/tasks/sweep-skill-runs.ts b/layers/artifact-delivery/server/tasks/sweep-skill-runs.ts new file mode 100644 index 00000000..87876138 --- /dev/null +++ b/layers/artifact-delivery/server/tasks/sweep-skill-runs.ts @@ -0,0 +1,108 @@ +/// + +import type { SkillRunFailure } from '../utils/run-sweep' +import { resolveCloudflareBindings } from '@harlan-zw/nuxt-cloudflare/bindings' +import { runObservedScheduledTask } from '~~/server/utils/scheduled-run' +import { reportJobRun } from '~~/server/utils/sync-job-reporter' +import { observedSchedulePolicy } from '#shared/schedule-policy' +import { enqueueArtifactBuild } from '../utils/queue' +import { fetchAdmittedSkillIdentity, requestResolution } from '../utils/request-resolution' +import { runSkillRunSweep } from '../utils/run-sweep' + +const CRON = '7,22,37,52 * * * *' +/** + * 25 checks every 15 minutes is 2,400 a day, so about 2,100 indexed Skills + * cycle in under a day. A ready build of the same commit is reused without a + * GitHub read, so a repeat check of an unchanged Skill costs D1 rows only. + */ +const BATCH_SIZE = 25 + +/** + * Scheduled task: check that every indexed Skill still runs. + * + * Each check is the Resolution request `skilld run OWNER/REPOSITORY/NAME` + * sends, through the same code and build queue. The next run reads how it + * ended. A failure a retry cannot change, on a Skill that did not fail that + * way before, fails the run: the cadence watchdog alerts on a failed run. + * + * The minutes avoid the top of the hour, when the hourly sync spends most of + * the GitHub quota. + */ +export default defineScheduledTask({ + name: 'sweep-skill-runs', + cron: '7,22,37,52 * * * *', + description: 'Check that each indexed Skill resolves through skilld run', + async run({ context }) { + const env = resolveCloudflareBindings(context) + const db = env?.DB as D1Database | undefined + if (!env || !db) { + emitOperationalEvent(createWideEvent({ operation: 'sweep-skill-runs', outcome: 'binding-missing' })) + return { result: { error: 'no-db' } } + } + + return await runObservedScheduledTask({ + db, + env, + context, + policy: observedSchedulePolicy('sweep-skill-runs'), + }, async () => { + const startedAt = Date.now() + const now = () => Math.floor(Date.now() / 1000) + const report = await runSkillRunSweep({ + db, + listRunnableSkills: async () => (await $fetch<{ items: Array<{ owner: string, repository: string, name: string }> }>('/api/skills/runnable')).items, + requestResolution: input => requestResolution({ + db, + lookupAdmitted: fetchAdmittedSkillIdentity(), + enqueue: resolutionId => enqueueArtifactBuild(env, resolutionId), + now, + }, input), + newIdempotencyKey: () => `sweep-${crypto.randomUUID()}`, + now, + batchSize: BATCH_SIZE, + }) + + for (const failure of report.newFailures) { + emitOperationalEvent(createWideEvent({ + operation: 'sweep-skill-runs', + outcome: 'new-failure', + repo: skillRef(failure), + reason: failure.tag, + }), 'error') + } + emitOperationalEvent(createWideEvent({ + 'operation': 'sweep-skill-runs', + 'outcome': 'completed', + 'item.count': report.started, + 'processed.count': report.settled, + 'failed.count': report.failing, + 'error.count': report.transientFailures.length, + }), 'info') + + const alarm = report.newFailures.length > 0 + ? `skilld run fails for ${report.newFailures.length} more Skill(s): ${report.newFailures.slice(0, 10).map(failure => `${skillRef(failure)} (${failure.tag})`).join(', ')}` + : null + await reportJobRun(db, 'sweep-skill-runs', { + cron: CRON, + status: alarm ? 'error' : 'ok', + durationMs: Date.now() - startedAt, + error: alarm, + }) + if (alarm) + throw new Error(alarm) + return { + result: { + started: report.started, + settled: report.settled, + failing: report.failing, + transient: report.transientFailures.length, + recovered: report.recovered.length, + }, + } + }) + }, +}) + +function skillRef(failure: SkillRunFailure): string { + return `${failure.owner}/${failure.repository}/${failure.name}` +} diff --git a/layers/artifact-delivery/server/utils/run-sweep.ts b/layers/artifact-delivery/server/utils/run-sweep.ts new file mode 100644 index 00000000..79a7815d --- /dev/null +++ b/layers/artifact-delivery/server/utils/run-sweep.ts @@ -0,0 +1,260 @@ +import type { SourceRequest } from '../schemas/contracts' +import type { ResolutionAccess } from './request-resolution' +import type { CreateResolutionResult, ResolutionRow } from './state' +import { z } from 'zod' +import { ACTIVE_BUILD_STATES, getResolution, parseCheckResults } from './state' + +/** One Skill whose page shows `skilld run OWNER/REPOSITORY/NAME`. */ +export interface RunnableSkill { + owner: string + repository: string + name: string +} + +export interface SkillRunFailure extends RunnableSkill { + /** What the CLI prints first: a problem code, or `CHECK_BLOCKED:`. */ + tag: string + detail: string | null +} + +export interface SkillRunSweepDependencies { + db: D1Database + /** The Skills the registry shows a run command for. */ + listRunnableSkills: () => Promise + /** The request `POST /api/v1/resolutions` makes, with no HTTP around it. */ + requestResolution: (input: { + source: SourceRequest + idempotencyKey: string + access: ResolutionAccess + }) => Promise + newIdempotencyKey: () => string + now: () => number + /** How many new checks one run starts. */ + batchSize: number +} + +export interface SkillRunSweepReport { + started: number + settled: number + /** Skills that failed for a reason a retry cannot change, and did not before. */ + newFailures: SkillRunFailure[] + /** Failures a later run can clear, such as a spent GitHub quota. Recorded, never alarmed. */ + transientFailures: SkillRunFailure[] + recovered: RunnableSkill[] + /** Runnable Skills whose last check failed for a reason a retry cannot change. */ + failing: number +} + +/** + * A check that stays pending this long counts as failed. The CLI gives up after + * 60 seconds, so a person would have seen a timeout long before. + */ +const PENDING_TIMEOUT_SECONDS = 30 * 60 + +const checkRowSchema = z.object({ + owner: z.string(), + repository: z.string(), + name: z.string(), + outcome: z.enum(['ready', 'failing']).nullable(), + tag: z.string().nullable(), + retryable: z.union([z.literal(0), z.literal(1)]), + failing_since: z.number().int().nullable(), + settled_at: z.number().int().nullable(), + pending_resolution_id: z.string().nullable(), + requested_at: z.number().int().nullable(), +}) + +type CheckRow = z.infer + +type Settled + = | { _tag: 'pending' } + | { _tag: 'ready' } + | { _tag: 'failing', tag: string, detail: string | null, retryable: boolean } + +/** + * One run of the skilld run sweep. + * + * It settles the checks the last run started, then starts new ones for the + * Skills checked longest ago. Each check is the Resolution request the CLI + * sends for `skilld run OWNER/REPOSITORY/NAME`: a named Skill, no reference, + * and the same build queue. A build takes longer than one Worker invocation + * allows to wait, so a check settles on the next run. + * + * A new failure is one a retry cannot change, on a Skill whose last check did + * not fail with the same tag. The caller alarms on those. + */ +export async function runSkillRunSweep(dependencies: SkillRunSweepDependencies): Promise { + const report: SkillRunSweepReport = { + started: 0, + settled: 0, + newFailures: [], + transientFailures: [], + recovered: [], + failing: 0, + } + const rows = await loadCheckRows(dependencies.db) + const now = dependencies.now() + + for (const row of rows.values()) { + if (!row.pending_resolution_id) + continue + const resolution = await getResolution(dependencies.db, row.pending_resolution_id) + const settled = settleResolution(resolution, now - (row.requested_at ?? now)) + if (settled._tag === 'pending') + continue + report.settled++ + const skill = { owner: row.owner, repository: row.repository, name: row.name } + if (settled._tag === 'ready') { + if (row.outcome === 'failing' && row.retryable === 0) + report.recovered.push(skill) + await writeSettled(dependencies.db, row, { outcome: 'ready', tag: null, detail: null, retryable: false, failingSince: null }, now) + rows.set(skillKey(skill), { ...row, outcome: 'ready', tag: null, retryable: 0, pending_resolution_id: null, settled_at: now }) + continue + } + const failure = { ...skill, tag: settled.tag, detail: settled.detail } + const sameFailure = row.outcome === 'failing' && row.tag === settled.tag + if (settled.retryable) + report.transientFailures.push(failure) + else if (!sameFailure) + report.newFailures.push(failure) + const failingSince = sameFailure ? row.failing_since ?? now : now + await writeSettled(dependencies.db, row, { + outcome: 'failing', + tag: settled.tag, + detail: settled.detail, + retryable: settled.retryable, + failingSince, + }, now) + rows.set(skillKey(skill), { + ...row, + outcome: 'failing', + tag: settled.tag, + retryable: settled.retryable ? 1 : 0, + failing_since: failingSince, + pending_resolution_id: null, + settled_at: now, + }) + } + + const runnable = await dependencies.listRunnableSkills() + report.failing = runnable.filter((skill) => { + const row = rows.get(skillKey(skill)) + return row?.outcome === 'failing' && row.retryable === 0 + }).length + + const due = runnable + .filter(skill => !rows.get(skillKey(skill))?.pending_resolution_id) + .sort((left, right) => + (rows.get(skillKey(left))?.settled_at ?? 0) - (rows.get(skillKey(right))?.settled_at ?? 0) + || compareText(skillKey(left), skillKey(right))) + .slice(0, Math.max(0, dependencies.batchSize)) + for (const skill of due) { + const result = await dependencies.requestResolution({ + source: { + provider: 'github', + owner: skill.owner, + repository: skill.repository, + selector: { type: 'named-skill', name: skill.name }, + }, + idempotencyKey: dependencies.newIdempotencyKey(), + access: { visibility: 'public' }, + }) + if (result._tag === 'idempotency-conflict') + throw new Error('A fresh sweep Idempotency-Key named another request') + await writePending(dependencies.db, skill, result.row.id, now) + report.started++ + } + return report +} + +/** + * The outcome `skilld run` would print for one Resolution. + * + * A blocked Resolution names its first failing required check, like the CLI. + */ +export function settleResolution(resolution: ResolutionRow | null, pendingSeconds: number): Settled { + if (!resolution) + return { _tag: 'failing', tag: 'RESOLUTION_MISSING', detail: null, retryable: true } + if ((ACTIVE_BUILD_STATES as readonly string[]).includes(resolution.state)) { + return pendingSeconds >= PENDING_TIMEOUT_SECONDS + ? { _tag: 'failing', tag: 'RESOLUTION_TIMEOUT', detail: `Pending at ${resolution.state}.`, retryable: true } + : { _tag: 'pending' } + } + if (resolution.state === 'ready') + return { _tag: 'ready' } + if (resolution.state === 'blocked') { + const failed = parseCheckResults(resolution.check_results_json) + .find(check => check.required && (check.outcome === 'fail' || check.outcome === 'error')) + return { + _tag: 'failing', + tag: `CHECK_BLOCKED:${failed?.name ?? 'unknown'}`, + detail: [failed?.summary, failed?.findings?.[0]].filter(Boolean).join(' ').slice(0, 500) || null, + retryable: false, + } + } + if (resolution.state === 'revoked') + return { _tag: 'failing', tag: 'ARTIFACT_REVOKED', detail: null, retryable: false } + return { + _tag: 'failing', + tag: resolution.error_code ?? 'SERVICE_UNAVAILABLE', + detail: null, + retryable: resolution.error_retryable === 1, + } +} + +async function loadCheckRows(db: D1Database): Promise> { + const result = await db.prepare( + `SELECT owner, repository, name, outcome, tag, retryable, failing_since, + settled_at, pending_resolution_id, requested_at + FROM artifact_run_checks`, + ).all>() + const rows = new Map() + for (const value of result.results ?? []) { + const row = checkRowSchema.parse(value) + rows.set(skillKey(row), row) + } + return rows +} + +async function writeSettled( + db: D1Database, + row: CheckRow, + settled: { outcome: 'ready' | 'failing', tag: string | null, detail: string | null, retryable: boolean, failingSince: number | null }, + now: number, +): Promise { + await db.prepare( + `UPDATE artifact_run_checks + SET outcome = ?4, tag = ?5, detail = ?6, retryable = ?7, failing_since = ?8, + settled_at = ?9, settled_resolution_id = pending_resolution_id, + pending_resolution_id = NULL + WHERE owner = ?1 AND repository = ?2 AND name = ?3`, + ).bind( + row.owner, + row.repository, + row.name, + settled.outcome, + settled.tag, + settled.detail, + settled.retryable ? 1 : 0, + settled.failingSince, + now, + ).run() +} + +async function writePending(db: D1Database, skill: RunnableSkill, resolutionId: string, now: number): Promise { + await db.prepare( + `INSERT INTO artifact_run_checks (owner, repository, name, pending_resolution_id, requested_at) + VALUES (?1, ?2, ?3, ?4, ?5) + ON CONFLICT (owner, repository, name) DO UPDATE SET + pending_resolution_id = excluded.pending_resolution_id, + requested_at = excluded.requested_at`, + ).bind(skill.owner, skill.repository, skill.name, resolutionId, now).run() +} + +function skillKey(skill: RunnableSkill): string { + return `${skill.owner}/${skill.repository}/${skill.name}` +} + +function compareText(left: string, right: string): number { + return left < right ? -1 : left > right ? 1 : 0 +} diff --git a/layers/registry/server/api/skills/runnable.get.ts b/layers/registry/server/api/skills/runnable.get.ts new file mode 100644 index 00000000..6ca20cec --- /dev/null +++ b/layers/registry/server/api/skills/runnable.get.ts @@ -0,0 +1,18 @@ +import { setHeader } from 'h3' +import { defineApiHandler } from '#shared/server/handler' +import { presentRunnableSkills } from '../../presenters/runnable-skills' +import { listAllSkillsForSitemap } from '../../utils/skills-registry' + +/** + * Every Skill the registry indexes, which is every Skill whose page shows a + * run command to search. The run sweep in artifact delivery reads this over + * HTTP (ADR-0001) and checks each one through the Resolution request the CLI + * sends. + */ +export default defineApiHandler({ + handler: async ({ event }) => { + setHeader(event, 'cache-control', 'private, no-store') + return await listAllSkillsForSitemap(event) + }, + presenter: presentRunnableSkills, +}) diff --git a/layers/registry/server/presenters/runnable-skills.ts b/layers/registry/server/presenters/runnable-skills.ts new file mode 100644 index 00000000..f2477f35 --- /dev/null +++ b/layers/registry/server/presenters/runnable-skills.ts @@ -0,0 +1,7 @@ +export interface RunnableSkillsDto { + items: Array<{ owner: string, repository: string, name: string }> +} + +export function presentRunnableSkills(rows: Array<{ owner: string, repo: string, name: string }>): RunnableSkillsDto { + return { items: rows.map(row => ({ owner: row.owner, repository: row.repo, name: row.name })) } +} diff --git a/migrations/0142_artifact_run_checks.sql b/migrations/0142_artifact_run_checks.sql new file mode 100644 index 00000000..11e3e797 --- /dev/null +++ b/migrations/0142_artifact_run_checks.sql @@ -0,0 +1,30 @@ +-- The skilld run sweep. One row is the latest `skilld run OWNER/REPO/NAME` +-- check of one Skill the registry indexes, made through the same Resolution +-- request the CLI sends. +-- +-- The `sweep-skill-runs` task writes this table and nothing else. A row keeps +-- the last settled outcome while the next check is pending, so a change from +-- ready to failing is visible when it settles. +-- Cull path: drop the task, then `DROP TABLE artifact_run_checks`. +CREATE TABLE IF NOT EXISTS artifact_run_checks ( + owner TEXT NOT NULL, + repository TEXT NOT NULL, + name TEXT NOT NULL, + -- 'ready' or 'failing'. NULL until the first check settles. + outcome TEXT CHECK (outcome IN ('ready', 'failing')), + -- The error the CLI would print: a problem code such as `INVALID_SOURCE`, + -- or `CHECK_BLOCKED:`. NULL when ready. + tag TEXT, + -- The check summary or first finding, when one exists. + detail TEXT, + -- 1 when the same request can work later, such as `RATE_LIMITED`. + retryable INTEGER NOT NULL DEFAULT 0 CHECK (retryable IN (0, 1)), + -- Unix seconds the current failure tag was first seen. + failing_since INTEGER, + settled_at INTEGER, + settled_resolution_id TEXT, + -- The Resolution the next settle reads. NULL when no check is in flight. + pending_resolution_id TEXT, + requested_at INTEGER, + PRIMARY KEY (owner, repository, name) +) WITHOUT ROWID; diff --git a/scripts/lib/scheduled-task-source.ts b/scripts/lib/scheduled-task-source.ts index 5f2661cb..14f1be0b 100644 --- a/scripts/lib/scheduled-task-source.ts +++ b/scripts/lib/scheduled-task-source.ts @@ -82,6 +82,7 @@ function filesBelow(root: string): string[] { function taskFiles(projectRoot: string): string[] { const roots = [ + resolve(projectRoot, 'layers/artifact-delivery/server/tasks'), resolve(projectRoot, 'layers/identity/server/tasks'), resolve(projectRoot, 'layers/registry/server/tasks'), resolve(projectRoot, 'server/tasks'), diff --git a/shared/schedule-policy.ts b/shared/schedule-policy.ts index ca8fc79d..3cd274a3 100644 --- a/shared/schedule-policy.ts +++ b/shared/schedule-policy.ts @@ -76,6 +76,9 @@ export const SCHEDULE_POLICY = [ { _tag: 'observed', taskName: 'scheduled-cadence-watchdog', cron: '*/5 * * * *', maxSilenceSeconds: 20 * 60, maxRuntimeSeconds: 14 * 60 }, { _tag: 'observed', taskName: 'send-digests', cron: '0 9 1 * *', maxSilenceSeconds: 35 * 24 * 60 * 60, maxRuntimeSeconds: 50 * 60 }, { _tag: 'observed', taskName: 'send-weekly', cron: '0 9 * * MON', maxSilenceSeconds: 8 * 24 * 60 * 60, maxRuntimeSeconds: 50 * 60, activeFromSeconds: 1787097600 }, + // A failed run means a Skill stopped running through `skilld run`. The task + // fails on purpose so this alert fires once per new failure. + { _tag: 'observed', taskName: 'sweep-skill-runs', cron: '7,22,37,52 * * * *', maxSilenceSeconds: 60 * 60, maxRuntimeSeconds: 10 * 60 }, // Two-hourly, so six hours of silence is a real outage rather than a quiet // stretch. Runtime is generous because throttle backoff can stretch a run. { _tag: 'observed', taskName: 'sync-bsky-mentions', cron: '17 */2 * * *', maxSilenceSeconds: 6 * 60 * 60, maxRuntimeSeconds: 10 * 60 }, diff --git a/test/unit/skill-run-sweep.test.ts b/test/unit/skill-run-sweep.test.ts new file mode 100644 index 00000000..bd99c8a8 --- /dev/null +++ b/test/unit/skill-run-sweep.test.ts @@ -0,0 +1,146 @@ +// @vitest-environment node +import type { ArtifactBuildDependencies } from '../../layers/artifact-delivery/server/utils/build' +import type { PublicGithubSourceClient, SourceRejection } from '../../layers/artifact-delivery/server/utils/github-source' +import type { RunnableSkill, SkillRunSweepDependencies } from '../../layers/artifact-delivery/server/utils/run-sweep' +import { describe, expect, it, vi } from 'vitest' +import { processArtifactBuild } from '../../layers/artifact-delivery/server/utils/build' +import { requestResolution } from '../../layers/artifact-delivery/server/utils/request-resolution' +import { runSkillRunSweep } from '../../layers/artifact-delivery/server/utils/run-sweep' +import { createSqliteD1 } from './helpers/d1-sqlite' + +const MIGRATIONS = [ + 'migrations/0017_users.sql', + 'migrations/0110_artifact_delivery.sql', + 'migrations/0111_github_app_delivery.sql', + 'migrations/0112_private_artifact_keys.sql', + 'migrations/0122_artifact_resolution_retry_after.sql', + 'migrations/0142_artifact_run_checks.sql', +] +const NOW = 1_791_000_000 +const archify: RunnableSkill = { owner: 'tt-a1i', repository: 'archify', name: 'archify' } +const busy: RunnableSkill = { owner: 'acme', repository: 'busy', name: 'busy' } + +describe('skilld run sweep', () => { + it('reports a Skill that `skilld run` cannot deliver, with the tag the CLI prints', async () => { + const harness = createHarness([archify, busy]) + + const first = await runSkillRunSweep(harness.dependencies) + expect(first).toMatchObject({ started: 2, settled: 0, newFailures: [] }) + expect(harness.enqueue).toHaveBeenCalledTimes(2) + await harness.buildEach({ + archify: { _tag: 'rejected', code: 'INVALID_SOURCE', summary: 'The Skill folder `archify` packs to 10.31 MiB. The limit is 10 MiB.', findings: [] }, + busy: { _tag: 'rejected', code: 'RATE_LIMITED', summary: 'GitHub refused the read: its rate limit is spent.', findings: [] }, + }) + + const second = await runSkillRunSweep(harness.dependencies) + + expect(second.settled).toBe(2) + expect(second.newFailures).toEqual([{ + ...archify, + tag: 'CHECK_BLOCKED:source-policy', + detail: 'The Skill folder `archify` packs to 10.31 MiB. The limit is 10 MiB.', + }]) + // A spent quota is a fact about the hour, not the Skill: recorded, never alarmed. + expect(second.transientFailures).toEqual([{ ...busy, tag: 'RATE_LIMITED', detail: null }]) + harness.close() + }) + + it('alarms once per failure, not on every check that sees it again', async () => { + const harness = createHarness([archify]) + const blocked: SourceRejection = { _tag: 'rejected', code: 'INVALID_SOURCE', summary: 'Too large.', findings: [] } + + await runSkillRunSweep(harness.dependencies) + await harness.buildEach({ archify: blocked }) + expect((await runSkillRunSweep(harness.dependencies)).newFailures).toHaveLength(1) + await harness.buildEach({ archify: blocked }) + const third = await runSkillRunSweep(harness.dependencies) + + expect(third.settled).toBe(1) + expect(third.newFailures).toEqual([]) + expect(third.failing).toBe(1) + harness.close() + }) + + it('checks the Skill checked longest ago first', async () => { + const harness = createHarness([archify, busy], 1) + + const first = await runSkillRunSweep(harness.dependencies) + await harness.buildEach({ busy: { _tag: 'rejected', code: 'SOURCE_NOT_FOUND', summary: 'Gone.', findings: [] }, archify: { _tag: 'rejected', code: 'SOURCE_NOT_FOUND', summary: 'Gone.', findings: [] } }) + const second = await runSkillRunSweep(harness.dependencies) + + expect(first.started).toBe(1) + expect(second.started).toBe(1) + expect(harness.requested()).toEqual(['acme/busy/busy', 'tt-a1i/archify/archify']) + harness.close() + }) +}) + +function createHarness(skills: RunnableSkill[], batchSize = 10) { + const sqlite = createSqliteD1(MIGRATIONS) + const enqueue = vi.fn(async (_resolutionId: string) => {}) + const requestedRefs: string[] = [] + let clock = NOW + let keys = 0 + const dependencies: SkillRunSweepDependencies = { + db: sqlite.db, + listRunnableSkills: async () => skills, + requestResolution: async (input) => { + requestedRefs.push(`${input.source.owner}/${input.source.repository}/${input.source.selector.type === 'named-skill' ? input.source.selector.name : input.source.selector.path}`) + return await requestResolution({ + db: sqlite.db, + lookupAdmitted: async () => null, + enqueue, + now: () => clock, + }, input) + }, + newIdempotencyKey: () => `sweep-test-key-${String(++keys).padStart(4, '0')}`, + now: () => clock, + batchSize, + } + + /** Run the queued builds, each rejected by GitHub as the test names. */ + async function buildEach(rejections: Partial>) { + const pending = sqlite.raw.prepare( + `SELECT id, selector_value FROM artifact_resolutions WHERE state = 'requested'`, + ).all() as Array<{ id: string, selector_value: string }> + for (const row of pending) { + const rejection = rejections[row.selector_value] + if (!rejection) + continue + const resolvesThenRejects = rejection.code === 'INVALID_SOURCE' + const github: PublicGithubSourceClient = { + resolve: vi.fn(async () => resolvesThenRejects + ? { + _tag: 'resolved' as const, + source: { + provider: 'github' as const, + repositoryId: 1, + owner: 'tt-a1i', + repository: 'archify', + visibility: 'public' as const, + commitSha: '0'.repeat(40), + treeSha: '1'.repeat(40), + skillPath: 'archify', + }, + } + : rejection), + load: vi.fn(async () => rejection), + } + await processArtifactBuild({ + db: sqlite.db, + github, + now: () => clock, + reportReuse: () => {}, + } as unknown as ArtifactBuildDependencies, row.id) + } + clock += 15 * 60 + } + + return { + dependencies, + enqueue, + buildEach, + requested: () => requestedRefs, + close: () => sqlite.close(), + } +}