fix(mcp): ok_untag must not call removing nothing a success

[andrei-hasna]

Closes 68fc5c1c. Stacked on PR #38 (fix/hc-00151-regenerate-mcp-bundle) — see Ordering.

The defect

src/mcp.js:1684 returned jsonText({ ok: true, item, removed: before - nextTags.length })
unconditionally. An untag that matched nothing came back ok: true, removed: 0 — a success
signal that cannot tell 1 removed from 0. Same 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. It was three defects, not one:

probe	before	after
absent tag ["gamma"] on ["alpha","beta"]	isError false, ok true, removed 0 — and updated_at bumped	isError true, No matching tag on k_absent: "gamma" not in ["alpha", "beta"], tags untouched, updated_at unchanged
["a,b,c"] on an item holding a,b,c separately	removed 0, item unchanged (CLI removes 3)	removed 3, tags ["keep"]
["alpha","nope"] on an item holding only alpha	removed 1, no mention of nope anywhere	removed 1, not_found ["nope"], message 'Removed 1 tag from k_partial (not found: "nope")'

The updated_at bump is the part that was not in the filed report: store.update ran before
any check, so a no-op was recorded as a write.

The fix

Ported from untag in src/cli.ts 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 appear in both not_found and message.

The exclusivity is per raw VALUE, not per call. Measured on an item holding
["a,b,c","a","b","c"]:

tags: ["a,b,c"]                  -> removed 1, leaves ["a","b","c"]
tags: ["a,b,c","a","b","c"]      -> removed 4 in ONE call
re-run contract                  -> removed 1, then 3, then isError
                                    'No matching tag on k_rerun: "a", "b", "c" not in []'

That claim has been misstated twice in this repo, so the test pins it by execution rather
than 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 is real and tracked as
bf0cd7b7. Aligning this tool with untag does not close that gap and is not meant to.)

An empty or separator-only request (tags: [], [","], [" , "]) is now its own error
rather than falling into the not-found path, where it would print 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.

A deliberate trade, stated rather than buried

A client that untags the same tag twice now gets isError on the second call. That is the
CLI's contract (exit 1) and this file's own idiom (errorText is how every other tool says it
could not do what was asked — ok_archive, ok_restore, ok_bulk_delete all use it). 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 "already gone" is distinguishable from "wrong name"
without another round trip. The alternative — ok: true, removed: 0 — is the defect.

Verification

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 the payload — asserting
the absence of an error is what let it through the first time — and opens with a positive control
proving the fixture really holds one glued tag rather than three.

It strips HASNA_KNOWLEDGE_API_URL/HASNA_KNOWLEDGE_API_KEY from the child env: either one flips
the server into cloud mode, which ignores store_path and would make the whole test measure
nothing. CI has neither set; a developer machine easily has both.

Mutation-tested, each plant reverted immediately, tree clean after:

M0 baseline                              rc=0   2 pass  0 fail
M1 drop the whole-value `continue`       rc=1   1 pass  1 fail
M2 drop the comma-split fallback         rc=1   1 pass  1 fail
M3 let removed:0 report ok:true          rc=1   1 pass  1 fail
M4 accept a malformed empty request      rc=1   1 pass  1 fail
M5 drop not_found from the payload       rc=1   1 pass  1 fail
M6 unquote the not-found names           rc=1   1 pass  1 fail
M7 no-op edit (control, must stay GREEN)  rc=0   2 pass  0 fail
M8 restored                              rc=0   2 pass  0 fail

M7 is the control that matters: it proves the suite is not simply red for any edit to the file.

Full suite, env -u HASNA_KNOWLEDGE_API_URL -u HASNA_KNOWLEDGE_API_KEY bun test --timeout 60000:

rc=1   260 pass / 2 skip / 4 fail / 2555 expect() calls, 266 tests across 38 files

None of the four are mine, and three are load flakes rather than real failures. This branch
touches only src/mcp.js, tests/mcp.test.ts and bin/knowledge-mcp.js, so it cannot affect
tests/cli.test.ts — yet three of the four failures are in that file:

✗ sync status, snapshot, machines, and conflicts use the project catalog  "timed out after 10000ms"
✗ sync dry-run and push copy a project catalog into a peer workspace      "timed out after 10000ms"
✗ search command returns hybrid source, wiki, and semantic results        (15000ms budget)
✗ context pack and proposal context commands return bounded agent JSON    <- the real pre-existing one

