diff --git a/bin/knowledge-mcp.js b/bin/knowledge-mcp.js index 3b48598..871532f 100755 --- a/bin/knowledge-mcp.js +++ b/bin/knowledge-mcp.js @@ -37517,7 +37517,7 @@ function buildServer() { const item = await store.update(existing.id, patch); return item ? jsonText({ ok: true, item }) : errorText(`Item not found: ${id}`); }); - registerTool(server, "ok_untag", "Remove tags from a knowledge item", "Remove specific tags from an item", { + registerTool(server, "ok_untag", "Remove tags from a knowledge item", "Remove specific tags from an item. Each value matches a stored tag whole before being split on commas. Removing nothing is an error, so removed is never 0.", { id: exports_external.string(), tags: exports_external.array(exports_external.string()), store_path: storePathField, @@ -37527,11 +37527,34 @@ function buildServer() { const current = await store.get(id); if (!current) return errorText(`Item not found: ${id}`); - const remove = new Set(tags.map((tag) => tag.toLowerCase())); - const before = (current.tags ?? []).length; - const nextTags = (current.tags ?? []).filter((tag) => !remove.has(tag.toLowerCase())); + const before = current.tags ?? []; + const stored = new Set(before.map((tag) => tag.toLowerCase())); + const remove = new Set; + for (const raw of tags) { + const whole = raw.trim().toLowerCase(); + if (whole.length > 0 && stored.has(whole)) { + remove.add(whole); + continue; + } + for (const name of raw.split(",").map((tag) => tag.trim().toLowerCase()).filter((tag) => tag.length > 0)) + remove.add(name); + } + if (remove.size === 0) + return errorText(`No tag names given for ${current.id}. Each entry must contain at least one non-empty tag name.`); + const nextTags = before.filter((tag) => !remove.has(tag.toLowerCase())); + const removed = before.length - nextTags.length; + const notFound = [...remove].filter((tag) => !stored.has(tag)); + if (removed === 0) { + return errorText(`No matching tag on ${current.id}: ${notFound.map((tag) => JSON.stringify(tag)).join(", ")} not in [${before.map((tag) => JSON.stringify(tag)).join(", ")}]`); + } const item = await store.update(current.id, { tags: nextTags }); - return item ? jsonText({ ok: true, item, removed: before - nextTags.length }) : errorText(`Item not found: ${id}`); + if (!item) + return errorText(`Item not found: ${id}`); + const missed = notFound.length > 0 ? ` (not found: ${notFound.map((tag) => JSON.stringify(tag)).join(", ")})` : ""; + const result = { ok: true, item, removed, message: `Removed ${removed} tag${removed === 1 ? "" : "s"} from ${item.id}${missed}` }; + if (notFound.length > 0) + result.not_found = notFound; + return jsonText(result); }); registerTool(server, "ok_bulk_delete", "Bulk delete knowledge items", "Delete multiple items by tag or search. Requires confirm=true.", { tag: exports_external.array(exports_external.string()).optional(), diff --git a/src/mcp.js b/src/mcp.js index 56ceab1..2b4691a 100644 --- a/src/mcp.js +++ b/src/mcp.js @@ -1668,7 +1668,36 @@ export function buildServer() { return item ? jsonText({ ok: true, item }) : errorText(`Item not found: ${id}`); }); - registerTool(server, 'ok_untag', 'Remove tags from a knowledge item', 'Remove specific tags from an item', { + // Ported from `untag` in src/cli.ts, which is the reference implementation for this + // behaviour. Three defects were fixed here, all measured over real stdio first: + // + // (a) `removed: 0` came back as `ok: true`. A success signal that cannot tell 1 removed + // from 0 is the same defect class as the multi-tag bug this tool's CLI twin was fixed + // for. It also called `store.update` before checking, so an untag that removed + // nothing still bumped `updated_at` — a write with no effect, recorded as one. + // (b) it never matched a comma-bearing value against separately-stored tags, so + // `tags: ["a,b,c"]` against an item holding a, b and c removed 0 while the CLI + // removed 3. + // (c) a partial miss reported nothing: `tags: ["alpha","nope"]` on an item holding only + // alpha returned `removed: 1` with no mention of "nope". + // + // WHOLE VALUE FIRST, THEN SPLIT — and note the scope precisely, because it has been + // misstated twice: the `continue` short-circuits ONE raw value, not the whole call. + // Repeated entries still accumulate, so `["a,b,c", "a", "b", "c"]` against an item holding + // both shapes removes 4 in one call. The narrower guarantee is the one the documented + // re-run contract needs: a lone `["a,b,c"]` takes the glued tag and leaves the split names, + // so calling again is meaningful. `list -t` matches the two shapes as a UNION instead — + // deliberately different, because a filter has nothing to destroy. Do not unify them. + // (`ok_list` has neither rule: it is exact-match only, tracked separately. Aligning THIS + // tool with `untag` does not close that gap and is not meant to.) + // + // REMOVING NOTHING IS AN ERROR, matching both the CLI (which exits 1) and the house idiom + // in this file, where errorText is how a tool says it could not do what was asked. This + // does mean a repeated untag of the same tag reports isError on the second call. That is + // intended and it is not destructive: the item is left exactly as it was, and the message + // names both the tags that missed and the tags the item actually holds, so a caller can + // tell "already gone" from "wrong name" without another round trip. + registerTool(server, 'ok_untag', 'Remove tags from a knowledge item', 'Remove specific tags from an item. Each value matches a stored tag whole before being split on commas. Removing nothing is an error, so removed is never 0.', { id: z.string(), tags: z.array(z.string()), store_path: storePathField, @@ -1677,11 +1706,39 @@ export function buildServer() { const store = itemStoreFor(store_path, scope); const current = await store.get(id); if (!current) return errorText(`Item not found: ${id}`); - const remove = new Set(tags.map((tag) => tag.toLowerCase())); - const before = (current.tags ?? []).length; - const nextTags = (current.tags ?? []).filter((tag) => !remove.has(tag.toLowerCase())); + const before = current.tags ?? []; + const stored = new Set(before.map((tag) => tag.toLowerCase())); + const remove = new Set(); + for (const raw of tags) { + const whole = raw.trim().toLowerCase(); + if (whole.length > 0 && stored.has(whole)) { remove.add(whole); continue; } + for (const name of raw.split(',').map((tag) => tag.trim().toLowerCase()).filter((tag) => tag.length > 0)) remove.add(name); + } + // Separated from the not-found case below on purpose. `tags: []` and `tags: [","]` both + // name no tag at all, which is a malformed request, not a tag that is missing — and + // reporting it as "not in [...]" with an empty list would read as the store's fault. The + // CLI cannot reach this: collectTagFlag rejects a separator-only value at parse time. + if (remove.size === 0) return errorText(`No tag names given for ${current.id}. Each entry must contain at least one non-empty tag name.`); + const nextTags = before.filter((tag) => !remove.has(tag.toLowerCase())); + const removed = before.length - nextTags.length; + const notFound = [...remove].filter((tag) => !stored.has(tag)); + // Checked BEFORE the write, so a no-op leaves updated_at alone. Both tag lists are quoted: + // joined raw, one glued "a,b,c" tag renders identically to three separate ones, so on + // exactly the damaged items this fallback exists for the message would deny a tag that is + // plainly listed. + if (removed === 0) { + return errorText(`No matching tag on ${current.id}: ${notFound.map((tag) => JSON.stringify(tag)).join(', ')} not in [${before.map((tag) => JSON.stringify(tag)).join(', ')}]`); + } const item = await store.update(current.id, { tags: nextTags }); - return item ? jsonText({ ok: true, item, removed: before - nextTags.length }) : errorText(`Item not found: ${id}`); + if (!item) return errorText(`Item not found: ${id}`); + // The partial miss goes in `message` as well as `not_found`, matching the CLI, so a client + // that renders only the message still sees it. JSON.stringify rather than a raw join + // because trim() strips only the ends: a name carrying a newline would otherwise break the + // single-line message in two. + const missed = notFound.length > 0 ? ` (not found: ${notFound.map((tag) => JSON.stringify(tag)).join(', ')})` : ''; + const result = { ok: true, item, removed, message: `Removed ${removed} tag${removed === 1 ? '' : 's'} from ${item.id}${missed}` }; + if (notFound.length > 0) result.not_found = notFound; + return jsonText(result); }); registerTool(server, 'ok_bulk_delete', 'Bulk delete knowledge items', 'Delete multiple items by tag or search. Requires confirm=true.', { diff --git a/tests/mcp.test.ts b/tests/mcp.test.ts index a7a2162..a12b423 100644 --- a/tests/mcp.test.ts +++ b/tests/mcp.test.ts @@ -857,4 +857,147 @@ describe('knowledge MCP', () => { await client.close(); } }, 10000); + + // ok_untag had no MCP-side coverage at all, which is how it kept returning `ok: true` on + // `removed: 0` through two PRs that fixed the same defect on the CLI side. Every assertion + // below reads the STORE back as well as the payload: asserting on the absence of an error is + // what let the original defect through. + test('ok_untag matches whole values before splitting and refuses to call a no-op a success', async () => { + const dir = makeTempDir('ok-untag-mcp-'); + const store = join(dir, 'db.json'); + const seededAt = '2026-07-06T14:31:34.606Z'; + const seed = (id: string, tags: string[]): void => { + writeFileSync(store, JSON.stringify({ + items: [{ + id, + title: `Item ${id}`, + content: 'Body', + url: null, + tags, + metadata: {}, + archived: false, + created_at: seededAt, + updated_at: seededAt, + }], + })); + }; + + const transport = new StdioClientTransport({ + command: 'bun', + args: [MCP], + cwd: dir, + stderr: 'pipe', + // The cloud-flip variables are stripped rather than inherited. If either is exported in + // the parent shell the child routes to the cloud API and every store_path below is + // ignored, which turns this test into one that measures nothing. CI has neither set; a + // developer machine can easily have both. + env: Object.fromEntries( + Object.entries(process.env).filter(([key]) => key !== 'HASNA_KNOWLEDGE_API_URL' && key !== 'HASNA_KNOWLEDGE_API_KEY') + ) as Record, + }); + const client = new Client({ name: 'knowledge-test', version: '0.0.0' }); + + /** Call ok_untag and return both the error flag and the parsed-or-raw body. */ + const untag = async (id: string, tags: string[]) => { + const result: any = await client.callTool({ name: 'ok_untag', arguments: { store_path: store, id, tags } }); + const text = result.content?.[0]?.text; + expect(typeof text).toBe('string'); + let parsed: any = null; + try { parsed = JSON.parse(text); } catch { parsed = null; } + return { isError: result.isError === true, text: text as string, parsed }; + }; + /** Read the item straight back out of the store, not out of the tool's own response. */ + const storedItem = (id: string) => { + const db = JSON.parse(readFileSync(store, 'utf8')) as { items: Array> }; + const found = db.items.find((item) => item.id === id); + expect(found).toBeDefined(); + return found!; + }; + + try { + await client.connect(transport); + + // Positive control: the fixture holds ONE glued tag, not three. Everything below depends + // on that distinction being real in the store. + seed('k_glued', ['a,b,c']); + expect(storedItem('k_glued').tags).toEqual(['a,b,c']); + const whole = await untag('k_glued', ['a,b,c']); + expect(whole.isError).toBe(false); + expect(whole.parsed.removed).toBe(1); + expect(storedItem('k_glued').tags).toEqual([]); + + // The defect this test was written for. An untag that removed nothing must not report + // success, and must not write: the old code called store.update first, so a no-op still + // bumped updated_at. + seed('k_absent', ['alpha', 'beta']); + const absent = await untag('k_absent', ['gamma']); + expect(absent.isError).toBe(true); + expect(absent.text).toBe('Error: No matching tag on k_absent: "gamma" not in ["alpha", "beta"]'); + expect(storedItem('k_absent').tags).toEqual(['alpha', 'beta']); + expect(storedItem('k_absent').updated_at).toBe(seededAt); + + // Parity with `untag` in the CLI: a comma-bearing value falls back to its split names when + // no stored tag equals the whole value. This returned removed: 0 before. + seed('k_split', ['a', 'b', 'c', 'keep']); + const split = await untag('k_split', ['a,b,c']); + expect(split.isError).toBe(false); + expect(split.parsed.removed).toBe(3); + expect(storedItem('k_split').tags).toEqual(['keep']); + + // The exclusivity is per raw VALUE, not per call — repeated entries still accumulate. This + // is the claim that has been misstated twice, so it is pinned by execution here. + seed('k_both', ['a,b,c', 'a', 'b', 'c']); + const accumulate = await untag('k_both', ['a,b,c', 'a', 'b', 'c']); + expect(accumulate.isError).toBe(false); + expect(accumulate.parsed.removed).toBe(4); + expect(storedItem('k_both').tags).toEqual([]); + + // ...and the narrower guarantee the re-run contract rests on: one glued value takes the + // glued tag and leaves the split names, so calling again does more work, and the third + // call is the terminal error rather than a third silent success. + seed('k_rerun', ['a,b,c', 'a', 'b', 'c']); + const first = await untag('k_rerun', ['a,b,c']); + expect(first.parsed.removed).toBe(1); + expect(storedItem('k_rerun').tags).toEqual(['a', 'b', 'c']); + const second = await untag('k_rerun', ['a,b,c']); + expect(second.parsed.removed).toBe(3); + expect(storedItem('k_rerun').tags).toEqual([]); + const third = await untag('k_rerun', ['a,b,c']); + expect(third.isError).toBe(true); + expect(third.text).toBe('Error: No matching tag on k_rerun: "a", "b", "c" not in []'); + + // A partial miss still succeeds, but the unmatched name has to be reported in BOTH the + // JSON and the message — a client that renders only `message` saw nothing before. + seed('k_partial', ['alpha']); + const partial = await untag('k_partial', ['alpha', 'nope']); + expect(partial.isError).toBe(false); + expect(partial.parsed.removed).toBe(1); + expect(partial.parsed.not_found).toEqual(['nope']); + expect(partial.parsed.message).toBe('Removed 1 tag from k_partial (not found: "nope")'); + expect(storedItem('k_partial').tags).toEqual([]); + + // Naming no tag at all is a malformed request, not a missing tag. Reporting it as + // `not in [...]` with an empty list would read as the store's fault. + seed('k_bad', ['alpha']); + for (const bad of [[], [','], [' , ']]) { + const rejected = await untag('k_bad', bad as string[]); + expect(rejected.isError).toBe(true); + expect(rejected.text).toBe('Error: No tag names given for k_bad. Each entry must contain at least one non-empty tag name.'); + expect(storedItem('k_bad').tags).toEqual(['alpha']); + expect(storedItem('k_bad').updated_at).toBe(seededAt); + } + + // trim() strips only the ends, so a newline inside a name survives into not_found. Joined + // raw it would break the single-line message in two; quoted it stays on one line. + seed('k_ws', ['alpha']); + const ws = await untag('k_ws', ['alpha', 'p\nq']); + expect(ws.isError).toBe(false); + expect(ws.parsed.not_found).toEqual(['p\nq']); + expect(ws.parsed.message).toBe('Removed 1 tag from k_ws (not found: "p\\nq")'); + expect(ws.parsed.message.split('\n')).toHaveLength(1); + expect(storedItem('k_ws').tags).toEqual([]); + } finally { + await client.close(); + } + }, 30000); });