Skip to content

Commit fa8bbfe

Browse files
authored
Refactor: Extract DataProvider abstraction from SourceIndexer (#13)
## Summary - Refactors the monolithic `SourceIndexer` class (~450 lines) into clean, separated concerns: - **DataProvider interface** + registry (`src/indexing/providers/`) — defines the contract between data acquisition and indexing - **FileDataProvider** — handles git clone/pull, file walking, pattern matching for all 4 file-based source types - **IndexingPipeline** — source-agnostic chunk → embed → upsert logic - **Shared utilities** (`src/indexing/utils.ts`) — `matchesPatterns`, `globToRegex`, `hasLowSemanticValue` - Migrates orchestrator and seed-index script to use the new abstractions - Deletes `SourceIndexer` entirely (net -562 lines deleted, +854 added across 14 files) - Pure refactor — zero behavioral changes, zero new features This is Phase 1 of the v1.5 Data Provider Abstraction, preparing the codebase for API-based sources (Slack, Notion, etc.) in Phase 2. ## Test plan - [ ] All 534 tests pass locally (54 test files) - [ ] Build clean (`tsc`, zero errors) - [ ] Gate 1: New code coexists with SourceIndexer (Tasks 1-4) - [ ] Gate 2: Orchestrator migrated, full test suite + build passes (Task 5) - [ ] Gate 3: Post-merge smoke test against both production instances: - `./scripts/smoke-test.sh https://mcp.copilotkit.ai` - `./scripts/smoke-test.sh https://mcp.pathfinder.copilotkit.dev` - Both must show identical behavior (same chunks, search results, tools)
2 parents a47da4b + abbbafa commit fa8bbfe

14 files changed

Lines changed: 861 additions & 563 deletions

scripts/seed-index.ts

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@
88
import { initializeSchema, getPool } from '../src/db/client.js';
99
import { getConfig, getServerConfig } from '../src/config.js';
1010
import { EmbeddingClient } from '../src/indexing/embeddings.js';
11-
import { SourceIndexer } from '../src/indexing/source-indexer.js';
11+
import { getProvider } from '../src/indexing/providers/index.js';
12+
import { IndexingPipeline } from '../src/indexing/pipeline.js';
1213

1314
// ---------------------------------------------------------------------------
1415
// Arg parsing
@@ -76,17 +77,18 @@ async function main(): Promise<void> {
7677
const start = Date.now();
7778
console.log(`--- Indexing source: ${sourceConfig.name} (${sourceConfig.type}) ---`);
7879

79-
const indexer = new SourceIndexer(
80-
sourceConfig,
81-
embeddingClient,
82-
config.cloneDir,
83-
config.githubToken || undefined,
84-
);
85-
86-
await indexer.fullIndex();
80+
const provider = getProvider(sourceConfig.type)(sourceConfig, {
81+
cloneDir: config.cloneDir,
82+
githubToken: config.githubToken || undefined,
83+
});
84+
const pipeline = new IndexingPipeline(embeddingClient, sourceConfig);
85+
const result = await provider.fullAcquire();
86+
if (result.items.length > 0) {
87+
await pipeline.indexItems(result.items, result.stateToken);
88+
}
8789

8890
const elapsed = ((Date.now() - start) / 1000).toFixed(1);
89-
console.log(`Source "${sourceConfig.name}" complete in ${elapsed}s\n`);
91+
console.log(`Source "${sourceConfig.name}" indexed ${result.items.length} items in ${elapsed}s\n`);
9092
}
9193

9294
const totalElapsed = ((Date.now() - overallStart) / 1000).toFixed(1);

scripts/test-path-filter.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
// Tests for path filter — verifies include/exclude glob logic in source-indexer
1+
// Tests for path filter — verifies include/exclude glob logic in indexing utils
22
//
33
// Usage: npx tsx scripts/test-path-filter.ts
44

5-
import { globToRegex, matchesPatterns, hasLowSemanticValue } from '../src/indexing/source-indexer.js';
5+
import { globToRegex, matchesPatterns, hasLowSemanticValue } from '../src/indexing/utils.js';
66
import type { SourceConfig } from '../src/types.js';
77

88
let passed = 0;
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
2+
import fs from 'node:fs';
3+
import path from 'node:path';
4+
import os from 'node:os';
5+
import { FileDataProvider } from '../indexing/providers/file.js';
6+
import type { SourceConfig } from '../types.js';
7+
8+
describe('FileDataProvider', () => {
9+
let tmpDir: string;
10+
11+
beforeEach(async () => {
12+
tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'fp-test-'));
13+
await fs.promises.writeFile(path.join(tmpDir, 'readme.md'), '# Hello\nWorld');
14+
await fs.promises.writeFile(path.join(tmpDir, 'guide.md'), '# Guide\nContent here');
15+
await fs.promises.writeFile(path.join(tmpDir, 'style.css'), 'body { color: red; }');
16+
await fs.promises.mkdir(path.join(tmpDir, 'sub'), { recursive: true });
17+
await fs.promises.writeFile(path.join(tmpDir, 'sub', 'nested.md'), '# Nested');
18+
});
19+
20+
afterEach(async () => {
21+
await fs.promises.rm(tmpDir, { recursive: true, force: true });
22+
});
23+
24+
function makeConfig(overrides?: Partial<SourceConfig>): SourceConfig {
25+
return {
26+
name: 'test',
27+
type: 'markdown',
28+
path: tmpDir,
29+
file_patterns: ['**/*.md'],
30+
chunk: { target_tokens: 600, overlap_tokens: 50 },
31+
...overrides,
32+
};
33+
}
34+
35+
it('fullAcquire returns matching files as ContentItems', async () => {
36+
const provider = new FileDataProvider(makeConfig(), { cloneDir: '/tmp/test-clones' });
37+
const result = await provider.fullAcquire();
38+
expect(result.items.length).toBe(3);
39+
expect(result.removedIds).toEqual([]);
40+
expect(result.stateToken).toMatch(/^local-/);
41+
const ids = result.items.map(i => i.id).sort();
42+
expect(ids).toContain('readme.md');
43+
expect(ids).toContain('guide.md');
44+
expect(ids).toContain('sub/nested.md');
45+
const readme = result.items.find(i => i.id === 'readme.md');
46+
expect(readme?.content).toBe('# Hello\nWorld');
47+
});
48+
49+
it('fullAcquire excludes non-matching patterns', async () => {
50+
const provider = new FileDataProvider(makeConfig(), { cloneDir: '/tmp/test-clones' });
51+
const result = await provider.fullAcquire();
52+
const ids = result.items.map(i => i.id);
53+
expect(ids).not.toContain('style.css');
54+
});
55+
56+
it('fullAcquire filters out low-semantic-value content', async () => {
57+
const svgContent = 'M0,0 L100,100 C50,50 200.5,300.7 '.repeat(100);
58+
await fs.promises.writeFile(path.join(tmpDir, 'data.md'), svgContent);
59+
const provider = new FileDataProvider(makeConfig(), { cloneDir: '/tmp/test-clones' });
60+
const result = await provider.fullAcquire();
61+
const ids = result.items.map(i => i.id);
62+
expect(ids).not.toContain('data.md');
63+
});
64+
65+
it('getCurrentStateToken returns local hash for local sources', async () => {
66+
const provider = new FileDataProvider(makeConfig(), { cloneDir: '/tmp/test-clones' });
67+
const token = await provider.getCurrentStateToken();
68+
expect(token).toMatch(/^local-/);
69+
});
70+
71+
it('getCurrentStateToken returns null when path does not exist', async () => {
72+
const provider = new FileDataProvider(
73+
makeConfig({ path: '/nonexistent/path' }),
74+
{ cloneDir: '/tmp/test-clones' },
75+
);
76+
const token = await provider.getCurrentStateToken();
77+
expect(token).toBeNull();
78+
});
79+
80+
it('incrementalAcquire falls back to fullAcquire for local sources', async () => {
81+
const provider = new FileDataProvider(makeConfig(), { cloneDir: '/tmp/test-clones' });
82+
const result = await provider.incrementalAcquire('old-token');
83+
expect(result.items.length).toBe(3);
84+
});
85+
});

