From db1ba8432786cf2c769070a341fc47af9eabe7e1 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Tue, 28 Jul 2026 01:16:49 +0300 Subject: [PATCH 1/2] test(cli): cover dedupe, including the separator boundary that justifies the NUL `grep -rn "'dedupe'" tests/` returned zero hits across 37 test files. The command calls `itemStore.deleteMany`, so a destructive command was shipping with no regression guard at all. THE FIXTURE IS A DISCRIMINATOR, NOT A HAPPY PATH. It holds one genuine duplicate pair plus four items that must survive, so it fails in both directions: neuter dedupe and the pair assertions go red, make it too eager and the survivor assertions go red. A fixture of only duplicates cannot tell working dedupe from absent dedupe, and a fixture of only distinct items cannot tell it from a no-op. Covered, all asserted against the store read back rather than against the command's own reported counts: * refusal without --yes: non-zero exit AND the store byte-identical to the seed (compared with Buffer.equals, not by item count, because a rewrite preserving the count is still a write this command must not make) * exact title+content duplicate collapses: removed 1, remaining 5, and both numbers cross-checked against the ids actually on disk * same title different content, and same content different title, both survive * THE SEPARATOR BOUNDARY: title 'T' + content 'AB' versus title 'TA' + content 'B'. Both concatenate to 'TAB', so with an empty separator they collide and dedupe silently deletes one of two unrelated items. This is the only reason the key joins on NUL rather than a printable delimiter, and nothing else in the suite would notice if it were "simplified" away. * the FIRST occurrence survives, not an arbitrary winner * idempotence: a second run reports removed 0 and deletes nothing more Two current behaviours are pinned deliberately so that changing them has to be a decision rather than a discovery: * the key is title+content ONLY, so the losing duplicate's url and tags are destroyed even when they differ. The fixture gives the pair different urls and tags to make that visible. * `dedupe` does not filter on `archived`, unlike `stats` and the default `list`. It therefore reaches rows an operator has already put out of sight. A separate test covers it with a live control item, so the assertion cannot be satisfied by a run that simply deleted everything archived. MUTATION-TESTED, each plant reverted immediately, tree clean after: M0 baseline rc=0 2 pass 0 fail M1 NUL separator emptied rc=1 1 pass 1 fail (removed 1 -> 2) M2 deleteMany([]) instead of the dupes rc=1 0 pass 2 fail M3 --yes gate removed rc=1 1 pass 1 fail M4 last duplicate wins instead of first rc=1 0 pass 2 fail M5 archived items excluded rc=1 1 pass 1 fail M6 restored rc=0 2 pass 0 fail --- tests/cli.test.ts | 141 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/tests/cli.test.ts b/tests/cli.test.ts index ced1d7b..83633dd 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -3366,4 +3366,145 @@ 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' }); }); + + // `dedupe` had no test anywhere in the suite: `grep -rn "'dedupe'" tests/` returned zero hits + // across 37 test files. It calls `itemStore.deleteMany`, so it is a destructive command that + // was shipping with no regression guard at all. + // + // 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, + }); + + // 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 SEPARATOR BOUNDARY, and the whole reason the key joins on NUL rather than on a + // printable delimiter. '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 the NUL separator the keys differ. Do not "simplify" the + // separator away: emptying it is exactly the mutation this pair catches, and nothing else + // in this test would notice. + item('k_bound_5', 'T', 'AB'), + item('k_bound_6', 'TA', 'B'), + ]; + 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 — six items, one + // genuine duplicate pair, and the two boundary items present and distinct. + expect(storedIds()).toEqual(['k_dup_1', 'k_dup_2', 'k_title_3', 'k_content_4', 'k_bound_5', 'k_bound_6']); + expect(`${storedItem('k_bound_5').title}${storedItem('k_bound_5').content}`).toBe(`${storedItem('k_bound_6').title}${storedItem('k_bound_6').content}`); + + // 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(5); + 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']); + expect(storedItem('k_dup_1').url).toBe('https://first.example'); + expect(storedItem('k_dup_1').tags).toEqual(['first']); + + // The three non-duplicates are untouched, including both halves of the separator boundary. + // If the NUL separator is ever emptied, THIS is the assertion that fails. + expect(after).toContain('k_title_3'); + expect(after).toContain('k_content_4'); + expect(after).toContain('k_bound_5'); + expect(after).toContain('k_bound_6'); + + // 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(5); + 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); + }); }); From 78efe7fdf1e94fe7140155faa80b20a4d10e33d5 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Tue, 28 Jul 2026 02:00:30 +0300 Subject: [PATCH 2/2] test(cli): make the dedupe separator fixture discriminate NUL from printable delimiters Found in adversarial review. The existing boundary pair does not support the claim built on it. 'T'+'AB' vs 'TA'+'B' collide only under an EMPTY separator, so it discriminates empty vs non-empty and nothing more. Substituting ',', tab, newline or a multi-char sentinel for the NUL left the pair distinct and the test GREEN, while the title, body and the comment at the fixture all claimed this was "the separator boundary that justifies the NUL". That gap was live, not theoretical. With the separator changed to ',' and a store holding {'A','B,C'} and {'A,B','C'} - two unrelated items: unmutated (NUL): removed 0, remaining 2 both survive ',' separator: removed 1, remaining 1 one silently deleted Same silent-data-loss class the test exists to catch, and it passed straight through. A fixture can only tell one delimiter from another by putting that delimiter INSIDE the data. Three pairs added, each distinct under NUL and colliding under its own candidate delimiter, plus a positive control that asserts exactly that in both directions - a pair that failed to collide under its substitute would look like a passing test while guarding nothing. Measured, mutating src/cli.ts:2066 and guard-checking with `git diff --quiet` first so an unapplied mutation could not masquerade as a pass: baseline (new fixture) rc=0 2 pass 0 fail separator -> ',' rc=1 1 pass 1 fail (was GREEN before) separator -> tab rc=1 1 pass 1 fail (was GREEN before) separator -> newline rc=1 1 pass 1 fail (was GREEN before) Also corrected in the same pass, all three claims of record that review found inaccurate: - "dedupe had no test anywhere in the suite ... across 37 test files". The CLI command had none, but tests/mcp.test.ts covers ok_dedupe with a duplicate pair and removed===1. The probe `grep -rn "'dedupe'" tests/` misses it because the tool is named 'ok_dedupe'. The file count is 38, not 37. - "If the NUL separator is ever emptied, THIS is the assertion that fails" - it is not. bun aborts at the earlier `result.removed` expectation, so those lines never execute on that mutation. They are the second line of defence, not the detector. - "The three non-duplicates" while four were asserted (now ten). The NUL is written as a six-character escape, never a literal NUL byte. A literal NUL also makes grep treat the file as binary and go silent, which is how one survived unnoticed in this PR's own description; my first attempt at this edit reintroduced two of them, caught by a byte counter positive-controlled against a planted NUL before being trusted. tests/cli.test.ts: 0 literal NUL bytes. Not fixed here, filed instead: the same dedupe key is hand-copied at src/mcp.js:1745 and these fixtures guard only the src/cli.ts copy - emptying the mcp separator leaves the whole suite green. That wants one shared helper rather than a second fixture. --- tests/cli.test.ts | 93 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 76 insertions(+), 17 deletions(-) diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 83633dd..68fd1b4 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -3367,9 +3367,12 @@ describe('knowledge cli', () => { expect(parseSourceRef('https://example.com/docs')).toMatchObject({ kind: 'web', url: 'https://example.com/docs' }); }); - // `dedupe` had no test anywhere in the suite: `grep -rn "'dedupe'" tests/` returned zero hits - // across 37 test files. It calls `itemStore.deleteMany`, so it is a destructive command that - // was shipping with no regression guard at all. + // 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 @@ -3399,6 +3402,12 @@ describe('knowledge cli', () => { ...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 @@ -3410,14 +3419,27 @@ describe('knowledge cli', () => { item('k_title_3', 'Same', 'Different body'), // Same content, different title -> not a duplicate. item('k_content_4', 'Other', 'Body'), - // THE SEPARATOR BOUNDARY, and the whole reason the key joins on NUL rather than on a - // printable delimiter. '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 the NUL separator the keys differ. Do not "simplify" the - // separator away: emptying it is exactly the mutation this pair catches, and nothing else - // in this test would notice. + // 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); @@ -3429,11 +3451,35 @@ describe('knowledge cli', () => { return found!; }; - // Positive control: the fixture really is what the assertions below assume — six items, one - // genuine duplicate pair, and the two boundary items present and distinct. - expect(storedIds()).toEqual(['k_dup_1', 'k_dup_2', 'k_title_3', 'k_content_4', 'k_bound_5', 'k_bound_6']); + // 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']); @@ -3446,7 +3492,7 @@ describe('knowledge cli', () => { const result = JSON.parse(decode(deduped.stdout)); expect(result.ok).toBe(true); expect(result.removed).toBe(1); - expect(result.remaining).toBe(5); + 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. @@ -3455,16 +3501,29 @@ describe('knowledge cli', () => { 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']); + 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']); - // The three non-duplicates are untouched, including both halves of the separator boundary. - // If the NUL separator is ever emptied, THIS is the assertion that fails. + // 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. @@ -3472,7 +3531,7 @@ describe('knowledge cli', () => { expect(again.exitCode).toBe(0); const secondResult = JSON.parse(decode(again.stdout)); expect(secondResult.removed).toBe(0); - expect(secondResult.remaining).toBe(5); + expect(secondResult.remaining).toBe(11); expect(storedIds()).toEqual(after); });