diff --git a/doc/plans/2026-10-09-repository-skill-packages.md b/doc/plans/2026-10-09-repository-skill-packages.md index 77ef830edeb..918bcbdd56c 100644 --- a/doc/plans/2026-10-09-repository-skill-packages.md +++ b/doc/plans/2026-10-09-repository-skill-packages.md @@ -27,3 +27,11 @@ The importer saves available repository files without asking for repair decision The production dialog shows every skill by default, selects all discovered skills, and collapses individual package files. It uses one collapsed warnings disclosure and leaves Import repository available. The importer has no packaging option: new imports keep repository files together. Storybook covers omitted files, warning details, missing references, setup problems, warnings after import, and mobile review. Focused scanner/delivery/PostgreSQL tests and UI tests verify saved warnings and available bytes; browser fixtures exercise the production UI. This revision has not rerun the paid local/Daytona campaigns above. Final acceptance also found and fixed duplicate saved warnings and an installed-file viewer that did not expose repository dependencies. The viewer now lists repository files and reads their saved bytes without GitHub access. Content errors for unsafe execution or secret disclosure omit the affected files with warnings; the remaining repository can still import. Database and scanner regressions verify these exclusions and file-reader company isolation. + +## October 14: dependency read batches + +Oversized repository imports read each Markdown dependency frontier through the existing bounded Git batch reader. The next frontier contains newly found references. The scanner retains its file and byte limits, content audits, reference order, and fallback for snapshot readers without batch support. This avoids one Git child process for each Markdown file during a source refresh. + +The regression checks a real oversized Git repository with 64 skills, transitive references, and a reference cycle. It compares every retained file's bytes and requires four batch reads with no individual reads. A local comparison of the old and final dependency walkers on 500 Markdown files and two shared references returned identical entries: three old samples took 4.9–6.2 seconds with 502 child processes, while the new samples took 62–85 milliseconds with three batches. These measurements exclude provider execution and do not establish agent response latency. + +The frontier uses a queue of newly added Markdown files. A regression at the 9,990-file limit retains every file, terminates a reference cycle, and bounds path visits. The earlier frontier rescan made 50,014,936 path visits in a local comparison. The queue makes 89,911 visits and retains the same output order. diff --git a/server/src/__tests__/github-skill-source.test.ts b/server/src/__tests__/github-skill-source.test.ts index 91f0b20470e..c82bfdea2f2 100644 --- a/server/src/__tests__/github-skill-source.test.ts +++ b/server/src/__tests__/github-skill-source.test.ts @@ -13,6 +13,28 @@ import { skillFileBytes } from '../services/skill-snapshot.js'; const sha = 'a'.repeat(40); const md = (name: string) => `---\nname: ${name}\ndescription: A useful skill\n---\nFollow these instructions.\n`; describe('GitHub skill repository discovery', () => { + it('batches each dependency frontier instead of spawning a Git read for every Markdown file', async () => { + const files = { + ...Object.fromEntries(Array.from({ length: 64 }, (_, i) => [`skills/skill-${i}/SKILL.md`, md(`skill-${i}`) + '\n[Guide](../../shared/guide.md)'])), + 'shared/guide.md': '[Details](details.md)', + 'shared/details.md': '[Guide](guide.md) [Data](data.json)', + 'shared/data.json': '{"example":true}', + ...Object.fromEntries(Array.from({ length: 10_000 }, (_, i) => [`unrelated/${i}.txt`, ''])), + }; + const fixture = githubFixture(files); + const snapshot = await fixture.openSnapshot({ repositoryUrl: '', ref: 'main' }); + const read = snapshot.readBlob; + const singleReads = vi.spyOn(snapshot, 'readBlob'); + const batches = vi.fn(async (entries: typeof snapshot.entries) => new Map(await Promise.all(entries.map(async entry => [entry.sha, await read(entry)] as const)))); + snapshot.readBlobs = batches; + fixture.openSnapshot = async () => snapshot; + const scan = await scanGitHubSkillsWithPackageFallback({ repositoryUrl: 'https://github.com/acme/skills', packageMode: 'repository' }, fixture); + expect(scan.repositoryFiles?.map(file => file.path).sort()).toEqual(Object.keys(files).filter(name => !name.startsWith('unrelated/')).sort()); + expect(scan.candidates.every(skill => skill.error === null)).toBe(true); + expect(singleReads).not.toHaveBeenCalled(); + expect(batches.mock.calls.map(([entries]) => entries.length)).toEqual([64, 1, 1, 1]); + }); + it('keeps skill folders and transitive shared files when unrelated repository files exceed the package limit', async () => { const fixture = githubFixture({ 'skills/main/SKILL.md': md('main') + '\nRead [shared](../../shared/guide.md) and [helper](../helper/SKILL.md).', diff --git a/server/src/__tests__/skill-repository-dependencies.test.ts b/server/src/__tests__/skill-repository-dependencies.test.ts new file mode 100644 index 00000000000..89b7ced8069 --- /dev/null +++ b/server/src/__tests__/skill-repository-dependencies.test.ts @@ -0,0 +1,22 @@ +import { expect, it, vi } from 'vitest'; +import { skillRepositoryDependencies } from '../services/skill-repository-dependencies.js'; +import { MAX_REPOSITORY_FILES } from '../services/skill-repository-package.js'; +import type { GitSkillTreeEntry } from '../services/skill-source-git-snapshot.js'; + +it('keeps path visits bounded for a reference chain at the repository file limit', async () => { + let pathVisits = 0; + const paths = ['skills/main/SKILL.md', ...Array.from({ length: MAX_REPOSITORY_FILES - 1 }, (_, i) => `shared/${i}.md`)]; + const entries: GitSkillTreeEntry[] = paths.map((name, i) => ({ + get path() { pathVisits++; return name; }, + sha: i.toString(16).padStart(40, '0'), mode: '100644', type: 'blob', size: 100, + })); + const content = new Map(paths.map((name, i) => [name, Buffer.from(i === 0 ? '[Next](../../shared/0.md)' + : `[Next](${i === paths.length - 1 ? '0' : String(i)}.md)`)])); + const read = vi.fn(async (entry: GitSkillTreeEntry) => content.get(entry.path)!); + const prefetch = vi.fn(async () => {}); + const selected = await skillRepositoryDependencies(entries, [entries[0]!], new Set(['skills/main']), read, prefetch); + expect(selected).toEqual(entries); + expect(read).toHaveBeenCalledTimes(MAX_REPOSITORY_FILES); + expect(prefetch).toHaveBeenCalledTimes(MAX_REPOSITORY_FILES); + expect(pathVisits).toBeLessThan(MAX_REPOSITORY_FILES * 100); +}); diff --git a/server/src/__tests__/skill-source-git-snapshot.test.ts b/server/src/__tests__/skill-source-git-snapshot.test.ts index 0ec3599cc6e..8fd01f570a0 100644 --- a/server/src/__tests__/skill-source-git-snapshot.test.ts +++ b/server/src/__tests__/skill-source-git-snapshot.test.ts @@ -25,7 +25,7 @@ vi.mock('node:child_process', async importOriginal => { } }; }); import { clearGitSkillSnapshotCache, openGitSkillSnapshot, parseGitDownloadProgress } from '../services/skill-source-git-snapshot.js'; -import { scanGitHubSkills, type GitHubRead } from '../services/github-skill-source.js'; +import { scanGitHubSkills, scanGitHubSkillsWithPackageFallback, type GitHubRead } from '../services/github-skill-source.js'; const exec = promisify(execFile); let directory: string, commit: string, laterCommit: string; const git = (...args: string[]) => exec('git', ['-C', directory, '-c', 'user.name=Test', '-c', 'user.email=test@example.test', ...args]); @@ -51,6 +51,45 @@ afterEach(async () => { vi.useRealTimers(); interception.hang = false; intercept afterAll(async () => { await rm(directory, { recursive: true, force: true }); }); describe('Git skill snapshots', () => { + it('uses real Git batches for transitive dependencies in an oversized repository', async () => { + const remote = await mkdtemp(path.join(os.tmpdir(), 'paperclip-dependency-blobs-')); + interception.remote = remote; + try { + await exec('git', ['-C', remote, 'init', '-b', 'main']); + await mkdir(path.join(remote, 'unrelated')); + await mkdir(path.join(remote, 'shared')); + const files: Record = { + 'shared/guide.md': '[Details](details.md)', + 'shared/details.md': '[Guide](guide.md) [Data](data.json)', + 'shared/data.json': '{"value":true}', + }; + for (let i = 0; i < 64; i++) { + await mkdir(path.join(remote, `skills/skill-${i}`), { recursive: true }); + files[`skills/skill-${i}/SKILL.md`] = `---\nname: skill-${i}\ndescription: Dependency fixture\n---\n[Guide](../../shared/guide.md)`; + } + for (const [file, content] of Object.entries(files)) await writeFile(path.join(remote, file), content); + const unrelated = Buffer.alloc(1024 * 1024, 'x'); + for (let i = 0; i < 62; i++) await writeFile(path.join(remote, `unrelated/${i}.bin`), unrelated); + await exec('git', ['-C', remote, 'add', '.']); + await exec('git', ['-C', remote, '-c', 'user.name=Test', '-c', 'user.email=test@example.test', 'commit', '-m', 'Dependency fixture']); + const commitSha = (await exec('git', ['-C', remote, 'rev-parse', 'HEAD'])).stdout.trim(); + const read = (async (url: string) => url.includes('/commits/') ? { sha: commitSha } : { id: 42, full_name: 'acme/skills', default_branch: 'main' }) as GitHubRead; + read.openSnapshot = (input, options) => openGitSkillSnapshot({ ...request(), ref: input.ref, commitSha: input.commitSha }, options); + const scan = await scanGitHubSkillsWithPackageFallback({ repositoryUrl: 'https://github.com/acme/skills', packageMode: 'repository' }, read); + expect(scan.repositorySubset).toBe(true); + expect(scan.repositoryFiles?.map(file => file.path).sort()).toEqual(Object.keys(files).sort()); + for (const file of scan.repositoryFiles!) { + const content = file.encoding === 'base64' ? Buffer.from(file.content, 'base64') : Buffer.from(file.content); + expect(content.toString()).toBe(files[file.path]); + } + const reads = interception.commands.filter(command => command.args.includes('cat-file')); + expect(reads).toHaveLength(4); + expect(reads.every(command => command.args.includes('--batch'))).toBe(true); + } finally { + await rm(remote, { recursive: true, force: true }); + } + }, 30_000); + it('reads many unrelated support files with one Git batch per scan, including cache hits', async () => { const remote = await mkdtemp(path.join(os.tmpdir(), 'paperclip-many-blobs-')); interception.remote = remote; diff --git a/server/src/services/github-skill-source.ts b/server/src/services/github-skill-source.ts index 41644dd1ba7..f348affd27e 100644 --- a/server/src/services/github-skill-source.ts +++ b/server/src/services/github-skill-source.ts @@ -160,7 +160,11 @@ async function scanRepository(input: SkillScanInput, providerRead: GitHubRead, o if (dependencyBytes > MAX_REPOSITORY_BYTES) throw unprocessable('Repository package exceeds the 61 MB runtime limit.'); dependencyBlobs.set(entry.sha, bytes); return bytes; - }) : entries.filter(entry => entry.type !== 'tree'); + }, snapshot.readBlobs ? async pending => { + options.signal?.throwIfAborted(); + const unread = pending.filter(entry => !dependencyBlobs.has(entry.sha) && (entry.size ?? 0) <= MAX_FILE_BYTES); + if (unread.length) for (const [sha, bytes] of await snapshot.readBlobs!(unread, options.signal)) dependencyBlobs.set(sha, bytes); + } : undefined) : entries.filter(entry => entry.type !== 'tree'); if (repositoryInventory && repositoryInventory.length > MAX_REPOSITORY_FILES) throw unprocessable('Repository package exceeds the 9,990 file limit.'); const readable = repositoryInventory?.filter(entry => entry.type === 'blob' && ['100644', '100755'].includes(entry.mode) && (entry.size ?? 0) <= MAX_FILE_BYTES); if (readable && readable.reduce((sum, entry) => sum + (entry.size ?? 0), 0) > MAX_REPOSITORY_BYTES) throw unprocessable('Repository package exceeds the 61 MB runtime limit.'); diff --git a/server/src/services/skill-repository-dependencies.ts b/server/src/services/skill-repository-dependencies.ts index f77d296ae04..a4a58592ef0 100644 --- a/server/src/services/skill-repository-dependencies.ts +++ b/server/src/services/skill-repository-dependencies.ts @@ -10,10 +10,12 @@ export async function skillRepositoryDependencies( initial: GitSkillTreeEntry[], skillDirectories: ReadonlySet, readBlob: (entry: GitSkillTreeEntry) => Promise, + prefetch?: (entries: GitSkillTreeEntry[]) => Promise, ): Promise { const paths = indexSkillPackagePaths(entries.map(entry => entry.path)); const byPath = new Map(entries.map(entry => [entry.path, entry])); const selected = new Map(); + let pending: GitSkillTreeEntry[] = []; let bytes = 0; const add = (entry: GitSkillTreeEntry) => { if (entry.type === 'tree' || selected.has(entry.path)) return; @@ -21,26 +23,31 @@ export async function skillRepositoryDependencies( if (selected.size > MAX_REPOSITORY_FILES) throw unprocessable('Repository package exceeds the 9,990 file limit.'); if (entry.type === 'blob' && ['100644', '100755'].includes(entry.mode) && (entry.size ?? 0) <= 1024 * 1024) bytes += entry.size ?? 0; if (bytes > MAX_REPOSITORY_BYTES) throw unprocessable('Repository package exceeds the 61 MB runtime limit.'); + if (entry.type === 'blob' && ['100644', '100755'].includes(entry.mode) && /\.md$/i.test(entry.path)) pending.push(entry); }; for (const entry of initial) add(entry); const manifest = byPath.get('paperclip.skills.json'); if (manifest) add(manifest); - for (const entry of selected.values()) { - if (entry.type !== 'blob' || !['100644', '100755'].includes(entry.mode) || !/\.md$/i.test(entry.path)) continue; - const content = await readBlob(entry); - if (!content) continue; - let root = path.posix.dirname(entry.path); - while (root !== '.' && !skillDirectories.has(root)) root = path.posix.dirname(root); - const file = snapshotFile(path.posix.relative(root, entry.path), 'reference', content); - const inspection = inspectSkillPackage(path.posix.join(root, 'SKILL.md'), [file], paths, {}, [], new Set([entry.path])); - for (const reference of inspection.references) { - const target = reference.resolvedPath; - if (target === '..' || target.startsWith('../') || path.posix.isAbsolute(target)) continue; - const exact = byPath.get(target); - if (exact && exact.type !== 'tree') add(exact); - else if (paths.has(target)) { - const prefix = target === '.' ? '' : `${target}/`; - for (const child of entries) if (child.path.startsWith(prefix)) add(child); + while (pending.length) { + const frontier = pending; + pending = []; + await prefetch?.(frontier); + for (const entry of frontier) { + const content = await readBlob(entry); + if (!content) continue; + let root = path.posix.dirname(entry.path); + while (root !== '.' && !skillDirectories.has(root)) root = path.posix.dirname(root); + const file = snapshotFile(path.posix.relative(root, entry.path), 'reference', content); + const inspection = inspectSkillPackage(path.posix.join(root, 'SKILL.md'), [file], paths, {}, [], new Set([entry.path])); + for (const reference of inspection.references) { + const target = reference.resolvedPath; + if (target === '..' || target.startsWith('../') || path.posix.isAbsolute(target)) continue; + const exact = byPath.get(target); + if (exact && exact.type !== 'tree') add(exact); + else if (paths.has(target)) { + const prefix = target === '.' ? '' : `${target}/`; + for (const child of entries) if (child.path.startsWith(prefix)) add(child); + } } } }