src/__tests__/pipeline.test.ts

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
import { describe, it, expect, vi } from 'vitest';
2+
import { IndexingPipeline } from '../indexing/pipeline.js';
3+
import type { ContentItem } from '../indexing/providers/types.js';
4+
import type { SourceConfig } from '../types.js';
5+
6+
// Mock the dependencies
7+
vi.mock('../indexing/chunking/index.js', () => ({
8+
getChunker: vi.fn().mockReturnValue(
9+
(content: string, _filePath: string, _config: unknown) => [{
10+
content,
11+
title: 'Test Title',
12+
chunkIndex: 0,
13+
}],
14+
),
15+
}));
16+
17+
vi.mock('../indexing/embeddings.js', () => {
18+
const MockEmbeddingClient = vi.fn().mockImplementation(function (this: Record<string, unknown>) {
19+
this.embedBatch = vi.fn().mockResolvedValue([[0.1, 0.2, 0.3]]);
20+
});
21+
return { EmbeddingClient: MockEmbeddingClient };
22+
});
23+
24+
vi.mock('../db/queries.js', () => ({
25+
upsertChunks: vi.fn().mockResolvedValue(undefined),
26+
deleteChunksByFile: vi.fn().mockResolvedValue(undefined),
27+
}));
28+
29+
vi.mock('../indexing/url-derivation.js', () => ({
30+
deriveUrl: () => 'https://example.com/test',
31+
}));
32+
33+
const { upsertChunks, deleteChunksByFile } = await import('../db/queries.js');
34+
const { EmbeddingClient } = await import('../indexing/embeddings.js');
35+
36+
const testConfig: SourceConfig = {
37+
name: 'test-source',
38+
type: 'markdown',
39+
path: 'docs/',
40+
file_patterns: ['**/*.md'],
41+
chunk: { target_tokens: 600, overlap_tokens: 50 },
42+
};
43+
44+
describe('IndexingPipeline', () => {
45+
it('indexes items: chunk → embed → delete old → upsert', async () => {
46+
const embeddingClient = new EmbeddingClient('key', 'model', 1536);
47+
const pipeline = new IndexingPipeline(embeddingClient, testConfig);
48+
49+
const items: ContentItem[] = [{
50+
id: 'docs/test.md',
51+
content: '# Hello\nSome content here',
52+
}];
53+
54+
await pipeline.indexItems(items, 'abc123');
55+
56+
expect(deleteChunksByFile).toHaveBeenCalledWith('test-source', 'docs/test.md');
57+
expect(upsertChunks).toHaveBeenCalledWith(
58+
expect.arrayContaining([
59+
expect.objectContaining({
60+
source_name: 'test-source',
61+
file_path: 'docs/test.md',
62+
commit_sha: 'abc123',
63+
}),
64+
]),
65+
);
66+
});
67+
68+
it('skips items that produce zero chunks', async () => {
69+
const { getChunker } = await import('../indexing/chunking/index.js');
70+
vi.mocked(getChunker).mockReturnValueOnce(() => []);
71+
72+
const embeddingClient = new EmbeddingClient('key', 'model', 1536);
73+
const pipeline = new IndexingPipeline(embeddingClient, testConfig);
74+
75+
vi.mocked(upsertChunks).mockClear();
76+
await pipeline.indexItems([{ id: 'empty.md', content: '' }], 'abc');
77+
expect(upsertChunks).not.toHaveBeenCalled();
78+
});
79+
80+
it('removes items by ID', async () => {
81+
const embeddingClient = new EmbeddingClient('key', 'model', 1536);
82+
const pipeline = new IndexingPipeline(embeddingClient, testConfig);
83+
84+
vi.mocked(deleteChunksByFile).mockClear();
85+
await pipeline.removeItems(['docs/old.md', 'docs/deleted.md']);
86+
87+
expect(deleteChunksByFile).toHaveBeenCalledTimes(2);
88+
expect(deleteChunksByFile).toHaveBeenCalledWith('test-source', 'docs/old.md');
89+
expect(deleteChunksByFile).toHaveBeenCalledWith('test-source', 'docs/deleted.md');
90+
});
91+
92+
it('passes sourceUrl from ContentItem when provided', async () => {
93+
const embeddingClient = new EmbeddingClient('key', 'model', 1536);
94+
const pipeline = new IndexingPipeline(embeddingClient, testConfig);
95+
96+
vi.mocked(upsertChunks).mockClear();
97+
await pipeline.indexItems([{
98+
id: 'docs/test.md',
99+
content: 'Content',
100+
sourceUrl: 'https://custom.url/test',
101+
}], 'abc');
102+
103+
expect(upsertChunks).toHaveBeenCalledWith(
104+
expect.arrayContaining([
105+
expect.objectContaining({
106+
source_url: 'https://custom.url/test',
107+
}),
108+
]),
109+
);
110+
});
111+
});
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { describe, it, expect, beforeEach } from 'vitest';
2+
import { registerProvider, getProvider } from '../indexing/providers/index.js';
3+
import type { DataProvider, ProviderOptions } from '../indexing/providers/types.js';
4+
import type { SourceConfig } from '../types.js';
5+
6+
describe('provider registry', () => {
7+
it('registers and retrieves a provider factory', () => {
8+
const mockFactory = (config: SourceConfig, options: ProviderOptions): DataProvider => ({
9+
fullAcquire: async () => ({ items: [], removedIds: [], stateToken: 'test' }),
10+
incrementalAcquire: async () => ({ items: [], removedIds: [], stateToken: 'test' }),
11+
getCurrentStateToken: async () => 'test',
12+
});
13+
14+
registerProvider('test-type', mockFactory);
15+
const factory = getProvider('test-type');
16+
expect(factory).toBe(mockFactory);
17+
});
18+
19+
it('throws for unknown provider type', () => {
20+
expect(() => getProvider('nonexistent')).toThrow('Unknown provider type: "nonexistent"');
21+
});
22+
});

