From 8e055327e925dcc580e95a3790036f8942308db5 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Sat, 14 Feb 2026 21:08:08 +0100 Subject: [PATCH 1/4] [#92] feat: flexible KB source resolution - Refactor detectSourceType to require FileSystemService (no global fallback) - Extract KB validation utilities (validateKBStructure) to content-ops - Extract retryable-download (downloadWithRetry) to content-ops - Thread httpClient as required param through entire download chain - Remove all global service defaults from deep call sites (composition root only) - Deduplicate DownloadOptions: pair-cli uses Pick<> from content-ops - Share protocol detection helpers (isRemoteUrl, isUnsupportedProtocol) across parsers - Add resolveDatasetRoot for install/update command dataset resolution - Add source-resolution smoke tests + --offline-only flag for run-all.sh - Make registry symlinks relative for portability - MockHttpClientService gains persistent error flag for retry testing Co-Authored-By: Claude Opus 4.6 --- apps/pair-cli/src/cli.e2e.test.ts | 8 +- .../src/commands/install/handler.test.ts | 1 + .../src/commands/install/parser.test.ts | 12 + apps/pair-cli/src/commands/install/parser.ts | 10 +- .../src/commands/update/parser.test.ts | 8 + apps/pair-cli/src/commands/update/parser.ts | 10 +- apps/pair-cli/src/config/bootstrap.ts | 12 +- apps/pair-cli/src/config/cli.ts | 5 +- apps/pair-cli/src/config/kb-resolver.test.ts | 48 +++- apps/pair-cli/src/config/kb-resolver.ts | 42 ++- .../src/kb-manager/download-manager.ts | 52 ++-- .../src/kb-manager/kb-availability.test.ts | 11 +- .../src/kb-manager/kb-availability.ts | 15 +- .../src/kb-manager/kb-installer.test.ts | 3 +- apps/pair-cli/src/kb-manager/kb-installer.ts | 246 +++--------------- apps/pair-cli/src/registry/operations.ts | 6 +- packages/content-ops/src/file-system/index.ts | 1 + .../src/file-system/kb-validation.test.ts | 157 +++++++++++ .../src/file-system/kb-validation.ts | 141 ++++++++++ .../src/http/download-manager.test.ts | 22 +- .../content-ops/src/http/download-manager.ts | 12 +- packages/content-ops/src/http/index.ts | 2 + .../src/http/retryable-download.test.ts | 108 ++++++++ .../src/http/retryable-download.ts | 73 ++++++ packages/content-ops/src/index.ts | 17 +- .../path-resolution/source-detector.test.ts | 75 +++--- .../src/path-resolution/source-detector.ts | 28 +- .../src/test-utils/in-memory-fs.ts | 5 +- .../test-utils/mock-http-client-service.ts | 10 +- scripts/smoke-tests/run-all.sh | 39 ++- .../scenarios/source-resolution.sh | 87 +++++++ 31 files changed, 919 insertions(+), 347 deletions(-) create mode 100644 packages/content-ops/src/file-system/kb-validation.test.ts create mode 100644 packages/content-ops/src/file-system/kb-validation.ts create mode 100644 packages/content-ops/src/http/retryable-download.test.ts create mode 100644 packages/content-ops/src/http/retryable-download.ts create mode 100755 scripts/smoke-tests/scenarios/source-resolution.sh diff --git a/apps/pair-cli/src/cli.e2e.test.ts b/apps/pair-cli/src/cli.e2e.test.ts index d6ce19a7..acbfa60e 100644 --- a/apps/pair-cli/src/cli.e2e.test.ts +++ b/apps/pair-cli/src/cli.e2e.test.ts @@ -282,6 +282,8 @@ describe('pair-cli e2e - list-targets', () => { it('list-targets shows available registries', async () => { const cwd = '/test-project' const fs = createDevScenarioFs(cwd) + // Add KB marker so local source validation passes when source='.' + await fs.writeFile(cwd + '/AGENTS.md', 'this is agents.md') await withTempConfig(fs, createTestConfig(), async () => { // Mock the CLI execution by calling the update command with listTargets option @@ -708,8 +710,9 @@ describe('pair-cli e2e - error scenarios', () => { }), } const fs = new InMemoryFileSystemService(seed, cwd, cwd) - await handleUpdateCommand(parseUpdateCommand({ source: '/nonexistent/path' }), fs) - // Should fail gracefully when source doesn't exist + await expect( + handleUpdateCommand(parseUpdateCommand({ source: '/nonexistent/path' }), fs), + ).rejects.toThrow('KB source path not found') }) it('install from ZIP fails gracefully when ZIP is corrupted', async () => { @@ -845,6 +848,7 @@ describe('pair-cli e2e - disjoint installation (source and target disjoint)', () version: '1.0.0', }), // KB Source content in a disjoint directory + [`${kbSourceDir}/AGENTS.md`]: '# KB source marker', [`${kbSourceDir}/knowledge/index.md`]: '# Knowledge Index', [`${kbSourceDir}/knowledge/guide.md`]: 'Follow the [Index](./index.md)', } diff --git a/apps/pair-cli/src/commands/install/handler.test.ts b/apps/pair-cli/src/commands/install/handler.test.ts index 405e7143..5a198f9c 100644 --- a/apps/pair-cli/src/commands/install/handler.test.ts +++ b/apps/pair-cli/src/commands/install/handler.test.ts @@ -55,6 +55,7 @@ describe('handleInstallCommand - real services integration', () => { await fs.mkdir(externalKbPath, { recursive: true }) await fs.mkdir(`${externalKbPath}/my-reg`, { recursive: true }) await fs.writeFile(`${externalKbPath}/my-reg/file.txt`, 'local content') + await fs.writeFile(`${externalKbPath}/AGENTS.md`, '# KB marker') const localConfig = { asset_registries: { diff --git a/apps/pair-cli/src/commands/install/parser.test.ts b/apps/pair-cli/src/commands/install/parser.test.ts index f05e3070..4bfc3158 100644 --- a/apps/pair-cli/src/commands/install/parser.test.ts +++ b/apps/pair-cli/src/commands/install/parser.test.ts @@ -104,5 +104,17 @@ describe('parseInstallCommand', () => { parseInstallCommand({ source: '' }) }).toThrow('Source path/URL cannot be empty') }) + + it('throws on unsupported ftp:// protocol', () => { + expect(() => { + parseInstallCommand({ source: 'ftp://example.com/kb.zip' }) + }).toThrow('Unsupported source protocol') + }) + + it('throws on unsupported file:// protocol', () => { + expect(() => { + parseInstallCommand({ source: 'file:///tmp/kb.zip' }) + }).toThrow('Unsupported source protocol') + }) }) }) diff --git a/apps/pair-cli/src/commands/install/parser.ts b/apps/pair-cli/src/commands/install/parser.ts index f5847f31..8c1ac020 100644 --- a/apps/pair-cli/src/commands/install/parser.ts +++ b/apps/pair-cli/src/commands/install/parser.ts @@ -1,5 +1,5 @@ -import { detectSourceType, SourceType } from '@pair/content-ops' import { validateCommandOptions } from '#config/cli' +import { isRemoteUrl, isUnsupportedProtocol } from '@pair/content-ops' /** * Discriminated union for install command with default resolution @@ -83,9 +83,13 @@ export function parseInstallCommand( } } + // Reject unsupported protocols early + if (isUnsupportedProtocol(source)) { + throw new Error(`Unsupported source protocol: ${source}`) + } + // Remote source - const sourceType = detectSourceType(source) - if (sourceType === SourceType.REMOTE_URL) { + if (isRemoteUrl(source)) { return { command: 'install', resolution: 'remote', diff --git a/apps/pair-cli/src/commands/update/parser.test.ts b/apps/pair-cli/src/commands/update/parser.test.ts index ea34fcb5..75128725 100644 --- a/apps/pair-cli/src/commands/update/parser.test.ts +++ b/apps/pair-cli/src/commands/update/parser.test.ts @@ -63,4 +63,12 @@ describe('parseUpdateCommand', () => { }).toThrow('Offline mode requires explicit --source with local path') }) }) + + describe('validation', () => { + it('throws on unsupported ftp:// protocol', () => { + expect(() => { + parseUpdateCommand({ source: 'ftp://example.com/kb.zip' }) + }).toThrow('Unsupported source protocol') + }) + }) }) diff --git a/apps/pair-cli/src/commands/update/parser.ts b/apps/pair-cli/src/commands/update/parser.ts index d444d5de..32ce7b61 100644 --- a/apps/pair-cli/src/commands/update/parser.ts +++ b/apps/pair-cli/src/commands/update/parser.ts @@ -1,4 +1,4 @@ -import { detectSourceType, SourceType } from '@pair/content-ops' +import { isRemoteUrl, isUnsupportedProtocol } from '@pair/content-ops' import { validateCommandOptions } from '#config/cli' /** @@ -83,9 +83,13 @@ export function parseUpdateCommand( } } + // Reject unsupported protocols early + if (isUnsupportedProtocol(source)) { + throw new Error(`Unsupported source protocol: ${source}`) + } + // Remote source - const sourceType = detectSourceType(source) - if (sourceType === SourceType.REMOTE_URL) { + if (isRemoteUrl(source)) { return { command: 'update', resolution: 'remote', diff --git a/apps/pair-cli/src/config/bootstrap.ts b/apps/pair-cli/src/config/bootstrap.ts index a94b29a4..4950fcb4 100644 --- a/apps/pair-cli/src/config/bootstrap.ts +++ b/apps/pair-cli/src/config/bootstrap.ts @@ -1,10 +1,4 @@ -import { - FileSystemService, - HttpClientService, - validateUrl, - detectSourceType, - SourceType, -} from '@pair/content-ops' +import { FileSystemService, HttpClientService, isRemoteUrl, validateUrl } from '@pair/content-ops' import { getKnowledgeHubDatasetPath, getKnowledgeHubDatasetPathWithFallback } from './kb-resolver' import { validateCliOptions } from '#kb-manager/cli-options' import { isDiagEnabled } from '#diagnostics' @@ -99,7 +93,7 @@ function shouldSkipKBDownload( return true } - if (customUrl && detectSourceType(customUrl) !== SourceType.REMOTE_URL) { + if (customUrl && !isRemoteUrl(customUrl)) { if (isDiagEnabled()) console.error(`[diag] Using local path: ${customUrl}`) return true } @@ -125,7 +119,7 @@ function checkKnowledgeHubDatasetAccessible( fsService: FileSystemService, customUrl?: string, ): void { - if (customUrl && detectSourceType(customUrl) !== SourceType.REMOTE_URL) { + if (customUrl && !isRemoteUrl(customUrl)) { return } diff --git a/apps/pair-cli/src/config/cli.ts b/apps/pair-cli/src/config/cli.ts index 09c966ec..a69229d6 100644 --- a/apps/pair-cli/src/config/cli.ts +++ b/apps/pair-cli/src/config/cli.ts @@ -1,4 +1,4 @@ -import { detectSourceType, SourceType } from '@pair/content-ops' +import { isRemoteUrl } from '@pair/content-ops' /** * Common CLI options validation. @@ -18,8 +18,7 @@ export function validateCommandOptions( if (!source) { throw new Error('Offline mode requires explicit --source with local path') } - const sourceType = detectSourceType(source) - if (sourceType === SourceType.REMOTE_URL) { + if (isRemoteUrl(source)) { throw new Error('Cannot use --offline with remote URL source') } } diff --git a/apps/pair-cli/src/config/kb-resolver.test.ts b/apps/pair-cli/src/config/kb-resolver.test.ts index f22a4043..efe80546 100644 --- a/apps/pair-cli/src/config/kb-resolver.test.ts +++ b/apps/pair-cli/src/config/kb-resolver.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from 'vitest' -import { InMemoryFileSystemService } from '@pair/content-ops' +import { InMemoryFileSystemService, MockHttpClientService } from '@pair/content-ops' import { getKnowledgeHubDatasetPath, getKnowledgeHubDatasetPathWithFallback, @@ -35,6 +35,7 @@ describe('kb-resolver', () => { const result = await getKnowledgeHubDatasetPathWithFallback({ fsService: fs, + httpClient: new MockHttpClientService(), version: '1.0.0', }) expect(result).toBe(`${cwd}/packages/knowledge-hub/dataset`) @@ -46,6 +47,7 @@ describe('kb-resolver', () => { const result = await getKnowledgeHubDatasetPathWithFallback({ fsService: fs, + httpClient: new MockHttpClientService(), version: '1.0.0', ensureKBAvailableFn: mockEnsure, isKBCachedFn: async () => false, @@ -87,13 +89,16 @@ describe('resolveDatasetRoot', () => { const result = await resolveDatasetRoot( fs, { resolution: 'remote', url: 'https://example.com/kb.zip' }, - { cliVersion: '1.0.0' }, + { cliVersion: '1.0.0', httpClient: new MockHttpClientService() }, ) expect(result).toBe(`${cwd}/packages/knowledge-hub/dataset`) }) - it('local resolution returns path directly for directories', async () => { + it('local resolution returns resolved absolute path for valid directory', async () => { const fs = new InMemoryFileSystemService({}, cwd, cwd) + // Create a valid KB structure at the target path + fs.mkdirSync('/external/kb-dataset') + await fs.writeFile('/external/kb-dataset/AGENTS.md', '# Agents') const result = await resolveDatasetRoot(fs, { resolution: 'local', @@ -102,6 +107,43 @@ describe('resolveDatasetRoot', () => { expect(result).toBe('/external/kb-dataset') }) + it('local resolution resolves relative path to absolute', async () => { + const fs = new InMemoryFileSystemService({}, cwd, cwd) + fs.mkdirSync(`${cwd}/my-kb`) + await fs.writeFile(`${cwd}/my-kb/AGENTS.md`, '# Agents') + + const result = await resolveDatasetRoot(fs, { + resolution: 'local', + path: 'my-kb', + }) + expect(result).toBe(`${cwd}/my-kb`) + }) + + it('local resolution throws for non-existent path', async () => { + const fs = new InMemoryFileSystemService({}, cwd, cwd) + + await expect( + resolveDatasetRoot(fs, { + resolution: 'local', + path: '/nonexistent/kb', + }), + ).rejects.toThrow('KB source path not found') + }) + + it('local resolution throws for invalid KB structure', async () => { + const fs = new InMemoryFileSystemService({}, cwd, cwd) + // Create a directory with no KB markers + fs.mkdirSync('/empty-dir') + await fs.writeFile('/empty-dir/random.txt', 'not a KB') + + await expect( + resolveDatasetRoot(fs, { + resolution: 'local', + path: '/empty-dir', + }), + ).rejects.toThrow('Invalid KB structure') + }) + it('local resolution with .zip calls installKBFromLocalZip', async () => { const kbInstaller = await import('#kb-manager/kb-installer') vi.spyOn(kbInstaller, 'installKBFromLocalZip').mockResolvedValue('/cached/unzipped') diff --git a/apps/pair-cli/src/config/kb-resolver.ts b/apps/pair-cli/src/config/kb-resolver.ts index d16c07a6..b4dce9bd 100644 --- a/apps/pair-cli/src/config/kb-resolver.ts +++ b/apps/pair-cli/src/config/kb-resolver.ts @@ -1,5 +1,5 @@ -import { join, dirname } from 'path' -import { FileSystemService, HttpClientService, NodeHttpClientService } from '@pair/content-ops' +import { join, dirname, resolve } from 'path' +import { FileSystemService, HttpClientService, validateKBStructure } from '@pair/content-ops' import { isInRelease, findNpmReleasePackage, @@ -100,7 +100,7 @@ async function downloadKBIfNeeded(options: { */ export async function getKnowledgeHubDatasetPathWithFallback(options: { fsService: FileSystemService - httpClient?: HttpClientService + httpClient: HttpClientService version: string customUrl?: string isKBCachedFn?: typeof isKBCached @@ -108,7 +108,7 @@ export async function getKnowledgeHubDatasetPathWithFallback(options: { }): Promise { const { fsService, - httpClient = new NodeHttpClientService(), + httpClient, version, customUrl, isKBCachedFn = isKBCached, @@ -140,6 +140,27 @@ export type DatasetResolvableConfig = export interface DatasetResolveOptions { cliVersion?: string | undefined httpClient?: HttpClientService | undefined + progressWriter?: { write(s: string): void } | undefined + isTTY?: boolean | undefined +} + +async function resolveLocalDataset( + fs: FileSystemService, + path: string, + version: string, +): Promise { + const resolved = resolve(fs.currentWorkingDirectory(), path) + if (path.endsWith('.zip')) { + return installKBFromLocalZip(version, resolved, fs) + } + if (!fs.existsSync(resolved)) { + throw new Error(`KB source path not found: ${resolved}`) + } + const valid = await validateKBStructure(resolved, fs) + if (!valid) { + throw new Error(`Invalid KB structure at: ${resolved}`) + } + return resolved } /** @@ -157,18 +178,19 @@ export async function resolveDatasetRoot( case 'default': return getKnowledgeHubDatasetPath(fs) - case 'remote': + case 'remote': { + if (!options?.httpClient) { + throw new Error('Remote resolution requires httpClient') + } return getKnowledgeHubDatasetPathWithFallback({ fsService: fs, + httpClient: options.httpClient, version, - ...(options?.httpClient && { httpClient: options.httpClient }), customUrl: config.url, }) + } case 'local': - if (config.path.endsWith('.zip')) { - return installKBFromLocalZip(version, config.path, fs) - } - return config.path + return resolveLocalDataset(fs, config.path, version) } } diff --git a/apps/pair-cli/src/kb-manager/download-manager.ts b/apps/pair-cli/src/kb-manager/download-manager.ts index 5c89544d..733498ab 100644 --- a/apps/pair-cli/src/kb-manager/download-manager.ts +++ b/apps/pair-cli/src/kb-manager/download-manager.ts @@ -1,13 +1,10 @@ import { downloadFile as genericDownloadFile, - type DownloadOptions as GenericDownloadOptions, + downloadWithRetry as genericDownloadWithRetry, + type DownloadOptions, type DownloadErrorHandler, - type ProgressWriter, - type HttpClientService, - NodeHttpClientService, + type RetryOptions, } from '@pair/content-ops' -import type { FileSystemService } from '@pair/content-ops' -import { fileSystemService } from '@pair/content-ops' /** * KB-specific error handler with helpful messages for knowledge base downloads @@ -37,12 +34,11 @@ const kbErrorHandler: DownloadErrorHandler = { }, } -interface DownloadOptions { - httpClient?: HttpClientService | undefined - fs?: FileSystemService | undefined - progressWriter?: ProgressWriter | undefined - isTTY?: boolean | undefined -} +/** Subset of DownloadOptions accepted by KB download wrappers (errorHandler + label are set internally) */ +export type KBDownloadOptions = Pick< + DownloadOptions, + 'httpClient' | 'fs' | 'progressWriter' | 'isTTY' +> /** * Download KB file with KB-specific error messages @@ -51,20 +47,32 @@ interface DownloadOptions { export async function downloadFile( url: string, destination: string, - options: DownloadOptions = {}, + options: KBDownloadOptions, ): Promise { - const httpClient = options.httpClient || new NodeHttpClientService() - const fs = options.fs || fileSystemService - const { progressWriter, isTTY } = options - - const genericOptions: GenericDownloadOptions = { - httpClient, - fs, - progressWriter, - isTTY, + const genericOptions: DownloadOptions = { + ...options, label: 'Downloading KB', errorHandler: kbErrorHandler, } return genericDownloadFile(url, destination, genericOptions) } + +/** + * Download KB file with retry on transient network errors. + * Uses KB-specific error handler and retries up to 3 times with exponential backoff. + */ +export async function downloadWithRetry( + url: string, + destination: string, + options: KBDownloadOptions, + retryOptions: RetryOptions = {}, +): Promise { + const genericOptions: DownloadOptions = { + ...options, + label: 'Downloading KB', + errorHandler: kbErrorHandler, + } + + return genericDownloadWithRetry(url, destination, genericOptions, retryOptions) +} diff --git a/apps/pair-cli/src/kb-manager/kb-availability.test.ts b/apps/pair-cli/src/kb-manager/kb-availability.test.ts index b4429a93..763ea06a 100644 --- a/apps/pair-cli/src/kb-manager/kb-availability.test.ts +++ b/apps/pair-cli/src/kb-manager/kb-availability.test.ts @@ -180,9 +180,14 @@ describe('KB Manager - Network failure', () => { const httpClient = new MockHttpClientService() httpClient.setRequestResponses([headResponse]) - httpClient.setGetError(new Error('ENOTFOUND: network unreachable')) + httpClient.setGetError(new Error('ENOTFOUND: network unreachable'), true) await expect( - ensureKBAvailable(testVersion, { httpClient, fs, extract: mockExtract }), + ensureKBAvailable(testVersion, { + httpClient, + fs, + extract: mockExtract, + retryOptions: { maxRetries: 0 }, + }), ).rejects.toThrow(/network/) }) }) @@ -363,6 +368,7 @@ describe('KB manager integration - ensure KB available', () => { const result = await import('#config').then(m => m.getKnowledgeHubDatasetPathWithFallback({ fsService: mockFs as unknown as FileSystemService, + httpClient: new MockHttpClientService(), version: '0.1.0', isKBCachedFn: mockIsKBCached, ensureKBAvailableFn: mockEnsureKBAvailable, @@ -391,6 +397,7 @@ describe('KB manager integration - custom URL', () => { const result = await import('#config').then(m => m.getKnowledgeHubDatasetPathWithFallback({ fsService: mockFs as unknown as FileSystemService, + httpClient: new MockHttpClientService(), version: '0.1.0', isKBCachedFn: mockIsKBCached, ensureKBAvailableFn: mockEnsureKBAvailable, diff --git a/apps/pair-cli/src/kb-manager/kb-availability.ts b/apps/pair-cli/src/kb-manager/kb-availability.ts index e08b476c..6887b5c1 100644 --- a/apps/pair-cli/src/kb-manager/kb-availability.ts +++ b/apps/pair-cli/src/kb-manager/kb-availability.ts @@ -1,5 +1,5 @@ -import type { FileSystemService, HttpClientService } from '@pair/content-ops' -import { fileSystemService, detectSourceType, SourceType } from '@pair/content-ops' +import type { FileSystemService, HttpClientService, RetryOptions } from '@pair/content-ops' +import { detectSourceType, SourceType } from '@pair/content-ops' import { join } from 'path' import { type ProgressWriter } from '@pair/content-ops/http' import cacheManager from './cache-manager' @@ -18,6 +18,7 @@ export interface KBManagerDeps { progressWriter?: ProgressWriter isTTY?: boolean customUrl?: string + retryOptions?: RetryOptions } // Internal: use cacheManager directly. Public re-exports live in the kb-manager index. @@ -36,7 +37,7 @@ function buildInstallerDeps(deps: KBManagerDeps): InstallerDeps { } export async function ensureKBAvailable(version: string, deps: KBManagerDeps): Promise { - const fs = deps.fs || fileSystemService + const fs = deps.fs const cachePath = getCachedKBPath(version) const cached = await isKBCached(version, fs) @@ -53,7 +54,7 @@ export async function ensureKBAvailable(version: string, deps: KBManagerDeps): P const installerDeps = buildInstallerDeps(deps) // Check if source is a local path instead of a remote URL - const sourceType = detectSourceType(sourceUrl) + const sourceType = detectSourceType(sourceUrl, fs) if (sourceType !== SourceType.REMOTE_URL) { if (sourceUrl.endsWith('.zip')) { // Local ZIP file @@ -65,5 +66,9 @@ export async function ensureKBAvailable(version: string, deps: KBManagerDeps): P } // Remote URL - use standard download - return installKB(version, cachePath, sourceUrl, { fs, ...installerDeps }) + return installKB(version, cachePath, sourceUrl, { + fs, + ...installerDeps, + ...(deps.retryOptions ? { retryOptions: deps.retryOptions } : {}), + }) } diff --git a/apps/pair-cli/src/kb-manager/kb-installer.test.ts b/apps/pair-cli/src/kb-manager/kb-installer.test.ts index 413e18a2..f66c3de4 100644 --- a/apps/pair-cli/src/kb-manager/kb-installer.test.ts +++ b/apps/pair-cli/src/kb-manager/kb-installer.test.ts @@ -349,7 +349,8 @@ describe('KB Installer - installKBFromLocalDirectory', () => { // Arrange const version = '0.2.0' const dirPath = './relative/kb' - const resolvedDirPath = join(process.cwd(), 'relative', 'kb') + // fs.currentWorkingDirectory() returns '/' so resolve('/', './relative/kb') = '/relative/kb' + const resolvedDirPath = '/relative/kb' const expectedCachePath = join(homedir(), '.pair', 'kb', version) const expectedDatasetRoot = join(expectedCachePath, '.pair') const fs = new InMemoryFileSystemService( diff --git a/apps/pair-cli/src/kb-manager/kb-installer.ts b/apps/pair-cli/src/kb-manager/kb-installer.ts index 7e8717f8..26c369ca 100644 --- a/apps/pair-cli/src/kb-manager/kb-installer.ts +++ b/apps/pair-cli/src/kb-manager/kb-installer.ts @@ -1,8 +1,14 @@ import { join } from 'path' import { tmpdir } from 'os' -import type { FileSystemService, HttpClientService } from '@pair/content-ops' -import { extractZip, cleanupFile, NodeHttpClientService } from '@pair/content-ops' -import { downloadFile } from './download-manager' +import type { FileSystemService, HttpClientService, RetryOptions } from '@pair/content-ops' +import { + extractZip, + cleanupFile, + validateKBStructure, + normalizeExtractedKB, + copyDirectoryContents, +} from '@pair/content-ops' +import { downloadWithRetry } from './download-manager' import cacheManager from './cache-manager' import checksumManager from './checksum-manager' import formatDownloadError from './error-formatter' @@ -21,27 +27,27 @@ async function doInstallSteps( cachePath: string, options: { fs: FileSystemService - httpClient?: HttpClientService + httpClient: HttpClientService progressWriter?: { write(s: string): void } isTTY?: boolean extract?: (zipPath: string, targetPath: string) => Promise + retryOptions?: RetryOptions }, ): Promise { - const { - fs, - httpClient = new NodeHttpClientService(), - progressWriter, - isTTY, - extract: customExtract, - } = options + const { fs, httpClient, progressWriter, isTTY, extract: customExtract, retryOptions } = options const extract = customExtract || extractZip - await downloadFile(downloadUrl, zipPath, { - httpClient, - fs, - progressWriter, - isTTY, - }) + await downloadWithRetry( + downloadUrl, + zipPath, + { + httpClient, + fs, + progressWriter, + isTTY, + }, + retryOptions, + ) const check = await checksumManager.validateFileWithRemoteChecksum( downloadUrl, @@ -73,44 +79,20 @@ function shouldPreserveError(err: Error): boolean { ) } -function buildInstallOptions(options: { - fs: FileSystemService - httpClient?: HttpClientService - progressWriter?: { write(s: string): void } - isTTY?: boolean - extract?: (zipPath: string, targetPath: string) => Promise -}): { +interface InstallOptions { fs: FileSystemService - httpClient?: HttpClientService + httpClient: HttpClientService progressWriter?: { write(s: string): void } isTTY?: boolean extract?: (zipPath: string, targetPath: string) => Promise -} { - const result: { - fs: FileSystemService - httpClient?: HttpClientService - progressWriter?: { write(s: string): void } - isTTY?: boolean - extract?: (zipPath: string, targetPath: string) => Promise - } = { fs: options.fs } - if (options.httpClient) result.httpClient = options.httpClient - if (options.progressWriter) result.progressWriter = options.progressWriter - if (options.isTTY) result.isTTY = options.isTTY - if (options.extract) result.extract = options.extract - return result + retryOptions?: RetryOptions } export async function installKB( version: string, cachePath: string, downloadUrl: string, - options: { - fs: FileSystemService - httpClient?: HttpClientService - progressWriter?: { write(s: string): void } - isTTY?: boolean - extract?: (zipPath: string, targetPath: string) => Promise - }, + options: InstallOptions, ): Promise { const cleanVersion = version.startsWith('v') ? version.slice(1) : version const zipPath = join(tmpdir(), `kb-${cleanVersion}.zip`) @@ -121,8 +103,7 @@ export async function installKB( await cacheManager.ensureCacheDirectory(cachePath, fs) try { - const installOptions = buildInstallOptions(options) - await doInstallSteps(downloadUrl, zipPath, cachePath, installOptions) + await doInstallSteps(downloadUrl, zipPath, cachePath, options) announceSuccess(cleanVersion, cachePath) // Prefer to return the dataset root when .pair exists inside cache so callers // resolve registries like 'knowledge' correctly (datasetRoot/knowledge) @@ -145,162 +126,6 @@ export async function installKB( } } -async function copyDirectoryInMemory( - fs: FileSystemService, - sourceDir: string, - targetDir: string, -): Promise { - // Ensure target directory exists - await fs.mkdir(targetDir, { recursive: true }) - - // Read all entries in the source directory - const entries = await fs.readdir(sourceDir) - - // Process each entry - for (const entry of entries) { - const sourcePath = join(sourceDir, entry.name) - const targetPath = join(targetDir, entry.name) - - if (entry.isDirectory()) { - // Recursively copy subdirectory - await copyDirectoryInMemory(fs, sourcePath, targetPath) - } else { - // Copy file - const content = await fs.readFile(sourcePath) - await fs.writeFile(targetPath, content) - } - } -} - -async function validateKBStructure(cachePath: string, fs: FileSystemService): Promise { - // Check for .pair directory or AGENTS.md at the root of cachePath - const hasPairDir = fs.existsSync(join(cachePath, '.pair')) - const hasAgentsMd = fs.existsSync(join(cachePath, 'AGENTS.md')) - const hasManifestJson = fs.existsSync(join(cachePath, 'manifest.json')) - - // If we have a .pair dir or AGENTS.md it's valid - if (hasPairDir || hasAgentsMd) return true - - // If there's a manifest.json, ensure there are other files present too (manifest alone is not enough) - if (hasManifestJson) { - try { - const entries = await fs.readdir(cachePath) - // If there's any non-hidden entry other than manifest.json, consider it valid - return entries.some(e => e.name !== 'manifest.json' && !e.name.startsWith('.')) - } catch { - return false - } - } - - return false -} - -/** - * Checks if a directory contains a single subdirectory (common in ZIP extractions) - * and returns that subdirectory path if it contains valid KB structure - */ -async function findKBStructureInSubdirectories( - cachePath: string, - fs: FileSystemService, -): Promise { - try { - const entries = await fs.readdir(cachePath) - // Filter out files and hidden directories - const dirs = entries.filter(e => e.isDirectory() && !e.name.startsWith('.')) - - // If there's exactly one directory, check if it has KB structure - if (dirs.length === 1) { - const subDir = join(cachePath, dirs[0]!.name) - if (await validateKBStructure(subDir, fs)) { - return subDir - } - } - - // Check if there's a .zip-temp directory (created by package command) - const zipTempDir = entries.find(e => e.isDirectory() && e.name === '.zip-temp') - if (zipTempDir) { - const subDir = join(cachePath, zipTempDir.name) - if (await validateKBStructure(subDir, fs)) { - return subDir - } - } - } catch { - // If we can't read the directory, return null - } - return null -} - -/** - * Moves all contents from source directory to target directory - */ -async function moveDirectoryContents( - sourceDir: string, - targetDir: string, - fs: FileSystemService, -): Promise { - const entries = await fs.readdir(sourceDir) - - for (const entry of entries) { - const sourcePath = join(sourceDir, entry.name) - const targetPath = join(targetDir, entry.name) - - // Move the entry (copy then delete for cross-device safety) - if (entry.isDirectory()) { - await fs.mkdir(targetPath, { recursive: true }) - await moveDirectoryContents(sourcePath, targetPath, fs) - await fs.rm(sourcePath, { recursive: true, force: true }) - } else { - const content = await fs.readFile(sourcePath) - await fs.writeFile(targetPath, content) - await fs.unlink(sourcePath) - } - } -} - -// Debug helper: log directory entries when running tests or debug mode -async function logDirEntriesIfDebug( - pathToLog: string, - fsService: FileSystemService, - label: string, -) { - try { - if (process.env['NODE_ENV'] === 'test' || process.env['DEBUG'] || process.env['PAIR_DIAG']) { - const entries = await fsService.readdir(pathToLog) - const names = entries.map(e => (e && e.name ? e.name : String(e))) - - // Use unified logger for debug output - const { logger } = await import('@pair/content-ops') - logger.debug(`[debug] ${label} ${pathToLog}`, names) - } - } catch { - // ignore logging failures - } -} - -/** - * Normalize the extracted cache path to ensure KB structure exists at the root. - * Returns true if the cachePath contains a valid KB after normalization. - */ -async function normalizeExtractedKB(cachePath: string, fs: FileSystemService): Promise { - // Initial validation - let kbStructureValid = await validateKBStructure(cachePath, fs) - if (kbStructureValid) return true - - await logDirEntriesIfDebug(cachePath, fs, 'normalizeExtractedKB entries:') - - // If not valid at root, try to find a single subdirectory that contains the KB - const subDirWithKB = await findKBStructureInSubdirectories(cachePath, fs) - if (subDirWithKB) { - await moveDirectoryContents(subDirWithKB, cachePath, fs) - await fs.rm(subDirWithKB, { recursive: true, force: true }) - return true - } - - // Final attempt: re-validate (covers manifest + other files case) - kbStructureValid = await validateKBStructure(cachePath, fs) - return kbStructureValid -} - export async function installKBFromLocalDirectory( version: string, dirPath: string, @@ -308,8 +133,10 @@ export async function installKBFromLocalDirectory( ): Promise { const cachePath = cacheManager.getCachedKBPath(version) - // Resolve relative paths - const resolvedDirPath = dirPath.startsWith('/') ? dirPath : join(process.cwd(), dirPath) + // Resolve relative paths using injectable cwd + const resolvedDirPath = dirPath.startsWith('/') + ? dirPath + : join(fs.currentWorkingDirectory(), dirPath) // Validate directory exists if (!fs.existsSync(resolvedDirPath)) { @@ -319,9 +146,7 @@ export async function installKBFromLocalDirectory( await cacheManager.ensureCacheDirectory(cachePath, fs) try { - // Copy directory contents recursively - // For in-memory filesystem, implement manual copy - await copyDirectoryInMemory(fs, resolvedDirPath, cachePath) + await copyDirectoryContents(fs, resolvedDirPath, cachePath) // Validate KB structure const kbStructureValid = await validateKBStructure(cachePath, fs) @@ -363,12 +188,7 @@ async function finalizeZipInstall( cachePath: string, fs: FileSystemService, ): Promise { - await logDirEntriesIfDebug(cachePath, fs, 'after extract entries:') const ok = await normalizeExtractedKB(cachePath, fs) - await logDirEntriesIfDebug(cachePath, fs, 'final entries at') - if (await fs.existsSync(join(cachePath, '.pair'))) { - await logDirEntriesIfDebug(join(cachePath, '.pair'), fs, '.pair entries:') - } if (!ok) throw new Error('Invalid KB structure') announceSuccess(version, cachePath) diff --git a/apps/pair-cli/src/registry/operations.ts b/apps/pair-cli/src/registry/operations.ts index e23a050e..cf7e177b 100644 --- a/apps/pair-cli/src/registry/operations.ts +++ b/apps/pair-cli/src/registry/operations.ts @@ -12,7 +12,7 @@ import { } from '@pair/content-ops' import { type SyncOptions, defaultSyncOptions } from '@pair/content-ops' import type { RegistryConfig } from './resolver' -import { isAbsolute, dirname } from 'path' +import { isAbsolute, dirname, relative } from 'path' /** * Performs the actual copy/mirror operation for a registry. @@ -271,5 +271,7 @@ async function createOrReplaceSymlink( if (fileService.existsSync(linkPath)) { await fileService.unlink(linkPath) } - await fileService.symlink(target, linkPath) + // Use relative path so symlinks are portable across machines + const relTarget = relative(dirname(linkPath), target) + await fileService.symlink(relTarget, linkPath) } diff --git a/packages/content-ops/src/file-system/index.ts b/packages/content-ops/src/file-system/index.ts index b172b4e8..3025d514 100644 --- a/packages/content-ops/src/file-system/index.ts +++ b/packages/content-ops/src/file-system/index.ts @@ -5,3 +5,4 @@ export * from './file-operations' export * from './integrity-validator' export * from './url-validator' export * from './archive-operations' +export * from './kb-validation' diff --git a/packages/content-ops/src/file-system/kb-validation.test.ts b/packages/content-ops/src/file-system/kb-validation.test.ts new file mode 100644 index 00000000..f967d134 --- /dev/null +++ b/packages/content-ops/src/file-system/kb-validation.test.ts @@ -0,0 +1,157 @@ +import { describe, it, expect } from 'vitest' +import { InMemoryFileSystemService } from '../test-utils/in-memory-fs' +import { + validateKBStructure, + findKBStructureInSubdirectories, + moveDirectoryContents, + copyDirectoryContents, + normalizeExtractedKB, +} from './kb-validation' + +describe('kb-validation', () => { + const cwd = '/test' + + function createFs(files: Record = {}) { + return new InMemoryFileSystemService(files, cwd, cwd) + } + + describe('validateKBStructure', () => { + it('returns true when .pair directory exists', async () => { + const fs = createFs({ '/cache/.pair/config.md': 'content' }) + expect(await validateKBStructure('/cache', fs)).toBe(true) + }) + + it('returns true when AGENTS.md exists', async () => { + const fs = createFs({ '/cache/AGENTS.md': '# Agents' }) + expect(await validateKBStructure('/cache', fs)).toBe(true) + }) + + it('returns true when manifest.json + other files exist', async () => { + const fs = createFs({ + '/cache/manifest.json': '{}', + '/cache/README.md': '# KB', + }) + expect(await validateKBStructure('/cache', fs)).toBe(true) + }) + + it('returns false when manifest.json is alone', async () => { + const fs = createFs({ '/cache/manifest.json': '{}' }) + expect(await validateKBStructure('/cache', fs)).toBe(false) + }) + + it('returns false for empty directory', async () => { + const fs = createFs() + await fs.mkdir('/cache', { recursive: true }) + expect(await validateKBStructure('/cache', fs)).toBe(false) + }) + + it('returns false for non-existent directory', async () => { + const fs = createFs() + expect(await validateKBStructure('/nope', fs)).toBe(false) + }) + }) + + describe('findKBStructureInSubdirectories', () => { + it('returns subdirectory path when single subdir has KB', async () => { + const fs = createFs({ '/cache/kb-v1/.pair/config.md': 'c' }) + const result = await findKBStructureInSubdirectories('/cache', fs) + expect(result).toBe('/cache/kb-v1') + }) + + it('returns null when multiple subdirectories exist', async () => { + const fs = createFs({ + '/cache/dir1/.pair/c.md': 'c', + '/cache/dir2/.pair/c.md': 'c', + }) + const result = await findKBStructureInSubdirectories('/cache', fs) + expect(result).toBeNull() + }) + + it('finds KB in .zip-temp subdirectory', async () => { + const fs = createFs({ '/cache/.zip-temp/AGENTS.md': '# Agents' }) + const result = await findKBStructureInSubdirectories('/cache', fs) + expect(result).toBe('/cache/.zip-temp') + }) + + it('returns null for empty directory', async () => { + const fs = createFs() + await fs.mkdir('/cache', { recursive: true }) + const result = await findKBStructureInSubdirectories('/cache', fs) + expect(result).toBeNull() + }) + }) + + describe('moveDirectoryContents', () => { + it('moves files from source to target', async () => { + const fs = createFs({ + '/src/a.md': 'file-a', + '/src/b.md': 'file-b', + }) + await fs.mkdir('/dst', { recursive: true }) + + await moveDirectoryContents('/src', '/dst', fs) + + expect(await fs.readFile('/dst/a.md')).toBe('file-a') + expect(await fs.readFile('/dst/b.md')).toBe('file-b') + expect(fs.existsSync('/src/a.md')).toBe(false) + }) + + it('moves nested directories recursively', async () => { + const fs = createFs({ + '/src/sub/deep.md': 'deep content', + }) + await fs.mkdir('/dst', { recursive: true }) + + await moveDirectoryContents('/src', '/dst', fs) + + expect(await fs.readFile('/dst/sub/deep.md')).toBe('deep content') + }) + }) + + describe('copyDirectoryContents', () => { + it('copies files from source to target', async () => { + const fs = createFs({ + '/src/a.md': 'file-a', + '/src/b.md': 'file-b', + }) + + await copyDirectoryContents(fs, '/src', '/dst') + + expect(await fs.readFile('/dst/a.md')).toBe('file-a') + expect(await fs.readFile('/dst/b.md')).toBe('file-b') + // Source still exists + expect(await fs.readFile('/src/a.md')).toBe('file-a') + }) + + it('copies nested directories recursively', async () => { + const fs = createFs({ + '/src/sub/deep.md': 'deep', + }) + + await copyDirectoryContents(fs, '/src', '/dst') + + expect(await fs.readFile('/dst/sub/deep.md')).toBe('deep') + }) + }) + + describe('normalizeExtractedKB', () => { + it('returns true when KB is already at root', async () => { + const fs = createFs({ '/cache/.pair/c.md': 'c' }) + expect(await normalizeExtractedKB('/cache', fs)).toBe(true) + }) + + it('moves nested KB to root and returns true', async () => { + const fs = createFs({ '/cache/nested/.pair/c.md': 'c' }) + + const result = await normalizeExtractedKB('/cache', fs) + + expect(result).toBe(true) + expect(fs.existsSync('/cache/.pair/c.md')).toBe(true) + }) + + it('returns false when no KB structure found', async () => { + const fs = createFs({ '/cache/random.txt': 'data' }) + expect(await normalizeExtractedKB('/cache', fs)).toBe(false) + }) + }) +}) diff --git a/packages/content-ops/src/file-system/kb-validation.ts b/packages/content-ops/src/file-system/kb-validation.ts new file mode 100644 index 00000000..68060e7e --- /dev/null +++ b/packages/content-ops/src/file-system/kb-validation.ts @@ -0,0 +1,141 @@ +/** + * KB structure validation and normalization utilities. + * Validates that a directory contains a valid Knowledge Base structure + * and normalizes extracted archives to ensure KB is at the root level. + */ + +import { join } from 'path' +import type { FileSystemService } from './file-system-service' + +/** + * Validates that a directory contains a valid KB structure. + * A valid KB has a .pair directory, an AGENTS.md file, or a manifest.json with other files. + */ +export async function validateKBStructure( + cachePath: string, + fs: FileSystemService, +): Promise { + const hasPairDir = fs.existsSync(join(cachePath, '.pair')) + const hasAgentsMd = fs.existsSync(join(cachePath, 'AGENTS.md')) + const hasManifestJson = fs.existsSync(join(cachePath, 'manifest.json')) + + if (hasPairDir || hasAgentsMd) return true + + if (hasManifestJson) { + try { + const entries = await fs.readdir(cachePath) + return entries.some(e => e.name !== 'manifest.json' && !e.name.startsWith('.')) + } catch { + return false + } + } + + return false +} + +/** + * Checks if a directory contains a single subdirectory with valid KB structure. + * Common in ZIP extractions where content is nested inside a single folder. + */ +export async function findKBStructureInSubdirectories( + cachePath: string, + fs: FileSystemService, +): Promise { + try { + const entries = await fs.readdir(cachePath) + const dirs = entries.filter(e => e.isDirectory() && !e.name.startsWith('.')) + + if (dirs.length === 1) { + const subDir = join(cachePath, dirs[0]!.name) + if (await validateKBStructure(subDir, fs)) { + return subDir + } + } + + const zipTempDir = entries.find(e => e.isDirectory() && e.name === '.zip-temp') + if (zipTempDir) { + const subDir = join(cachePath, zipTempDir.name) + if (await validateKBStructure(subDir, fs)) { + return subDir + } + } + } catch { + // If we can't read the directory, return null + } + return null +} + +/** + * Moves all contents from source directory to target directory. + * Uses copy-then-delete for cross-device safety. + */ +export async function moveDirectoryContents( + sourceDir: string, + targetDir: string, + fs: FileSystemService, +): Promise { + const entries = await fs.readdir(sourceDir) + + for (const entry of entries) { + const sourcePath = join(sourceDir, entry.name) + const targetPath = join(targetDir, entry.name) + + if (entry.isDirectory()) { + await fs.mkdir(targetPath, { recursive: true }) + await moveDirectoryContents(sourcePath, targetPath, fs) + await fs.rm(sourcePath, { recursive: true, force: true }) + } else { + const content = await fs.readFile(sourcePath) + await fs.writeFile(targetPath, content) + await fs.unlink(sourcePath) + } + } +} + +/** + * Copies all contents from source directory to target directory recursively. + */ +export async function copyDirectoryContents( + fs: FileSystemService, + sourceDir: string, + targetDir: string, +): Promise { + await fs.mkdir(targetDir, { recursive: true }) + + const entries = await fs.readdir(sourceDir) + + for (const entry of entries) { + const sourcePath = join(sourceDir, entry.name) + const targetPath = join(targetDir, entry.name) + + if (entry.isDirectory()) { + await copyDirectoryContents(fs, sourcePath, targetPath) + } else { + const content = await fs.readFile(sourcePath) + await fs.writeFile(targetPath, content) + } + } +} + +/** + * Normalize the extracted cache path to ensure KB structure exists at the root. + * If KB content is nested inside a single subdirectory, moves it up. + * Returns true if the cachePath contains a valid KB after normalization. + */ +export async function normalizeExtractedKB( + cachePath: string, + fs: FileSystemService, +): Promise { + let kbStructureValid = await validateKBStructure(cachePath, fs) + if (kbStructureValid) return true + + const subDirWithKB = await findKBStructureInSubdirectories(cachePath, fs) + if (subDirWithKB) { + await moveDirectoryContents(subDirWithKB, cachePath, fs) + await fs.rm(subDirWithKB, { recursive: true, force: true }) + return true + } + + kbStructureValid = await validateKBStructure(cachePath, fs) + return kbStructureValid +} diff --git a/packages/content-ops/src/http/download-manager.test.ts b/packages/content-ops/src/http/download-manager.test.ts index 576caf99..214682eb 100644 --- a/packages/content-ops/src/http/download-manager.test.ts +++ b/packages/content-ops/src/http/download-manager.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from 'vitest' import { IncomingMessage } from 'http' import { downloadFile } from './download-manager' import { InMemoryFileSystemService } from '@pair/content-ops' +import { NodeHttpClientService } from './http-client-service' import { buildTestResponse, toIncomingMessage, @@ -114,14 +115,20 @@ describe('Download Manager - downloadFile', () => { it('should download file successfully', async () => { const fs = new InMemoryFileSystemService({}, '/', '/') setupBasicDownload() - await downloadFile('https://example.com/test.zip', '/tmp/test.zip', { fs }) + await downloadFile('https://example.com/test.zip', '/tmp/test.zip', { + httpClient: new NodeHttpClientService(), + fs, + }) expect(fs.existsSync('/tmp/test.zip')).toBe(true) }) it('should handle redirect (301)', async () => { const fs = new InMemoryFileSystemService({}, '/', '/') const tracker = setupRedirect() - await downloadFile('https://example.com/test.zip', '/tmp/test.zip', { fs }) + await downloadFile('https://example.com/test.zip', '/tmp/test.zip', { + httpClient: new NodeHttpClientService(), + fs, + }) expect(tracker.callCount()).toBe(2) expect(fs.existsSync('/tmp/test.zip')).toBe(true) }) @@ -131,6 +138,7 @@ describe('Download Manager - downloadFile', () => { const progressWriter = { write: vi.fn() } setupProgress() await downloadFile('https://example.com/test.zip', '/tmp/test.zip', { + httpClient: new NodeHttpClientService(), fs, progressWriter, isTTY: true, @@ -168,7 +176,10 @@ describe('Download Manager - downloadFile', () => { }) await expect(async () => { - await downloadFile('https://example.com/notfound.zip', '/tmp/nf.zip', { fs }) + await downloadFile('https://example.com/notfound.zip', '/tmp/nf.zip', { + httpClient: new NodeHttpClientService(), + fs, + }) }).rejects.toThrow() }) @@ -194,7 +205,10 @@ describe('Download Manager - downloadFile', () => { }) await expect(async () => { - await downloadFile('https://example.com/fail.zip', '/tmp/fail.zip', { fs }) + await downloadFile('https://example.com/fail.zip', '/tmp/fail.zip', { + httpClient: new NodeHttpClientService(), + fs, + }) }).rejects.toThrow(/network|error/i) }) }) diff --git a/packages/content-ops/src/http/download-manager.ts b/packages/content-ops/src/http/download-manager.ts index 4a234cf5..fe9c3fb6 100644 --- a/packages/content-ops/src/http/download-manager.ts +++ b/packages/content-ops/src/http/download-manager.ts @@ -4,9 +4,7 @@ import type { IncomingMessage, ClientRequest, RequestOptions } from 'http' import type { FileSystemService } from '../file-system' -import { fileSystemService } from '../file-system' import type { HttpClientService } from './http-client-service' -import { NodeHttpClientService } from './http-client-service' import { ProgressReporter, type ProgressWriter } from './progress-reporter' import { getPartialFilePath, @@ -140,8 +138,8 @@ function createProgressReporter( } export interface DownloadOptions { - httpClient?: HttpClientService | undefined - fs?: FileSystemService | undefined + httpClient: HttpClientService + fs: FileSystemService progressWriter?: ProgressWriter | undefined isTTY?: boolean | undefined label?: string | undefined @@ -195,7 +193,7 @@ function executeDownload(params: { options: DownloadOptions }): Promise { const { url, destination, resumeFrom, fs, options } = params - const httpClient = options.httpClient || new NodeHttpClientService() + const { httpClient } = options return new Promise((resolve, reject) => { const headers: Record = resumeFrom > 0 ? { Range: `bytes=${resumeFrom}-` } : {} @@ -315,9 +313,9 @@ function processDownloadStream(ctx: { export async function downloadFile( url: string, destination: string, - options: DownloadOptions = {}, + options: DownloadOptions, ): Promise { - const fs = options.fs || fileSystemService + const { fs } = options const { progressWriter, isTTY, label } = options const ctx: DownloadContext = { url, destination, fs, progressWriter, isTTY } const { totalBytes, resumeFrom } = await setupResumeContext(ctx) diff --git a/packages/content-ops/src/http/index.ts b/packages/content-ops/src/http/index.ts index 544e3844..d81c367e 100644 --- a/packages/content-ops/src/http/index.ts +++ b/packages/content-ops/src/http/index.ts @@ -26,3 +26,5 @@ export { type ResumeDecision, type DownloadContext, } from './resume-manager' + +export { downloadWithRetry, isRetryableError, type RetryOptions } from './retryable-download' diff --git a/packages/content-ops/src/http/retryable-download.test.ts b/packages/content-ops/src/http/retryable-download.test.ts new file mode 100644 index 00000000..cb71ee3f --- /dev/null +++ b/packages/content-ops/src/http/retryable-download.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, vi } from 'vitest' +import { downloadWithRetry, isRetryableError } from './retryable-download' +import type { DownloadOptions } from './download-manager' + +describe('isRetryableError', () => { + it('returns true for ECONNRESET', () => { + expect(isRetryableError(new Error('read ECONNRESET'))).toBe(true) + }) + + it('returns true for ETIMEDOUT', () => { + expect(isRetryableError(new Error('connect ETIMEDOUT'))).toBe(true) + }) + + it('returns true for ECONNREFUSED', () => { + expect(isRetryableError(new Error('connect ECONNREFUSED'))).toBe(true) + }) + + it('returns true for socket hang up', () => { + expect(isRetryableError(new Error('socket hang up'))).toBe(true) + }) + + it('returns false for 404 error', () => { + expect(isRetryableError(new Error('Resource not found (404)'))).toBe(false) + }) + + it('returns false for 403 error', () => { + expect(isRetryableError(new Error('Access denied (403)'))).toBe(false) + }) +}) + +describe('downloadWithRetry', () => { + it('succeeds on first attempt', async () => { + const downloadFn = vi + .fn<(url: string, dest: string, opts: DownloadOptions) => Promise>() + .mockResolvedValueOnce(undefined) + + await downloadWithRetry( + 'https://example.com/kb.zip', + '/test/kb.zip', + {}, + { downloadFn, delays: [0] }, + ) + + expect(downloadFn).toHaveBeenCalledTimes(1) + }) + + it('retries on transient error and succeeds', async () => { + const downloadFn = vi + .fn<(url: string, dest: string, opts: DownloadOptions) => Promise>() + .mockRejectedValueOnce(new Error('read ECONNRESET')) + .mockResolvedValueOnce(undefined) + + await downloadWithRetry( + 'https://example.com/kb.zip', + '/test/kb.zip', + {}, + { maxRetries: 2, delays: [0, 0], downloadFn }, + ) + + expect(downloadFn).toHaveBeenCalledTimes(2) + }) + + it('throws after exhausting retries on transient error', async () => { + const downloadFn = vi + .fn<(url: string, dest: string, opts: DownloadOptions) => Promise>() + .mockRejectedValue(new Error('connect ETIMEDOUT')) + + await expect( + downloadWithRetry( + 'https://example.com/kb.zip', + '/test/kb.zip', + {}, + { maxRetries: 2, delays: [0, 0], downloadFn }, + ), + ).rejects.toThrow('ETIMEDOUT') + + // initial attempt + 2 retries = 3 + expect(downloadFn).toHaveBeenCalledTimes(3) + }) + + it('does not retry non-retryable errors', async () => { + const downloadFn = vi + .fn<(url: string, dest: string, opts: DownloadOptions) => Promise>() + .mockRejectedValue(new Error('Resource not found (404)')) + + await expect( + downloadWithRetry( + 'https://example.com/kb.zip', + '/test/kb.zip', + {}, + { maxRetries: 3, delays: [0, 0, 0], downloadFn }, + ), + ).rejects.toThrow('404') + + expect(downloadFn).toHaveBeenCalledTimes(1) + }) + + it('passes options through to downloadFn', async () => { + const downloadFn = vi + .fn<(url: string, dest: string, opts: DownloadOptions) => Promise>() + .mockResolvedValueOnce(undefined) + const opts: DownloadOptions = { isTTY: true } + + await downloadWithRetry('https://example.com/kb.zip', '/out.zip', opts, { downloadFn }) + + expect(downloadFn).toHaveBeenCalledWith('https://example.com/kb.zip', '/out.zip', opts) + }) +}) diff --git a/packages/content-ops/src/http/retryable-download.ts b/packages/content-ops/src/http/retryable-download.ts new file mode 100644 index 00000000..f75d5e49 --- /dev/null +++ b/packages/content-ops/src/http/retryable-download.ts @@ -0,0 +1,73 @@ +/** + * Retry wrapper for downloadFile with exponential backoff. + * Only retries transient network errors, not HTTP 404/403. + */ + +import { downloadFile, type DownloadOptions } from './download-manager' + +type DownloadFn = (url: string, destination: string, options: DownloadOptions) => Promise + +export interface RetryOptions { + maxRetries?: number + delays?: number[] + /** Injectable download function for testing. Defaults to downloadFile. */ + downloadFn?: DownloadFn +} + +const DEFAULT_MAX_RETRIES = 3 +const DEFAULT_DELAYS = [1000, 2000, 4000] + +const RETRYABLE_PATTERNS = [ + 'econnreset', + 'etimedout', + 'econnrefused', + 'socket hang up', + 'enotfound', + 'epipe', +] + +export function isRetryableError(error: Error): boolean { + const msg = (error.message || '').toLowerCase() + return RETRYABLE_PATTERNS.some(pattern => msg.includes(pattern)) +} + +function resolveRetryConfig(retryOptions: RetryOptions) { + return { + maxRetries: retryOptions.maxRetries ?? DEFAULT_MAX_RETRIES, + delays: retryOptions.delays ?? DEFAULT_DELAYS, + download: retryOptions.downloadFn ?? downloadFile, + } +} + +export async function downloadWithRetry( + url: string, + destination: string, + options: DownloadOptions, + retryOptions: RetryOptions = {}, +): Promise { + const { maxRetries, delays, download } = resolveRetryConfig(retryOptions) + + let lastError: Error | undefined + + for (let attempt = 0; attempt <= maxRetries; attempt++) { + try { + await download(url, destination, options) + return + } catch (error) { + lastError = error as Error + + if (!isRetryableError(lastError) || attempt >= maxRetries) { + throw lastError + } + + const delay = delays[attempt] ?? delays[delays.length - 1] ?? 4000 + await sleep(delay) + } + } + + throw lastError +} + +function sleep(ms: number): Promise { + return new Promise(resolve => setTimeout(resolve, ms)) +} diff --git a/packages/content-ops/src/index.ts b/packages/content-ops/src/index.ts index 0f4d2c15..21c056f2 100644 --- a/packages/content-ops/src/index.ts +++ b/packages/content-ops/src/index.ts @@ -14,7 +14,19 @@ export { } from './file-system/integrity-validator' export { isValidHttpUrl, validateUrl } from './file-system/url-validator' export { extractZip } from './file-system/archive-operations' -export { detectSourceType, SourceType } from './path-resolution/source-detector' +export { + validateKBStructure, + findKBStructureInSubdirectories, + moveDirectoryContents, + copyDirectoryContents, + normalizeExtractedKB, +} from './file-system/kb-validation' +export { + detectSourceType, + SourceType, + isRemoteUrl, + isUnsupportedProtocol, +} from './path-resolution/source-detector' export { SyncOptions, defaultSyncOptions } from './ops/SyncOptions' export { @@ -89,4 +101,7 @@ export { getPartialFileSize, cleanupPartialFile, shouldResume, + downloadWithRetry, + isRetryableError, } from './http' +export type { RetryOptions } from './http' diff --git a/packages/content-ops/src/path-resolution/source-detector.test.ts b/packages/content-ops/src/path-resolution/source-detector.test.ts index e1c7d949..97ca72a2 100644 --- a/packages/content-ops/src/path-resolution/source-detector.test.ts +++ b/packages/content-ops/src/path-resolution/source-detector.test.ts @@ -1,52 +1,59 @@ import { detectSourceType, SourceType } from './source-detector' -import * as fs from 'fs' -import * as path from 'path' -import { describe, it, expect, beforeAll, afterAll } from 'vitest' - -const tmpDir = path.join(__dirname, '../../.tmp/source-detector-test') -const zipFile = path.join(tmpDir, 'test.zip') -const dir = path.join(tmpDir, 'testdir') - -beforeAll(() => { - fs.mkdirSync(tmpDir, { recursive: true }) - fs.writeFileSync(zipFile, 'dummy') - fs.mkdirSync(dir, { recursive: true }) -}) - -afterAll(() => { - fs.rmSync(tmpDir, { recursive: true, force: true }) -}) +import { InMemoryFileSystemService } from '../test-utils/in-memory-fs' +import { describe, it, expect } from 'vitest' describe('Source Detector', () => { - it('detects REMOTE_URL', () => { - expect(detectSourceType('https://example.com/kb.zip')).toBe(SourceType.REMOTE_URL) - expect(detectSourceType('http://example.com/kb.zip')).toBe(SourceType.REMOTE_URL) + const cwd = '/test/project' + + function createFs(files: Record = {}) { + return new InMemoryFileSystemService(files, cwd, cwd) + } + + it('detects REMOTE_URL for https', () => { + expect(detectSourceType('https://example.com/kb.zip', createFs())).toBe(SourceType.REMOTE_URL) }) - it('rejects unsafe protocols', () => { - expect(detectSourceType('file:///tmp/kb.zip')).toBe(SourceType.INVALID) - expect(detectSourceType('ftp://example.com/kb.zip')).toBe(SourceType.INVALID) + it('detects REMOTE_URL for http', () => { + expect(detectSourceType('http://example.com/kb.zip', createFs())).toBe(SourceType.REMOTE_URL) }) - it('detects LOCAL_ZIP (absolute)', () => { - expect(detectSourceType(zipFile)).toBe(SourceType.LOCAL_ZIP) + it('rejects file:// protocol', () => { + expect(detectSourceType('file:///tmp/kb.zip', createFs())).toBe(SourceType.INVALID) }) - it('detects LOCAL_ZIP (relative)', () => { - const rel = path.relative(process.cwd(), zipFile) - expect(detectSourceType(rel)).toBe(SourceType.LOCAL_ZIP) + it('rejects ftp:// protocol', () => { + expect(detectSourceType('ftp://example.com/kb.zip', createFs())).toBe(SourceType.INVALID) }) - it('detects LOCAL_DIRECTORY (absolute)', () => { - expect(detectSourceType(dir)).toBe(SourceType.LOCAL_DIRECTORY) + it('detects LOCAL_ZIP for absolute path', () => { + const fs = createFs({ '/data/kb.zip': 'zipdata' }) + expect(detectSourceType('/data/kb.zip', fs)).toBe(SourceType.LOCAL_ZIP) }) - it('detects LOCAL_DIRECTORY (relative)', () => { - const rel = path.relative(process.cwd(), dir) - expect(detectSourceType(rel)).toBe(SourceType.LOCAL_DIRECTORY) + it('detects LOCAL_ZIP for relative path', () => { + const fs = createFs({ [`${cwd}/my-kb.zip`]: 'zipdata' }) + expect(detectSourceType('my-kb.zip', fs)).toBe(SourceType.LOCAL_ZIP) + }) + + it('detects LOCAL_DIRECTORY for absolute path', () => { + const fs = createFs() + fs.mkdirSync('/data/kb-dataset') + expect(detectSourceType('/data/kb-dataset', fs)).toBe(SourceType.LOCAL_DIRECTORY) + }) + + it('detects LOCAL_DIRECTORY for relative path', () => { + const fs = createFs() + fs.mkdirSync(`${cwd}/kb-dataset`) + expect(detectSourceType('kb-dataset', fs)).toBe(SourceType.LOCAL_DIRECTORY) }) it('returns INVALID for non-existent path', () => { - expect(detectSourceType('/not/a/real/path')).toBe(SourceType.INVALID) + expect(detectSourceType('/not/a/real/path', createFs())).toBe(SourceType.INVALID) + }) + + it('requires fs parameter', () => { + const fs = createFs() + expect(detectSourceType('https://example.com/kb.zip', fs)).toBe(SourceType.REMOTE_URL) + expect(detectSourceType('ftp://example.com/kb.zip', fs)).toBe(SourceType.INVALID) }) }) diff --git a/packages/content-ops/src/path-resolution/source-detector.ts b/packages/content-ops/src/path-resolution/source-detector.ts index a4a8ea44..c19d89e5 100644 --- a/packages/content-ops/src/path-resolution/source-detector.ts +++ b/packages/content-ops/src/path-resolution/source-detector.ts @@ -2,8 +2,7 @@ * Source type detector for local and remote paths */ -import * as path from 'path' -import * as fs from 'fs' +import type { FileSystemService } from '../file-system' export enum SourceType { REMOTE_URL = 'REMOTE_URL', @@ -12,23 +11,32 @@ export enum SourceType { INVALID = 'INVALID', } +/** True when source is an http:// or https:// URL */ +export function isRemoteUrl(source: string): boolean { + return /^https?:\/\//i.test(source) +} + +/** True when source uses an unsupported protocol (file://, ftp://) */ +export function isUnsupportedProtocol(source: string): boolean { + return /^(file|ftp):\/\//i.test(source) +} + /** * Detect the type of source (remote URL, local ZIP, or local directory) - * @param source - Source string (URL or file path) - * @returns SourceType enum value */ -export function detectSourceType(source: string): SourceType { +export function detectSourceType(source: string, fs: FileSystemService): SourceType { // Reject unsafe protocols - if (/^(file|ftp):\/\//i.test(source)) return SourceType.INVALID + if (isUnsupportedProtocol(source)) return SourceType.INVALID // Remote URL - if (/^https?:\/\//i.test(source)) return SourceType.REMOTE_URL + if (isRemoteUrl(source)) return SourceType.REMOTE_URL + // Local path resolution + const resolved = fs.resolve(fs.currentWorkingDirectory(), source) // Local ZIP (absolute or relative) - const resolved = path.resolve(process.cwd(), source) - if (source.endsWith('.zip') && fs.existsSync(resolved) && fs.statSync(resolved).isFile()) { + if (source.endsWith('.zip') && fs.existsSync(resolved)) { return SourceType.LOCAL_ZIP } // Local directory - if (fs.existsSync(resolved) && fs.statSync(resolved).isDirectory()) { + if (fs.existsSync(resolved)) { return SourceType.LOCAL_DIRECTORY } return SourceType.INVALID diff --git a/packages/content-ops/src/test-utils/in-memory-fs.ts b/packages/content-ops/src/test-utils/in-memory-fs.ts index ccbee12b..b7b7b2f4 100644 --- a/packages/content-ops/src/test-utils/in-memory-fs.ts +++ b/packages/content-ops/src/test-utils/in-memory-fs.ts @@ -336,7 +336,10 @@ export class InMemoryFileSystemService implements FileSystemService { async symlink(target: string, path: string): Promise { const resolvedPath = this.resolvePath(path) - const resolvedTarget = this.resolvePath(target) + // Resolve relative targets from the symlink's parent directory (matching OS behavior) + const resolvedTarget = isAbsolute(target) + ? this.resolvePath(target) + : resolve(dirname(resolvedPath), target) if (this.symlinks.has(resolvedPath) || this.files.has(resolvedPath)) { throw new Error(`Path already exists: ${path}`) } diff --git a/packages/content-ops/src/test-utils/mock-http-client-service.ts b/packages/content-ops/src/test-utils/mock-http-client-service.ts index 0ddda217..611d2667 100644 --- a/packages/content-ops/src/test-utils/mock-http-client-service.ts +++ b/packages/content-ops/src/test-utils/mock-http-client-service.ts @@ -28,6 +28,7 @@ export class MockHttpClientService implements HttpClientService { private requestCallIndex = 0 private getRequestUrls: string[] = [] private getError: Error | null = null + private persistGetError = false /** * Configure mock responses for GET requests @@ -50,10 +51,12 @@ export class MockHttpClientService implements HttpClientService { } /** - * Configure error to be thrown on next GET request + * Configure error to be thrown on GET requests. + * @param persistent When true, error persists across all calls (useful for retry tests). */ - setGetError(error: Error): void { + setGetError(error: Error, persistent = false): void { this.getError = error + this.persistGetError = persistent } get( @@ -68,7 +71,7 @@ export class MockHttpClientService implements HttpClientService { if (this.getError && errCb) { const error = this.getError - this.getError = null + if (!this.persistGetError) this.getError = null setImmediate(() => (errCb as (error: Error) => void)(error)) return this.createMockRequest() } @@ -122,6 +125,7 @@ export class MockHttpClientService implements HttpClientService { this.requestCallIndex = 0 this.getRequestUrls = [] this.getError = null + this.persistGetError = false } /** diff --git a/scripts/smoke-tests/run-all.sh b/scripts/smoke-tests/run-all.sh index 0eef27de..90b6dee4 100755 --- a/scripts/smoke-tests/run-all.sh +++ b/scripts/smoke-tests/run-all.sh @@ -10,30 +10,33 @@ REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" # Help function usage() { - echo "Usage: $0 [--binary ] [--kb-source ] [--cleanup] [--ci]" - echo " --binary Path to the 'pair' executable to test." - echo " If omitted, defaults to detected 'apps/pair-cli/dist/cli.js'" - echo " --kb-source Path to a local Knowledge Base directory." - echo " If omitted, defaults to detected 'packages/knowledge-hub/dataset'" - echo " --cleanup Remove temporary directories after success" - echo " --ci Run in CI mode (enables cleanup, detailed logs, and selects CI-safe tests)" + echo "Usage: $0 [--binary ] [--kb-source ] [--cleanup] [--ci] [--offline-only]" + echo " --binary Path to the 'pair' executable to test." + echo " If omitted, defaults to detected 'apps/pair-cli/dist/cli.js'" + echo " --kb-source Path to a local Knowledge Base directory." + echo " If omitted, defaults to detected 'packages/knowledge-hub/dataset'" + echo " --cleanup Remove temporary directories after success" + echo " --ci Run in CI mode (enables cleanup, detailed logs, and selects CI-safe tests)" + echo " --offline-only Only run scenarios tagged OFFLINE_SAFE=true (no network required)" exit 1 } # Parse args CLEANUP="false" IS_CI="false" +OFFLINE_ONLY="false" while [[ "$#" -gt 0 ]]; do case $1 in --binary) BINARY_PATH="$2"; shift ;; --kb-source) KB_SOURCE="$2"; shift ;; --cleanup) CLEANUP="true" ;; - --ci) + --ci) IS_CI="true" - CLEANUP="true" + CLEANUP="true" export PAIR_DIAG=1 ;; + --offline-only) OFFLINE_ONLY="true" ;; --help) usage ;; *) echo "Unknown parameter: $1"; usage ;; esac @@ -194,6 +197,15 @@ echo "|----------|--------|" >> "$REPORT_FILE" # Run Scenarios FAILED_TESTS=() +# Check if a scenario is offline-safe by sourcing its OFFLINE_SAFE variable. +# Scenarios without the variable are treated as offline-safe (default true). +is_offline_safe() { + local script="$1" + local val + val=$(grep -m1 '^OFFLINE_SAFE=' "$script" 2>/dev/null | cut -d= -f2) + [ "${val:-true}" = "true" ] +} + run_scenario() { local script="$1" local name=$(basename "$script") @@ -222,11 +234,16 @@ if [ "$IS_CI" = "true" ]; then "links.sh" "lifecycle-kb.sh" "validate-config.sh" + "source-resolution.sh" ) for t in "${CI_TESTS[@]}"; do script="$SCENARIOS_DIR/$t" if [ -f "$script" ]; then + if [ "$OFFLINE_ONLY" = "true" ] && ! is_offline_safe "$script"; then + echo " Skipping (not offline-safe): $t" + continue + fi run_scenario "$script" else echo -e "\033[0;31m[ERROR]\033[0m CI Test not found: $t" @@ -249,6 +266,10 @@ else if [ "$(basename "$script")" = "00-create-install-package.sh" ]; then continue fi + if [ "$OFFLINE_ONLY" = "true" ] && ! is_offline_safe "$script"; then + echo " Skipping (not offline-safe): $(basename "$script")" + continue + fi run_scenario "$script" done fi diff --git a/scripts/smoke-tests/scenarios/source-resolution.sh b/scripts/smoke-tests/scenarios/source-resolution.sh new file mode 100755 index 00000000..624b5566 --- /dev/null +++ b/scripts/smoke-tests/scenarios/source-resolution.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +source "$(dirname "$0")/../lib/utils.sh" + +# Tag: this scenario is offline-safe (no network required) +OFFLINE_SAFE=true + +TEST_NAME="Source Resolution Scenarios" +echo "=== Running $TEST_NAME ===" + +# Setup: create a valid KB directory source +MOCK_KB=$(setup_workspace "mock-kb-source-resolution") +mkdir -p "$MOCK_KB/.pair/knowledge" +mkdir -p "$MOCK_KB/.github" +echo "# AGENTS" > "$MOCK_KB/AGENTS.md" +echo '{"version":"0.0.0"}' > "$MOCK_KB/manifest.json" +echo "# Mock Knowledge" > "$MOCK_KB/.pair/knowledge/index.md" +echo "# Mock GitHub" > "$MOCK_KB/.github/README.md" + +# ------------------------------------------------------------------- +# Test 1: Install from local directory (absolute path) +# ------------------------------------------------------------------- +log_info "Test 1: Install from local directory (absolute path)" +TEST_DIR=$(setup_workspace "source-res-dir-abs") +cd "$TEST_DIR" +run_pair install --source "$MOCK_KB" +assert_success || exit 1 +assert_dir ".pair" +log_succ "Install from absolute directory succeeded" + +# ------------------------------------------------------------------- +# Test 2: Install with --offline --source (local dir) +# ------------------------------------------------------------------- +log_info "Test 2: Install with --offline --source (local directory)" +TEST_DIR=$(setup_workspace "source-res-offline") +cd "$TEST_DIR" +run_pair install --source "$MOCK_KB" --offline +assert_success || exit 1 +assert_dir ".pair" +log_succ "Offline install from local directory succeeded" + +# ------------------------------------------------------------------- +# Test 3: Update from local directory +# ------------------------------------------------------------------- +log_info "Test 3: Update from local directory" +TEST_DIR=$(setup_workspace "source-res-update") +cd "$TEST_DIR" +# First install +run_pair install --source "$MOCK_KB" +assert_success || exit 1 +# Then update +run_pair update --source "$MOCK_KB" +assert_success || exit 1 +assert_dir ".pair" +log_succ "Update from local directory succeeded" + +# ------------------------------------------------------------------- +# Test 4: Error - install with non-existent path +# ------------------------------------------------------------------- +log_info "Test 4: Error on non-existent source path" +TEST_DIR=$(setup_workspace "source-res-noexist") +cd "$TEST_DIR" +run_pair install --source "/nonexistent/path/to/kb" +assert_failure || exit 1 +log_succ "Non-existent path correctly rejected" + +# ------------------------------------------------------------------- +# Test 5: Error - --offline without --source +# ------------------------------------------------------------------- +log_info "Test 5: Error on --offline without --source" +TEST_DIR=$(setup_workspace "source-res-offline-nosrc") +cd "$TEST_DIR" +run_pair install --offline +assert_failure || exit 1 +log_succ "Offline without source correctly rejected" + +# ------------------------------------------------------------------- +# Test 6: Install to custom target from local dir +# ------------------------------------------------------------------- +log_info "Test 6: Install to custom target from local directory" +TEST_DIR=$(setup_workspace "source-res-custom-target") +cd "$TEST_DIR" +run_pair install ./my-docs --source "$MOCK_KB" +assert_success || exit 1 +assert_dir "my-docs/.pair" +log_succ "Install to custom target from local dir succeeded" + +echo "=== $TEST_NAME Completed ===" From 3faab86e47d86941182093f046e9bbc6a7a3ab74 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Sat, 14 Feb 2026 21:13:38 +0100 Subject: [PATCH 2/4] update symlinks --- .agent/skills | 2 +- .agents/skills | 2 +- .cursor/skills | 2 +- .github/skills | 2 +- .windsurf/skills | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.agent/skills b/.agent/skills index 079662b1..454b8427 120000 --- a/.agent/skills +++ b/.agent/skills @@ -1 +1 @@ -/Users/gianlucacarucci/projects/pair/.claude/skills \ No newline at end of file +../.claude/skills \ No newline at end of file diff --git a/.agents/skills b/.agents/skills index 079662b1..454b8427 120000 --- a/.agents/skills +++ b/.agents/skills @@ -1 +1 @@ -/Users/gianlucacarucci/projects/pair/.claude/skills \ No newline at end of file +../.claude/skills \ No newline at end of file diff --git a/.cursor/skills b/.cursor/skills index 079662b1..454b8427 120000 --- a/.cursor/skills +++ b/.cursor/skills @@ -1 +1 @@ -/Users/gianlucacarucci/projects/pair/.claude/skills \ No newline at end of file +../.claude/skills \ No newline at end of file diff --git a/.github/skills b/.github/skills index 079662b1..454b8427 120000 --- a/.github/skills +++ b/.github/skills @@ -1 +1 @@ -/Users/gianlucacarucci/projects/pair/.claude/skills \ No newline at end of file +../.claude/skills \ No newline at end of file diff --git a/.windsurf/skills b/.windsurf/skills index 079662b1..454b8427 120000 --- a/.windsurf/skills +++ b/.windsurf/skills @@ -1 +1 @@ -/Users/gianlucacarucci/projects/pair/.claude/skills \ No newline at end of file +../.claude/skills \ No newline at end of file From e3129cf0030a01b9186bd53f7c32cc6e1cb31b38 Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Sat, 14 Feb 2026 21:43:29 +0100 Subject: [PATCH 3/4] =?UTF-8?q?[#92]=20feat:=20CLI=20UX=20=E2=80=94=20CliP?= =?UTF-8?q?resenter,=20progress=20tracking,=20help=20formatting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.5 --- apps/pair-cli/package.json | 4 +- apps/pair-cli/src/cli.ts | 20 ++- apps/pair-cli/src/commands/install/handler.ts | 79 +++++++--- apps/pair-cli/src/commands/update/handler.ts | 91 ++++++++--- apps/pair-cli/src/ui/index.ts | 7 + apps/pair-cli/src/ui/presenter.test.ts | 147 ++++++++++++++++++ apps/pair-cli/src/ui/presenter.ts | 136 ++++++++++++++++ apps/pair-cli/tsconfig.json | 4 +- 8 files changed, 435 insertions(+), 53 deletions(-) create mode 100644 apps/pair-cli/src/ui/index.ts create mode 100644 apps/pair-cli/src/ui/presenter.test.ts create mode 100644 apps/pair-cli/src/ui/presenter.ts diff --git a/apps/pair-cli/package.json b/apps/pair-cli/package.json index abed698a..2ab7f77a 100644 --- a/apps/pair-cli/package.json +++ b/apps/pair-cli/package.json @@ -27,7 +27,9 @@ "#kb-manager/*": "./src/kb-manager/*.ts", "#diagnostics": "./src/diagnostics.ts", "#test-utils": "./src/test-utils/index.ts", - "#test-utils/*": "./src/test-utils/*.ts" + "#test-utils/*": "./src/test-utils/*.ts", + "#ui": "./src/ui/index.ts", + "#ui/*": "./src/ui/*.ts" }, "scripts": { "build": "tsc -b tsconfig.build.json", diff --git a/apps/pair-cli/src/cli.ts b/apps/pair-cli/src/cli.ts index 592b89fe..d8cd67e6 100644 --- a/apps/pair-cli/src/cli.ts +++ b/apps/pair-cli/src/cli.ts @@ -77,6 +77,16 @@ export async function runCli( .option('--no-kb', 'Skip knowledge base download') // Prevent Commander from calling process.exit() automatically .exitOverride() + .configureHelp({ sortSubcommands: true }) + + program.addHelpText( + 'beforeAll', + `\n ${chalk.bold(pkg.name)} ${chalk.dim(`v${pkg.version}`)}\n ${chalk.dim(pkg.description)}\n`, + ) + program.addHelpText( + 'afterAll', + `\n Run ${chalk.dim('pair --help')} for detailed usage of a specific command.\n`, + ) runDiagnostics(fsService) setupCommands(program, { fsService, httpClient, version: pkg.version }) @@ -135,12 +145,14 @@ function addCommandOptions( } function buildCommandHelpText(examples: readonly string[], notes: readonly string[]): string { + const exLines = examples.map((ex: string) => ` ${chalk.dim('$')} ${ex}`).join('\n') + const noteLines = notes.map((note: string) => ` ${chalk.dim('•')} ${note}`).join('\n') return ` -Examples: -${examples.map((ex: string) => ` $ ${ex}`).join('\n')} +${chalk.bold('Examples:')} +${exLines} -Usage Notes: -${notes.map((note: string) => ` • ${note}`).join('\n')} +${chalk.bold('Usage Notes:')} +${noteLines} ` } diff --git a/apps/pair-cli/src/commands/install/handler.ts b/apps/pair-cli/src/commands/install/handler.ts index 275a147b..a1b1577a 100644 --- a/apps/pair-cli/src/commands/install/handler.ts +++ b/apps/pair-cli/src/commands/install/handler.ts @@ -20,6 +20,7 @@ import { import { applyLinkTransformation } from '../update-link/logic' import type { HttpClientService } from '@pair/content-ops' import { type SkillNameMap } from '@pair/content-ops' +import { createCliPresenter, type CliPresenter, type RegistryResult } from '#ui' /** * Install options for handler @@ -31,6 +32,7 @@ interface InstallHandlerOptions { minLogLevel?: LogEntry['level'] httpClient?: HttpClientService cliVersion?: string + presenter?: CliPresenter } /** @@ -47,12 +49,12 @@ export async function handleInstallCommand( options?.minLogLevel ?? 'info' const { pushLog } = createLogger(logLevel as LogEntry['level']) + const presenter = options?.presenter ?? createCliPresenter(pushLog) try { const { datasetRoot, registries, baseTarget } = await setupInstallContext(fs, config, options) await validateInstallContext(fs, registries, baseTarget) - await executeInstall({ fs, datasetRoot, registries, baseTarget, options, pushLog }) - pushLog('info', 'Installation completed successfully') + await executeInstall({ fs, datasetRoot, registries, baseTarget, options, pushLog, presenter }) } catch (err) { pushLog('error', `Installation failed: ${String(err)}`) throw err @@ -104,8 +106,11 @@ async function installRegistry(ctx: { datasetRoot: string baseTarget: string pushLog: (level: LogEntry['level'], message: string) => void -}): Promise { - const { fs, registryName, registryConfig, datasetRoot, baseTarget, pushLog } = ctx + presenter: CliPresenter + index: number + total: number +}): Promise<{ skillNameMap?: SkillNameMap | undefined; result: RegistryResult }> { + const { fs, registryName, registryConfig, datasetRoot, baseTarget, presenter, index, total } = ctx const resolved = resolveRegistryPaths({ name: registryName, config: registryConfig, @@ -123,8 +128,14 @@ async function installRegistry(ctx: { const effectiveDatasetRoot = registryConfig.flatten || registryConfig.prefix ? baseTarget : datasetRoot - pushLog('info', `Installing '${registryName}' from '${datasetPath}' to '${effectiveTarget}'`) - const result = await doCopyAndUpdateLinks(fs, { + presenter.registryStart({ + name: registryName, + index, + total, + source: datasetPath, + target: effectiveTarget, + }) + const copyResult = await doCopyAndUpdateLinks(fs, { source: datasetPath, target: effectiveTarget, datasetRoot: effectiveDatasetRoot, @@ -132,46 +143,68 @@ async function installRegistry(ctx: { }) await postCopyOps({ fs, registryConfig, effectiveTarget, datasetPath, baseTarget }) - pushLog('info', `Successfully installed registry '${registryName}'`) - return result['skillNameMap'] as SkillNameMap | undefined + presenter.registryDone(registryName) + return { + skillNameMap: copyResult['skillNameMap'] as SkillNameMap | undefined, + result: { name: registryName, target: effectiveTarget, ok: true }, + } } -async function executeInstall(context: { +type InstallContext = { fs: FileSystemService datasetRoot: string registries: Record baseTarget: string options: InstallHandlerOptions | undefined pushLog: (level: LogEntry['level'], message: string) => void -}): Promise { - const { fs, datasetRoot, registries, baseTarget, options, pushLog } = context - const accumulatedSkillNameMap: SkillNameMap = new Map() + presenter: CliPresenter +} + +async function installAllRegistries(ctx: InstallContext): Promise<{ + results: RegistryResult[] + skillNameMap: SkillNameMap +}> { + const { fs, datasetRoot, registries, baseTarget, pushLog, presenter } = ctx + const accumulated: SkillNameMap = new Map() + const total = Object.keys(registries).length - await forEachRegistry(registries, async (registryName, registryConfig) => { - const skillNameMap = await installRegistry({ + const results = await forEachRegistry(registries, async (registryName, registryConfig, index) => { + const out = await installRegistry({ fs, registryName, registryConfig, datasetRoot, baseTarget, pushLog, + presenter, + index, + total, }) - if (skillNameMap) { - for (const [k, v] of skillNameMap) accumulatedSkillNameMap.set(k, v) + if (out.skillNameMap) { + for (const [k, v] of out.skillNameMap) accumulated.set(k, v) } + return out.result }) + return { results, skillNameMap: accumulated } +} - if (accumulatedSkillNameMap.size > 0) { - await applySkillRefsToNonSkillRegistries( - { fs, baseTarget, pushLog }, - registries, - accumulatedSkillNameMap, - ) - } +async function executeInstall(context: InstallContext): Promise { + const { fs, registries, baseTarget, options, pushLog, presenter } = context + const total = Object.keys(registries).length + const startTime = Date.now() + + presenter.startOperation('install', total) + const { results, skillNameMap } = await installAllRegistries(context) + + if (skillNameMap.size > 0) { + await applySkillRefsToNonSkillRegistries({ fs, baseTarget, pushLog }, registries, skillNameMap) + } if (options?.linkStyle) { await applyLinkTransformation(fs, { linkStyle: options.linkStyle }, pushLog, 'install') } + + presenter.summary(results, 'install', Date.now() - startTime) } /** diff --git a/apps/pair-cli/src/commands/update/handler.ts b/apps/pair-cli/src/commands/update/handler.ts index 7d8f4fb5..006a1779 100644 --- a/apps/pair-cli/src/commands/update/handler.ts +++ b/apps/pair-cli/src/commands/update/handler.ts @@ -19,6 +19,7 @@ import { import { applyLinkTransformation } from '../update-link/logic' import type { HttpClientService } from '@pair/content-ops' import { BackupService, type SkillNameMap } from '@pair/content-ops' +import { createCliPresenter, type CliPresenter, type RegistryResult } from '#ui' /** * Update options for handler @@ -32,6 +33,7 @@ interface UpdateHandlerOptions { autoRollback?: boolean httpClient?: HttpClientService cliVersion?: string + presenter?: CliPresenter } type UpdateContext = { @@ -41,6 +43,7 @@ type UpdateContext = { baseTarget: string options: UpdateHandlerOptions | undefined pushLog: (level: LogEntry['level'], message: string) => void + presenter: CliPresenter } /** @@ -57,11 +60,11 @@ export async function handleUpdateCommand( options?.minLogLevel ?? 'info' const { pushLog } = createLogger(logLevel as LogEntry['level']) + const presenter = options?.presenter ?? createCliPresenter(pushLog) try { const { datasetRoot, registries, baseTarget } = await setupUpdateContext(fs, config, options) - await executeUpdate({ fs, datasetRoot, registries, baseTarget, options, pushLog }) - pushLog('info', 'Update completed successfully') + await executeUpdate({ fs, datasetRoot, registries, baseTarget, options, pushLog, presenter }) } catch (err) { pushLog('error', `Update failed: ${String(err)}`) throw err @@ -133,12 +136,12 @@ async function runUpdateSequence( } async function performBackup(backupService: BackupService, context: UpdateContext): Promise { - const { fs, registries, baseTarget, pushLog } = context + const { fs, registries, baseTarget, presenter } = context const backupConfig: Record = {} for (const [name, config] of Object.entries(registries)) { backupConfig[name] = resolveTarget(name, config, fs, baseTarget) } - pushLog('info', 'Creating backup before update...') + presenter.phase('Creating backup...') await backupService.backupAllRegistries(backupConfig) } @@ -160,15 +163,40 @@ async function logDatasetEntries( } } -async function updateSingleRegistry(ctx: { +function resolveEffectiveDatasetRoot( + config: RegistryConfig, + baseTarget: string, + datasetRoot: string, +): string { + return config.flatten || config.prefix ? baseTarget : datasetRoot +} + +interface UpdateRegistryCtx { fs: FileSystemService datasetRoot: string registryName: string registryConfig: RegistryConfig baseTarget: string pushLog: (level: LogEntry['level'], message: string) => void -}): Promise { - const { fs, datasetRoot, registryName, registryConfig, baseTarget, pushLog } = ctx + presenter: CliPresenter + index: number + total: number +} + +async function updateSingleRegistry( + ctx: UpdateRegistryCtx, +): Promise<{ skillNameMap?: SkillNameMap | undefined; result: RegistryResult }> { + const { + fs, + datasetRoot, + registryName, + registryConfig, + baseTarget, + pushLog, + presenter, + index, + total, + } = ctx const resolved = resolveRegistryPaths({ name: registryName, config: registryConfig, @@ -176,47 +204,58 @@ async function updateSingleRegistry(ctx: { fs, baseTarget, }) - const effectiveTarget = resolved.target + const { target: effectiveTarget, source: datasetPath } = resolved await ensureDir(fs, dirname(effectiveTarget)) - const datasetPath = resolved.source - const copyOptions = buildCopyOptions(registryConfig) - // For flatten+prefix registries (skills), use baseTarget as the effective - // datasetRoot so that link re-rooting correctly maps source paths (potentially - // deep in node_modules) to installed paths relative to the target root. - const effectiveDatasetRoot = - registryConfig.flatten || registryConfig.prefix ? baseTarget : datasetRoot + const effectiveDatasetRoot = resolveEffectiveDatasetRoot(registryConfig, baseTarget, datasetRoot) await logDatasetEntries(fs, datasetPath, pushLog) - pushLog('info', `Updating registry '${registryName}' at '${effectiveTarget}'`) - const result = await doCopyAndUpdateLinks(fs, { + presenter.registryStart({ + name: registryName, + index, + total, + source: datasetPath, + target: effectiveTarget, + }) + const copyResult = await doCopyAndUpdateLinks(fs, { source: datasetPath, target: effectiveTarget, datasetRoot: effectiveDatasetRoot, - options: copyOptions, + options: buildCopyOptions(registryConfig), }) await postCopyOps({ fs, registryConfig, effectiveTarget, datasetPath, baseTarget }) - pushLog('info', `Successfully updated registry '${registryName}'`) - return result['skillNameMap'] as SkillNameMap | undefined + presenter.registryDone(registryName) + return { + skillNameMap: copyResult['skillNameMap'] as SkillNameMap | undefined, + result: { name: registryName, target: effectiveTarget, ok: true }, + } } -async function updateRegistries(context: UpdateContext): Promise { - const { fs, datasetRoot, registries, baseTarget, pushLog } = context +async function updateRegistries(context: UpdateContext): Promise { + const { fs, datasetRoot, registries, baseTarget, pushLog, presenter } = context const accumulatedSkillNameMap: SkillNameMap = new Map() + const total = Object.keys(registries).length + const startTime = Date.now() + + presenter.startOperation('update', total) - await forEachRegistry(registries, async (registryName, registryConfig) => { - const skillNameMap = await updateSingleRegistry({ + const results = await forEachRegistry(registries, async (registryName, registryConfig, index) => { + const { skillNameMap, result } = await updateSingleRegistry({ fs, datasetRoot, registryName, registryConfig, baseTarget, pushLog, + presenter, + index, + total, }) if (skillNameMap) { for (const [k, v] of skillNameMap) accumulatedSkillNameMap.set(k, v) } + return result }) if (accumulatedSkillNameMap.size > 0) { @@ -226,6 +265,9 @@ async function updateRegistries(context: UpdateContext): Promise { accumulatedSkillNameMap, ) } + + presenter.summary(results, 'update', Date.now() - startTime) + return results } async function executeRollback( @@ -234,6 +276,7 @@ async function executeRollback( context: UpdateContext, ): Promise { const { options, pushLog } = context + context.presenter.phase('Rolling back...') pushLog('warn', 'Update failed, attempting rollback...') await handleBackupRollback( backupService, diff --git a/apps/pair-cli/src/ui/index.ts b/apps/pair-cli/src/ui/index.ts new file mode 100644 index 00000000..52773073 --- /dev/null +++ b/apps/pair-cli/src/ui/index.ts @@ -0,0 +1,7 @@ +export { + createCliPresenter, + createSilentPresenter, + type CliPresenter, + type RegistryProgress, + type RegistryResult, +} from './presenter' diff --git a/apps/pair-cli/src/ui/presenter.test.ts b/apps/pair-cli/src/ui/presenter.test.ts new file mode 100644 index 00000000..5ec6590c --- /dev/null +++ b/apps/pair-cli/src/ui/presenter.test.ts @@ -0,0 +1,147 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { createCliPresenter, createSilentPresenter, type RegistryResult } from './presenter' +import type { LogEntry } from '#diagnostics' + +type PushLog = (level: LogEntry['level'], message: string) => void + +describe('createCliPresenter', () => { + let pushLog: PushLog + let consoleSpy: ReturnType + + beforeEach(() => { + pushLog = vi.fn() + consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + }) + + afterEach(() => { + consoleSpy.mockRestore() + }) + + it('startOperation prints header and calls pushLog', () => { + const presenter = createCliPresenter(pushLog) + presenter.startOperation('install', 4) + + expect(consoleSpy).toHaveBeenCalledTimes(2) + expect(pushLog).toHaveBeenCalledWith('info', 'Installing 4 registries') + }) + + it('startOperation uses singular for 1 registry', () => { + const presenter = createCliPresenter(pushLog) + presenter.startOperation('update', 1) + + expect(pushLog).toHaveBeenCalledWith('info', 'Updating 1 registry') + }) + + it('registryStart prints counter and paths', () => { + const presenter = createCliPresenter(pushLog) + presenter.registryStart({ + name: 'github', + index: 0, + total: 4, + source: '.github', + target: '.github', + }) + + expect(consoleSpy).toHaveBeenCalledTimes(1) + expect(pushLog).toHaveBeenCalledWith('info', '[1/4] github: .github → .github') + }) + + it('registryDone prints check mark', () => { + const presenter = createCliPresenter(pushLog) + presenter.registryDone('github') + + expect(consoleSpy).toHaveBeenCalledTimes(1) + expect(pushLog).toHaveBeenCalledWith('info', "Successfully processed registry 'github'") + }) + + it('registryError prints error', () => { + const presenter = createCliPresenter(pushLog) + presenter.registryError('broken', 'file not found') + + expect(consoleSpy).toHaveBeenCalledTimes(1) + expect(pushLog).toHaveBeenCalledWith( + 'error', + "Failed to process registry 'broken': file not found", + ) + }) + + it('phase prints message', () => { + const presenter = createCliPresenter(pushLog) + presenter.phase('Creating backup...') + + expect(consoleSpy).toHaveBeenCalledTimes(1) + expect(pushLog).toHaveBeenCalledWith('info', 'Creating backup...') + }) + + it('summary prints success when all ok', () => { + const presenter = createCliPresenter(pushLog) + const results: RegistryResult[] = [ + { name: 'a', target: '/a', ok: true }, + { name: 'b', target: '/b', ok: true }, + ] + presenter.summary(results, 'install', 1234) + + expect(consoleSpy).toHaveBeenCalledTimes(3) + expect(pushLog).toHaveBeenCalledWith('info', 'Installation complete: 2 ok, 0 failed (1.2s)') + }) + + it('summary prints warning when some failed', () => { + const presenter = createCliPresenter(pushLog) + const results: RegistryResult[] = [ + { name: 'a', target: '/a', ok: true }, + { name: 'b', target: '/b', ok: false, error: 'bad' }, + ] + presenter.summary(results, 'update', 500) + + expect(pushLog).toHaveBeenCalledWith('info', 'Update complete: 1 ok, 1 failed (500ms)') + }) + + it('summary formats sub-second elapsed as ms', () => { + const presenter = createCliPresenter(pushLog) + presenter.summary([{ name: 'a', target: '/a', ok: true }], 'install', 42) + + expect(pushLog).toHaveBeenCalledWith('info', 'Installation complete: 1 ok, 0 failed (42ms)') + }) +}) + +describe('createSilentPresenter', () => { + let pushLog: PushLog + + beforeEach(() => { + pushLog = vi.fn() + }) + + it('does not write to console', () => { + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + const presenter = createSilentPresenter(pushLog) + + presenter.startOperation('install', 2) + presenter.registryStart({ name: 'x', index: 0, total: 2, source: '/src', target: '/dst' }) + presenter.registryDone('x') + presenter.phase('backup') + presenter.summary([{ name: 'x', target: '/dst', ok: true }], 'install', 100) + + expect(consoleSpy).not.toHaveBeenCalled() + consoleSpy.mockRestore() + }) + + it('still calls pushLog for all operations', () => { + const presenter = createSilentPresenter(pushLog) + + presenter.startOperation('install', 2) + presenter.registryStart({ name: 'x', index: 0, total: 2, source: '/src', target: '/dst' }) + presenter.registryDone('x') + presenter.registryError('y', 'fail') + presenter.phase('backup') + presenter.summary( + [ + { name: 'x', target: '/dst', ok: true }, + { name: 'y', target: '/dst2', ok: false }, + ], + 'install', + 100, + ) + + expect(pushLog).toHaveBeenCalledTimes(6) + }) +}) diff --git a/apps/pair-cli/src/ui/presenter.ts b/apps/pair-cli/src/ui/presenter.ts new file mode 100644 index 00000000..e0ac10d0 --- /dev/null +++ b/apps/pair-cli/src/ui/presenter.ts @@ -0,0 +1,136 @@ +import chalk from 'chalk' +import type { LogEntry } from '#diagnostics' + +export interface RegistryResult { + name: string + target: string + ok: boolean + error?: string | undefined +} + +type PushLog = (level: LogEntry['level'], message: string) => void + +export interface RegistryProgress { + name: string + index: number + total: number + source: string + target: string +} + +export interface CliPresenter { + startOperation(operation: 'install' | 'update', registryCount: number): void + registryStart(reg: RegistryProgress): void + registryDone(name: string): void + registryError(name: string, error: string): void + phase(message: string): void + summary(results: RegistryResult[], operation: 'install' | 'update', elapsedMs: number): void +} + +const SEPARATOR = '──────────────────────────────────────' + +function formatElapsed(ms: number): string { + if (ms < 1000) return `${ms}ms` + return `${(ms / 1000).toFixed(1)}s` +} + +function opLabel(operation: 'install' | 'update'): string { + return operation === 'install' ? 'Installing' : 'Updating' +} + +function summaryLabel(operation: 'install' | 'update'): string { + return operation === 'install' ? 'Installation' : 'Update' +} + +function plural(count: number): string { + return count === 1 ? 'registry' : 'registries' +} + +function printSummaryBlock( + results: RegistryResult[], + operation: 'install' | 'update', + elapsedMs: number, +): string { + const ok = results.filter(r => r.ok).length + const failed = results.length - ok + const elapsed = formatElapsed(elapsedMs) + const label = summaryLabel(operation) + + console.log(`\n ${chalk.dim(SEPARATOR)}`) + if (failed === 0) { + console.log( + ` ${chalk.green('✓')} ${label} complete (${results.length} ${plural(results.length)}, ${elapsed})`, + ) + } else { + console.log( + ` ${chalk.yellow('!')} ${label} finished with errors (${ok} ok, ${failed} failed, ${elapsed})`, + ) + } + console.log() + return `${label} complete: ${ok} ok, ${failed} failed (${elapsed})` +} + +export function createCliPresenter(pushLog: PushLog): CliPresenter { + return { + startOperation(operation, registryCount) { + const msg = `${opLabel(operation)} ${registryCount} ${plural(registryCount)}` + console.log(`\n ${chalk.bold(msg)}`) + console.log(` ${chalk.dim(SEPARATOR)}\n`) + pushLog('info', msg) + }, + + registryStart({ name, index, total, source, target }) { + const counter = chalk.dim(`[${index + 1}/${total}]`) + console.log( + ` ${counter} ${chalk.bold(name)} ${chalk.dim(source)} ${chalk.dim('→')} ${chalk.dim(target)}`, + ) + pushLog('info', `[${index + 1}/${total}] ${name}: ${source} → ${target}`) + }, + + registryDone(name) { + console.log(` ${chalk.green('✓')} done`) + pushLog('info', `Successfully processed registry '${name}'`) + }, + + registryError(name, error) { + console.log(` ${chalk.red('✗')} ${error}`) + pushLog('error', `Failed to process registry '${name}': ${error}`) + }, + + phase(message) { + console.log(` ${chalk.dim('›')} ${message}`) + pushLog('info', message) + }, + + summary(results, operation, elapsedMs) { + pushLog('info', printSummaryBlock(results, operation, elapsedMs)) + }, + } +} + +export function createSilentPresenter(pushLog: PushLog): CliPresenter { + return { + startOperation(operation, registryCount) { + const label = operation === 'install' ? 'Installing' : 'Updating' + pushLog('info', `${label} ${registryCount} registries`) + }, + registryStart({ name, index, total, source, target }) { + pushLog('info', `[${index + 1}/${total}] ${name}: ${source} → ${target}`) + }, + registryDone(name) { + pushLog('info', `Successfully processed registry '${name}'`) + }, + registryError(name, error) { + pushLog('error', `Failed to process registry '${name}': ${error}`) + }, + phase(message) { + pushLog('info', message) + }, + summary(results, operation, elapsedMs) { + const ok = results.filter(r => r.ok).length + const failed = results.length - ok + const label = operation === 'install' ? 'Installation' : 'Update' + pushLog('info', `${label} complete: ${ok} ok, ${failed} failed (${formatElapsed(elapsedMs)})`) + }, + } +} diff --git a/apps/pair-cli/tsconfig.json b/apps/pair-cli/tsconfig.json index 2521d82e..eb973e44 100644 --- a/apps/pair-cli/tsconfig.json +++ b/apps/pair-cli/tsconfig.json @@ -39,7 +39,9 @@ "#kb-manager/*": ["src/kb-manager/*"], "#diagnostics": ["src/diagnostics.ts"], "#test-utils": ["src/test-utils/index.ts"], - "#test-utils/*": ["src/test-utils/*"] + "#test-utils/*": ["src/test-utils/*"], + "#ui": ["src/ui/index.ts"], + "#ui/*": ["src/ui/*"] } }, "references": [ From ad1ef4ed19f19673ac0d91052ceaef65253bef0a Mon Sep 17 00:00:00 2001 From: Gianluca Carucci Date: Sat, 14 Feb 2026 22:07:12 +0100 Subject: [PATCH 4/4] remove unused default_target_folders --- apps/pair-cli/config.json | 3 +-- apps/pair-cli/src/registry/resolver.ts | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/apps/pair-cli/config.json b/apps/pair-cli/config.json index 67c28460..3765987c 100644 --- a/apps/pair-cli/config.json +++ b/apps/pair-cli/config.json @@ -43,6 +43,5 @@ { "path": ".windsurf/skills/", "mode": "symlink" } ] } - }, - "default_target_folders": { "pair": ".pair", "github": ".github" } + } } diff --git a/apps/pair-cli/src/registry/resolver.ts b/apps/pair-cli/src/registry/resolver.ts index fa6f0b0d..fff779ad 100644 --- a/apps/pair-cli/src/registry/resolver.ts +++ b/apps/pair-cli/src/registry/resolver.ts @@ -19,8 +19,6 @@ export interface RegistryConfig { */ export interface Config { asset_registries: Record - default_target_folders?: Record - folders_to_include?: Record [key: string]: unknown }