From 466f65f55973ff314a66ada427d61e328b196dfe Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 04:19:26 +1100 Subject: [PATCH 1/3] feat(artifact-delivery): follow symbolic links in a Skill folder 108 Skills failed every run with INVALID_SOURCE because their folder held a symbolic link. 103 are simota/agent-skills, whose Skills link the shared _common and _templates folders at the Repository root. A build now resolves each link at the same commit by POSIX rules and packs the files it names at the link path, each checked against its own Git blob SHA. Links that leave the Repository, dangle, loop, chain past 8, or point into .git still refuse the Skill. A link inside a followed folder, and a link to a folder inside the Skill that holds it, are left out: tar --dereference recursed 5,800 folders deep on simota/agent-skills. The non-required symbolic-links check lists every link. Policy 2026-10-07.4 keeps 2026-10-07.3 signable, since that policy refused every link. --- .../server/utils/archive-reader.ts | 196 ++++++--- .../server/utils/artifact-pack.ts | 5 +- .../artifact-delivery/server/utils/build.ts | 12 + .../artifact-delivery/server/utils/checks.ts | 59 ++- .../server/utils/github-source.ts | 96 ++++- .../artifact-delivery/server/utils/state.ts | 6 +- .../server/utils/symbolic-links.ts | 324 +++++++++++++++ test/fixtures/loaded-source.ts | 2 + test/unit/artifact-archive-reader.test.ts | 2 + test/unit/artifact-github-source.test.ts | 4 - test/unit/artifact-pack.test.ts | 5 +- test/unit/artifact-signer.test.ts | 1 + test/unit/artifact-source-checks.test.ts | 2 +- test/unit/artifact-symbolic-links.test.ts | 372 ++++++++++++++++++ test/unit/private-artifact-delivery.test.ts | 1 + 15 files changed, 1013 insertions(+), 74 deletions(-) create mode 100644 layers/artifact-delivery/server/utils/symbolic-links.ts create mode 100644 test/unit/artifact-symbolic-links.test.ts diff --git a/layers/artifact-delivery/server/utils/archive-reader.ts b/layers/artifact-delivery/server/utils/archive-reader.ts index 2948e39c..042cecd2 100644 --- a/layers/artifact-delivery/server/utils/archive-reader.ts +++ b/layers/artifact-delivery/server/utils/archive-reader.ts @@ -17,6 +17,12 @@ const GITHUB_FILE_BASE_TIMEOUT_MS = 30_000 /** A pax record or GNU long name holds one path. Anything longer is not an archive GitHub wrote. */ const MAX_HEADER_RECORD_BYTES = 1024 * 1024 const BLOCK_SIZE = 512 +/** + * Archive bytes one pass holds in memory for files whose turn has not come. + * Only symbolic links put files out of archive order. simota/agent-skills + * links a shared folder of 1.2 MB from each Skill, twice in most of them. + */ +const MAX_HELD_BYTES = 8 * 1024 * 1024 /** * Files a build may still read from GitHub one by one. Each costs one Worker @@ -36,13 +42,20 @@ export interface ArchiveReport { * Reads the packed files of one Skill from the Repository archive at the * exact commit, as codeload streams it. * - * Nothing is buffered beyond one network chunk: entries outside the Skill - * folder are skipped as they pass, each Skill file goes to the sink chunk by - * chunk, and the request is cancelled after the last Skill file. A file the + * Without symbolic links, nothing is buffered beyond one network chunk: + * entries outside the Skill folder are skipped as they pass, each Skill file + * goes to the sink chunk by chunk, and the request is cancelled after the + * last Skill file. A file the * archive leaves out, or holds with another size, is read from GitHub at the * same commit in its place, so the sink still receives Artifact order. The * sink checks every file against its Git blob digest, so neither host is * trusted for content. + * + * A file a symbolic link put in the Skill reads the bytes of its target, + * which sit elsewhere in the archive. An entry that passes before its turn + * stays in memory, up to `MAX_HELD_BYTES` in all. A link file whose target + * comes later waits for it when the files it holds back fit in that memory. + * Any other file reads its bytes from GitHub. */ export function createGithubArchiveReader(input: { fetch: typeof globalThis.fetch @@ -52,6 +65,11 @@ export function createGithubArchiveReader(input: { skillPath: string /** In Artifact order. */ files: readonly PackedFile[] + /** + * The files a symbolic link put in the Skill, each with the Repository path + * of its bytes. Every caller passes it, so none reads the link path instead. + */ + linkSources: ReadonlyMap /** Uncompressed archive bytes one pass reads at most. */ maxArchiveBytes: number budget: GithubReadBudget @@ -64,6 +82,7 @@ export function createGithubArchiveReader(input: { const gunzip = input.gunzip ?? gunzipStream const repositoryPath = (path: string) => input.skillPath === '.' ? path : `${input.skillPath}/${path}` const encodedRepository = `${encodeURIComponent(input.owner)}/${encodeURIComponent(input.repository)}` + const sourceOf = (file: PackedFile) => input.linkSources.get(file.path) ?? repositoryPath(file.path) const readFromGithub = async (file: PackedFile, sink: FileSink): Promise => { if (input.budget.githubReads <= 0) { @@ -74,7 +93,7 @@ export function createGithubArchiveReader(input: { ) } input.budget.githubReads-- - const url = `${FILE_HOST}/${encodedRepository}/${input.commitSha}/${repositoryPath(file.path).split('/').map(encodeURIComponent).join('/')}` + const url = `${FILE_HOST}/${encodedRepository}/${input.commitSha}/${sourceOf(file).split('/').map(encodeURIComponent).join('/')}` const sent = await fetchNoRedirect(input.fetch, url, { headers: { 'User-Agent': 'skilld.dev' }, signal: AbortSignal.timeout(GITHUB_FILE_BASE_TIMEOUT_MS + Math.ceil(file.size / 1024)), @@ -121,13 +140,90 @@ export function createGithubArchiveReader(input: { return async (sink, githubFiles) => { let next = 0 - /** Reads from GitHub every file that sorts before `path`, or every file left when it is null. */ - const readMissingBefore = async (path: string | null): Promise => { + // The archive entries the files still to pass read, counted by Repository path. + const needed = new Map() + const sizeAt = new Map() + for (const file of input.files) { + if (githubFiles.has(file.path)) + continue + const position = sourceOf(file) + needed.set(position, (needed.get(position) ?? 0) + 1) + sizeAt.set(position, file.size) + } + // Entries that passed before their turn, by Repository path. + const held = new Map() + let heldBytes = 0 + // The link file that chose to wait for its target, by index. + let waiting = -1 + + const passedOne = (position: string) => { + const left = (needed.get(position) ?? 1) - 1 + if (left > 0) { + needed.set(position, left) + return + } + needed.delete(position) + const bytes = held.get(position) + if (bytes) { + held.delete(position) + heldBytes -= bytes.byteLength + } + } + const fromMemory = async (file: PackedFile, bytes: Uint8Array): Promise => { + await sink.begin(file, 'archive') + await sink.chunk(bytes) + const ended = await sink.end() + return ended._tag === 'mismatch' ? ended : null + } + const fromGithub = async (file: PackedFile): Promise => { + if (!githubFiles.has(file.path)) + passedOne(sourceOf(file)) + return await readFromGithub(file, sink) + } + /** Bytes the files after `index` would hold while the archive moves from the entry at `from` to `to`. */ + const holdCost = (index: number, from: string, to: string): number => { + const counted = new Set() + let total = 0 + for (let later = index + 1; later < input.files.length; later++) { + const file = input.files[later]! + const position = sourceOf(file) + if (githubFiles.has(file.path) || held.has(position) || counted.has(position)) + continue + if (compareArtifactPaths(position, from) >= 0 && compareArtifactPaths(position, to) < 0) { + counted.add(position) + total += file.size + } + } + return total + } + + /** + * Passes every file whose bytes are ready before the archive entry at + * `position`, or every file left when it is null. A file whose entry + * passed without being held is read from GitHub. A link file whose target + * is still ahead waits for it when the files it holds back fit in + * memory, and reads it from GitHub otherwise. + */ + const drain = async (position: string | null): Promise => { for (; next < input.files.length; next++) { const file = input.files[next]! - if (path !== null && compareArtifactPaths(repositoryPath(file.path), path) >= 0) - break - const outcome = await readFromGithub(file, sink) + const at = sourceOf(file) + const bytes = githubFiles.has(file.path) ? undefined : held.get(at) + if (bytes) { + const outcome = await fromMemory(file, bytes) + passedOne(at) + if (outcome) + return outcome + continue + } + const ahead = position !== null && !githubFiles.has(file.path) && compareArtifactPaths(at, position) >= 0 + if (ahead && (compareArtifactPaths(at, position) === 0 || !input.linkSources.has(file.path) || waiting === next)) + return null + if (ahead && heldBytes + holdCost(next, position, at) <= MAX_HELD_BYTES) { + waiting = next + return null + } + const outcome = await fromGithub(file) if (outcome) return outcome } @@ -143,6 +239,12 @@ export function createGithubArchiveReader(input: { } else { const entries = archive.entries + const stopped = (read: BodyRead): boolean => { + if (read._tag === 'ok') + return false + input.report?.({ reason: read._tag === 'too-large' ? 'too-large' : 'malformed', detail: read.detail }) + return true + } try { while (next < input.files.length) { const entry = await entries.next() @@ -152,58 +254,62 @@ export function createGithubArchiveReader(input: { input.report?.({ reason: entry.reason, detail: entry.detail }) break } - if (entry.type !== 'file') { - const skipped = await entries.skip(entry.size) - if (skipped._tag !== 'ok') { - input.report?.({ reason: skipped._tag === 'too-large' ? 'too-large' : 'malformed', detail: skipped.detail }) + if (entry.type === 'file') { + const missing = await drain(entry.path) + if (missing) + return missing + } + // An entry no file reads, or one that holds another size, passes. + // Each file that reads it then reads from GitHub. + if (entry.type !== 'file' || !needed.has(entry.path) || entry.size !== sizeAt.get(entry.path)) { + if (stopped(await entries.skip(entry.size))) break - } continue } - const missing = await readMissingBefore(entry.path) - if (missing) - return missing const file = input.files[next] - if (!file || repositoryPath(file.path) !== entry.path) { - const skipped = await entries.skip(entry.size) - if (skipped._tag !== 'ok') { - input.report?.({ reason: skipped._tag === 'too-large' ? 'too-large' : 'malformed', detail: skipped.detail }) - break + if (file && !githubFiles.has(file.path) && sourceOf(file) === entry.path && needed.get(entry.path) === 1) { + // The common case: the next file, and no other reads it. + await sink.begin(file, 'archive') + const forwarded = await entries.forward(entry.size, async bytes => await sink.chunk(bytes)) + const ended = await sink.end() + if (forwarded._tag !== 'ok') { + input.report?.({ reason: forwarded._tag === 'too-large' ? 'too-large' : 'malformed', detail: forwarded.detail }) + // The file arrived short. The next pass reads it from GitHub. + return ended._tag === 'mismatch' + ? ended + : { _tag: 'mismatch', path: file.path, origin: 'archive', reason: forwarded.detail } } + if (ended._tag === 'mismatch') + return ended + passedOne(entry.path) + next++ continue } - if (githubFiles.has(file.path) || entry.size !== file.size) { - const skipped = await entries.skip(entry.size) - const outcome = await readFromGithub(file, sink) - if (outcome) - return outcome - next++ - if (skipped._tag !== 'ok') { - input.report?.({ reason: skipped._tag === 'too-large' ? 'too-large' : 'malformed', detail: skipped.detail }) + if (heldBytes + entry.size > MAX_HELD_BYTES) { + if (stopped(await entries.skip(entry.size))) break - } continue } - await sink.begin(file, 'archive') - const forwarded = await entries.forward(entry.size, async bytes => await sink.chunk(bytes)) - const ended = await sink.end() - if (forwarded._tag !== 'ok') { - input.report?.({ reason: forwarded._tag === 'too-large' ? 'too-large' : 'malformed', detail: forwarded.detail }) - // The file arrived short. The next pass reads it from GitHub. - return ended._tag === 'mismatch' - ? ended - : { _tag: 'mismatch', path: file.path, origin: 'archive', reason: forwarded.detail } - } - if (ended._tag === 'mismatch') - return ended - next++ + const bytes = new Uint8Array(entry.size) + let offset = 0 + const collected = await entries.forward(entry.size, async (piece) => { + bytes.set(piece, offset) + offset += piece.byteLength + }) + if (stopped(collected)) + break + held.set(entry.path, bytes) + heldBytes += bytes.byteLength + const ready = await drain(entry.path) + if (ready) + return ready } } finally { await entries.cancel() } } - const rest = await readMissingBefore(null) + const rest = await drain(null) return rest ?? { _tag: 'read' } } } diff --git a/layers/artifact-delivery/server/utils/artifact-pack.ts b/layers/artifact-delivery/server/utils/artifact-pack.ts index a1b0da61..354d3cc7 100644 --- a/layers/artifact-delivery/server/utils/artifact-pack.ts +++ b/layers/artifact-delivery/server/utils/artifact-pack.ts @@ -2,6 +2,7 @@ import type { Hash } from 'node:crypto' import type { ResolvedSource } from '../schemas/contracts' import type { ArtifactFileObserver, CheckedArtifactSource } from './checks' import type { OmittedArtifactFile, SourceRejection } from './github-source' +import type { SymbolicLinkNote } from './symbolic-links' import { createHash } from 'node:crypto' import { createArtifactCheckScanner } from './checks' import { projectedUstarBytes, USTAR_END_BYTES, ustarHeader, ustarPadding } from './ustar' @@ -152,6 +153,8 @@ export async function scanArtifact(input: { files: readonly PackedFile[] read: SkillFileReader omitted: OmittedArtifactFile[] + /** The symbolic links the load followed or left out. The `symbolic-links` check lists them. */ + symbolicLinks: SymbolicLinkNote[] spoolBytes: number }): Promise { const contentBytes = projectedUstarBytes(input.files.map(file => file.size)) @@ -186,7 +189,7 @@ export async function scanArtifact(input: { _tag: 'scanned', contentSha256: digest.digest('hex'), contentBytes, - checked: scanner.finish(input.omitted), + checked: scanner.finish(input.omitted, input.symbolicLinks), spool, readFromGithub, } diff --git a/layers/artifact-delivery/server/utils/build.ts b/layers/artifact-delivery/server/utils/build.ts index d5051976..a7c1848e 100644 --- a/layers/artifact-delivery/server/utils/build.ts +++ b/layers/artifact-delivery/server/utils/build.ts @@ -571,6 +571,7 @@ async function loadStoredBuild( !attestation || (attestation.policyVersion !== ARTIFACT_POLICY_VERSION && !BYTE_COMPATIBLE_POLICY_VERSIONS.has(attestation.policyVersion)) || !omittedNothing(attestation) + || !followedNoSymbolicLink(attestation) || !attestationMatchesRecord(attestation, ready) || !await verifyArtifactAttestation(attestation, dependencies.trustedRoot, dependencies.now()) ) { @@ -612,6 +613,16 @@ function omittedNothing(attestation: ArtifactAttestation): boolean { return omitted === undefined || omitted.outcome === 'pass' } +/** + * Whether a stored build met no symbolic link. Its stored files cannot show + * which ones a link put there, so the check could not be made again from + * them. Policies before the `symbolic-links` check refused every link. + */ +function followedNoSymbolicLink(attestation: ArtifactAttestation): boolean { + const links = attestation.checkResults.find(check => check.name === 'symbolic-links') + return links === undefined || links.outcome === 'pass' +} + /** * The source for a build that resumes after `fetching` in a new invocation. * A ready build is reused only when it matches the check results and content @@ -799,6 +810,7 @@ async function loadAndCheck( files: plan.files, read: plan.read, omitted: plan.omitted, + symbolicLinks: plan.symbolicLinks, spoolBytes: row.visibility === 'private' ? Number.POSITIVE_INFINITY : ARTIFACT_SPOOL_BYTES, }) if (scanned._tag === 'rejected') diff --git a/layers/artifact-delivery/server/utils/checks.ts b/layers/artifact-delivery/server/utils/checks.ts index c0d6d5f4..8b14cf42 100644 --- a/layers/artifact-delivery/server/utils/checks.ts +++ b/layers/artifact-delivery/server/utils/checks.ts @@ -1,6 +1,7 @@ import type { Hash } from 'node:crypto' import type { ArtifactFile, CheckResult, ResolvedSource } from '../schemas/contracts' import type { ArtifactSourceFile, OmittedArtifactFile } from './github-source' +import type { SymbolicLinkNote } from './symbolic-links' import { createHash } from 'node:crypto' import { ARTIFACT_POLICY_VERSION } from './state' @@ -28,6 +29,12 @@ const EXECUTABLE_FILES_VERSION = '1' * accepts any check result that is not required. */ const OMITTED_FILES_VERSION = '1' +/** + * Lists each symbolic link in the Skill folder: followed, so the files it + * names are packed at the link path, or left out. It is not required, for + * the same reason as `omitted-files`. ADR-0014. + */ +const SYMBOLIC_LINKS_VERSION = '1' /** The skilld CLI refuses a check result with more findings, or a longer one. */ const MAX_CHECK_FINDINGS = 100 const MAX_CHECK_FINDING_CHARACTERS = 500 @@ -35,7 +42,8 @@ const MAX_CHECK_FINDING_CHARACTERS = 500 /** The checks a statement under one policy carries, by check name. */ export type ArtifactCheckSet = ReadonlyMap -const CURRENT_ARTIFACT_CHECKS: ArtifactCheckSet = new Map([ +/** The checks of policy `2026-10-07.3`, which refused every symbolic link. */ +const POLICY_BEFORE_SYMBOLIC_LINKS_CHECKS: ArtifactCheckSet = new Map([ ['path-policy', { version: PATH_POLICY_VERSION, required: true }], ['agent-skills-spec', { version: AGENT_SKILLS_CHECK_VERSION, required: false }], ['credential-material', { version: CREDENTIAL_MATERIAL_VERSION, required: true }], @@ -43,6 +51,11 @@ const CURRENT_ARTIFACT_CHECKS: ArtifactCheckSet = new Map([ ['omitted-files', { version: OMITTED_FILES_VERSION, required: false }], ]) +const CURRENT_ARTIFACT_CHECKS: ArtifactCheckSet = new Map([ + ...POLICY_BEFORE_SYMBOLIC_LINKS_CHECKS, + ['symbolic-links', { version: SYMBOLIC_LINKS_VERSION, required: false }], +]) + /** * Every policy the artifact signer signs, with the checks its statements carry. * @@ -58,9 +71,9 @@ const CURRENT_ARTIFACT_CHECKS: ArtifactCheckSet = new Map([ */ export const SIGNABLE_ARTIFACT_POLICIES: ReadonlyMap = new Map([ [ARTIFACT_POLICY_VERSION, CURRENT_ARTIFACT_CHECKS], - // ADR-0013 changed loading and packaging only, so the policy before it - // carries the same checks. - ['2026-10-07.2', CURRENT_ARTIFACT_CHECKS], + // ADR-0014 follows symbolic links and adds the `symbolic-links` check. The + // policy before it refused every link, so the bump closes no safety gap. + ['2026-10-07.3', POLICY_BEFORE_SYMBOLIC_LINKS_CHECKS], ]) export interface CheckedArtifactSource { @@ -72,6 +85,7 @@ export async function checkArtifactSource( source: ResolvedSource, files: ArtifactSourceFile[], omitted: OmittedArtifactFile[] = [], + symbolicLinks: SymbolicLinkNote[] = [], ): Promise { const scanner = createArtifactCheckScanner(source) for (const file of files) { @@ -79,7 +93,7 @@ export async function checkArtifactSource( scanner.chunk(file.bytes) scanner.end() } - return scanner.finish(omitted) + return scanner.finish(omitted, symbolicLinks) } /** Receives the Skill files one at a time, each as a run of chunks. */ @@ -90,7 +104,7 @@ export interface ArtifactFileObserver { } export interface ArtifactCheckScanner extends ArtifactFileObserver { - finish: (omitted: OmittedArtifactFile[]) => CheckedArtifactSource + finish: (omitted: OmittedArtifactFile[], symbolicLinks: SymbolicLinkNote[]) => CheckedArtifactSource } /** @@ -146,7 +160,7 @@ export function createArtifactCheckScanner(source: ResolvedSource): ArtifactChec if (file.skill) skillBytes = file.skill }, - finish(omitted) { + finish(omitted, symbolicLinks) { if (current) throw new Error(`The check scanner did not end ${current.path}`) const pathPolicy: CheckResult = pathFindings.length > 0 @@ -202,6 +216,7 @@ export function createArtifactCheckScanner(source: ResolvedSource): ArtifactChec credentialMaterial, executableFiles, omittedFilesResult(omitted), + symbolicLinksResult(symbolicLinks), ], } }, @@ -378,6 +393,36 @@ function omittedFilesResult(omitted: OmittedArtifactFile[]): CheckResult { } } +/** One symbolic link, as the `symbolic-links` check lists it. */ +function symbolicLinkFinding(note: SymbolicLinkNote): string { + if (note._tag === 'followed') + return `${note.path} -> ${note.target}` + if (note._tag === 'holds-itself') + return `${note.path} -> ${note.target}: left out, the folder holds the link` + return `${note.path}: left out, it is a link inside a followed folder` +} + +function symbolicLinksResult(notes: SymbolicLinkNote[]): CheckResult { + if (notes.length === 0) + return { name: 'symbolic-links', version: SYMBOLIC_LINKS_VERSION, outcome: 'pass', required: false } + const followed = notes.filter(note => note._tag === 'followed').length + const leftOut = notes.length - followed + const sentences = [ + followed === 1 ? '1 symbolic link was followed. Its files are packed at the link path.' : null, + followed > 1 ? `${followed} symbolic links were followed. Their files are packed at the link paths.` : null, + leftOut === 1 ? '1 symbolic link was left out.' : null, + leftOut > 1 ? `${leftOut} symbolic links were left out.` : null, + ] + return { + name: 'symbolic-links', + version: SYMBOLIC_LINKS_VERSION, + outcome: 'warn', + required: false, + summary: `${sentences.filter(sentence => sentence !== null).join(' ')}${listedNote(notes.map(symbolicLinkFinding))}`, + findings: boundedFindings(notes.map(symbolicLinkFinding)), + } +} + export function checksBlockArtifact(checks: CheckResult[], checkSet: ArtifactCheckSet = CURRENT_ARTIFACT_CHECKS): boolean { const byName = new Map(checks.map(check => [check.name, check])) if (byName.size !== checks.length) diff --git a/layers/artifact-delivery/server/utils/github-source.ts b/layers/artifact-delivery/server/utils/github-source.ts index 6a5bb0b6..06da8ddf 100644 --- a/layers/artifact-delivery/server/utils/github-source.ts +++ b/layers/artifact-delivery/server/utils/github-source.ts @@ -2,6 +2,7 @@ import type { LinkedArtifactFile, ProblemCode, ResolvedSource, SourceRequest } f import type { ArchiveReport } from './archive-reader' import type { PackedFile, SkillFileReader } from './artifact-pack' import type { GithubReadTry } from './github-read' +import type { GitTreeEntry, LinkSourceReader, LinkText, SkillEntry, SymbolicLinkNote } from './symbolic-links' import { z } from 'zod' import { canonicalSkillFolder, isRegistrySkillPath, slugifySkillName } from '#shared/skill-path' import { createGithubArchiveReader } from './archive-reader' @@ -17,6 +18,7 @@ import { isTruncatedBody, readGithubWithRetry, } from './github-read' +import { followSymbolicLinks, isSymbolicLink, MAX_LINK_TARGET_BYTES } from './symbolic-links' import { compareArtifactPaths, projectedUstarBytes } from './ustar' const GITHUB_API = 'https://api.github.com' @@ -164,7 +166,7 @@ const blobResponseSchema = z.object({ content: z.string(), }) -type TreeEntry = z.infer +type TreeEntry = z.infer & GitTreeEntry /** Where the GitHub API serves a Repository now. */ interface RepositoryName { @@ -201,6 +203,8 @@ export interface LoadedArtifactSource { omitted: OmittedArtifactFile[] /** Files the attestation lists for the skilld CLI to read from GitHub. */ linked: LinkedArtifactFile[] + /** The symbolic links in the Skill folder, followed or left out. */ + symbolicLinks: SymbolicLinkNote[] /** Streams the packed files' bytes. Each call reads them again. */ read: SkillFileReader } @@ -750,25 +754,36 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const listed = await listTreeBounded(at.owner, at.repository, skillTree) if (listed._tag !== 'listed') return listed + // A private build follows links by the same rules. It reads one blob + // per file, so the reads the links cost come out of its file limit. + const followed = await followSymbolicLinks({ + entries: listed.entries, + skillPath: source.skillPath, + rootTreeSha: source.treeSha, + read: linkSourceReader(at), + }) + if (followed._tag === 'rejected') + return followed const limits = expectedVisibility === 'private' - ? PRIVATE_ARTIFACT_LIMITS + ? { ...PRIVATE_ARTIFACT_LIMITS, maxFiles: PRIVATE_ARTIFACT_LIMITS.maxFiles - followed.reads } : { ...PUBLIC_ARTIFACT_LIMITS, maxLinkedBytes: loadOptions.linkedFiles ? MAX_LINKED_BYTES : null } - const selected = selectArtifactEntries(listed.entries, source.skillPath, limits) + const selected = selectArtifactEntries(followed.entries, source.skillPath, limits) if (selected._tag === 'rejected') return selected const files = selected.entries.map(packedFile) const omitted = selected.omitted.map(entry => ({ path: entry.path, bytes: entry.size, - url: githubBlobUrl(source, entry.path), + url: githubBlobUrl(source, entry.from ?? repositoryPathOf(source.skillPath, entry.path)), })) const linked = selected.linked.map(packedFile) + const symbolicLinks = followed.notes if (expectedVisibility === 'private') { const fromBlobs = await loadFromBlobs(source, at, selected.entries) if (fromBlobs._tag !== 'loaded') return fromBlobs - return { _tag: 'loaded', value: { source, files, omitted, linked, read: bufferedReader(fromBlobs.files) } } + return { _tag: 'loaded', value: { source, files, omitted, linked, symbolicLinks, read: bufferedReader(fromBlobs.files) } } } // A public build reads the Repository archive from codeload and any file // it lacks from raw.githubusercontent.com. Neither spends REST quota, and @@ -780,16 +795,51 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi commitSha: source.commitSha, skillPath: source.skillPath, files, + linkSources: new Map(selected.entries.flatMap(entry => entry.from === undefined ? [] : [[entry.path, entry.from] as const])), maxArchiveBytes: ARCHIVE_MAX_UNCOMPRESSED_BYTES, budget: { githubReads: MAX_GITHUB_FILE_READS }, gunzip: options.gunzip, report: report => reportArchive(source, report), now, }) - return { _tag: 'loaded', value: { source, files, omitted, linked, read } } + return { _tag: 'loaded', value: { source, files, omitted, linked, symbolicLinks, read } } }, } + /** + * The reads that follow symbolic links: trees by their SHA, and each link + * blob through the REST API. Both are immutable, so a public build reads + * each one from GitHub once. simota/agent-skills links the same `_common` + * folder from 103 Skills. + */ + function linkSourceReader(at: RepositoryName): LinkSourceReader { + const repositoryPath = `/repos/${encodeURIComponent(at.owner)}/${encodeURIComponent(at.repository)}` + return { + async level(sha) { + const response = await getTree(at.owner, at.repository, sha, false) + if (response._tag === 'too-large' || (response._tag === 'ok' && response.value.truncated)) + return sourceLimitRejection('GitHub returned an incomplete tree for a symbolic link target.') + return response._tag === 'ok' ? response.value.tree : readRejection(response) + }, + async all(sha) { + const listed = await listTreeBounded(at.owner, at.repository, sha) + return listed._tag === 'listed' ? listed.entries : listed + }, + async linkText(sha): Promise { + const response = await requestImmutableJson(`blob:${sha}`, 'blob', `${repositoryPath}/git/blobs/${sha}`, blobResponseSchema) + if (response._tag !== 'ok') + return readRejection(response) + const content = base64ToBytes(response.value.content.replaceAll('\n', '')) + if (response.value.sha !== sha || await gitBlobShaHex(content) !== sha) + return reject('INVALID_SOURCE', 'A Git blob failed its Git digest check.', [sha]) + if (content.byteLength > MAX_LINK_TARGET_BYTES || content.includes(0)) + return { _tag: 'invalid' } + const text = decodeUtf8(content) + return text === null ? { _tag: 'invalid' } : { _tag: 'text', value: text } + }, + } + } + async function loadFromBlobs( source: ResolvedSource, at: RepositoryName, @@ -895,7 +945,7 @@ function normalizeRequestedSkillPath(input: string): string | null { return path } -type SizedEntry = TreeEntry & { size: number } +type SizedEntry = SkillEntry & { size: number } interface SelectedEntries { _tag: 'selected' @@ -920,8 +970,11 @@ interface SelectedEntries { * * A Skill at the Repository root has the whole Repository as its folder, so * its README images leave rather than count against it. + * + * A file a symbolic link put in the Skill is never a linked file. The skilld + * CLI reads a linked file at its own path, and GitHub serves no file there. */ -export function selectArtifactEntries(entries: TreeEntry[], skillPath: string, limits: ArtifactLimits): SelectedEntries | SourceRejection { +export function selectArtifactEntries(entries: SkillEntry[], skillPath: string, limits: ArtifactLimits): SelectedEntries | SourceRejection { const findings: string[] = [] const identities = new Map() const blobs: SizedEntry[] = [] @@ -937,7 +990,8 @@ export function selectArtifactEntries(entries: TreeEntry[], skillPath: string, l if (entry.type === 'commit' || entry.mode === '160000') findings.push(`${entry.path} is a Git submodule`) - if (entry.type === 'blob' && entry.mode === '120000') + // `followSymbolicLinks` replaces every link first. One that reaches here was never followed. + if (isSymbolicLink(entry)) findings.push(`${entry.path} is a symbolic link`) if (entry.type === 'blob' && entry.mode !== '100644' && entry.mode !== '100755' && entry.mode !== '120000') findings.push(`${entry.path} has unsupported mode ${entry.mode}`) @@ -998,7 +1052,7 @@ export function selectArtifactEntries(entries: TreeEntry[], skillPath: string, l for (const entry of candidates) { if (packedBytes <= limits.maxArchiveBytes) break - if (maxLinkedBytes !== null && linkedBytes + entry.size <= maxLinkedBytes) { + if (maxLinkedBytes !== null && entry.from === undefined && linkedBytes + entry.size <= maxLinkedBytes) { linked.push(entry) linkedBytes += entry.size } @@ -1160,9 +1214,8 @@ function isReadBySkill(path: string): boolean { return dot > 0 && TEXT_EXTENSIONS.has(name.slice(dot + 1)) } -/** The GitHub page of one Skill file at the Artifact's commit. */ -function githubBlobUrl(source: ResolvedSource, path: string): string { - const repositoryPath = source.skillPath === '.' ? path : `${source.skillPath}/${path}` +/** The GitHub page of one Repository file at the Artifact's commit. */ +function githubBlobUrl(source: ResolvedSource, repositoryPath: string): string { return `https://github.com/${encodeURIComponent(source.owner)}/${encodeURIComponent(source.repository)}/blob/${source.commitSha}/${repositoryPath.split('/').map(encodeURIComponent).join('/')}` } @@ -1183,6 +1236,23 @@ export function storedFilesPassLoadRules(files: ArtifactSourceFile[], skillPath: return selected._tag === 'selected' && selected.omitted.length === 0 && selected.linked.length === 0 } +/** The Repository path of a path inside the Skill folder. */ +function repositoryPathOf(skillPath: string, path: string): string { + return skillPath === '.' ? path : `${skillPath}/${path}` +} + +/** UTF-8 text, or null for bytes that are not. */ +function decodeUtf8(bytes: Uint8Array): string | null { + try { + // A byte order mark is part of a POSIX path, so it stays. + return new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(bytes) + } + catch { + // Invalid UTF-8 is an answer here: the bytes hold no path. + return null + } +} + function skillFolderLabel(skillPath: string): string { return skillPath === '.' ? 'The Skill folder' : `The Skill folder \`${skillPath}\`` } diff --git a/layers/artifact-delivery/server/utils/state.ts b/layers/artifact-delivery/server/utils/state.ts index f09cac12..2d0b8fed 100644 --- a/layers/artifact-delivery/server/utils/state.ts +++ b/layers/artifact-delivery/server/utils/state.ts @@ -26,7 +26,7 @@ import { canonicalJson, digestHex } from './encoding' * The signer also signs the policy before this one, so a deploy window fails * no run. Update `SIGNABLE_ARTIFACT_POLICIES` in `checks.ts` with each bump. */ -export const ARTIFACT_POLICY_VERSION = '2026-10-07.3' +export const ARTIFACT_POLICY_VERSION = '2026-10-07.4' /** * Earlier policies under which every ready build packed the bytes this policy @@ -36,6 +36,8 @@ export const ARTIFACT_POLICY_VERSION = '2026-10-07.3' * A bump that changes only checks adds the version it replaces. A bump that * changes the bytes of a folder an earlier policy accepted empties this set. * + * - `2026-10-07.3`: ADR-0014 follows the symbolic links that policy refused. + * A folder it accepted held no link, so it packs the same files. * - `2026-10-07.2`: ADR-0013 streams the archive and raises the limits, so * it packs folders that policy refused or left files out of. A folder it * packed whole packs the same files. Artifact order moved from UTF-16 to @@ -46,7 +48,7 @@ export const ARTIFACT_POLICY_VERSION = '2026-10-07.3' * refused. Every folder it accepted packs the same files. * - `2026-08-20.1`: #481 changed checks and Skill name resolution only. */ -export const BYTE_COMPATIBLE_POLICY_VERSIONS: ReadonlySet = new Set(['2026-10-07.2', '2026-10-07.1', '2026-08-20.1']) +export const BYTE_COMPATIBLE_POLICY_VERSIONS: ReadonlySet = new Set(['2026-10-07.3', '2026-10-07.2', '2026-10-07.1', '2026-08-20.1']) export const ACTIVE_BUILD_STATES = [ 'requested', diff --git a/layers/artifact-delivery/server/utils/symbolic-links.ts b/layers/artifact-delivery/server/utils/symbolic-links.ts new file mode 100644 index 00000000..e9d5f918 --- /dev/null +++ b/layers/artifact-delivery/server/utils/symbolic-links.ts @@ -0,0 +1,324 @@ +import type { SourceRejection } from './github-source' +import { compareArtifactPaths } from './ustar' + +/** One Git tree entry, as GitHub lists it. */ +export interface GitTreeEntry { + path: string + mode: string + type: 'blob' | 'tree' | 'commit' + sha: string + size?: number +} + +/** + * A tree entry of the Skill. `from` is the Repository path of its bytes when + * a symbolic link put it at `path`. Without `from`, the bytes sit at `path` + * inside the Skill folder. + */ +export type SkillEntry = GitTreeEntry & { from?: string } + +/** + * A symbolic link in the Skill folder, and what the build did with it. The + * `symbolic-links` check result lists each one. Paths are inside the Skill + * folder; `target` is the path the link holds. + */ +export type SymbolicLinkNote + = | { _tag: 'followed', path: string, target: string } + /** A link to a folder that holds the link. The Skill already packs every file in it. */ + | { _tag: 'holds-itself', path: string, target: string } + /** A link inside a folder another link names. It is not read. */ + | { _tag: 'inside-followed-folder', path: string } + +export interface FollowedLinks { + _tag: 'followed' + entries: SkillEntry[] + notes: SymbolicLinkNote[] + /** GitHub reads the links cost, each one a Worker subrequest. */ + reads: number +} + +export type LinkText = { _tag: 'text', value: string } | { _tag: 'invalid' } + +/** The GitHub reads that following links needs. Each may answer a read failure. */ +export interface LinkSourceReader { + /** The entries of one tree, one level deep. */ + level: (treeSha: string) => Promise + /** Every entry under one tree, with paths relative to it. */ + all: (treeSha: string) => Promise + /** The path a symbolic link's blob holds. */ + linkText: (blobSha: string) => Promise +} + +/** Links one path may pass through after the link itself. */ +export const MAX_LINK_HOPS = 8 +/** Symbolic links one Skill folder may hold. */ +const MAX_SKILL_LINKS = 64 +/** + * GitHub reads one Skill may spend to follow its links: link blobs and trees. + * simota/agent-skills spends at most 12 per Skill, and a public build caches + * each read by its SHA. + */ +const MAX_LINK_READS = 128 +/** Entries the folders that links name may add, as one Skill folder listing may hold. */ +const MAX_FOLLOWED_FOLDER_ENTRIES = 20_000 +/** The longest path a Unix system follows. A longer link blob holds no path. */ +export const MAX_LINK_TARGET_BYTES = 4096 + +const SYMBOLIC_LINK_MODE = '120000' + +type Refusal = 'absolute' | 'outside' | 'git' | 'missing' | 'submodule' | 'loop' | 'chain' | 'holds-skill' | 'invalid' + +const REFUSALS: Record = { + 'absolute': 'the target is an absolute path', + 'outside': 'the target is outside the Repository', + 'git': 'the target is inside .git', + 'missing': 'the target does not exist at this commit', + 'submodule': 'the target is in a Git submodule', + 'loop': 'the link loops', + 'chain': `the target passes through more than ${MAX_LINK_HOPS} links`, + 'holds-skill': 'the target folder holds the Skill folder', + 'invalid': 'the link does not hold a valid path', +} + +type Resolved + = | { _tag: 'file', entry: GitTreeEntry, path: string } + | { _tag: 'folder', path: string } + | { _tag: 'refused', refusal: Refusal } + +/** True for a tree entry that is a symbolic link. */ +export function isSymbolicLink(entry: GitTreeEntry): boolean { + return entry.type === 'blob' && entry.mode === SYMBOLIC_LINK_MODE +} + +/** + * The Skill entries with each symbolic link in the Skill folder replaced by + * the files it names, as `tar --dereference` would pack them. + * + * A link resolves from its own folder by POSIX rules, at the same commit: + * `..` leaves the folder that holds the link, and a link on the way resolves + * in turn. It must end at a file or a folder inside the Repository. A file + * becomes one entry at the link path. A folder becomes every file in it, + * under the link path. Each entry keeps its Git blob SHA, so the build checks + * the bytes as it checks any other file. + * + * Two kinds of link are left out, and the check result names them: + * + * - A link inside a folder that a link names. tar --dereference follows + * these too, and on simota/agent-skills it recursed 5,800 folders deep: + * `_common` links every Skill folder, and each Skill links `_common`. + * - A link to a folder inside the Skill that holds the link, such as + * `atlas/reference/atlas -> ../../atlas`. The Skill already packs those files. + * + * Any other link that does not end at a file or folder refuses the Skill. + */ +export async function followSymbolicLinks(input: { + /** The Skill folder listing, with paths inside the Skill folder. */ + entries: GitTreeEntry[] + skillPath: string + rootTreeSha: string + read: LinkSourceReader +}): Promise { + const links = input.entries.filter(isSymbolicLink) + if (links.length === 0) + return { _tag: 'followed', entries: input.entries, notes: [], reads: 0 } + if (links.length > MAX_SKILL_LINKS) + return limitRejection(`The Skill folder holds more than ${MAX_SKILL_LINKS} symbolic links.`) + + const skillFolder = input.skillPath === '.' ? '' : input.skillPath + const inRepository = (path: string) => skillFolder ? `${skillFolder}/${path}` : path + let reads = 0 + const overBudget = () => ++reads > MAX_LINK_READS + const budgetRejection = () => limitRejection(`The symbolic links of the Skill need more than ${MAX_LINK_READS} GitHub reads.`) + + // The Skill folder listing answers every lookup inside it with no read. + const levels = new Map([[skillFolder, []]]) + for (const entry of input.entries) { + if (entry.type === 'tree' && !levels.has(inRepository(entry.path))) + levels.set(inRepository(entry.path), []) + const separator = entry.path.lastIndexOf('/') + const folder = separator === -1 ? skillFolder : inRepository(entry.path.slice(0, separator)) + const level = levels.get(folder) ?? [] + level.push({ ...entry, path: entry.path.slice(separator + 1) }) + levels.set(folder, level) + } + const levelAt = async (segments: string[]): Promise => { + const key = segments.join('/') + const known = levels.get(key) + if (known) + return known + let sha = input.rootTreeSha + if (segments.length > 0) { + const parent = await levelAt(segments.slice(0, -1)) + if (parent === null || !Array.isArray(parent)) + return parent + const entry = parent.find(candidate => candidate.path === segments.at(-1) && candidate.type === 'tree') + if (!entry) + return null + sha = entry.sha + } + if (overBudget()) + return budgetRejection() + const listed = await input.read.level(sha) + if (Array.isArray(listed)) + levels.set(key, listed) + return listed + } + const texts = new Map() + const textOf = async (entry: GitTreeEntry): Promise => { + if (entry.size !== undefined && entry.size > MAX_LINK_TARGET_BYTES) + return { _tag: 'invalid' } + const known = texts.get(entry.sha) + if (known) + return known + if (overBudget()) + return budgetRejection() + const read = await input.read.linkText(entry.sha) + if (read._tag !== 'rejected') + texts.set(entry.sha, read) + return read + } + + /** Where the link at `linkPath`, a Repository path, ends. */ + const resolve = async (linkPath: string, text: string): Promise => { + if (text.startsWith('/')) + return { _tag: 'refused', refusal: 'absolute' } + const folder = linkPath.split('/').slice(0, -1) + const pending = text.split('/') + const seen = new Set([linkPath]) + let hops = 0 + while (pending.length > 0) { + const name = pending.shift()! + if (name === '' || name === '.') + continue + if (name === '..') { + if (folder.length === 0) + return { _tag: 'refused', refusal: 'outside' } + folder.pop() + continue + } + if (name.toLowerCase() === '.git') + return { _tag: 'refused', refusal: 'git' } + const level = await levelAt(folder) + if (level !== null && !Array.isArray(level)) + return level + const entry = level?.find(candidate => candidate.path === name) + if (!entry) + return { _tag: 'refused', refusal: 'missing' } + const path = [...folder, name].join('/') + if (isSymbolicLink(entry)) { + if (seen.has(path)) + return { _tag: 'refused', refusal: 'loop' } + if (++hops > MAX_LINK_HOPS) + return { _tag: 'refused', refusal: 'chain' } + seen.add(path) + const next = await textOf(entry) + if (next._tag === 'rejected') + return next + if (next._tag === 'invalid') + return { _tag: 'refused', refusal: 'invalid' } + if (next.value.startsWith('/')) + return { _tag: 'refused', refusal: 'absolute' } + pending.unshift(...next.value.split('/')) + continue + } + if (entry.type === 'tree') { + folder.push(name) + continue + } + if (entry.type === 'commit') + return { _tag: 'refused', refusal: 'submodule' } + // A file cannot hold the rest of the path, not even a trailing slash. + return pending.length > 0 ? { _tag: 'refused', refusal: 'missing' } : { _tag: 'file', entry, path } + } + return { _tag: 'folder', path: folder.join('/') } + } + + const listings = new Map() + let followedFolderEntries = 0 + const entries: SkillEntry[] = input.entries.filter(entry => !isSymbolicLink(entry)) + const notes: SymbolicLinkNote[] = [] + const refusals: string[] = [] + for (const link of [...links].sort((left, right) => compareArtifactPaths(left.path, right.path))) { + const text = await textOf(link) + if (text._tag === 'rejected') + return text + if (text._tag === 'invalid') { + refusals.push(`${link.path}: ${REFUSALS.invalid}`) + continue + } + const linkPath = inRepository(link.path) + const resolved = await resolve(linkPath, text.value) + if (resolved._tag === 'rejected') + return resolved + if (resolved._tag === 'refused') { + refusals.push(`${link.path} -> ${text.value}: ${REFUSALS[resolved.refusal]}`) + continue + } + if (resolved._tag === 'file') { + entries.push({ ...resolved.entry, path: link.path, from: resolved.path }) + notes.push({ _tag: 'followed', path: link.path, target: text.value }) + continue + } + if (resolved.path === '' || linkPath.startsWith(`${resolved.path}/`)) { + const insideSkill = skillFolder === '' || resolved.path === skillFolder || resolved.path.startsWith(`${skillFolder}/`) + if (insideSkill) + notes.push({ _tag: 'holds-itself', path: link.path, target: text.value }) + else + refusals.push(`${link.path} -> ${text.value}: ${REFUSALS['holds-skill']}`) + continue + } + const listed = await listingOf(resolved.path) + if (!Array.isArray(listed)) + return listed + followedFolderEntries += listed.length + if (followedFolderEntries > MAX_FOLLOWED_FOLDER_ENTRIES) + return limitRejection(`The folders the symbolic links of the Skill name hold more than ${MAX_FOLLOWED_FOLDER_ENTRIES.toLocaleString('en-US')} entries.`) + for (const entry of listed) { + const path = `${link.path}/${entry.path}` + if (isSymbolicLink(entry)) + notes.push({ _tag: 'inside-followed-folder', path }) + else if (entry.type === 'tree') + entries.push({ ...entry, path }) + else + entries.push({ ...entry, path, from: `${resolved.path}/${entry.path}` }) + } + notes.push({ _tag: 'followed', path: link.path, target: text.value }) + } + if (refusals.length > 0) + return { _tag: 'rejected', code: 'INVALID_SOURCE', summary: 'The Skill source layout was rejected.', findings: refusals.slice(0, 100) } + return { _tag: 'followed', entries, notes: notes.sort((left, right) => compareArtifactPaths(left.path, right.path)), reads } + + /** + * Every entry under a folder `resolve` reached. A folder inside the Skill + * comes from its own listing. Any other is read once, by its tree SHA. + */ + async function listingOf(path: string): Promise { + if (skillFolder === '' || path.startsWith(`${skillFolder}/`)) { + const prefix = `${skillFolder === '' ? path : path.slice(skillFolder.length + 1)}/` + return input.entries + .filter(entry => entry.path.startsWith(prefix)) + .map(entry => ({ ...entry, path: entry.path.slice(prefix.length) })) + } + const segments = path.split('/') + // `resolve` listed every folder on the way, so this reads nothing. + const parent = await levelAt(segments.slice(0, -1)) + if (parent !== null && !Array.isArray(parent)) + return parent + const folder = parent?.find(candidate => candidate.path === segments.at(-1) && candidate.type === 'tree') + if (!folder) + throw new Error(`A resolved folder is missing from its parent listing: ${path}`) + const known = listings.get(folder.sha) + if (known) + return known + if (overBudget()) + return budgetRejection() + const read = await input.read.all(folder.sha) + if (Array.isArray(read)) + listings.set(folder.sha, read) + return read + } +} + +function limitRejection(summary: string): SourceRejection { + return { _tag: 'rejected', code: 'INVALID_SOURCE', summary, findings: [] } +} diff --git a/test/fixtures/loaded-source.ts b/test/fixtures/loaded-source.ts index 60c0fd71..3c906a82 100644 --- a/test/fixtures/loaded-source.ts +++ b/test/fixtures/loaded-source.ts @@ -22,6 +22,7 @@ export function loadedFromFiles( source, files: ordered.map(file => ({ path: file.path, mode: file.mode, size: file.bytes.byteLength, gitBlobSha: file.gitBlobSha })), omitted: extra.omitted ?? [], + symbolicLinks: [], linked: extra.linked ?? [], read: async (sink) => { for (const file of ordered) { @@ -46,6 +47,7 @@ export async function readLoadedFiles(loaded: LoadedArtifactSource): Promise<{ _ files: loaded.files, read: loaded.read, omitted: loaded.omitted, + symbolicLinks: loaded.symbolicLinks, spoolBytes: Number.POSITIVE_INFINITY, }) if (scanned._tag === 'rejected') diff --git a/test/unit/artifact-archive-reader.test.ts b/test/unit/artifact-archive-reader.test.ts index e4f02fa6..291739de 100644 --- a/test/unit/artifact-archive-reader.test.ts +++ b/test/unit/artifact-archive-reader.test.ts @@ -81,6 +81,7 @@ describe('reading Skill files from the Repository archive', () => { commitSha, skillPath: 'skills/demo', files: plan, + linkSources: new Map(), maxArchiveBytes: 64 * 1024 * 1024, budget: { githubReads: 0 }, }) @@ -359,6 +360,7 @@ async function readAll( commitSha, skillPath: options.skillPath ?? 'skills/demo', files, + linkSources: new Map(), maxArchiveBytes: options.maxArchiveBytes ?? 64 * 1024 * 1024, budget: { githubReads: options.githubReads ?? 100 }, gunzip: inflated.stream, diff --git a/test/unit/artifact-github-source.test.ts b/test/unit/artifact-github-source.test.ts index ddf4dbbf..5a931d7f 100644 --- a/test/unit/artifact-github-source.test.ts +++ b/test/unit/artifact-github-source.test.ts @@ -230,10 +230,6 @@ describe('public GitHub Artifact source', () => { }) it.each([ - { - name: 'symbolic link', - unsafe: { path: 'references/latest', mode: '120000', type: 'blob', sha: '3'.repeat(40), size: 4 }, - }, { name: 'Git submodule', unsafe: { path: 'references/vendor', mode: '160000', type: 'commit', sha: '4'.repeat(40) }, diff --git a/test/unit/artifact-pack.test.ts b/test/unit/artifact-pack.test.ts index bdb6dda7..6abd359d 100644 --- a/test/unit/artifact-pack.test.ts +++ b/test/unit/artifact-pack.test.ts @@ -98,6 +98,7 @@ describe('artifact scan', () => { files: files.map(file => file.packed), read: memoryReader(files, 3), omitted: [], + symbolicLinks: [], spoolBytes: 1024 * 1024, }) @@ -120,6 +121,7 @@ describe('artifact scan', () => { files: files.map(file => file.packed), read: memoryReader(files, 1000), omitted: [], + symbolicLinks: [], spoolBytes: 2048, }) @@ -139,7 +141,7 @@ describe('artifact scan', () => { return await memoryReader(served, 5)(sink, readFromGithub) } - const scanned = await scanArtifact({ source, files: files.map(file => file.packed), read, omitted: [], spoolBytes: 1024 * 1024 }) + const scanned = await scanArtifact({ source, files: files.map(file => file.packed), read, omitted: [], symbolicLinks: [], spoolBytes: 1024 * 1024 }) expect(scanned).toMatchObject({ _tag: 'scanned', readFromGithub: new Set(['references/guide.md']) }) expect(asked.map(paths => [...paths])).toEqual([[], ['references/guide.md']]) @@ -156,6 +158,7 @@ describe('artifact scan', () => { files: files.map(file => file.packed), read: memoryReader(tampered, 5), omitted: [], + symbolicLinks: [], spoolBytes: 1024 * 1024, }) diff --git a/test/unit/artifact-signer.test.ts b/test/unit/artifact-signer.test.ts index a38255f2..b1183c68 100644 --- a/test/unit/artifact-signer.test.ts +++ b/test/unit/artifact-signer.test.ts @@ -300,6 +300,7 @@ async function createSignerFixture(options: SignerFixtureOptions = {}) { { name: 'credential-material', version: '2', outcome: 'pass', required: true }, { name: 'executable-files', version: '1', outcome: 'pass', required: false }, { name: 'omitted-files', version: '1', outcome: 'pass', required: false }, + { name: 'symbolic-links', version: '1', outcome: 'pass', required: false }, ] const statement = encodeAttestationStatement({ ...createAttestationStatement({ diff --git a/test/unit/artifact-source-checks.test.ts b/test/unit/artifact-source-checks.test.ts index 4eb834b3..92902acf 100644 --- a/test/unit/artifact-source-checks.test.ts +++ b/test/unit/artifact-source-checks.test.ts @@ -150,7 +150,7 @@ function streamedCheck(extra: ArtifactSourceFile, chunkSize: number) { scanner.chunk(entry.bytes.subarray(offset, offset + chunkSize)) scanner.end() } - return scanner.finish([]).checkResults + return scanner.finish([], []).checkResults } function credentialResult(results: Awaited>) { diff --git a/test/unit/artifact-symbolic-links.test.ts b/test/unit/artifact-symbolic-links.test.ts new file mode 100644 index 00000000..1a4531fd --- /dev/null +++ b/test/unit/artifact-symbolic-links.test.ts @@ -0,0 +1,372 @@ +import type { CheckResult } from '../../layers/artifact-delivery/server/schemas/contracts' +import type { LoadSourceResult } from '../../layers/artifact-delivery/server/utils/github-source' +import { createHash } from 'node:crypto' +import { describe, expect, it, vi } from 'vitest' +import { scanArtifact } from '../../layers/artifact-delivery/server/utils/artifact-pack' +import { + createGithubSourceClient, + createPublicGithubSourceClient, + MAX_LINKED_BYTES, + PUBLIC_ARTIFACT_LIMITS, + selectArtifactEntries, +} from '../../layers/artifact-delivery/server/utils/github-source' +import { readDeterministicUstar } from '../../layers/artifact-delivery/server/utils/ustar' +import { tarGzFixture } from '../fixtures/tar-archive' + +const commitSha = '0123456789abcdef0123456789abcdef01234567' +const topLevel = 'skilld-dev-skills-0123456' +const archiveUrl = `https://codeload.github.com/skilld-dev/skills/tar.gz/${commitSha}` +const rawBase = `https://raw.githubusercontent.com/skilld-dev/skills/${commitSha}/` +const skillText = '---\nname: demo\ndescription: Use this Skill for demo work.\n---\nRead _common/BOUNDARIES.md.\n' +const boundariesText = '# Boundaries\nStay in scope.\n' +const checkText = 'print("check")\n' +const guideText = 'Read this first.\n' + +/** A Repository entry: a file with its text, or a symbolic link with the path it holds. */ +type RepositoryEntry + = | { path: string, text: string, mode?: '100644' | '100755' } + | { path: string, link: string } + +const sharedFolder: RepositoryEntry[] = [ + { path: '_common/BOUNDARIES.md', text: boundariesText }, + { path: '_common/scripts/check.py', text: checkText, mode: '100755' }, +] +const demoSkill: RepositoryEntry[] = [ + { path: 'skills/demo/SKILL.md', text: skillText }, + { path: 'skills/demo/references/guide.md', text: guideText }, +] + +describe('symbolic links in a Skill folder', () => { + it('packs the files of a folder link at the link path, from the one archive read', async () => { + // simota/agent-skills: every Skill folder links the shared `_common` folder at the Repository root. + const github = fakeGithub([...sharedFolder, ...demoSkill, { path: 'skills/demo/_common', link: '../../_common' }]) + + const built = await loadAndScan(github.fetch) + + expect(built.files).toEqual([ + { path: 'SKILL.md', mode: 420, text: skillText }, + { path: '_common/BOUNDARIES.md', mode: 420, text: boundariesText }, + { path: '_common/scripts/check.py', mode: 493, text: checkText }, + { path: 'references/guide.md', mode: 420, text: guideText }, + ]) + expect(github.requested().filter(url => url.startsWith(rawBase))).toEqual([]) + expect(github.requested().filter(url => url === archiveUrl)).toHaveLength(1) + expect(built.check).toMatchObject({ + name: 'symbolic-links', + required: false, + outcome: 'warn', + findings: ['_common -> ../../_common'], + }) + }) + + it('packs a file link as a copy of the file it names', async () => { + // austintgriffith/ethskills: AGENTS.md is a link to SKILL.md. + const github = fakeGithub([...demoSkill, { path: 'skills/demo/AGENTS.md', link: 'SKILL.md' }]) + + const built = await loadAndScan(github.fetch) + + expect(built.files).toEqual([ + { path: 'AGENTS.md', mode: 420, text: skillText }, + { path: 'SKILL.md', mode: 420, text: skillText }, + { path: 'references/guide.md', mode: 420, text: guideText }, + ]) + expect(built.check).toMatchObject({ outcome: 'warn', findings: ['AGENTS.md -> SKILL.md'] }) + }) + + it('reads a link target from GitHub when waiting for it would hold more than 8 MiB', async () => { + // garrytan/gstack: `connect-chrome` links a folder that sorts after the + // rest of the Repository, and the Repository is the Skill folder. + const large = 'x'.repeat(9 * 1024 * 1024) + const github = fakeGithub([ + ...demoSkill, + { path: 'skills/demo/browser', link: '../zeta' }, + { path: 'skills/demo/media/reel.txt', text: large }, + { path: 'skills/zeta/notes.md', text: guideText }, + ]) + + const built = await loadAndScan(github.fetch) + + expect(built.files.map(file => file.path)).toEqual(['SKILL.md', 'browser/notes.md', 'media/reel.txt', 'references/guide.md']) + expect(github.requested().filter(url => url.startsWith(rawBase))).toEqual([`${rawBase}skills/zeta/notes.md`]) + }) + + it('holds the files a link file passes over while it waits for its target', async () => { + // simota/agent-skills/anvil sits in `.archive/`, which the archive lists before `_common`. + const github = fakeGithub([ + ...sharedFolder, + { path: '.archive/demo/SKILL.md', text: skillText }, + { path: '.archive/demo/references/guide.md', text: guideText }, + { path: '.archive/demo/_common', link: '../../_common' }, + ]) + const client = createPublicGithubSourceClient({ fetch: github.fetch as unknown as typeof fetch }) + + const built = await scan(await client.load({ ...resolvedSource(github.rootTreeSha), skillPath: '.archive/demo' }, { linkedFiles: false })) + + expect(built.files.map(file => file.path)).toEqual(['SKILL.md', '_common/BOUNDARIES.md', '_common/scripts/check.py', 'references/guide.md']) + expect(github.requested().filter(url => url.startsWith(rawBase))).toEqual([]) + }) + + it('waits in the archive for a link target when no later file needs an earlier entry', async () => { + const github = fakeGithub([ + ...demoSkill, + { path: 'skills/demo/zz', link: '../zeta' }, + { path: 'skills/zeta/notes.md', text: guideText }, + ]) + + const built = await loadAndScan(github.fetch) + + expect(built.files.map(file => file.path)).toEqual(['SKILL.md', 'references/guide.md', 'zz/notes.md']) + expect(github.requested().filter(url => url.startsWith(rawBase))).toEqual([]) + }) + + it('packs no symbolic-links finding for a Skill without links', async () => { + const built = await loadAndScan(fakeGithub(demoSkill).fetch) + + expect(built.check).toEqual({ name: 'symbolic-links', version: '1', required: false, outcome: 'pass' }) + }) + + it('leaves out a link to a folder that holds it, and every link inside a followed folder', async () => { + // simota/agent-skills/atlas links itself at reference/atlas, and `_common` + // links every Skill folder back. tar --dereference recursed 5,800 folders deep on it. + const github = fakeGithub([ + ...sharedFolder, + { path: '_common/demo', link: '../skills/demo' }, + ...demoSkill, + { path: 'skills/demo/_common', link: '../../_common' }, + { path: 'skills/demo/references/demo', link: '../../demo' }, + ]) + + const built = await loadAndScan(github.fetch) + + expect(built.files.map(file => file.path)).toEqual([ + 'SKILL.md', + '_common/BOUNDARIES.md', + '_common/scripts/check.py', + 'references/guide.md', + ]) + expect(built.check?.findings).toEqual([ + '_common -> ../../_common', + '_common/demo: left out, it is a link inside a followed folder', + 'references/demo -> ../../demo: left out, the folder holds the link', + ]) + }) + + it('follows a link that passes through another link', async () => { + const github = fakeGithub([ + ...sharedFolder, + ...demoSkill, + { path: 'shared', link: '_common' }, + { path: 'skills/demo/BOUNDARIES.md', link: '../../shared/BOUNDARIES.md' }, + ]) + + const built = await loadAndScan(github.fetch) + + expect(built.files).toContainEqual({ path: 'BOUNDARIES.md', mode: 420, text: boundariesText }) + }) + + it.each([ + { name: 'leaves the Repository root', link: '../../../outside', finding: 'escape -> ../../../outside: the target is outside the Repository' }, + { name: 'is an absolute path', link: '/etc/passwd', finding: 'escape -> /etc/passwd: the target is an absolute path' }, + { name: 'points into .git', link: '../../.git/config', finding: 'escape -> ../../.git/config: the target is inside .git' }, + { name: 'names a missing file', link: 'missing.md', finding: 'escape -> missing.md: the target does not exist at this commit' }, + { name: 'passes through a file', link: 'SKILL.md/more', finding: 'escape -> SKILL.md/more: the target does not exist at this commit' }, + { name: 'holds the Skill folder', link: '..', finding: 'escape -> ..: the target folder holds the Skill folder' }, + ])('rejects a link that $name before it reads the archive', async ({ link, finding }) => { + const github = fakeGithub([...demoSkill, { path: 'skills/demo/escape', link }]) + + const loaded = await load(github.fetch) + + expect(loaded).toMatchObject({ + _tag: 'rejected', + code: 'INVALID_SOURCE', + summary: 'The Skill source layout was rejected.', + findings: [finding], + }) + expect(github.requested().filter(url => url === archiveUrl || url.startsWith(rawBase))).toEqual([]) + }) + + it('rejects links that loop', async () => { + const github = fakeGithub([ + ...demoSkill, + { path: 'skills/demo/a.md', link: 'b.md' }, + { path: 'skills/demo/b.md', link: 'a.md' }, + ]) + + const loaded = await load(github.fetch) + + expect(loaded).toMatchObject({ + _tag: 'rejected', + summary: 'The Skill source layout was rejected.', + findings: ['a.md -> b.md: the link loops', 'b.md -> a.md: the link loops'], + }) + }) + + it('rejects a chain of more than 8 links', async () => { + const chain: RepositoryEntry[] = Array.from({ length: 9 }, (_, index) => ({ path: `skills/demo/l${index}.md`, link: `l${index + 1}.md` })) + const github = fakeGithub([...demoSkill, ...chain, { path: 'skills/demo/l9.md', link: 'SKILL.md' }]) + + const loaded = await load(github.fetch) + + expect(loaded).toMatchObject({ _tag: 'rejected', summary: 'The Skill source layout was rejected.' }) + if (loaded._tag === 'rejected') + expect(loaded.findings[0]).toBe('l0.md -> l1.md: the target passes through more than 8 links') + }) + + it('follows links the same way for a private build, reading each file as a blob', async () => { + const github = fakeGithub([...sharedFolder, ...demoSkill, { path: 'skills/demo/_common', link: '../../_common' }], { visibility: 'private' }) + const client = createGithubSourceClient({ fetch: github.fetch as unknown as typeof fetch, token: 'installation-token', visibility: 'private' }) + + const loaded = await client.load({ ...resolvedSource(github.rootTreeSha), visibility: 'private' }, { linkedFiles: false }) + const built = await scan(loaded) + + expect(built.files.map(file => file.path)).toEqual(['SKILL.md', '_common/BOUNDARIES.md', '_common/scripts/check.py', 'references/guide.md']) + expect(github.requested().filter(url => url.includes('codeload') || url.startsWith(rawBase))).toEqual([]) + }) +}) + +describe('choosing files when a link put some in the Skill', () => { + it('never offers a file a link put in the Skill as a linked file', () => { + // The skilld CLI reads a linked file at the link path, where GitHub serves no file. + const selected = selectArtifactEntries([ + { path: 'SKILL.md', mode: '100644', type: 'blob', sha: 'a'.repeat(40), size: 100 }, + { path: 'shared/model.bin', mode: '100644', type: 'blob', sha: 'b'.repeat(40), size: 70 * 1024 * 1024, from: 'models/model.bin' }, + ], 'skills/demo', { ...PUBLIC_ARTIFACT_LIMITS, maxLinkedBytes: MAX_LINKED_BYTES }) + + expect(selected).toMatchObject({ _tag: 'selected', linked: [], omitted: [{ path: 'shared/model.bin' }] }) + }) +}) + +async function load(fetchMock: ReturnType['fetch']): Promise { + const client = createPublicGithubSourceClient({ fetch: fetchMock as unknown as typeof fetch }) + return await client.load(resolvedSource(rootTree()), { linkedFiles: false }) +} + +async function loadAndScan(fetchMock: ReturnType['fetch']) { + return await scan(await load(fetchMock)) +} + +/** Scans a load the way a build does: the packed files, and the symbolic-links check result. */ +async function scan(loaded: LoadSourceResult) { + if (loaded._tag !== 'loaded') + throw new Error(`The load failed: ${loaded.summary} ${loaded.findings.join('; ')}`) + const scanned = await scanArtifact({ + source: loaded.value.source, + files: loaded.value.files, + read: loaded.value.read, + omitted: loaded.value.omitted, + symbolicLinks: loaded.value.symbolicLinks, + spoolBytes: Number.POSITIVE_INFINITY, + }) + if (scanned._tag === 'rejected') + throw new Error(`The scan failed: ${scanned.summary} ${scanned.findings.join('; ')}`) + const files = await readDeterministicUstar(scanned.spool!) + if (!files) + throw new Error('The scan packed an archive it cannot read back') + return { + files: files.map(file => ({ path: file.path, mode: file.mode, text: new TextDecoder().decode(file.bytes) })), + check: scanned.checked.checkResults.find((check: CheckResult) => check.name === 'symbolic-links'), + } +} + +/** + * GitHub for one Repository: the REST trees and blobs, the codeload archive + * with each link as a tar link entry, and raw.githubusercontent.com, which + * answers a link with the path it holds and a path through a folder link + * with 404, as GitHub does. + */ +function fakeGithub(entries: RepositoryEntry[], options: { visibility?: 'public' | 'private' } = {}) { + const encoder = new TextEncoder() + const contentOf = (entry: RepositoryEntry) => 'link' in entry ? entry.link : entry.text + const blobs = entries.map(entry => ({ + path: entry.path, + mode: 'link' in entry ? '120000' : entry.mode ?? '100644', + type: 'blob', + sha: gitBlobSha(contentOf(entry)), + size: encoder.encode(contentOf(entry)).byteLength, + })) + const folders = new Set(['']) + for (const entry of entries) { + const segments = entry.path.split('/') + for (let index = 1; index < segments.length; index++) + folders.add(segments.slice(0, index).join('/')) + } + const folderBySha = new Map([...folders].map(folder => [treeShaOf(folder), folder])) + const listing = (folder: string, recursive: boolean) => { + const prefix = folder ? `${folder}/` : '' + const trees = [...folders] + .filter(other => other !== '' && other !== folder && other.startsWith(prefix)) + .map(other => ({ path: other.slice(prefix.length), mode: '040000', type: 'tree', sha: treeShaOf(other) })) + const files = blobs + .filter(blob => blob.path.startsWith(prefix)) + .map(blob => ({ ...blob, path: blob.path.slice(prefix.length) })) + const all = [...trees, ...files] + return recursive ? all : all.filter(entry => !entry.path.includes('/')) + } + const archive = tarGzFixture(topLevel, [...entries] + .sort((left, right) => left.path < right.path ? -1 : 1) + .map(entry => 'link' in entry + ? { path: entry.path, typeflag: '2', linkname: entry.link, mode: '0000777' } + : { path: entry.path, bytes: encoder.encode(entry.text) }), { globalComment: commitSha }) + + 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: options.visibility === 'private', default_branch: 'main' }) + const treeMatch = /\/git\/trees\/([a-f0-9]{40})(\?recursive=1)?$/.exec(url) + if (treeMatch) { + const folder = folderBySha.get(treeMatch[1]!) + return folder === undefined + ? json({}, 404) + : json({ sha: treeMatch[1], truncated: false, tree: listing(folder, Boolean(treeMatch[2])) }) + } + const blobMatch = /\/git\/blobs\/([a-f0-9]{40})$/.exec(url) + if (blobMatch) { + const entry = entries.find(candidate => gitBlobSha(contentOf(candidate)) === blobMatch[1]) + return entry + ? json({ sha: blobMatch[1], size: encoder.encode(contentOf(entry)).byteLength, encoding: 'base64', content: Buffer.from(contentOf(entry), 'utf8').toString('base64') }) + : json({}, 404) + } + if (url === archiveUrl) + return new Response(archive, { status: 200 }) + if (url.startsWith(rawBase)) { + const path = url.slice(rawBase.length).split('/').map(decodeURIComponent).join('/') + const entry = entries.find(candidate => candidate.path === path) + return entry ? new Response(contentOf(entry), { status: 200 }) : new Response('404: Not Found', { status: 404 }) + } + return json({}, 404) + }) + return { + fetch: fetchMock, + rootTreeSha: treeShaOf(''), + requested: () => fetchMock.mock.calls.map(call => String(call[0])), + } +} + +function rootTree(): string { + return treeShaOf('') +} + +function treeShaOf(folder: string): string { + return createHash('sha1').update(`tree ${folder}`).digest('hex') +} + +function resolvedSource(treeSha: string) { + return { + provider: 'github' as const, + repositoryId: 123, + owner: 'skilld-dev', + repository: 'skills', + visibility: 'public' as const, + commitSha, + treeSha, + skillPath: 'skills/demo', + } +} + +function gitBlobSha(value: string): string { + return createHash('sha1').update(`blob ${Buffer.byteLength(value)}\0${value}`).digest('hex') +} + +function json(value: unknown, status = 200): Response { + return new Response(JSON.stringify(value), { status, headers: { 'content-type': 'application/json' } }) +} diff --git a/test/unit/private-artifact-delivery.test.ts b/test/unit/private-artifact-delivery.test.ts index 4061d9df..7c5f8940 100644 --- a/test/unit/private-artifact-delivery.test.ts +++ b/test/unit/private-artifact-delivery.test.ts @@ -561,5 +561,6 @@ function requiredChecks() { { name: 'credential-material', version: '2', outcome: 'pass' as const, required: true }, { name: 'executable-files', version: '1', outcome: 'pass' as const, required: false }, { name: 'omitted-files', version: '1', outcome: 'pass' as const, required: false }, + { name: 'symbolic-links', version: '1', outcome: 'pass' as const, required: false }, ] } From f35f6d5899c2c9b2b3266b07e9d70bcc2a57c3da Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 04:19:27 +1100 Subject: [PATCH 2/3] docs(adr): record ADR-0014 for symbolic links in a Skill folder Names symbolic link in the glossary, apart from ADR-0013's linked file. --- GLOSSARY.md | 7 ++ .../0014-symbolic-links-in-a-skill-folder.md | 101 ++++++++++++++++++ docs/arch/README.md | 1 + 3 files changed, 109 insertions(+) create mode 100644 docs/adr/0014-symbolic-links-in-a-skill-folder.md diff --git a/GLOSSARY.md b/GLOSSARY.md index 155f7d64..fe98bd98 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -13,6 +13,7 @@ Shared terms match the skilld CLI glossary (`skilld-dev/skilld/GLOSSARY.md`). Th | Repository | GitHub | Repository 1—N Skill | "repository" | | Artifact | skilld.dev API | Skill commit 1—1 Artifact | not shown | | linked file | Artifact attestation | Artifact 1—N linked file | not shown | +| symbolic link | Skill folder, `symbolic-links` check result | Skill folder 1—N symbolic link | not shown | | source status | lockfile, skilld.dev API | Artifact 1—1 source status | "Verified", "Unverified", "Local" | | run | skilld CLI | Skill 1—1 run command | "run" | | demo | Skill page, homepage | Skill 1—N demo | "demo" | @@ -275,6 +276,12 @@ Existing route paths, table names, and protocol fields keep their identifiers. **Never:** skipped file, excluded file, dropped file. +### symbolic link + +**Is:** a Git tree entry of mode `120000` whose blob holds a path. A build packs the files a link in the Skill folder names at the link path, or leaves the link out. The `symbolic-links` check result lists each one. See ADR-0014. + +**Never:** linked file (that is a file the CLI reads from GitHub), alias, shortcut, symlink in prose. + ### check result **Is:** one named check, its version, its finding, and its outcome for one Artifact. diff --git a/docs/adr/0014-symbolic-links-in-a-skill-folder.md b/docs/adr/0014-symbolic-links-in-a-skill-folder.md new file mode 100644 index 00000000..ae48d855 --- /dev/null +++ b/docs/adr/0014-symbolic-links-in-a-skill-folder.md @@ -0,0 +1,101 @@ +# ADR-0014: Symbolic links in a Skill folder + +Date: 2026-10-07 + +Amends [ADR-0013](0013-artifact-size-limits-and-linked-files.md). + +## Context + +A build refused every Skill folder that held a symbolic link, with `INVALID_SOURCE: The Skill source layout was rejected.` +The 2026-10-07 sweep found 108 such Skills, the largest class still refused. + +- 103 are `simota/agent-skills/*`. Each Skill folder links the shared `_common` and `_templates` folders at the Repository root. +- `austintgriffith/ethskills` links `AGENTS.md`, `CLAUDE.md`, and `llms.txt` to `SKILL.md`. +- `garrytan/gstack` links `connect-chrome` to `open-gstack-browser`. +- The two `twostraws` Skills link `skills//references` to the Skill's own `references`. +- `oaustegard/claude-skills/agent-routing` links `agent-routing` to `../../agent-routing`, outside the Repository. + +`tar --dereference` cannot pack simota/agent-skills. +`_common` links every Skill folder back, so tar recursed 5,800 folders deep and crashed. + +## Decision + +A build follows each symbolic link in the Skill folder at the same commit. +The skilld CLI receives regular files, so the attestation, the Resolution answer, and the CLI are unchanged. + +### Resolution + +The link blob holds a path. The build reads it through the REST API and checks it against the blob SHA. +It resolves the path from the link's own folder by POSIX rules: `..` leaves the folder, and a link on the way resolves in turn. + +- A file becomes one file at the link path. +- A folder becomes every file in it, under the link path. + +Each file keeps its own Git blob SHA from the tree, so the build checks its bytes like any other file. +The files count against the limits of ADR-0013. A file a link put in the Skill is never a linked file: [GitHub](https://github.com) serves no file at the link path. + +### Refused + +These links refuse the Skill, with the existing summary and one finding each: + +| Link | Finding | +|---|---| +| An absolute path | `the target is an absolute path` | +| `..` past the Repository root | `the target is outside the Repository` | +| A path through `.git` | `the target is inside .git` | +| A path that does not exist at the commit | `the target does not exist at this commit` | +| A path into a Git submodule | `the target is in a Git submodule` | +| A chain that comes back to a link it passed | `the link loops` | +| A chain through more than 8 links | `the target passes through more than 8 links` | +| A folder above the Skill folder that holds it | `the target folder holds the Skill folder` | +| A blob that is not a UTF-8 path of at most 4,096 bytes | `the link does not hold a valid path` | + +A Skill folder may hold 64 links, and following them may cost 128 GitHub reads. + +### Left out + +Two kinds of link are left out. Following them would repeat the same files without end. + +- A link inside a folder that a link names. The build does not read it. +- A link to a folder inside the Skill that holds the link, such as `atlas/reference/atlas -> ../../atlas`. The Skill already packs every file in it. + +Refusing the second kind would keep 22 of the 103 simota Skills refused. + +### The check result + +The `symbolic-links` check is not required, so a released CLI accepts it. +It warns when the Skill folder holds a link, and lists each one: `_common -> ../_common` when followed, or the link and the reason when left out. +A stored build that followed a link is not checked again from its bytes, because the bytes do not show which files a link put there. + +### The archive + +The bytes of a file a link put in the Skill sit at the target path in the Repository archive, not at the link path. +So the archive no longer lists every file in Artifact order. + +- An entry that passes before its turn stays in memory, up to 8 MiB in one pass. +- A file whose target comes later waits for it, when the files it holds back fit in that memory. +- Any other file reads its bytes from `raw.githubusercontent.com` at the target path. + +A Skill without links streams as before and holds nothing. + +### Private builds + +A private build follows links by the same rules. It reads one blob per file, so the reads the links cost come out of its 900-file limit. + +## Consequences + +A sweep built the 108 Skills with this change on 2026-10-07: + +| | Before | After | +|---|---|---| +| Ready | 0 | 107 | +| Refused | 108 | 1, `oaustegard/claude-skills/agent-routing`, whose only link leaves the Repository | +| Files read from `raw.githubusercontent.com` | | 2, both in `garrytan/gstack` | +| Slowest build | | 1.1 s | +| Most memory one build added | | 11.5 MiB, `garrytan/gstack` | + +The policy is `2026-10-07.4`. The previous policy `2026-10-07.3` stays signable, because it refused every link. +A ready build under it held no link, so it is checked again from its bytes. + +The skilld CLI reads an installed copy back as a local Skill on `skilld update`, and refuses one of more than 512 files. +`simota/agent-skills/hone` packs 527 files and `garrytan/gstack` 2,000. A run is not affected. diff --git a/docs/arch/README.md b/docs/arch/README.md index 2a72f4ae..3f0de4f7 100644 --- a/docs/arch/README.md +++ b/docs/arch/README.md @@ -60,6 +60,7 @@ Public grants without the header select the newest attestation. Private grants r A public build streams the Repository archive from codeload and never holds the whole Skill. The limits, the constraint behind each, and linked files: [ADR-0013](../adr/0013-artifact-size-limits-and-linked-files.md). +A build follows each symbolic link in the Skill folder and packs the files it names: [ADR-0014](../adr/0014-symbolic-links-in-a-skill-folder.md). After a contract change in the `skilld-dev/skilld` repository, run `pnpm --filter skilld-sdk generate` and commit the document. Publish the new SDK version from that repository's `release.yml`. Then update this site's exact catalog pin. From 84be2059fe1e1b5d0990fe5d76769565bc86f148 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 04:21:41 +1100 Subject: [PATCH 3/3] docs(adr): say which shared folders simota Skills link --- docs/adr/0014-symbolic-links-in-a-skill-folder.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0014-symbolic-links-in-a-skill-folder.md b/docs/adr/0014-symbolic-links-in-a-skill-folder.md index ae48d855..4aa80fd1 100644 --- a/docs/adr/0014-symbolic-links-in-a-skill-folder.md +++ b/docs/adr/0014-symbolic-links-in-a-skill-folder.md @@ -9,7 +9,7 @@ Amends [ADR-0013](0013-artifact-size-limits-and-linked-files.md). A build refused every Skill folder that held a symbolic link, with `INVALID_SOURCE: The Skill source layout was rejected.` The 2026-10-07 sweep found 108 such Skills, the largest class still refused. -- 103 are `simota/agent-skills/*`. Each Skill folder links the shared `_common` and `_templates` folders at the Repository root. +- 103 are `simota/agent-skills/*`. Each Skill folder links the shared `_common` folder at the Repository root, and some link `_templates` too. - `austintgriffith/ethskills` links `AGENTS.md`, `CLAUDE.md`, and `llms.txt` to `SKILL.md`. - `garrytan/gstack` links `connect-chrome` to `open-gstack-browser`. - The two `twostraws` Skills link `skills//references` to the Skill's own `references`.