src/__tests__/utils.test.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { globToRegex, matchesPatterns, hasLowSemanticValue } from '../indexing/utils.js';
3+
import type { SourceConfig } from '../types.js';
4+
5+
describe('globToRegex', () => {
6+
it('matches ** glob patterns', () => {
7+
expect(globToRegex('**/*.ts').test('src/foo.ts')).toBe(true);
8+
expect(globToRegex('**/*.ts').test('foo.ts')).toBe(true);
9+
expect(globToRegex('**/*.ts').test('foo.js')).toBe(false);
10+
});
11+
12+
it('matches * glob patterns', () => {
13+
expect(globToRegex('*.ts').test('foo.ts')).toBe(true);
14+
expect(globToRegex('*.ts').test('src/foo.ts')).toBe(false);
15+
});
16+
});
17+
18+
describe('matchesPatterns', () => {
19+
const config: SourceConfig = {
20+
name: 'test',
21+
type: 'code',
22+
path: '.',
23+
file_patterns: ['**/*.ts', '**/*.tsx'],
24+
exclude_patterns: ['**/test/**', '**/*.test.*'],
25+
chunk: { target_lines: 80, overlap_lines: 10 },
26+
};
27+
28+
it('includes matching files', () => {
29+
expect(matchesPatterns('src/index.ts', config)).toBe(true);
30+
expect(matchesPatterns('src/deep/path/file.tsx', config)).toBe(true);
31+
});
32+
33+
it('excludes matching patterns', () => {
34+
expect(matchesPatterns('src/test/helper.ts', config)).toBe(false);
35+
expect(matchesPatterns('src/foo.test.ts', config)).toBe(false);
36+
});
37+
38+
it('rejects non-matching extensions', () => {
39+
expect(matchesPatterns('src/index.js', config)).toBe(false);
40+
});
41+
});
42+
43+
describe('hasLowSemanticValue', () => {
44+
it('returns false for short content', () => {
45+
expect(hasLowSemanticValue('short')).toBe(false);
46+
});
47+
48+
it('returns true for SVG-like content', () => {
49+
const svgData = 'M0,0 L100,100 C50,50 200.5,300.7 '.repeat(100);
50+
expect(hasLowSemanticValue(svgData)).toBe(true);
51+
});
52+
53+
it('returns false for normal text', () => {
54+
const text = 'This is a normal document with some text content that describes how things work. '.repeat(20);
55+
expect(hasLowSemanticValue(text)).toBe(false);
56+
});
57+
});

0 commit comments

Comments
 (0)