Skip to content

fix(mcp): land ok_untag truthful-removal on main (PR #39 merge-plumbing recovery) - #41

Merged
andrei-hasna merged 2 commits into
mainfrom
fix/hc-00151-regenerate-mcp-bundle
Jul 27, 2026
Merged

fix(mcp): land ok_untag truthful-removal on main (PR #39 merge-plumbing recovery)#41
andrei-hasna merged 2 commits into
mainfrom
fix/hc-00151-regenerate-mcp-bundle

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What this is

A merge-plumbing recovery, not new work. The code here is exactly PR #39, already
adversarially reviewed and approved in
#39 (comment). No source change has been
made since that review.

Why it is needed

PR #39 was stacked on PR #38 (base: fix/hc-00151-regenerate-mcp-bundle). #38 was merged to
main first, in the correct order — but GitHub did not retarget #39's base to main when its
parent merged, and I merged #39 without re-checking the base. So #39 merged into the
already-merged parent branch instead of into main:

gh pr view 39 --json baseRefName,mergeCommit,state
  base=fix/hc-00151-regenerate-mcp-bundle  mergeCommit=e3fd4ad  state=MERGED

git show 1b8e127:src/mcp.js | grep -c "No matching tag on"
  0      <-- the ok_untag fix was NOT on main

My error, recorded here rather than quietly re-pushed. The lesson for the next stacked pair:
after merging the parent, explicitly retarget the child's base to main and re-read it before
merging — a stacked child's base does not follow its parent automatically.

Why this branch is the right thing to merge

Nothing was lost and nothing extra is carried. e3fd4ad is main plus PR #39's three files and
nothing else:

git diff --stat 1b8e127 e3fd4ad
 bin/knowledge-mcp.js |  33 ++++++++++--
 src/mcp.js           |  67 ++++++++++++++++++++++--
 tests/mcp.test.ts    | 143 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 233 insertions(+), 10 deletions(-)

git show e3fd4ad:.github/workflows/ci.yml | grep -c 'bun-version: 1.3.14'   -> 2
git show e3fd4ad:src/mcp.js | grep -c 'No matching tag on'                  -> 1

So it carries both #38's merged state (including the CI bun pin fix) and #39's ok_untag fix,
with no duplicated or reverted content.

The change itself (unchanged from #39)

ok_untag returned ok: true, removed: 0 when it matched nothing, and called store.update
before any check so a no-op still bumped updated_at. Three defects fixed, ported from
untag in src/cli.ts so the two surfaces agree. Re-verified by me against the committed blob:

my mutation rc result
baseline, unmutated 0 2 pass 0 fail
delete the removed === 0 guard 1 1 pass 1 fail
move store.update before the guard 1 1 pass 1 fail
drop the whole-value continue 1 1 pass 1 fail

The legitimate re-run still succeeds — only a true no-op errors, matching the CLI's documented
exits 1 if nothing removed:

k_rerun = ["a,b,c","a","b","c"]
  untag ["a,b,c"] -> removed 1, ok
  untag ["a,b,c"] -> removed 3, ok     <-- idempotent re-run SUCCEEDS
  untag ["a,b,c"] -> isError           <-- only the true no-op errors

bun run verify:generated at this content: rc=0, 6 bundles byte-identical.
Full suite: 269 pass / 2 skip / 1 fail, sole failure the pre-existing
context pack and proposal context commands return bounded agent JSON, red on main too.

Tasks 68fc5c1c (the fix) and 20803062 (the parent).


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

`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.
fix(mcp): ok_untag must not call removing nothing a success
@andrei-hasna
andrei-hasna merged commit 2972036 into main Jul 27, 2026
0 of 7 checks passed
andrei-hasna added a commit that referenced this pull request Jul 27, 2026
…blished package

The redaction fix in this PR lives in src/private-ref.ts, but package.json
`files` ships only bin/ and dist/ — src/ is not published. Measured: the
committed bin/knowledge.js, bin/knowledge-mcp.js and dist/index.js still
carried the old Linux-only `home|tmp` pattern and none of the new
`home|Users` / `folders|tmp` patterns, so merging the source change alone
would have fixed the repository while leaving @hasna/knowledge's CLI, MCP
and SDK exporting unredacted macOS and Windows paths.

The original reason for deferring regeneration was that verify:generated
already exited 1 on pristine main. That is no longer true: PR #41
(fix/hc-00151-regenerate-mcp-bundle) has landed and main at 5f7d297 now
reports all 6 generated bundles rebuilding byte-identically
(BASELINE_VERIFY_RC=0). After rebasing onto that main, verify:generated
exits 1 and the only drift is this PR's own redactString change — so the
gate is now correctly demanding the rebuild rather than reporting
pre-existing staleness.

Rebuilt with bun 1.3.14, which equals the bun-version pinned in
.github/workflows/ci.yml, per the byte-gate guidance in
scripts/verify-generated-artifacts.mjs. verify:generated exits 0 after
this commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant