From 88b00aa3b7fc9dd12190219e4e9a3f1db84ad78a Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Tue, 28 Jul 2026 01:05:20 +0300 Subject: [PATCH] fix(mcp): ok_untag must not call removing nothing a success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ok_untag` returned `jsonText({ ok: true, item, removed: before - nextTags.length })` unconditionally, so an untag that matched nothing came back `ok: true, removed: 0` — a success signal that cannot tell 1 removed from 0. Same defect class the CLI twin was fixed for in #34/#35, still live on the MCP surface. Reproduced over real stdio before touching anything, driving src/mcp.js with the MCP SDK client. Three separate defects, not one: (a) absent tag -> isError false, ok true, removed 0 -> and updated_at WAS bumped: store.update ran before any check, so a no-op was recorded as a write (b) tags:["a,b,c"] against an item holding a, b, c separately -> removed 0, item unchanged. The CLI removes 3 there. (c) tags:["alpha","nope"] on an item holding only alpha -> removed 1, no mention of "nope" anywhere in the payload Now, measured on the same probes after the change: (a) -> isError true, 'No matching tag on k_absent: "gamma" not in ["alpha", "beta"]', tags untouched, updated_at unchanged (b) -> removed 3, tags ["keep"] (c) -> removed 1, not_found ["nope"], message 'Removed 1 tag from k_partial (not found: "nope")' Ported from src/cli.ts `untag` rather than reinvented, so the two surfaces agree: whole-value match short-circuits before the comma split, removed === 0 is an error, and unmatched names are reported in both `not_found` and `message`. The exclusivity is PER RAW VALUE, not per call. Measured, item holding ["a,b,c","a","b","c"]: tags:["a,b,c"] removes 1 and leaves the split names; tags:["a,b,c","a","b","c"] removes 4 in one call; and the re-run contract holds 1 -> 3 -> isError. That claim has been misstated twice in this repo, so the test pins it by execution instead of restating it in prose. `list -t`'s union rule is deliberately NOT copied here — a filter has nothing to destroy, a write does. (`ok_list` has neither rule; it is exact-match only. That gap is real and tracked separately. Aligning this tool with `untag` does not close it.) An empty or separator-only request is now its own error rather than falling into the not-found path, where it would have printed `not in [...]` against an empty list and read as the store's fault. The CLI cannot reach that case at all: collectTagFlag rejects a separator-only value at parse time. REMOVING NOTHING BEING AN ERROR IS A DELIBERATE TRADE, stated rather than buried: a client that untags the same tag twice now gets isError on the second call. It is not destructive — the item is untouched and the message names both the tags that missed and the tags the item actually holds, so "already gone" is distinguishable from "wrong name" without another round trip. The alternative, `ok: true, removed: 0`, is the defect. tests/mcp.test.ts had zero ok_untag coverage, which is how this survived two PRs about the same bug. The new test asserts on the STORE read back as well as on the payload — asserting the absence of an error is what let it through the first time — and it starts from a positive control proving the fixture really holds one glued tag rather than three. It strips HASNA_KNOWLEDGE_API_URL/KEY from the child env: either one flips the server into cloud mode, which ignores store_path and would turn the whole test into one that measures nothing. bin/knowledge-mcp.js is rebuilt so the shipped artifact carries the fix; the diff is 28 insertions confined to this tool. --- bin/knowledge-mcp.js | 33 ++++++++-- src/mcp.js | 67 ++++++++++++++++++-- tests/mcp.test.ts | 143 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 233 insertions(+), 10 deletions(-) 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); });