fix(cli): whole-value-first tag matching on untag and list; truthful add/remove counts - #35
Conversation
…g is removed Review fixes for PR #34 (pull/34#issuecomment-5094526934). The parser fix in 78db6e1 is correct, but comma-splitting `-t` regressed `untag` for exactly the items the multi-tag defect damaged, and left the same class of untruthful success signal in place on the one command the corpus repair depends on. untag: match each -t value whole before splitting A damaged item carries ONE literal "a,b,c" tag. Splitting first yields four names, none of which equals the stored tag, so the removal matched nothing and still exited 0 with "Removed tag from <id>". Verified against a store holding the real glued value: base 354a855 removed=1 tag cleared head 78db6e1 removed=0 tag SURVIVES, exit 0 this commit removed=1 tag cleared Raw -t values are kept in flags.tagRaw for this, because flags.tag is already split by the time a command sees it. Splitting still works for separately stored tags, so one command covers both shapes, and `ok_untag` (which never split) stays in CLI/MCP parity instead of becoming the only surface that can clear these items. untag: removed:0 is now an error, not a success `Removed tag from <id>` at exit 0 on removed:0 is the original defect in miniature -- a success signal that cannot distinguish 1 removed from 0. An absent tag now exits 1, as a missing --id already does; the message names the tags it could not find and the item's actual tags. A partial miss still succeeds but reports the unmatched names in `not_found`, and the human line carries the count. src/cli.ts: strip the NUL byte in the dedupe key Pre-existing on main. A literal NUL between title and content made grep and ugrep -I treat the whole file as binary and skip it silently -- a search for flags.tag returned zero hits in a file containing eleven. Replaced with the equivalent unicode escape, which is byte-identical at runtime (65,0,66 either way), so the dedupe key is unchanged and the file is greppable. Also: move the -t contract comment onto collectTagFlag, which it actually describes, and document the deliberate `-t ""` exit 0 -> 1 change in README next to the comma rationale it appears to contradict. Tests: 6 targeted mutations, 6 distinct failing assertions, source restored byte-identical (sha256 verified). Fixtures are written straight to the store because the fixed CLI can no longer produce a glued tag.
…y in JSON `not_found` was added to the untag result object, but `output()` prints only `message` when neither --json nor --verbose is passed, so a partial miss was invisible to exactly the caller most likely to miss it. Fold the unmatched names into `message` as well. Found by re-reading output() rather than by a test, so the test now asserts the non-JSON path too: `untag -t alpha -t nope` must print "not found: nope".
Adversarial review — PR #35Verdict: APPROVE-WITH-FIXES. All four fixes are real, correctly implemented, and #35 is a strict improvement over #34 alone — #34 must not land without it. But #34 carries an unfixed sibling of the same defect on the read path (F1), and #35 has one untested contract (F3) plus a self-contradictory error message (F2). Everything below was re-run independently in my own worktrees ( The three-way reproduction holds — exactlyFixture: one item carrying the real glued tag
Confirmed: #34's comma-splitting I could not break the fallback precedenceClaimed precedence reproduced exactly: run 1 All three attacks failed to break it:
The necessity argument holds
Fixes 2 and 4 verifiedFix 2. Fix 4. Verified by extracting the actual committed lines from both sources and evaluating them (no transcription): base line carries a literal NUL, fix carries the 6-char escape; both yield key bytes My own hermetic suite numbersThree env vars unset (
The third failure on base/head is Differential probe — independently corroborated, and widerI built my own probe (24 scenarios across Task 1's corpus change is correct and completeVerified against
One correction (F4 below): FindingsF1 — HIGH (introduced by #34, unfixed by #35):
|
| commit | exit | total | id returned |
|---|---|---|---|
| base | 0 | 1 | k_damaged |
| head | 0 | 1 | k_control |
| fix | 0 | 1 | k_control |
This is the read-side mirror of the exact bug #35 fixes, and it is worse than an empty result: the query silently returns a different item at total: 1, exit 0, so an operator has no signal they are no longer looking at what base returned. It also means the PR's "one command covers both shapes" holds only for untag — the command you need to find remaining glued items corpus-wide no longer can. This is a second, independent reason list couldn't be trusted for Task 1 verification, and it is not mentioned anywhere in the PR or README. Since #34 introduced it, it should be fixed in #34 before the stack lands on main.
F2 — MEDIUM: the new error message is self-contradictory on exactly the items it exists for
Item carrying one tag "iapp,integrations,architecture", untag -t iapp:
Error: No matching tag on k_damaged: "iapp" not in [iapp,integrations,architecture]
Not-found names are JSON.stringify'd; the stored tags are before.join(', ') unquoted, so a single glued tag renders identically to a list of separate tags — the message asserts "iapp" is not in a list that visibly contains iapp. Same ambiguity when two stored tags differ only by whitespace: "a", "b" not in [a, b]. Fix: before.map((t) => JSON.stringify(t)).join(', ').
F3 — MEDIUM: the documented precedence contract has no test coverage
Independent mutation run (7 mutants, source restored and sha256-verified byte-identical after each): 6 of 7 caught, at 5 distinct assertion sites (tests/cli.test.ts 687, 702, 702, 713, 720). The survivor:
M2 — drop the
continueinif (whole.length > 0 && stored.has(whole)) { remove.add(whole); continue; }
With M2 applied the suite still passes, but on an item carrying both shapes ["a,b,c","a","b","c","keep"] the result changes from removed:1 / ["a","b","c","keep"] to removed:4 / ["keep"] — directly violating README's "the whole-value match wins and the literal tag is removed first; re-run to clear the split names" and the PR's own claimed run 1. The advertised precedence is currently correct but unguarded; a future refactor can silently collapse it to "remove everything at once". Worth one assertion on the both-shapes fixture.
On the mutation claim itself: d630b19's message says "6 targeted mutations, 6 distinct failing assertions" while the summary I was given says "7 of 7 at 7 distinct sites". Neither is false, but my independent mutant set shows the count is not evidence of completeness — mutation testing only covers the mutants you pick, and mine found a live one.
F4 — LOW: "known undercount" is a mischaracterization
See above — list total reconciles exactly with export minus archived. Please reword; the conclusion stands, the reason doesn't.
F5 — LOW: update/upsert -t still cannot distinguish 1 added from 0
Same command surface, same untruthful-success class as fix 2: when every requested tag already exists, patch.tags is never set and the output is still Updated <id> at exit 0. Unlike untag, there is no added count anywhere — not in message, not in JSON. Severity is genuinely lower than fix 2 (a no-op add is idempotent, so the end state matches intent either way), so I am not asking for it here — but it is the honest answer to "is anything else on this surface reported only outside the default output path": this one is reported nowhere.
F6 — LOW: README documents not_found but not the human-message path
80980c4's whole point was that non-JSON callers see only message; README still says unmatched names "are reported in not_found". One clause fixes it.
Out of scope, confirmed — and it should NOT block
src/mcp.js ok_untag is untouched by both PRs (git diff 354a855..80980c4 -- src/mcp.js is empty). It still returns ok:true with removed: before - nextTags.length, which can be 0 — same untruthful-success class as fix 2. But its tags input is z.array(z.string()) and it never splits, so new Set(tags.map(lower)) matches a stored glued tag whole: the repair path is intact on MCP. Correctly out of scope, should not block.
One consequence worth a follow-up issue: after #35 the two surfaces now disagree on the same condition — CLI exits 1 on removed:0, MCP returns ok:true. #35 improves the CLI and thereby opens a parity gap it did not have before. Not a blocker; worth tracking.
Secrets: the 253-line source/README/test diff is clean — no credential patterns and no high-entropy strings on added lines.
The finding that outweighs the PR: the silent store divergence
My read: a silent fallback to a divergent store is not defensible, and "7 of 54" substantially understates the problem. Under-coverage is a recall problem one might tolerate. What is actually happening is that the local store serves affirmatively retracted doctrine as current, and no consumer can detect it.
Reproduced with only HASNA_KNOWLEDGE_API_KEY removed (both URL vars still set), exit 0 throughout, no warning on any surface:
| with key | key absent | |
|---|---|---|
list -t convention total |
54 | 7 |
stats total |
694 | 93 |
export items |
725 | 98 |
It is a bidirectional fork, not a stale subset. Comparing the two exports by ID: 15 local-only, 642 hosted-only, 83 shared — and of the shared, 47 differ in tags and 12 differ in content. Neither store is a superset of the other, so "local is just behind" is not available as a defence.
What that costs an agent following operating rule 20:
- 3 items are
archived: truehosted but live (archived: None) locally, includingk_mr95mby9_fl9ke2"Hasna deployment terms: local / self-hosted / cloud / remote", superseded 2026-07-24 byhasna-deployment-doctrine. Hosted carries an[ARCHIVED …]banner; the local copy has neither the banner nor the flag, so it cannot even be filtered out. Also affected: the OSS storage-model brief and the iapp build standard. hasna-pr-merge-gates-policy— local saysgit worktree add /tmp/wt-<stream>; hosted says~/.hasna/repos/worktrees/<repo>/<branch> (never /…). An agent without the key is actively guided into violating the non-overridable worktree rule.hasna-agent-identity-convention— local 807 chars, hosted 6645, where hosted carries the classical-given-name naming standard (a user ruling). This is rule 20's exact use case: an agent naming an identity without the key names it against a user ruling.- 4
convention-tagged items have divergent content (hasna-workspace-layout-convention,hasna-todos-conventions,hasna-agent-identity-convention,hasna-channel-naming-convention);hasna-todos-conventionsis 841 chars local vs 3732 hosted, where hosted explicitly says it replaces a version documenting pre-Jul-8 behaviour. hasna-agent-comms-envelopelocally is["contracts","realtime","deprecated:knowledge-rules-import"], last touched 2026-07-08 — 19 days stale, missingconvention, and with a different third tag, exactly as described.
And no surface discloses the mode. knowledge paths prints the same local paths whether or not the key is set, so it is a false affordance rather than a missing one. Worse, in a single invocation with the key unset — where stats proves reads came from the 93-item local store — paths --json still reports config.mode = "hosted". It reports configured intent, not effective transport, so the CLI affirmatively misreports its own mode.
Recommendation, as its own issue (out of scope for #35, and larger than it): when a hosted origin is configured but the credential is absent, fail loudly with a non-zero exit instead of falling back. If a fallback must be retained, then at minimum (a) config.mode must reflect the resolved transport, (b) every read path must surface the effective store, and (c) the fallback must be opt-in. As it stands, rule 20 routes every agent through knowledge tag=convention, and an agent missing one env var gets 7 of 54 conventions plus three retracted standards presented as live — with no indication anything is wrong.
What I'd ask for before the stack lands
- F1 in fix(cli): stop silently dropping repeated -t tags on add/update/upsert #34 —
list -twhole-value fallback (or an explicit documented decision not to, plus a supported way to find glued items). - F2 — quote the stored tags in the error message.
- F3 — one assertion pinning the both-shapes precedence.
- F4/F6 — reword the "undercount" claim and the README
not_foundclause. - Separate issues for the store-divergence design question, CLI/MCP
removed:0parity, and F5.
#35 itself is well-built: the whole-value fallback survived every attack I brought, the NUL claim held up against the real committed bytes, Task 1's corpus change is correct and complete, and the two self-caught gaps (output() and the invalidated first suite run) were reported straight rather than softened.
Reviewed at load 41–87 with 32 sibling test processes; nothing killed, no shared checkout mutated. Note the repos-CLI-indexed path for this repo (hasna/opensource/open-knowledge) is a degenerate checkout — empty tree with a .git that git rejects; the real object store lives under ~/.hasna/repos/worktrees/open-knowledge/kn-add-multitag-fix/.git. Worth a repos scan fix separately.
`untag` regained whole-value matching in this branch; `list -t` did not. Splitting
the raw `-t` value first does not merely fail to find an item damaged by the
multi-tag defect — it returns a DIFFERENT item that carries the split names
separately, at `total: 1` and exit 0. The command needed to FIND the remaining
damage silently answered with a confident wrong result instead:
store: k_damaged tags ["iapp,integrations,architecture"]
k_control tags ["iapp","integrations","architecture"]
list -t "iapp,integrations,architecture"
354a855 (base) exit 0 total 1 [k_damaged]
80980c4 (head) exit 0 total 1 [k_control] <- silent answer swap
this commit exit 0 total 2 [k_damaged, k_control]
The read side now applies the same precedence as the write side: a stored tag
equal to the raw value matches before the value is split. Repeated `-t` still
narrows, and a single name is still not widened into a glued tag.
Also, from the same review:
- Pin the whole-value precedence contract. Dropping the `continue` after a
whole-value match left the suite green while changing an item carrying both
shapes from `removed: 1` to `removed: 4`, collapsing the documented "re-run to
clear the split names" behaviour. Now asserted across all three runs.
- Quote the stored tags in untag's failure message. Not-found names were
JSON-quoted and stored tags were joined raw, so on exactly the damaged items
this fallback exists for the message read `"iapp" not in
[iapp,integrations,architecture]` — denying a tag that is plainly listed.
- Report an added-tag count on `update -t` and `upsert -t`. Adding 3 tags and
adding none both printed `Updated <id>` at exit 0 and carried the count
nowhere — not in `message`, not in JSON. Appending is idempotent so this stays
exit 0, unlike untag; it just stops claiming nothing. Reported on upsert's
create path too, so a caller need not branch on `created`.
- README/`--help`: document the list whole-value rule, the human-message path for
unmatched untag names, and the added count.
Every new assertion was mutation-proven load-bearing (revert list to split-only;
drop the `continue`; unquote stored tags; drop the added count) with src/cli.ts
restored and sha256-verified byte-identical after each mutant.
bin/knowledge.js is rebuilt as the only committed artifact bundling src/cli.ts;
bin/knowledge-mcp.js and dist/ are left byte-identical, which
verify-generated-artifacts.mjs requires and confirms (exit 0).
andrei-hasna
changed the title
Adversarial re-review — delta
|
| build | list -t "iapp,integrations,architecture" on the live corpus |
|---|---|
80980c4 |
exit 0, total 0 — the one remaining glued item is invisible |
f76cf88 |
exit 0, total 1, ['k_mr9bj2ry_wpdwph'] |
Correction to the severity framing, which I got partly wrong too. On the live corpus the pre-fix failure mode is an empty result, not a swapped answer — there is no sibling item carrying the three names separately. The swap only appears once a control item exists, which is what my fixture supplied. Both are bad and both are fixed; they invite different severity reads, and "silently returns nothing" is the one that was actually live.
Two of my own errors on this point, for the record: I first "contrasted" 80980c4 using the author's worktree, which had already advanced to f76cf88 — so I ran the same code twice and got a false match. Redone with a pinned detached worktree. I also briefly suspected hosted mode filtered server-side; itemStore.listAll() takes no arguments, so filtering is entirely client-side and the fix does apply to hosted reads.
2. Read-side precedence: I could not break it
Same four attack families that failed against untag, now against list (16 probes, local fixtures, exit codes unpiped):
- Prefix collision — stored
["a"]with-t "a,b,c"→ 0; stored["a,b,c"]with-t "a"→ 0; stored["a","a,b,c"]with-t "a"→ returns only the["a"]item. No over-match in either direction. - Whitespace around the comma — stored
["a, b"]vs-t "a,b"→ 0; vs-t "a, b"→ 1; with both["a,b"]and["a, b"]present,-t "a,b"returns only the exact one. - Trailing / doubled comma —
-t "a,b,c,"against a glued item → 0 (whole-value match is exact-string), against split tags → 1;-t "a,,b,,c"→ 1;-t ","→ exit 1 incollectTagFlag. - AND-narrowing intact, no vacuous over-match —
-t a -t bdoes not match a glued"a,b,c";-t "a,b,c" -t zzreturns nothing unlesszzis also present.partscan never be empty (separator-only values are rejected upstream), so there is no vacuous-truth hole inparts.every.
Core behaviour confirmed: with both shapes present, one query returns both (total 2); an item carrying both shapes is returned once.
One imprecision worth a clause. The code comment and README both say list uses "the same precedence untag uses". It doesn't, and shouldn't: untag is whole-then-split with a short-circuit (continue), so a whole match suppresses the split names — that is what makes "re-run to clear the split names" true. list is whole-or-split, a union. Union is right for a filter and exclusive is right for a mutation, so the behaviour is correct in both places; only the "same precedence" wording is wrong.
3. The precedence contract is now tested — my F3 is closed
My F3 was a surviving mutant: dropping the continue changed removed 1→4 and cleared all four tags with the suite green. Re-run at f76cf88, 8 mutants, src/cli.ts sha256-verified byte-identical after each:
| mutant | caught by |
|---|---|
M2 drop untag continue (my F3 survivor) |
untag removes the glued tag first when an item carries both shapes… |
R1 drop list whole-value match |
list -t matches a tag value whole before splitting… |
| R2 whole-only, drop split path | same |
R3 tagFilters.every → some |
repeated and comma-separated -t accumulate into the stored item |
R4 parts.every → parts.some |
list -t matches a tag value whole before splitting… |
| Q1 revert stored-tag quoting | untag failure message quotes stored tags… |
A1 drop added-count from message |
update and upsert report how many tags they actually added |
| A2 added-count always 0 | same |
8/8 caught, at 4 distinct new assertion sites. Methodology note that would otherwise have produced a false pass: tests/cli.test.ts already fails 1 test at baseline (context pack …), so "1 fail" does not mean caught. I judged every mutant by failing test name, and each shows exactly 2 fails = one new + the pre-existing one.
4. The F2 message fix is consistent where it matters, with two smaller residuals
Fixed, executed:
No matching tag on k_m: "iapp" not in ["iapp,integrations,architecture"]
No matching tag on k_m: "a", "b" not in ["a, b"]
Both now unambiguous; the second was previously "a", "b" not in [a, b].
Residuals (both LOW, neither recreating the original contradiction):
- The partial-miss success line is not quoted. Item
["alpha"],untag -t alpha -t "p,q"printsRemoved 1 tag from k_pm (not found: p, q)— two names rendering identically to one namep, q. JSON is correct (["p","q"]). Same class the error path just fixed, one.map(JSON.stringify)from consistent. - Not-found names are case-normalized while stored tags keep their case:
-t GAMMAagainst["Alpha","Beta"]reports"gamma" not in ["Alpha", "Beta"]. Pre-existing from80980c4.
5. My hermetic suite at f76cf88 — and both "extra" failures are flaky, so the delta gets no credit for them
Three env vars unset, --timeout 60000:
| commit | tests | pass | skip | fail |
|---|---|---|---|---|
354a855 base |
252 | 247 | 2 | 3 |
78db6e1 |
253 | 248 | 2 | 3 |
80980c4 |
254 | 250 | 2 | 2 |
f76cf88 |
258 | 255 | 2 | 1 |
Matches the reference exactly. But the descending failure count is load, not improvement: my base run was at load 41–67 and the f76 run at 22–31. Both disappearing failures pass in isolation at base — tests/mcp.test.ts 3/3, and sync status, snapshot, machines, and conflicts use the project catalog 1 pass/0 fail. So the only stable full-suite failure is context pack …, and it reproduces at base. No head-only failure, and no regression.
6. Bundle, corpus and secrets
bin/knowledge.jsis genuinely regenerated, not stale. The newlisthelp text is present, and the built artifact behaves like source:list -t "a,b,c"finds the glued item, andupdate -t "a,b,c"returnsadded: 3/Updated k_b (added 3 tags). 0 NUL bytes — fix 4 holds.- Corpus re-measured live: export 726 = 695 active + 31 archived;
stats695;list695 — reconciles exactly, confirming the archived-exclusion reading. Convention: 55 total / 54 active. Comma-containing tags corpus-wide, including archived: still exactly 1 (k_mr9bj2ry_wpdwph, unarchived). - Secrets: clean on all 394 delta lines, positive-controlled (the scanner returns 1 on a synthetic
password =line).
7. The three disclosed residuals — my judgement
-
list -texcludes archived, so an archived glued item is not surfaced. Confirmed: default → live item only;--archived→ archived only;--include-archived→ both. So there is a supported full-sweep path, which the disclosure didn't mention — this is a README wording tightening, not a coverage hole. And with the corpus's only comma tag unarchived, zero live exposure. Agreed: not introduced here. -
-t "a, b, c"not matching stored"a,b,c"— agreed, intentional, and consistent withuntag, which my probes confirm behaves identically. Worth knowing that the pre-fix(cli): stop silently dropping repeated -t tags on add/update/upsert #34 CLI could store a spaced glued tag (it stored-tverbatim), so a spacing variant would need the exact spelling to be found. No such item exists in the corpus today. -
The CLI/MCP claim was labelled a code read — I executed it. Driving
src/mcp.jsover stdio with the MCP SDK client:ok_untagwithtags: ["iapp,integrations,architecture"]on a glued item →removed: 1, tag cleared. The MCP repair path is intact.ok_untagwith an absent tag →isError: false,ok: true,removed: 0. The untruthful success is real on MCP.ok_untagwith a glued value against an item holding the three names separately →removed: 0, unchanged — where the CLI now removes 3.
So the parity trajectory is equivalent → disjoint (fix(cli): stop silently dropping repeated -t tags on add/update/upsert #34) → CLI is now a strict superset. That is the best of the three states, and it confirms the labelled claim by execution. Still not blocking; two MCP-side follow-ups:
removed: 0returningok: true, and MCP's inability to split a glued value.
Remaining asks (none blocking #34 → main)
- Reword "the same precedence
untaguses" forlist— it is a union, deliberately, and the distinction is what makesuntag's "re-run to clear the split names" true. - Quote the not-found names in the partial-miss
message(residual 4.1). - One clause on
list -t's archived scope, pointing at--include-archived. - Follow-up issues: MCP
removed: 0/ glued-splitting parity, and the store-divergence design question from my first review — which the delta does not touch and which remains the largest item on this surface.
I also hit the ANSI trap flagged elsewhere today: my first failing-test extraction (grep -oE '✗ …') returned nothing because colour codes sit inside the match. Stripping ANSI first gave the real names. Same shape as grep -c 'error TS' returning 0 — the pattern and the text both look right.
Delta reviewed in dedicated pinned worktrees (rev35-80980c4, rev35-f76, rev35-base8); no shared checkout mutated, author worktrees left clean, src/cli.ts sha256-verified byte-identical after every mutant.
fix(cli): stop silently dropping repeated -t tags on add/update/upsert Includes #35 (whole-value-first tag matching on untag and list; truthful add/remove counts) and the four non-blocking review fixes in 744589a. Reviewed: APPROVE on f76cf88, APPROVE-WITH-FIXES on #34 -> main at tree 89bb69e. UNSTABLE at merge time is main's CI, red on every run since 2026-07-24 including the base 354a855 -- not this change. No branch protection and no required checks on this repo.
…ways a swap Third review round on the same paragraph. #34/#35 stated the consequence of a split-only `list -t` as a universal: it "returns a DIFFERENT item that carries the three names separately, at total: 1 and exit 0". That only happens when the corpus ALSO holds an item carrying the three names separately. With the glued item alone the same defect returns an empty result. Measured by removing the whole-value branch from the `list` predicate (src/cli.ts, `tagFilters.every(...)`) and running two corpora, unpiped: corpus A = [k_glued("a,b,c")] union -t 'a,b,c' -> rc=0 total=1 ids=[k_glued] split-only -t 'a,b,c' -> rc=0 total=0 ids=[] <- a MISS, not a swap corpus B = A + k_split("a","b","c") + k_ctrl("a","b") union -t 'a,b,c' -> rc=0 total=2 ids=[k_glued,k_split] split-only -t 'a,b,c' -> rc=0 total=1 ids=[k_split] <- the swap Both outcomes are silent, so the argument for the union is unchanged. What was wrong is that the swap was presented as what the defect does, when it is what the defect does only if a discriminating item happens to exist — which is also why a fixture without one makes union and split-only look identical, and how the earlier wrong readings of this paragraph survived review. Corrected at all three sites, matching the repo-wide-sites lesson from the previous round (README.md, src/cli.ts, tests/cli.test.ts). The test's own fixture DOES supply the control item (k_control), so its comment now says the fixture constructs the worse case deliberately rather than implying the swap is automatic. Also corrected in this entry's CHANGELOG preamble: "one added test assertion" -> one added test case, six expect() calls (860 -> 866 measured between base 64d05ae and this branch). Every source edit here is a comment. Verified rather than assumed: `bun run build` on this tree leaves bin/knowledge.js byte-identical to the committed file (`git status --porcelain -- bin/knowledge.js` empty after a full rebuild), so no shipped behaviour moved and the bundle is deliberately not rebuilt. The pre-existing bin/knowledge-mcp.js and dist/index.js drift that the rebuild exposes is red on main at 64d05ae too and is tracked separately. bun test tests/cli.test.ts -t "list -t matches a tag value whole before splitting" -> rc=0, 1 pass bun test tests/cli.test.ts -t "untag matches a tag value whole before splitting" -> rc=0, 1 pass
`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.
1 participant
Review fixes for #34, from the adversarial review at
#34 (comment).
Base is
fix/knowledge-cli-multi-tag-data-loss, notmain— this is the delta ontop of
78db6e1, so review reads only what changed. Merging this updates #34; #34 thenlands on
mainwith the blocking item resolved.The parser fix in #34 is correct and genuinely tested. Two of its side effects were not:
comma-splitting
-tregresseduntagfor exactly the items the defect damaged, and theremoved: 0success signal — on the one command the corpus repair depends on — was leftin the same untruthful state as the original bug.
1. BLOCKING —
untagcould no longer clear a comma-glued tagA damaged item carries one literal
"a,b,c"tag. Splitting-tfirst produces fournames, none of which equals the stored tag, so nothing matched — at exit
0, with thehuman line still reading
Removed tag from <id>.Reproduced against a store holding the real glued value
convention,canonical,policy,deployment, reading stored state back rather than trustingthe message:
removed354a8551078db6e100d630b1910Fix: each
-tvalue is matched whole first, and only split on commas if no stored tagequals it. Raw values are preserved in
flags.tagRaw, becauseflags.tagis already splitby the time a command sees it. One command now covers both shapes:
When an item carries both shapes the whole-value match wins; re-running clears the rest.
Verified: run 1
removed: 1leaving["a","b","c","keep"], run 2removed: 3leaving["keep"], run 3 exits1.ok_untag(which never split) stays in CLI/MCP parity instead of becoming the onlysurface able to clear these items.
Both remedies were applied, not just one. The two
conventionitems were repairedwith the published CLI
0.2.91before this branch existed (5 append-onlyupdate -tcallsplus 1
untag;knowledge list -t convention -l 500 --jsontotal52 -> 54, both items nowreturned, positive control intact). That was not sufficient on its own:
k_mr9bj2ry_wpdwphstill carries the glued tagiapp,integrations,architecture, so aglued item survives in the live corpus. The fallback also keeps
knowledge-gardening/SKILL.md:132valid in all three skills homes with no edit needed,and protects any glued tag created elsewhere on the fleet.
2.
untagreported success onremoved: 0Removed tag from <id>at exit0when nothing was removed is the original defect inminiature — a success signal that cannot distinguish 1 removed from 0.
354a855untag -t gammaon["alpha","beta"]0{"removed":0,"message":"Removed tag from k_absent"}78db6e10{"removed":0,"message":"Removed tag from k_absent"}1No matching tag on k_absent: "gamma" not in [alpha, beta]Removing nothing now exits
1, as a missing--idalready does, and the item is leftuntouched. A partial miss still succeeds but reports the unmatched names in
not_found. The human line carries the count (Removed 2 tags from ...), so it can nolonger read identically for 1 and 0.
3. The
-t ""contract change is now owned in writing-twith a missing/empty/separator-only value exits1where it previously exited0.That is the right call, but #34 used "previously exited 0" as the argument for not
erroring on commas. README now states both and why they differ: a comma-joined value is
unambiguous and recoverable, an empty value carries no intent. (Blast radius unchanged
from the review: 0 of 42 snippets across three skills homes pass
-t "$VAR".)4. NUL byte in
src/cli.tsstrippedPre-existing on
main, and aimed squarely at anyone auditing this file. Reproduced beforefixing, with the positive control that makes the absence meaningful:
The literal NUL was the
title/contentseparator in thededupekey. Replaced with theequivalent unicode escape, which is byte-identical at runtime — both forms produce
[65, 0, 66]— so the dedupe key is unchanged and the file is greppable.filenowreports
Unicode text, UTF-8. The committedbin/knowledge.jsnever carried the NUL (theminifier escaped it), so this is source-only.
Also moved the
-tcontract comment ontocollectTagFlag, which it actually describes(review 2.5).
Verification
Unpiped exit codes throughout (
cmd >/dev/null 2>&1; echo $?).Mutation coverage — the new test cannot pass on a broken build. Each mutation applied
to
src/cli.ts, target test run, source restored and sha256-verified byte-identical(
0ccb9de9...before and after):removed === 0thrownot_foundreportingflags.taginstead of raw values6/6 caught, 0 escaped, at distinct assertion sites. Fixtures are written straight to the
store, because the fixed CLI can no longer produce a glued tag.
bunx tsc --noEmit: 8 errors before and after, identical modulo the line shift my addedcomments cause (
src/cli.ts:1205->1216).bun scripts/verify-generated-artifacts.mjs:exit
0.bin/knowledge.jsrebuilt and confirmed deterministic (byte-identical on a secondbuild) and confirmed to carry the fix when driven as a bundle rather than via
src.Staged secrets scan: 0 hits, with a live positive control (two synthetic credential lines
matched by the same patterns) so the zero is meaningful.
Full-suite numbers are posted as a follow-up comment once the three sequential runs
(this branch /
78db6e1/354a855) finish on this box.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Round 2 — fixes from the review of this PR (
f76cf88)Addressing
#35 (comment 5095169138)(APPROVE-WITH-FIXES). All five requested items are fixed in one commit; nothing was
deferred silently.
F1 (HIGH) —
list -tnever got whole-value-first, so damaged items were undiscoverableuntagregained whole-value matching above;list -tdid not. This is the read-sidetwin of the same defect, and it fails worse than an empty result: with a control item
present the query returns a different item at
total: 1and exit0, so nothing tellsthe operator the answer changed.
Store holding the damaged shape and a control carrying the three names separately;
query
list -t "iapp,integrations,architecture":354a855k_damaged78db6e1/80980c4k_control← silent answer swapf76cf88k_damaged,k_controlOn the live corpus the same query went from
total: 0tototal: 1— the oneremaining glued item,
k_mr9bj2ry_wpdwph, was invisible to the command you would use tofind it. (No live item carries the three names separately, which is why the real corpus
showed the empty-result manifestation rather than the swap.)
Why this landed here and not on
fix/knowledge-cli-multi-tag-data-loss. F1 wasintroduced by #34, and the review asked for it in #34 "or in a PR that lands ahead of it".
This PR's base is
fix/knowledge-cli-multi-tag-data-loss, so merging it puts the fix on#34's branch before #34 reaches
main— the gate is satisfied with no rebase of the stack.Committing to #34's branch directly would fork it from this PR's merge-base and force one.
F1 is also the read-side twin of this PR's write-side fix, so it belongs in the same review
surface.
The read path now applies the same precedence as the write path: a stored tag equal to the
raw value matches before the value is split. Repeated
-tstill narrows (AND), and asingle name is still never widened into a glued tag — both asserted.
F3 (MEDIUM) — the precedence contract is now pinned
The review's surviving mutant (drop the
continueafter a whole-value match) left thewhole suite green while changing an item carrying both shapes from
removed: 1toremoved: 4, collapsing the documented "re-run to clear the split names" behaviour. Nowasserted across all three runs:
removed: 1→removed: 3→ exit1.F2 (MEDIUM) — stored tags are quoted in the failure message
Not-found names were
JSON.stringify'd while stored tags were joined raw, so on exactlythe damaged items this fallback exists for the message denied a tag that is plainly listed.
The
(not found: …)list in the success message is deliberately left unquoted: entriescan only reach
not_foundvia the split branch (a whole-value match is by construction instored, so it is filtered out), which means those names never contain a comma and arealready unambiguous.
F5 (LOW) —
update -t/upsert -treport an added countAdding 3 tags and adding none both printed
Updated <id>at exit 0 and carried the countnowhere — not in
message, not in JSON. Nowaddedin JSON andUpdated <id> (added 2 tags)inmessage. Appending an existing tag is idempotent, sounlike
untagthis stays exit 0 — it just stops claiming nothing. Reported onupsert'screate path too, so a caller never branches on
createdto knowaddedis meaningful.F6 (LOW) — docs
README and
--helpnow document thelistwhole-value rule, the human-messagepath forunmatched
untagnames, and the added count. README also notes that thelisttablerenders
["a,b,c"]and["a","b","c"]two spaces apart, so--jsonis the reliable wayto tell the shapes apart.
Verification
Every new assertion was mutation-proven load-bearing, with
src/cli.tsrestored andsha256-verified byte-identical after each mutant:
list -tto split-onlyExpected to contain "k_damaged" / Received [ "k_control" ]continue(the review's survivor)Expected: 1 / Received: 4"iapp" not in [iapp,integrations,architecture]Expected: 2 / Received: undefinedFull suite, three ambient env vars unset, exit codes unpiped:
255 pass / 2 skip / 1 fail (258 tests, 153.94s, load 42.95). Test declarations in
tests/cli.test.tswent 69 → 73, exactly the four added.The single failure —
context pack and proposal context commands return bounded agent JSON— reproduces at base354a855(= currentorigin/maintip), exit 1.Pre-existing; there is no head-only failing test name.
bunx tsc --noEmit: 8 errorsbefore and after, the same 8 identities (only a line-number shift in
src/cli.tsfromadded lines).
Artifacts
bin/knowledge.jsis rebuilt as the only committed artifact bundlingsrc/cli.ts.bin/knowledge-mcp.js,bin/knowledge-serve.jsanddist/are left byte-identical(sha256-verified before and after), which is exactly what
verify-generated-artifacts.mjsgates on — it passes at exit 0. The rebuilt bundle waschecked behaviourally, not by grep:
bun bin/knowledge.js list -t "iapp,…"returns bothshapes.
Not fixed here, deliberately
ok_untag/ok_list— untouched by both PRs and their repair path is intact(
z.array(z.string()), never split), so they should not block. The CLI/MCP parity gapthis PR opens (CLI exits 1 on
removed: 0, MCP returnsok: true) is trackedseparately as todos
68fc5c1c.config.modemisreporting — with the owner, needsa design decision rather than a patch.