fix(cli): stop silently dropping repeated -t tags on add/update/upsert - #34
Conversation
`-t/--tag` was parsed into a single string field, so each occurrence
OVERWROTE the previous one. `knowledge add -t a -t b -t c` exited 0, logged
"Item added", and persisted only `c`; the other tags were discarded with no
error, no warning, and no way for the caller to notice. `-t "a,b,c"` stored
the literal comma string as one tag. `update` and `upsert` had the same
parse bug — they only appeared to accumulate because they merge the single
parsed tag into the item's existing tags, so ONE tag per invocation landed.
This is a check that could not fail: the success signal ("Item added",
exit 0) carried no information about how many tags were stored, so it could
not distinguish 5 stored from 1 stored. Any tag-scoped query silently
returned an incomplete answer for affected items.
Fix:
- `-t/--tag` now accumulates across occurrences AND splits on commas, so
`-t a -t b` and `-t "a,b"` are equivalent. Comma-splitting matches the
`todos` CLI (`-t, --tags <tags>` / "Comma-separated tags"); the previous
behaviour of storing a comma string as one tag is never correct.
- Tags dedupe case-insensitively, preserving first-seen casing and order.
- `list -t x -t y` narrows: an item must carry EVERY requested tag. `untag`
removes every named tag in one pass. Both now match the array-typed
`ok_list` / `ok_untag` MCP tools, which already had the right semantics —
the CLI was the only surface out of parity.
- A `-t` with no usable value now fails loudly instead of dropping the tag.
- `[INFO] Item added` reports the stored tag count, so a partial write can
no longer pass unnoticed.
Regression test asserts the PERSISTED result — it reads each item back with
`get` and compares the full tag array. Asserting on exit code alone would
reproduce the original bug. Reverting the parser accumulation fails it on
the first assertion (5 tags expected, 1 stored).
bin/knowledge.js is rebuilt because it is the only artifact that bundles
src/cli.ts. bin/knowledge-mcp.js and dist/ are left at HEAD: they do not
contain CLI code, and rebuilding them produces unrelated toolchain churn
that already makes `verify:generated` fail on main.
Adversarial review — PR #34Anchored at head Verdict: APPROVE-WITH-FIXESThe parser fix is correct and, unusually, actually tested — 10 of 10 targeted mutations fail the new test at 10 distinct assertions. One blocking item: the comma-splitting 1. What I verified about the fix
Mutation coverage — every assertion has an input that fails itEach mutation applied to a scratch copy of
Ten distinct mutations, ten distinct failure sites — this is real coverage, not one assertion absorbing everything. Suite numbers — the PR body's figures are not reproducible; the conclusion isBoth runs on this box with
Not 250/2/1 and 246/2/4. Every failure in both runs is a 10s-timeout in the sync / context-pack / search / MCP-stdio suites. The one test that failed on the branch but not on base ( Note for anyone reproducing: unsetting 2. Required fixes2.1 BLOCKING — comma-splitting
|
| set | items | overlap |
|---|---|---|
| audit's confirmed | 24 | — |
| recovered from the unread transcript shape (exact title match and stored set == last intended tag) | 54 | 0 with audit |
| recovered independently by a second pass | 12 | 0 with audit, 8 with the above |
| union | 82 | all 82 exist in a store; no phantom ids |
Of those 82: 28 are in the live hosted store, 54 are legacy-store-only (~/.hasna/apps/knowledge/db.json, 42 from one 2026-06-08 session). The four live-store additions the audit missed:
k_mr7muoka_9011hc ['operating-rule'] never updated Chief identity rule: Friday
k_mr9ajfab_4pzffd ['foundation'] never updated Foundation SDK layer
k_mr6f1aa9_71vpu8 ['aws'] never updated AWS centralization decisions: API-mediated…
k_mrnavboq_gq6r7m ['provisioning'] never updated ASUS NUC 14 Essential as agentic SSH station
k_mr9ajfab_4pzffd explains the duplicated row in kn-correlate2.txt: the hosted store holds two non-archived items titled exactly Foundation SDK layer — k_mr9brdr2_drqx05 ['architecture'] and k_mr9ajfab_4pzffd ['foundation']. Both invocations resolved onto the first; the second was never seen. Duplicate titles defeat title-keyed resolution and will defeat the next pass too.
Also: the earliest proven damage is 2026-06-08, four weeks before the audit's earliest confirmed item, so the damage window is understated too.
So the floor is 82 (28 live), not 24 — and that is still a floor, because 66 newly-visible multi-tag invocations have no exact title match, and writes executed over ssh targeted other machines' stores entirely.
3.4 The 24 also contains at least one false positive — the repair list is not safe to run as-is
k_mr9j7bkr_g2ikqw stores ['scaffolds','oss']. Neither is the last intended tag, and the audit's intent list is ['scaffold','hasna-scaffolds','us-taxes'] — pooled from a chained command. The item is about scaffolds; the string tax appears 0 times in its content. The repair list would write us-taxes onto it, plus scaffold next to the existing scaffolds. Chained-command segmentation errs in both directions, and this one writes a wrong tag.
Two more defects in the repair list: k_mr9bj2ry_wpdwph also carries a comma-glued tag ('iapp,integrations,architecture') but gets no untag line at all; and the MISSING sets in kn-correlate2.txt sum to 61 while the list emits 63 update -t lines. Every line needs eyeballing against the item before execution.
3.5 The 339 — yes, there is a discriminator, and it is outside the app
The knowledge app cannot answer this: there is no item-level event log. Positive control — knowledge events list returns 19,778 todos task.created events carrying full payloads including tags, and exactly one knowledge-source event, a test.ping with data: {}. No revisions/history/created_by fields; item keys are archived, content, created_at, id, metadata, short_id, tags, tenant_id, title, updated_at, url with metadata: {}. Git cannot date the defect either: the local mirror is a 1-commit squash and the canonical checkout's .git is a husk.
The discriminator is the caller-side transcripts — the original command lines are on disk. That is what produced §3.3. Applied to the single-tag population it gives, on the ~12% of single-tag items with a recovered creation command, a 22/43 damage rate; extrapolated that is ~140-185 damaged items among 361-362 single-tag items, and the extrapolation is biased upward (matched items over-represent heavy taggers), so treat 140-185 as a working range with 233 a soft ceiling — and treat only the 82 as proven.
Two corrections to the audit's framing of that population:
- The ceiling population is wrong.
-t ""or a trailing-tunder the old parser stored zero tags at exit 0 — and-t a -t ""stored zero despite two intended. There are 263 zero-tag items in the union (24% of the corpus). Single-tag-only is not the ceiling. - Timestamp clustering was proposed but never run, and it contradicts "no mass damage": week 2026-W24 has 168 items created and zero with ≥2 tags.
Two arithmetic notes: the corpus union is 1086 distinct ids, not 1085 (the hosted store is live — it grew by one item, k_ms3gu0zn_starj3 at 16:53Z, between the audit's export and mine, which also moves single-tag from 360 to 361). And knowledge list --json reports total: 688 while export returns 720 — a second undercount trap next to the stats one already flagged.
3.6 The two convention items — confirmed, and a two-item repair is safe and sufficient
knowledge list -t convention -l 500 --json → total 51
hasna-agent-comms-envelope present: False stored ['contracts','realtime']
k_ms2s6acl_0emq6i present: False stored ['convention,canonical,policy,deployment']
positive control, same query, ids returned: hasna-oss-repo-naming-policy,
hasna-workspace-layout-convention, hasna-channel-naming-convention,
hasna-agent-identity-convention, hasna-loop-naming-convention
Both confirmed unreachable, with the query proven working. Yes — repairing just these two is safe and sufficient to restore the rule-20 path, and it should be decided separately from the mass rewrite. It is 5 update -t calls plus 1 untag, all append-only or single-tag-removal, on two items whose intended tags are known from the item's own glued tag and from an exact transcript match — no inference. It is independent of every disputed number above, and it is the only part of the repair with zero misattribution risk. Do it with the currently-published CLI, before this merges (see §2.1), or land (b)/(c) first.
The remaining 63-tag rewrite should wait for a corrected extractor: 3.5× more invocations to examine, per-invocation -t scoping so chained commands stop pooling, rejection of non-shell payloads (two "commands" in the audit's own output are JavaScript workflow definitions that never executed, and two scrape todos add -t … from a different CLI), and duplicate-title de-collision.
4. What neither of us can see
- Whether the 54 legacy-only items were damaged by this bug or written when the CLI never supported repeated
-tat all. Tags are lost relative to intent either way; causal attribution is not proven, and the defect cannot be dated because the git history for this package is not present on this machine. - Writes executed over
ssh(39 command strings) landed in other machines' stores. Unbounded from here. - Whether the tag-complete items were transiently damaged and later backfilled, or written correctly. Tag ordering (last-intended-tag first) is suggestive of an
updateloop but is not proof, and no per-item tag history exists. - 66 newly-visible multi-tag invocations with no exact title match — deleted, pruned, probes, or another machine. Unresolved between the floor of 82 and the single-tag ceiling.
5. Highest-leverage follow-up
knowledge is the only app in this fleet that answers "what was this item created with?" with nothing, while the event bus next to it carries full create payloads for todos. Emitting knowledge.item.created/updated would have made this entire audit a one-command query, and would make the next one cheap. That is a bigger win than the remaining tag repair.
…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".
`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).
Adversarial verdict on
|
…ence; quote partial-miss names Applies the four non-blocking fixes from the APPROVE-WITH-FIXES review of #34 -> main (tree 89bb69e). No blocking defect was found; three of these are wording, one is a one-expression output change. 1. The docs contradicted a contract the tests actually pin. Both the code comment and README said `list -t` uses "the same precedence `untag` uses". It does not, and must not: - `list` is a UNION - whole value OR its comma-split names. - `untag` is EXCLUSIVE - whole value, and only then split, via `continue`. That distinction is exactly what makes `untag`'s documented "re-run to clear the split names" behaviour true, and a test pins it (removed: 1 then 3). A reader trusting the old wording would "fix" the union into an exclusive match and silently break `list -t` for the one job it exists for here - finding the remaining glued items. Both sites now state the two rules separately, say why each is right for its own command, and warn against unifying them. Same for the `list` help text, which had the imprecise wording too. 2. `untag`'s partial-miss message rendered `(not found: p, q)` - two names reading as one. Now quoted, `(not found: "p", "q")`, matching the fix already applied to the error path. JSON `not_found` was already unambiguous. Pinned by a new assertion that also asserts the unquoted form is absent. 3. README: `list` excludes archived items by default, so a glued tag on an archived item is not surfaced; `--include-archived` sweeps both and is a supported full-sweep path. Documented, plus the two archived flags that were missing from the flag table. No behaviour change - the corpus's only comma-containing tag is unarchived, so there is no live exposure. 4. Two follow-ups filed rather than fixed here: MCP `ok_untag` truthfulness and glued-value splitting (KNO2 68fc5c1c, extended with executed stdio evidence), and the store-divergence design decision (KNO2-00002), which neither PR touches and which is owner-held. Verification (unpiped exit codes, hermetic - three env vars unset, --timeout 60000): bun test tests/cli.test.ts 72 pass / 1 fail / 73 - IDENTICAL to baseline. The one failure is `context pack ...`, which reproduces at base; judged by failing test NAME, not count, because the file already fails 1 test at baseline. mutant, Fix 2 quoting reverted 2 fails = `untag matches a tag value whole before splitting ...` (new) + the pre-existing one. src/cli.ts sha256-verified byte-identical after revert. bunx tsc --noEmit same 8 pre-existing errors (ANSI stripped before counting); none in the hunks touched here. bun scripts/verify-generated-artifacts.mjs exit 0 bin/knowledge.js rebuilt bundle behaviour matches source, 0 NUL bytes. bin/knowledge-mcp.js and dist/ deliberately left at HEAD, as in the rest of this PR.
Review fixes applied —
|
| command | rule | why |
|---|---|---|
list -t |
union — whole value OR its split names | a filter destroys nothing; narrowing to one shape would hide the very items the query exists to find |
untag -t |
exclusive — whole value, then split, via continue |
a mutation must take one shape per run — that is what makes the documented "re-run to clear the split names" behaviour true |
Both directions now carry an explicit "do not unify these" note, since the failure mode is a future reader "fixing" the union into an exclusive match. The list help text had the same imprecise wording and is corrected too. Verified by execution: untag run1 removed: 1 → run2 removed: 3 (exclusive); list -t "a,b,c" → total 2, both shapes, one query (union).
2 — partial-miss message quoted. (not found: p, q) → (not found: "p", "q"), matching the error path. JSON not_found was already correct and is unchanged (['p','q']).
One honest refinement of the framing: I probed whether untag can actually emit a single comma-containing not-found name, and it cannot — a whole value only enters remove when stored.has(whole) is true, in which case it is found by definition, so every not_found entry is a comma-free split name (-t 'p, q', 'p,q', 'p ,q', 'a,,b' all → ['p','q']/['a','b']). So this closes a reading ambiguity, not a reachable wrong answer. Still worth fixing — a caller cannot be expected to derive that invariant from the output — but it is a smaller claim than "same ambiguity as the error path", where the glued case was live.
3 — archived scope documented. README now states list excludes archived by default and points at --include-archived for a full sweep (--archived for archived only). The two flags were also missing from the list flag table and are added. No behaviour change, no feature added.
4 — two follow-ups filed, not fixed here.
- MCP parity — extended existing task
68fc5c1cwith the executed-over-stdio evidence, including the finding thatok_untagreturnsremoved: 0where the CLI now removes 3. Recorded that the CLI is now a strict superset, and that the union/exclusive asymmetry must be preserved when porting (ok_list→ union,ok_untag→ exclusive). - Store divergence — new task
KNO2-00002, filed as an owner-held design decision, not a patch: missingHASNA_KNOWLEDGE_API_KEYsilently redirects reads to a divergent local store serving three retracted standards as live (one instructinggit worktree add /tmp/..., against the non-overridable worktree rule) whileconfig.modereports"hosted".
Verification — unpiped exit codes, hermetic (three env vars unset, --timeout 60000)
| check | result |
|---|---|
bun test tests/cli.test.ts baseline 2531d0d |
72 pass / 1 fail / 73 |
bun test tests/cli.test.ts after fixes |
72 pass / 1 fail / 73 — identical |
| failing test name, both runs | context pack and proposal context commands return bounded agent JSON — reproduces at base |
| mutant: Fix 2 quoting reverted | caught — 2 fails = untag matches a tag value whole before splitting… (new) + the pre-existing one |
src/cli.ts sha256 after mutant revert |
byte-identical (a343d83c…) |
bunx tsc --noEmit |
same 8 pre-existing errors, ANSI stripped before counting; none in the hunks touched here |
bun scripts/verify-generated-artifacts.mjs |
0 |
| staged secrets scan | clean, positive-controlled (detector fires on a synthetic password = line and AKIA…) |
Judged by failing test name, not count — tests/cli.test.ts already fails 1 test at baseline, so a 1 fail result would otherwise read as "mutant killed" when nothing was killed.
bin/knowledge.js is rebuilt (it bundles src/cli.ts, and Fix 2 is a behaviour change): the bundle prints (not found: "p", "q") and carries the new help wording, 0 NUL bytes. bin/knowledge-mcp.js and dist/ were regenerated by bun run build and deliberately restored to HEAD, consistent with the rest of this PR — only the four intended paths were staged.
Post-merge corrections to this PR's delta — landed in #36Four claims in this PR's delta were re-measured after merge and are corrected in #36. Posting as a comment rather than editing this body: GitHub keeps no PR-body revision history and emits no timeline event for a body edit, so a body edit would be an unattributable, untimestamped change to a merged artifact of record. (I did edit this body earlier today and have restored it byte-for-byte to its pre-edit text — 10736 chars, verified by readback. That was the wrong channel; this is the right one.)
Reconfirmed unchanged, not corrected: Separately found while verifying, not caused by this PR and tracked on their own: |
…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
The defect
-t/--tagwas parsed into a single string field, so each occurrence overwrote the previous one:knowledge add "T" "B" -t a -t b -t cexited 0, logged[INFO] Item added, and persisted onlyc. The other tags were discarded with no error, no warning, and no way for the caller to notice.-t "a,b,c"stored the literal comma string as one tag.updateandupserthad the same parse bug. They only appeared to accumulate, because they merge the single parsed tag into the item's existing tags — so exactly one tag per invocation landed. On an untagged item,update -t p -t q -t rstored["r"].Two other repeatable flags in the same
switchalready had the correct idiom (--domain,--source-refboth do[...(flags.x ?? []), argv[i + 1]]);--tagwas the outlier.Why the tests did not catch it
The success signal carried no information about how many tags were stored, so exit 0 could not distinguish "3 tags stored" from "1 tag stored". A test asserting
addexits 0 reproduces the bug rather than catching it — the check could not fail. Every assertion in the new test therefore reads the item back withgetand compares the full stored tag array.The comma decision
-t "a,b,c"now splits rather than erroring. Reasons:todosCLI documents-t, --tags <tags>as "Comma-separated tags", so splitting is the established idiom for this flag across the toolchain, not a coin flip.Owning the inconsistency (raised in review 2.3): this PR does return non-zero on input that previously exited 0 — a
-twhose value is missing, empty, or separator-only now exits1. The two cases are treated differently on purpose, and the difference is now documented in README: a comma-joined value carries unambiguous intent and is recoverable by splitting, whereas an empty value carries no intent at all, so silently storing nothing is the same defect this PR exists to fix. Blast radius checked: 0 of 42knowledge add|update|upsertsnippets across~/.codewith/skills,~/.claude/skillsand~/.agents/skillspass-t "$VAR", so-t ""from an empty expansion has no confirmed caller.Repeated
-tand comma lists compose:-t a -t "b,c"stores three tags.Changes
-t/--tagaccumulates across occurrences and splits on commas; tags dedupe case-insensitively, preserving first-seen casing and order.list -t x -t ynarrows — an item must carry every requested tag.untag -t x -t yremoves both in one pass. Both now match the array-typedok_list("item must match all tags") andok_untagMCP tools, which already had the right semantics. The CLI was the only surface out of CLI/MCP parity.-twith no usable value (missing, empty, or only separators) now fails loudly instead of dropping the tag.[INFO] Item addedreports the stored tag count, so a partial write can no longer pass unnoticed.Verification
Unpiped exit codes (
cmd >/dev/null 2>&1; echo \$?):010Reverting only the parser line fails the test on its first assertion:
Full suite, hermetic — note that three env vars must be unset, not one:
KNOWLEDGE_API_URL,HASNA_KNOWLEDGE_API_URLandHASNA_KNOWLEDGE_API_KEY. With only the first unset the local catalog pipeline refuses to run and the suite reports ~100 failures from a single cause.No new failing tests versus baseline.
The absolute pass/fail counts previously quoted here have been removed, because they are not a property of this branch — they are a property of this box under load. Three independent runs of the same commit
78db6e1disagree:78db6e1#issuecomment-5094526934Load ranged 58–112 with up to 15 concurrent
bun testprocesses on the machine. Every failure in every run is timeout-shaped (≥5s wall, in the MCP-stdio / sync / search / context-pack suites), so each run is another sample of the same noise and no additional run can settle it. Use--timeout 60000.The load-invariant way to check this repo
Compare the set of failing test names, not the counts, and re-run any candidate in isolation — an isolated run does not contend for the machine, so it is not a load sample:
Applied to every test that failed on
78db6e1:354a85578db6e1registers tools and can add/get through stdio0sync dry-run and push copy a project catalog into a peer workspace0sync export and import move a bundle through stdin/stdout00search command returns hybrid source, wiki, and semantic results0sync status, snapshot, machines, and conflicts use the project catalog11maincontext pack and proposal context commands return bounded agent JSON11mainHead-only failing test names: zero. Four of the six suite failures pass when run alone; the two that genuinely fail also fail on pristine
354a855.sync export and import …is worth calling out — it was the one branch-only name in the review's counts, and in isolation it passes on both sides, so it was a flake rather than a regression.bunx tsc --noEmitreports the same 8 pre-existing errors before and after.Status of the blocking review item
The review's blocking item — comma-splitting
-tmakinguntagunable to remove acomma-glued tag (
removed: 0at exit0, tag survives) — is fixed in #35, which isstacked on this branch. Merge #35 into this branch before merging this PR, otherwise
the CLI ships unable to clear the very items this PR's audit identified.
Both remedies from review 2.1 were applied, not one: the two
conventionitems wererepaired with the published CLI
0.2.91before #35 existed (knowledge list -t conventiontotal
52 -> 54, both items now returned, re-confirmed againstexport), and #35 addsthe whole-value fallback. The repair alone was not sufficient —
k_mr9bj2ry_wpdwphstillcarries the glued tag
iapp,integrations,architecture, so a glued item survives in the livecorpus and the CLI still needs to be able to clear it.
Artifacts
bin/knowledge.jsis rebuilt: it is the only committed artifact that bundlessrc/cli.ts.bin/knowledge-mcp.jsanddist/are deliberately left at HEAD — they contain no CLI code, and rebuilding them produces unrelated toolchain churn that already makesverify:generatedfail onmain(reproduced by rebuilding pristinemain). Note thatverify-generated-artifacts.mjsonly requiresbin/knowledge-mcp.jsanddistto be byte-stable, which this PR preserves.Notes for the maintainer
The read-side twin of the
untagregression (review F1) is fixed in fix(cli): whole-value-first tag matching on untag and list; truthful add/remove counts #35, which lands into this branch. This branch'slist -tsplit the raw-tvalue before matching, so an item carrying one literal"a,b,c"tag was undiscoverable by the very command used to find it. Do not merge this PR tomainahead of fix(cli): whole-value-first tag matching on untag and list; truthful add/remove counts #35.Correction to an earlier claim in this PR (review F4).
knowledge statswas described here as "undercounting the corpus". That is wrong, and it sends the next reader hunting a bug that does not exist.statsandlistreport the non-archived count, and it reconciles withexportexactly — measured 2026-07-27:export726 items = 695 active + 31 archived,statstotal = 695,listtotal = 695. That is archived-exclusion by design. Useexportfor corpus-wide figures because it includes archived items, not becausestats/listare broken.One pre-existing issue found while verifying, not addressed here: the test suite is not hermetic — an ambient cloud-mode API env var makes 34/67 CLI tests fail, which is a misleading signal for anyone running
bun teston a configured machine.Still unguarded:
-t --jsonconsumes the next flag as a tag value. No tag in the corpus begins with-, so a guard would be safe, but it is a distinct hazard and out of scope here.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.