tests/cli.test.ts carries 10 hardcoded per-test timeouts (}, 10000) / 20000 / 15000)
which --timeout 60000 does not override, and machine load average was 52-60 during the
run. On an earlier run of the same suite at lower load the identical branch state produced
1 failure — the context pack… one, which is the pre-existing failure already recorded on
main and on PR #36. Filed as a separate finding rather than folded in here.

bin/knowledge-mcp.js is rebuilt so the shipped artifact carries the fix — 28 insertions, all
confined to this tool.

Ordering

Stacked on PR #38. This branch changes src/mcp.js and therefore rebuilds
bin/knowledge-mcp.js, which is the exact artifact #38 un-stales. Landing this against main
directly would either ship a bundle still carrying #33-era package.json, or silently absorb
#38's zod churn into a PR about tag semantics. Merge order: #37, then #38, then this.

Task: 68fc5c1c. Not merged by me — needs an independent adversarial review first.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[andrei-hasna]

Adversarial review — APPROVE (no fixes required)

Verified independently on spark01, cloud-flip vars neutralised (env -u HASNA_KNOWLEDGE_API_URL -u HASNA_KNOWLEDGE_API_KEY), resolved store kind confirmed local first. Unpiped exit codes.

All three defects confirmed fixed at the committed head

git show 88b00aa:src/mcp.js — the guard is in the committed blob (lines 1721, 1730) and in
the committed bundle (bin/knowledge-mcp.js, 1 hit for No matching tag on). So the earlier
process error of mutation-testing an uncommitted fix did not survive into this head; the fix is
what the tests run against by construction.

I re-ran mutation testing myself, against that committed file, and each plant flipped the result:

my mutation	rc	result
baseline, unmutated	0	2 pass 0 fail
delete the removed === 0 guard (restore ok: true on removed: 0)	1	1 pass 1 fail
move store.update before the guard (restore the updated_at bump)	1	1 pass 1 fail
drop the whole-value continue (split-first)	1	1 pass 1 fail

Tree restored clean after each (git status --porcelain empty, HEAD unchanged at 88b00aa).
The second row is the one that matters most — it is the defect that was not in the filed
report, and the test genuinely detects it.

The overcorrection risk was checked specifically, and it is not present

"Removing nothing is an error" does not mean every no-change is a failure. Measured
distinction, pinned by execution in the new test:

k_rerun = ["a,b,c","a","b","c"]
  untag ["a,b,c"]  ->  removed 1, ok    (glued tag taken, split names left)
  untag ["a,b,c"]  ->  removed 3, ok    <-- the legitimate re-run still SUCCEEDS
  untag ["a,b,c"]  ->  isError          <-- only the true no-op errors

And it is parity, not invention. src/cli.ts untag throws on removed === 0 and calls
itemStore.update only after that check, and the CLI's own help says so:
untag --id <id> -t <tag> Remove tag(s) from an item (-t repeatable; exits 1 if nothing removed).
So this aligns the MCP surface with its reference implementation rather than overshooting it.

Scope boundary confirmed present

The ok_list note is in the source at the tool registration — "(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.)" That is the thing that stops a future reader inferring parity from this
PR, and it survived into the merged text.

Suite and bundle

env -u HASNA_KNOWLEDGE_API_URL -u HASNA_KNOWLEDGE_API_KEY bun test --timeout 60000
rc=1   269 pass / 2 skip / 1 fail   Ran 272 tests across 39 files  [191.08s]
sole failure: knowledge cli > context pack and proposal context commands return bounded agent JSON
git status --porcelain after the suite: empty

That is the known pre-existing failure, red on main too. My run showed 1 fail where this
PR's description reported 4 — the other 3 were load artefacts in tests/cli.test.ts, a file this
branch does not touch. Contention can only redden, so the lower count is the trustworthy one.

bun run verify:generated at 88b00aa: rc=0, 6 bundles byte-identical, tree clean — the
stacked mcp bundle really is in sync with the changed source. Note the test drives
src/mcp.js directly (tests/mcp.test.ts:13), not the bundle, so #38's byte-sync gate is what
covers the shipped artifact. That is exactly why stacking on #38 was the right call.

Non-blocking observations, not fixed here

• Returning isError for a repeat untag is protocol-visible: an MCP client that treats isError
  as retryable could loop on a call that will never succeed. The trade is documented in the source
  and matches the house idiom, so I am not changing it — flagging it as a consequence to watch.
• tests/mcp.test.ts contains only 2 tests total, both with hardcoded timeouts (10000,
  30000). Generous for the work done, but the same class as the tests/cli.test.ts timeouts
  filed separately.

Task 68fc5c1c. Formal approval is impossible from this identity, so this comment is the review
of record. Merging after #38, per the stated order.