diff --git a/.changeset/fix-306-learnings-dedupe-index.md b/.changeset/fix-306-learnings-dedupe-index.md new file mode 100644 index 000000000..4d6d0aa3c --- /dev/null +++ b/.changeset/fix-306-learnings-dedupe-index.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 306 +--- +Build the learnings dedupe index once per bulk import instead of rescanning the whole store on every write (#306). diff --git a/get-shit-done/bin/lib/learnings.cjs b/get-shit-done/bin/lib/learnings.cjs index dc99db3c4..1f59a07ef 100644 --- a/get-shit-done/bin/lib/learnings.cjs +++ b/get-shit-done/bin/lib/learnings.cjs @@ -106,7 +106,33 @@ function learningsWrite(entry, opts) { const hash = contentHash(entry.learning, entry.source_project); - // Check for duplicate by scanning existing files + // #306: In bulk-import paths, callers may supply a pre-built dedupeIndex + // (Map) to avoid the per-write O(N) store scan. + // When present, use it for the dedupe check instead of scanning the directory. + // After writing a new record, add it to the index so subsequent items in the + // same bulk operation dedupe against it. + // When absent (single-write path), fall back to the existing full-store scan. + if (opts && opts.dedupeIndex) { + const dedupeIndex = opts.dedupeIndex; + if (dedupeIndex.has(hash)) { + return { id: dedupeIndex.get(hash), created: false, content_hash: hash }; + } + const id = generateId(); + const record = { + id, + source_project: entry.source_project, + date: new Date().toISOString(), + context: entry.context || '', + learning: entry.learning, + tags: entry.tags || [], + content_hash: hash, + }; + platformWriteSync(path.join(dir, `${id}.json`), JSON.stringify(record, null, 2)); + dedupeIndex.set(hash, id); + return { id, created: true, content_hash: hash }; + } + + // Check for duplicate by scanning existing files (single-write path, unchanged) const files = fs.readdirSync(dir).filter(f => f.endsWith('.json')); for (const file of files) { const existing = readLearningFile(path.join(dir, file)); @@ -229,6 +255,22 @@ function learningsCopyFromProject(planningDir, opts) { const content = fs.readFileSync(learningsPath, 'utf-8'); const sourceProject = (opts && opts.sourceProject) || path.basename(path.resolve(planningDir, '..')); + // #306: Build the content_hash -> id dedupe index once before the loop so + // that learningsWrite does not re-scan the entire store on every call — + // O(K*N) -> O(N+K). + const dir = getStoreDir(opts); + ensureStoreDir(dir); + const dedupeIndex = new Map(); + for (const file of fs.readdirSync(dir).filter(f => f.endsWith('.json'))) { + const existing = readLearningFile(path.join(dir, file)); + // First-seen-wins, matching the legacy scan path's first-match return so the + // dedupe-hit `id` is identical on both paths even if the store already holds + // duplicate content_hashes. (#306) + if (existing && existing.content_hash && !dedupeIndex.has(existing.content_hash)) { + dedupeIndex.set(existing.content_hash, existing.id); + } + } + // Parse markdown: split on ## headings const sections = content.split(/^## /m).slice(1); // skip preamble before first ## let created = 0; @@ -248,7 +290,7 @@ function learningsCopyFromProject(planningDir, opts) { learning: body, context: title, tags, - }, opts); + }, { ...opts, dedupeIndex }); if (result.created) { created++; diff --git a/tests/learnings.test.cjs b/tests/learnings.test.cjs index 2618d823c..1b58d2e41 100644 --- a/tests/learnings.test.cjs +++ b/tests/learnings.test.cjs @@ -524,3 +524,108 @@ describe('CLI integration', () => { assert.ok(res.error.includes('Unknown learnings subcommand')); }); }); + +// ─── Dedupe Scaling (#306) ─────────────────────────────────────────────────── + +/** + * Build a LEARNINGS.md string with k unique ## sections. + * Mirrors the ## heading format that learningsCopyFromProject parses. + */ +function makeLearningsMd(k) { + const sections = []; + for (let i = 1; i <= k; i++) { + sections.push(`## Title ${i}\n\nBody content for item ${i}, unique text.`); + } + return `# Project Learnings\n\n${sections.join('\n\n')}\n`; +} + +describe('learnings dedupe scaling (#306)', () => { + test('store directory scan count is independent of imported item count', (t) => { + // Test A: assert readdirSync call count does NOT scale with K (regression guard for #306) + // BEFORE the fix: count scales 1:1 with K (one scan per learningsWrite call). + // AFTER the fix: count is constant (one index-build scan per learningsCopyFromProject call). + + const storeDir2 = makeTempDir(); + const projectDir2 = makeTempDir(); + t.after(() => { + cleanupDir(storeDir2); + cleanupDir(projectDir2); + }); + + const storeDir6 = makeTempDir(); + const projectDir6 = makeTempDir(); + t.after(() => { + cleanupDir(storeDir6); + cleanupDir(projectDir6); + }); + + // K=2 + fs.writeFileSync(path.join(projectDir2, 'LEARNINGS.md'), makeLearningsMd(2), 'utf-8'); + const spy2 = t.mock.method(fs, 'readdirSync'); + const before2 = spy2.mock.calls.length; + learningsCopyFromProject(projectDir2, { storeDir: storeDir2, sourceProject: 'proj-a' }); + const c1 = spy2.mock.calls.length - before2; + spy2.mock.restore(); + + // K=6 + fs.writeFileSync(path.join(projectDir6, 'LEARNINGS.md'), makeLearningsMd(6), 'utf-8'); + const spy6 = t.mock.method(fs, 'readdirSync'); + const before6 = spy6.mock.calls.length; + learningsCopyFromProject(projectDir6, { storeDir: storeDir6, sourceProject: 'proj-b' }); + const c6 = spy6.mock.calls.length - before6; + spy6.mock.restore(); + + assert.strictEqual(c1, c6, + `store directory scan count must be independent of imported item count (#306) — got ${c1} for K=2 vs ${c6} for K=6`); + }); + + test('dedupe semantics preserved: duplicate entry in existing store is skipped', (t) => { + // Test B part 1: importing a section that duplicates an already-stored entry → skipped + const storeDir = makeTempDir(); + const projectDir = makeTempDir(); + t.after(() => { + cleanupDir(storeDir); + cleanupDir(projectDir); + }); + + // Pre-seed one entry that matches the first section of our LEARNINGS.md + learningsWrite({ + source_project: 'my-proj', + learning: 'Body content for item 1, unique text.', + context: 'Title 1', + tags: ['title'], + }, { storeDir }); + + // LEARNINGS.md has 3 sections; section 1 duplicates the pre-seeded entry + fs.writeFileSync(path.join(projectDir, 'LEARNINGS.md'), makeLearningsMd(3), 'utf-8'); + const result = learningsCopyFromProject(projectDir, { storeDir, sourceProject: 'my-proj' }); + + assert.strictEqual(result.created, 2, 'two new sections should be created'); + assert.strictEqual(result.skipped, 1, 'one duplicate section should be skipped'); + + const all = learningsList({ storeDir }); + assert.strictEqual(all.length, 3); + }); + + test('dedupe semantics preserved: two identical sections in same file → one created, one skipped', (t) => { + // Test B part 2: exercises the index.set during-loop dedup path + const storeDir = makeTempDir(); + const projectDir = makeTempDir(); + t.after(() => { + cleanupDir(storeDir); + cleanupDir(projectDir); + }); + + // Two identical ## sections in the same LEARNINGS.md + const md = `# Learnings\n\n## Duplicate Section\n\nExact same body text.\n\n## Duplicate Section\n\nExact same body text.\n`; + fs.writeFileSync(path.join(projectDir, 'LEARNINGS.md'), md, 'utf-8'); + + const result = learningsCopyFromProject(projectDir, { storeDir, sourceProject: 'my-proj' }); + + assert.strictEqual(result.created, 1, 'exactly one entry should be created'); + assert.strictEqual(result.skipped, 1, 'the duplicate should be skipped'); + + const all = learningsList({ storeDir }); + assert.strictEqual(all.length, 1); + }); +});