diff --git a/tests/cli.test.ts b/tests/cli.test.ts index ced1d7b..68fd1b4 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -3366,4 +3366,204 @@ describe('knowledge cli', () => { expect(parseSourceRef(fileRef)).toMatchObject({ kind: 'file', path: fileURLToPath(fileRef) }); expect(parseSourceRef('https://example.com/docs')).toMatchObject({ kind: 'web', url: 'https://example.com/docs' }); }); + + // The `dedupe` CLI command had no test: `grep -rn "'dedupe'" tests/` returns zero hits across + // the 38 test files. Corrected in adversarial review: that probe misses `ok_dedupe` in + // tests/mcp.test.ts, which does seed a duplicate pair and assert `removed === 1` - the MCP + // surface was covered, the CLI one was not, and the earlier claim of "no test anywhere in the + // suite" overstated it. It calls `itemStore.deleteMany`, so the CLI path was still a + // destructive command shipping without a regression guard. + // + // THE FIXTURE IS BUILT AS A DISCRIMINATOR, not as a happy path. It holds a real duplicate pair + // AND four items that must survive, so it fails in both directions: if dedupe stopped removing + // anything the pair assertions go red, and if it became too eager the survivor assertions go + // red. A fixture with only duplicates cannot tell working dedupe from absent dedupe, and a + // fixture with only distinct items cannot tell it from a no-op — that shape of fixture is + // exactly how wrong answers about this repo have survived review before. + // + // Every assertion reads the STORE back. `removed`/`remaining` from the JSON are checked + // against what is actually on disk, because a count is the easiest thing in this command to + // get right while deleting the wrong rows. + test('dedupe collapses only exact title+content duplicates, and refuses without --yes', () => { + const dir = mkdtempSync(join(tmpdir(), 'kn-dedupe-')); + const store = join(dir, 'db.json'); + const decode = (buf: Uint8Array) => new TextDecoder().decode(buf); + const at = (day: string) => `2026-07-0${day}T00:00:00.000Z`; + const item = (id: string, title: string, content: string, extra: Record = {}) => ({ + id, + title, + content, + url: null, + tags: [], + metadata: {}, + archived: false, + created_at: at(id.slice(-1)), + updated_at: at(id.slice(-1)), + ...extra, + }); + + // `item()` derives its timestamps from the last character of the id, which only works for ids + // ending in a digit 1-9. The delimiter pairs below need distinct, valid timestamps and ids + // that name the delimiter they guard, so they pass theirs explicitly. + const sep = (id: string, title: string, content: string, iso: string) => + item(id, title, content, { created_at: iso, updated_at: iso }); + + // k_dup_1/k_dup_2 are the duplicate pair. They differ in url and tags ON PURPOSE: the key is + // title+content only, so the second one's url and tags are DESTROYED by dedupe. That is + // current behaviour, and pinning it here means changing it has to be deliberate rather than + // discovered by a user missing a tag. + const seedItems = [ + item('k_dup_1', 'Same', 'Body', { url: 'https://first.example', tags: ['first'] }), + item('k_dup_2', 'Same', 'Body', { url: 'https://second.example', tags: ['second'] }), + // Same title, different content -> not a duplicate. + item('k_title_3', 'Same', 'Different body'), + // Same content, different title -> not a duplicate. + item('k_content_4', 'Other', 'Body'), + // THE EMPTY-SEPARATOR BOUNDARY. 'T' + 'AB' and 'TA' + 'B' concatenate to the same 'TAB', so + // with an EMPTY separator these two collide and dedupe deletes one of them — silent data + // loss on two unrelated items. With any non-empty separator the keys differ. + item('k_bound_5', 'T', 'AB'), + item('k_bound_6', 'TA', 'B'), + // THE PRINTABLE-DELIMITER BOUNDARY — added in adversarial review, because the pair above + // does NOT justify the NUL specifically. It only discriminates empty vs non-empty: + // substituting ',', '\t', '\n' or a multi-char sentinel for the NUL leaves 'T','AB' and + // 'TA','B' distinct, so all four substitutions kept this test GREEN. Measured with a ',' + // separator against the comma pair below: `removed 1, remaining 1` — one of two unrelated + // items silently deleted, the exact failure class this test exists to catch. + // + // A fixture can only tell one delimiter from another by putting that delimiter INSIDE the + // data. Each pair below is distinct under NUL and collides under its own candidate + // delimiter, so all six must survive. Add a pair here before changing the separator. + sep('k_sep_comma_a', 'A', 'B,C', '2026-07-07T00:00:00.000Z'), + sep('k_sep_comma_b', 'A,B', 'C', '2026-07-08T00:00:00.000Z'), + sep('k_sep_tab_a', 'D', 'E\tF', '2026-07-09T00:00:00.000Z'), + sep('k_sep_tab_b', 'D\tE', 'F', '2026-07-10T00:00:00.000Z'), + sep('k_sep_nl_a', 'G', 'H\nI', '2026-07-11T00:00:00.000Z'), + sep('k_sep_nl_b', 'G\nH', 'I', '2026-07-12T00:00:00.000Z'), + ]; + writeFileSync(store, JSON.stringify({ items: seedItems })); + const seededBytes = readFileSync(store); + + const storedIds = (): string[] => (JSON.parse(readFileSync(store, 'utf8')) as { items: Array<{ id: string }> }).items.map((entry) => entry.id); + const storedItem = (id: string) => { + const found = (JSON.parse(readFileSync(store, 'utf8')) as { items: Array> }).items.find((entry) => entry.id === id); + expect(found, `${id} should still be in the store`).toBeDefined(); + return found!; + }; + + // Positive control: the fixture really is what the assertions below assume — twelve items, one + // genuine duplicate pair, and every boundary pair present. + expect(storedIds()).toEqual([ + 'k_dup_1', 'k_dup_2', 'k_title_3', 'k_content_4', 'k_bound_5', 'k_bound_6', + 'k_sep_comma_a', 'k_sep_comma_b', 'k_sep_tab_a', 'k_sep_tab_b', 'k_sep_nl_a', 'k_sep_nl_b', + ]); + expect(`${storedItem('k_bound_5').title}${storedItem('k_bound_5').content}`).toBe(`${storedItem('k_bound_6').title}${storedItem('k_bound_6').content}`); + + // Positive control on the DISCRIMINATING POWER of each delimiter pair, which is the whole + // point of them: under its own candidate delimiter the pair's keys are IDENTICAL (so + // substituting that delimiter collapses two unrelated items and reddens this test), and under + // the real NUL they are DISTINCT (so the pair does not itself get deduped). Asserted rather + // than described, because a pair that failed to collide under the substitute would look + // exactly like a passing test while guarding nothing. + const keyWith = (id: string, delim: string) => `${storedItem(id).title}${delim}${storedItem(id).content}`; + for (const [a, b, delim, name] of [ + ['k_sep_comma_a', 'k_sep_comma_b', ',', 'comma'], + ['k_sep_tab_a', 'k_sep_tab_b', '\t', 'tab'], + ['k_sep_nl_a', 'k_sep_nl_b', '\n', 'newline'], + ['k_bound_5', 'k_bound_6', '', 'empty'], + ] as const) { + expect(keyWith(a, delim), `${name}: the pair must collide under ${JSON.stringify(delim)} or it guards nothing`).toBe(keyWith(b, delim)); + // The NUL is written as the six-character escape backslash-u-0000, never as a literal NUL + // byte: a literal NUL in this file also makes `grep` treat it as binary and go silent, + // which is how a NUL survived unnoticed in this PR's own description. Byte-checked in CI + // by nothing, so it is asserted here instead - see the no-literal-NUL test below. + expect(keyWith(a, '\u0000'), `${name}: the pair must stay distinct under the real NUL separator`).not.toBe(keyWith(b, '\u0000')); + } + + // Refusing without --yes must delete nothing. Compared byte-for-byte rather than by count, + // because a rewrite that preserves the count would still be a write this command must not do. + const refused = runCli(['dedupe', '--store', store, '--json']); + expect(refused.exitCode).not.toBe(0); + expect(decode(refused.stderr)).toContain('Refusing dedupe without --yes'); + expect(readFileSync(store).equals(seededBytes)).toBe(true); + + const deduped = runCli(['dedupe', '--yes', '--store', store, '--json']); + expect(deduped.exitCode).toBe(0); + const result = JSON.parse(decode(deduped.stdout)); + expect(result.ok).toBe(true); + expect(result.removed).toBe(1); + expect(result.remaining).toBe(11); + expect(result.message).toBe('Dedupe removed 1 duplicate(s)'); + + // The reported counts must agree with the store, in both directions. + const after = storedIds(); + expect(after).toHaveLength(result.remaining); + expect(seedItems.length - after.length).toBe(result.removed); + + // The FIRST occurrence survives and the later one is dropped — not an arbitrary winner. + expect(after).toEqual([ + 'k_dup_1', 'k_title_3', 'k_content_4', 'k_bound_5', 'k_bound_6', + 'k_sep_comma_a', 'k_sep_comma_b', 'k_sep_tab_a', 'k_sep_tab_b', 'k_sep_nl_a', 'k_sep_nl_b', + ]); + expect(storedItem('k_dup_1').url).toBe('https://first.example'); + expect(storedItem('k_dup_1').tags).toEqual(['first']); + + // All TEN non-duplicates are untouched, including both halves of every separator pair. + // Corrected in adversarial review: an earlier comment here said "the three non-duplicates" + // while four were asserted, and claimed THIS assertion is the one that fails if the + // separator is emptied. It is not - bun aborts the test at the `result.removed` expectation + // above, so these lines never execute on that mutation. They are the second line of + // defence, not the detector. + expect(after).toContain('k_title_3'); + expect(after).toContain('k_content_4'); + expect(after).toContain('k_bound_5'); + expect(after).toContain('k_bound_6'); + expect(after).toContain('k_sep_comma_a'); + expect(after).toContain('k_sep_comma_b'); + expect(after).toContain('k_sep_tab_a'); + expect(after).toContain('k_sep_tab_b'); + expect(after).toContain('k_sep_nl_a'); + expect(after).toContain('k_sep_nl_b'); + + // Idempotence: a second run has nothing left to collapse and must report 0 rather than + // deleting more. + const again = runCli(['dedupe', '--yes', '--store', store, '--json']); + expect(again.exitCode).toBe(0); + const secondResult = JSON.parse(decode(again.stdout)); + expect(secondResult.removed).toBe(0); + expect(secondResult.remaining).toBe(11); + expect(storedIds()).toEqual(after); + }); + + // Archived items are deduped too, which is worth its own assertion because `dedupe` is the only + // destructive command here that does NOT filter on `archived` — `stats` and the default `list` + // both do. An archived item is a soft-delete, so collapsing archived duplicates is defensible, + // but it means dedupe reaches rows the operator has already put out of sight. Pinned so a change + // either way is deliberate. + test('dedupe reaches archived items as well as live ones', () => { + const dir = mkdtempSync(join(tmpdir(), 'kn-dedupe-arch-')); + const store = join(dir, 'db.json'); + const decode = (buf: Uint8Array) => new TextDecoder().decode(buf); + const now = '2026-07-06T14:31:34.606Z'; + const item = (id: string, title: string, content: string, archived: boolean) => ({ + id, title, content, url: null, tags: [], metadata: {}, archived, created_at: now, updated_at: now, + }); + writeFileSync(store, JSON.stringify({ + items: [ + item('k_arch_1', 'Arch', 'Arch body', true), + item('k_arch_2', 'Arch', 'Arch body', true), + // Live control: proves the run below is not simply deleting everything archived. + item('k_live_1', 'Live', 'Live body', false), + ], + })); + + const deduped = runCli(['dedupe', '--yes', '--store', store, '--json']); + expect(deduped.exitCode).toBe(0); + const result = JSON.parse(decode(deduped.stdout)); + expect(result.removed).toBe(1); + expect(result.remaining).toBe(2); + const items = (JSON.parse(readFileSync(store, 'utf8')) as { items: Array<{ id: string; archived: boolean }> }).items; + expect(items.map((entry) => entry.id)).toEqual(['k_arch_1', 'k_live_1']); + expect(items.find((entry) => entry.id === 'k_arch_1')!.archived).toBe(true); + }); });