From 59d1d65968a07d99b6570ed43556a409a2039bf4 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Mon, 27 Jul 2026 23:18:19 +0300 Subject: [PATCH 1/3] docs(cli): untag exclusivity is per -t value, not per run; fix three more misstated claims Wording-only corrections to claims landed by #34/#35. No behaviour change: the only non-comment source edit is one added `list --help` line. 1. "one shape per run" was measurably false (4 sites) The `continue` that enforces untag's whole-value-before-split rule sits INSIDE `for (const raw of flags.tagRaw ?? flags.tag)`, so the exclusivity is per raw `-t` value, not per run. On an item holding both shapes: untag -t 'a,b,c' -t a -> removed=2 untag -t 'a,b,c' -t a -t b -t c -> removed=4 removed=4 in a single run IS "matching both shapes at once", which README.md:323, README.md:417-418, src/cli.ts:1804-1805 and src/cli.ts:1920-1921 all said untag deliberately does not do. The re-run contract itself is true and unchanged (removed=1 -> removed=3 -> exit 1); only its stated reason was wrong, and the reason is what a reader would reimplement from. 2. the quoting justification cited an unreachable case README.md:434-435 and the matching src/cli.ts comment motivated quoting the partial-miss names with "(not found: p, q) cannot be told apart from one missing tag literally named p, q". That collision is unreachable: a comma-bearing value only enters `remove` via the whole-value branch, which requires stored.has(whole), so it is found by definition and never appears in notFound; every other entry is a comma-split part. 12 probes confirm no comma ever reaches notFound. The reachable justification is whitespace-bearing names -- `-t "p q"` gives notFound ["p q"], and unquoted that prints `(not found: p q)` with nothing marking where the one name ends -- plus symmetry with the failure message, where commas in STORED tags make the quoting load-bearing for real. Quoting was under-claimed, not wrong. 3. an undocumented flag precedence on list `--archived --include-archived` returns archived items ONLY: --archived wins, in either argument order, because of the `else if`. A reader would expect the wider flag to win. Documented in README, the flags table, a source comment and `list --help`; behaviour untouched. 4. verify:generated was cited for something it does not check scripts/verify-generated-artifacts.mjs only git-diffs bin/knowledge-mcp.js and dist, then greps four files for two stale Windows-path regexes. It never compares bin/knowledge.js against src/cli.ts -- proven: it exits 0 with src/cli.ts diverged from the bundle, and exits 0 even with bin/knowledge.js itself corrupted by 30 junk bytes (it does not require that file to be byte-stable at all). It does exit 1 on a planted `file://${x}` pattern, so the check it has works. The CHANGELOG citation now says what the verifier proves and points at the check that actually establishes bundle/source sync: rebuild to a temp outfile with the `build` command and diff against the committed file after per-line trailing-whitespace stripping. bin/knowledge.js is rebuilt for the one added help line and nothing else -- the line-level diff against 64d05ae is exactly that one string. bin/knowledge-mcp.js, bin/knowledge-serve.js and dist are untouched, so verify:generated still exits 0. --- CHANGELOG.md | 38 ++++++++++++++++++++++++++-- README.md | 65 ++++++++++++++++++++++++++++++++++++------------ bin/knowledge.js | 3 ++- src/cli.ts | 50 ++++++++++++++++++++++++++----------- 4 files changed, 122 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85af382..2801bd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,32 @@ # Changelog +## Unreleased — documentation corrections to the multi-tag work + +Wording-only corrections to claims landed by #34/#35. No behaviour change: the +only non-comment source edit is one added `list --help` line, and the removal +semantics, exit codes and messages are untouched. + +- The `untag` whole-value-versus-split exclusivity is **per raw `-t` value**, not + "one shape per run". Four sites said per run (`README.md` twice, `src/cli.ts` + twice); repeated `-t` still accumulates, so on an item carrying both shapes + `untag -t "a,b,c" -t a` removes 2 and `-t "a,b,c" -t a -t b -t c` removes 4 in a + single run. The re-run contract itself is true and unchanged (`removed: 1`, then + `removed: 3`, then exit 1) — only its stated reason was wrong, and the reason is + what a reader would reimplement from. +- Corrected the justification for quoting names in the partial-miss `untag` + message. The cited case — `(not found: p, q)` colliding with one tag literally + named `p, q` — is **unreachable**: a comma-bearing value only enters the removal + set via the whole-value branch, which requires the tag to be stored, so it is + found by definition and never appears in `not_found`. The reachable + justification is whitespace-bearing names (`-t "p q"` → `(not found: p q)`), plus + symmetry with the failure message, where commas in *stored* tags make the + quoting load-bearing for real. +- Documented an undocumented flag precedence on `list`: when both `--archived` and + `--include-archived` are passed, `--archived` wins (archived items only) in + either order. Documented, not changed. +- Corrected the `verify:generated` citation under "inventory paths block fix" — + see that entry for what the verifier actually checks. + ## 0.2.91 Harden the local JSON item-store against lock corruption and add a sanctioned @@ -59,8 +86,15 @@ self_hosted/cloud (api) mode, where it disagreed with `knowledge paths`. - test(knowledge): `tests/cloud-inventory.test.ts` now asserts `inventory.paths` equals `service.paths()` for all four path fields, that the `/v1` URL never appears in the paths block, and that it is reported on `legacy_store.path`. -- Rebuilt generated bundles so shipped artifacts carry the fix and - `verify:generated` passes. +- Rebuilt generated bundles so shipped artifacts carry the fix. Note what + `verify:generated` does and does not prove: `scripts/verify-generated-artifacts.mjs` + only (a) `git diff --exit-code`s `bin/knowledge-mcp.js` and `dist`, and (b) greps + four generated files for two stale Windows-path patterns. It never compares a + bundle against its source, so its exit 0 is evidence that the *untouched* + artifacts are untouched — not that a rebuilt bundle matches the source it was + built from. For that, rebuild the bundle to a temp outfile with the `build` + script's command and diff it against the committed file after per-line + trailing-whitespace stripping (see `scripts/strip-generated-trailing-whitespace.mjs`). ## Unreleased — Search overhaul, Stage 2 (Postgres full-text parity) diff --git a/README.md b/README.md index 5c5fbdb..aa9577b 100644 --- a/README.md +++ b/README.md @@ -310,7 +310,7 @@ state, or keyword retrieval across active compatibility notes too. | `-t, --tag ` | Filter by tag; repeatable/comma-separated, an item must match **all** of them | | `--sort created\|title` | Sort field (default: created) | | `--desc` | Sort descending | -| `--archived` | Return archived items *only* | +| `--archived` | Return archived items *only*; **wins** over `--include-archived` if both are passed | | `--include-archived` | Return live **and** archived items (default: live only) | | `--verbose` | Print the full paginated item objects | | `--json` | Print the stable machine-readable list response | @@ -321,14 +321,16 @@ Each `-t` value matches an item when the item's stored tags contain the **whole falling back to the split names *only* when no stored tag equals the whole value. Both rules are right for their own command. `untag` writes, so it must take one shape per -run — that is exactly what makes its documented "re-run to clear the split names" -behaviour true. `list` only reads, so matching both shapes destroys nothing, while -matching only one would hide items from the very query used to find them. Items written -before the multi-tag parse fix can carry a single literal `"a,b,c"` tag, which none of the -split names equals, so a split-only filter does not merely miss the damaged item: it -answers with a *different* item that carries the three names separately, at `total: 1` and -exit 0, giving the operator no signal that the result changed. The union therefore finds -both shapes in one query: +`-t` **value** — that is exactly what makes its documented "re-run to clear the split +names" behaviour true. The exclusivity is per raw `-t` value, **not** per run: repeating +`-t` still accumulates, so `untag -t "a,b,c" -t a -t b -t c` removes four tags from an item +carrying both shapes in a single run. `list` only reads, so matching both shapes destroys +nothing, while matching only one would hide items from the very query used to find them. +Items written before the multi-tag parse fix can carry a single literal `"a,b,c"` tag, which +none of the split names equals, so a split-only filter does not merely miss the damaged +item: it answers with a *different* item that carries the three names separately, at +`total: 1` and exit 0, giving the operator no signal that the result changed. The union +therefore finds both shapes in one query: ```bash # returns items tagged with the literal "a,b,c" AND items tagged a + b + c separately @@ -343,6 +345,10 @@ tag that sits on an archived item. Add `--include-archived` to sweep live and ar items together, or `--archived` for archived items only — use the former when auditing the corpus for tag damage rather than reading live knowledge. +If **both** are passed, `--archived` wins and the result is archived items *only* — the +narrower flag, not the wider one, regardless of the order they appear in. So +`--archived --include-archived` is not a way to widen the sweep; drop `--archived` for that. + ### inventory ```bash knowledge inventory [--scope local|global|project] [--limit ] [--include-archived] [--verbose] [--json] @@ -415,9 +421,24 @@ knowledge untag --id -t "a,b,c" When an item somehow carries both shapes, the whole-value match wins and the literal tag is removed first; re-run to clear the split names. That exclusivity is the point: one -shape per run is what makes re-running meaningful, so `untag` deliberately does **not** -match both shapes at once. [`list -t`](#list) does — a union — because a filter has nothing -to remove and suppressing a shape would hide items. Do not "unify" the two rules. +shape per `-t` **value** is what makes re-running meaningful, so a *single* `-t` deliberately +does **not** match both shapes at once. [`list -t`](#list) does — a union — because a filter +has nothing to remove and suppressing a shape would hide items. Do not "unify" the two rules. + +The exclusivity is scoped to one `-t` value, **not** to the whole run. Repeated `-t` still +accumulates, so on an item carrying both shapes: + +```bash +# stored tags ["a,b,c","a","b","c"] +knowledge untag --id -t "a,b,c" # removed: 1 (whole value only) +knowledge untag --id -t "a,b,c" -t a # removed: 2 +knowledge untag --id -t "a,b,c" -t a -t b -t c # removed: 4 (both shapes, one run) +``` + +Naming the split names explicitly is a caller's way of asking for both shapes at once. What +the `-t`-level exclusivity guarantees is narrower and is the part the re-run contract rests +on: one `-t "a,b,c"` clears the glued tag and leaves the split names, so re-running the same +command is meaningful (`removed: 1`, then `removed: 3`, then exit 1). Removing nothing **exits 1**. `removed: 0` at exit 0 with a `Removed tag from ` message is the same class of defect as the original bug — a success signal that @@ -429,16 +450,28 @@ succeeds and the unmatched names are reported both in `message` (`Removed 1 tag invisible to exactly the callers most likely to be reading it. Tag names are quoted on **both** paths — the stored tags in the failure message, and the -unmatched names in the partial-miss success line. Unquoted, a single glued `"a,b,c"` tag -renders identically to three separate ones, so the message appears to deny a tag that is -plainly listed, and `(not found: p, q)` cannot be told apart from one missing tag literally -named `p, q`: +unmatched names in the partial-miss success line: ``` Error: No matching tag on k_x: "iapp" not in ["iapp,integrations,architecture"] Removed 1 tag from k_y (not found: "p", "q") ``` +On the **failure** path the quoting resolves a real collision: stored tags can contain +commas, so unquoted, a single glued `"a,b,c"` tag renders identically to three separate +ones and the message appears to deny a tag that is plainly listed. + +On the **partial-miss** path the justification is different, and it is *not* the comma +case. A `not_found` entry can never contain a comma: a comma-bearing value only enters the +removal set through the whole-value branch, which requires the stored tags to already +contain it — so it is found by definition — and every other entry comes from splitting on +commas. `(not found: p, q)` therefore always means two names, and one missing tag literally +named `p, q` is **unreachable**. What is reachable is **whitespace-bearing** names: `-t "p q"` +yields `not_found: ["p q"]`, and unquoted that prints `(not found: p q)`, which gives the +reader no marker of where one name ends — one tag named `p q` reads exactly like a pair. +Quoting makes each name's extent explicit and keeps the two messages symmetric, so a reader +does not have to know which of the two lines happens to be ambiguity-proof. + ### delete ```bash knowledge delete|rm --id --yes diff --git a/bin/knowledge.js b/bin/knowledge.js index ac19099..67e0011 100755 --- a/bin/knowledge.js +++ b/bin/knowledge.js @@ -1342,7 +1342,8 @@ Prune Options: \`-t "a,b,c"\` finds items carrying a legacy literal "a,b,c" tag as well as items carrying the three names separately. (\`untag\` differs on purpose: it stops at the whole-value match.) Use --json to tell those two shapes apart; the table renders them near-identically. - Archived items are excluded by default; add --include-archived to sweep both.`);return}if(_==="get"){console.log("Usage: knowledge get --id [--json]");return}if(_==="update"||_==="edit"){console.log(`Usage: knowledge update|edit --id [--title ] [--content <content>] [--url <url>] [-t <tag>]... [--json] + Archived items are excluded by default; add --include-archived to sweep both. + If both --archived and --include-archived are passed, --archived wins (archived items only).`);return}if(_==="get"){console.log("Usage: knowledge get --id <id> [--json]");return}if(_==="update"||_==="edit"){console.log(`Usage: knowledge update|edit --id <id> [--title <title>] [--content <content>] [--url <url>] [-t <tag>]... [--json] -t/--tag is repeatable and accepts comma-separated values; tags are added, never replaced. With -t the output reports how many tags were actually added, so 0 added is not read as 3.`);return}if(_==="archive"){console.log("Usage: knowledge archive --id <id> [--json]");return}if(_==="restore"||_==="unarchive"){console.log("Usage: knowledge restore|unarchive --id <id> [--json]");return}if(_==="upsert"){console.log(`Usage: knowledge upsert [title] [content] [--id <id>] [--title <title>] [--content <content>] [--url <url>] [-t <tag>]... [--json] -t/--tag is repeatable and accepts comma-separated values; tags are added, never replaced. diff --git a/src/cli.ts b/src/cli.ts index bd5e4ad..dac1e29 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -439,7 +439,7 @@ Prune Options: function printCommandHelp(command: string): void { if (command === 'add') { console.log('Usage: knowledge add <title> <content> [--url <url>] [-t <tag>]... [--json]\n -t/--tag is repeatable and accepts comma-separated values: -t a -t b == -t "a,b"'); return; } - if (command === 'list' || command === 'ls') { console.log('Usage: knowledge list|ls [--format table|json] [-p <page>] [-l <limit>] [-s <search>] [-t <tag>]... [--sort created|title] [--desc] [--archived] [--include-archived] [--verbose] [--json]\n -t/--tag is repeatable and accepts comma-separated values; repeated -t narrows (an item must carry every tag).\n Each value matches an item carrying the whole value OR all of its comma-split names — a union, so\n `-t "a,b,c"` finds items carrying a legacy literal "a,b,c" tag as well as items carrying the three\n names separately. (`untag` differs on purpose: it stops at the whole-value match.)\n Use --json to tell those two shapes apart; the table renders them near-identically.\n Archived items are excluded by default; add --include-archived to sweep both.'); return; } + if (command === 'list' || command === 'ls') { console.log('Usage: knowledge list|ls [--format table|json] [-p <page>] [-l <limit>] [-s <search>] [-t <tag>]... [--sort created|title] [--desc] [--archived] [--include-archived] [--verbose] [--json]\n -t/--tag is repeatable and accepts comma-separated values; repeated -t narrows (an item must carry every tag).\n Each value matches an item carrying the whole value OR all of its comma-split names — a union, so\n `-t "a,b,c"` finds items carrying a legacy literal "a,b,c" tag as well as items carrying the three\n names separately. (`untag` differs on purpose: it stops at the whole-value match.)\n Use --json to tell those two shapes apart; the table renders them near-identically.\n Archived items are excluded by default; add --include-archived to sweep both.\n If both --archived and --include-archived are passed, --archived wins (archived items only).'); return; } if (command === 'get') { console.log('Usage: knowledge get --id <id> [--json]'); return; } if (command === 'update' || command === 'edit') { console.log('Usage: knowledge update|edit --id <id> [--title <title>] [--content <content>] [--url <url>] [-t <tag>]... [--json]\n -t/--tag is repeatable and accepts comma-separated values; tags are added, never replaced.\n With -t the output reports how many tags were actually added, so 0 added is not read as 3.'); return; } if (command === 'archive') { console.log('Usage: knowledge archive --id <id> [--json]'); return; } @@ -1802,11 +1802,11 @@ async function run(argv: string[]): Promise<void> { // A raw value matches an item when the stored tags contain the WHOLE value OR all of // its comma-split names — a UNION. This is deliberately NOT the rule `untag` uses: // there a whole-value hit short-circuits (`continue`), so the two shapes are mutually - // exclusive within one run. + // exclusive per RAW -t VALUE — not per run, since repeated -t still accumulates there. // // Each rule is right for its own command. `untag` mutates, so it must take one shape - // per run — that exclusivity is exactly what makes its documented "re-run to clear the - // split names" behaviour true, and a test pins it. `list` only reads, so matching both + // per -t value — that exclusivity is exactly what makes its documented "re-run to clear + // the split names" behaviour true, and a test pins it. `list` only reads, so matching both // shapes destroys nothing, while narrowing to one would hide items. An item damaged by // the multi-tag defect carries one literal `"a,b,c"` tag that none of the split names // equals, so a split-only filter does not merely fail to find it — it returns a @@ -1825,6 +1825,11 @@ async function run(argv: string[]): Promise<void> { const useJson = flags.json || flags.format === 'json'; let filtered = db.items; + // Flag precedence, documented because it is not the intuitive one: --archived WINS over + // --include-archived when both are passed (the `else if` never reaches the wider flag), + // in either argument order. `--archived --include-archived` therefore returns archived + // items ONLY, not a widened sweep. A reader would reasonably expect the wider flag to + // win, so this is documented rather than changed — flipping it would alter behaviour. if (flags.archived) filtered = filtered.filter((x) => x.archived === true); else if (!flags.includeArchived) filtered = filtered.filter((x) => !x.archived); if (search) filtered = filtered.filter((x) => x.title.toLowerCase().includes(search) || x.content.toLowerCase().includes(search)); @@ -1918,11 +1923,20 @@ async function run(argv: string[]): Promise<void> { // clear those items, and `ok_untag` (which never split) stays in parity. // // The `continue` is load-bearing, not an optimisation: it is what keeps the two shapes - // mutually exclusive per run, and therefore what makes the documented "re-run to clear - // the split names" behaviour true. Drop it and an item carrying both shapes goes from - // removed: 1 to removed: 4. `list -t` matches the two shapes as a UNION instead — - // correct there, because a filter has nothing to destroy and suppressing a shape would - // defeat the search it exists for. Same inputs, two different rules, both deliberate. + // mutually exclusive PER RAW -t VALUE, and therefore what makes the documented "re-run + // to clear the split names" behaviour true. Drop it and one `-t "a,b,c"` against an item + // carrying both shapes goes from removed: 1 to removed: 4. + // + // Note the scope precisely: the `continue` sits INSIDE this loop, so it bounds one raw + // value, not the whole invocation. Repeated -t still accumulates, so `-t "a,b,c" -t a` + // removes 2 and `-t "a,b,c" -t a -t b -t c` removes 4 in a single run. That is intended — + // naming the split names is how a caller asks for both shapes. The narrower guarantee is + // the one the re-run contract needs: a lone `-t "a,b,c"` takes the glued tag and leaves + // the split names, so re-running the same command is meaningful (1, then 3, then exit 1). + // + // `list -t` matches the two shapes as a UNION instead — correct there, because a filter + // has nothing to destroy and suppressing a shape would defeat the search it exists for. + // Same inputs, two different rules, both deliberate. const remove = new Set<string>(); for (const raw of flags.tagRaw ?? flags.tag) { const whole = raw.trim().toLowerCase(); @@ -1946,12 +1960,18 @@ async function run(argv: string[]): Promise<void> { } const item = await itemStore.update(current.id, { tags }); // A partial miss must be visible too, not just the all-miss case — and it has to be - // in `message`, because non-JSON output prints nothing else. Quoted for the same - // reason the failure message above quotes: joined raw, two missing names render as - // `(not found: p, q)`, which is indistinguishable from ONE missing tag literally named - // `p, q` — the same ambiguity that made the all-miss message appear to deny a tag that - // was plainly present. `not_found` in the JSON was already unambiguous; this is the - // human line catching up. + // in `message`, because non-JSON output prints nothing else. + // + // Quoted, but NOT for the comma reason the failure message above has: `notFound` can + // never hold a comma. A comma-bearing value only reaches `remove` through the + // whole-value branch, which requires `stored.has(whole)`, so it is found by definition + // and filtered out of `notFound`; every other entry is a comma-split part. So + // `(not found: p, q)` always means two names, and ONE tag literally named `p, q` is + // unreachable here. The reachable ambiguity is WHITESPACE: `-t "p q"` gives + // `notFound: ["p q"]`, and unquoted that prints `(not found: p q)` with nothing to mark + // where the single name ends. Quoting fixes that and keeps the two messages symmetric, + // so no reader has to work out which line happens to be ambiguity-proof. `not_found` in + // the JSON was already unambiguous; this is the human line catching up. const missed = notFound.length > 0 ? ` (not found: ${notFound.map((tag) => JSON.stringify(tag)).join(', ')})` : ''; const result: Record<string, unknown> = { ok: true, item, removed, message: `Removed ${removed} tag${removed === 1 ? '' : 's'} from ${item?.id ?? current.id}${missed}` }; if (notFound.length > 0) result.not_found = notFound; From 05ee01cdfbd3fe33ac7c3342232a4f75e5edd2d9 Mon Sep 17 00:00:00 2001 From: Andrei Hasna <andrei@hasna.com> Date: Mon, 27 Jul 2026 23:56:35 +0300 Subject: [PATCH 2/3] docs(cli): correct three residual claims from the adversarial review of 59d1d65 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciles an independent adversarial review of the previous commit. Kept as a second commit rather than an amend: the previous commit's message contains a wrong sentence (see 2 below), and rewriting history to hide it would be the same mistake as editing a merged PR body instead of commenting on it. 1. the corrected claim survived in a third file (review F1, also self-found) tests/cli.test.ts:722-725 still carried the unreachable comma justification that this PR corrected in README.md and src/cli.ts. Repo-wide, the claim had four sites, not three. Comment-only; the assertions were already correct and are unchanged. 2. "verify:generated still exits 0" was wrong (review F2) The previous commit message and the PR body both said it. Measured: bun scripts/verify-generated-artifacts.mjs -> exit 0 bun run verify:generated -> exit 1 package.json:58 makes verify:generated rebuild first, and the rebuild does not reproduce the committed bin/knowledge-mcp.js or dist/index.js. Two causes, both pre-existing on main and neither introduced here: package.json is inlined into the bundles and gained a `files` entry in #33 that the committed mcp bundle predates, and zod's codegen has drifted. bin/knowledge.js is unaffected because it is minified. Confirmed not a regression of this branch: git diff --exit-code 64d05ae 59d1d65 -- bin/knowledge-mcp.js dist/index.js exits 0, and this PR does not touch package.json or src/mcp.js. Tracked separately; main ships a stale mcp bundle. 3. the replacement for the old overclaim was itself an overclaim (review F3) "one tag named `p q` reads exactly like a pair" is false: a pair renders `p, q`. Since no not_found entry can hold a comma, joining on ', ' is injective, so a plain space is a legibility problem, not a collision — and calling it an "ambiguity" contradicted the injectivity the same comment had just established. The review found the case that IS load-bearing, and it is stronger: trim() strips only the ends, so whitespace inside a name survives. `-t $'p\nq'` yields not_found ["p\nq"], and joined raw that breaks the single-line message in two: unquoted: 'Removed 1 tag from k_w (not found: p\nq)' -> 2 lines quoted: 'Removed 1 tag from k_w (not found: "p\\nq")' -> 1 line Tab behaves the same. Now cited instead, and pinned by a new assertion rather than left as prose — the one added test assertion in this PR. 4. dependency-state caveat on the rebuild-and-compare check (review F4) @hasna/events is imported by src/cli.ts and is NOT in the build's --external list, so it is bundled; it is declared ^0.1.3 and this repo commits no lockfile. 0.1.14 reproduces the committed bundle, 0.1.13 does not (1040485 vs 1045325 stripped). The CHANGELOG now carries that caveat, since it recommends the check. Tracked separately. 5. smaller review items The `list` block's comment said "repeated -t still accumulates there" while sitting inside `list`, where repeated -t NARROWS (measured: 3 -> 2 -> 1 matches as values are added). Names the command explicitly now, in both README and source. The untag example block now says its three lines are each run against the same starting state rather than as a sequence. The 9-line bullet added to the unrelated "inventory paths block fix" section is back to a 4-line pointer. bin/knowledge.js is deliberately NOT rebuilt: every source edit here is a comment, and rebuilding to a temp outfile reproduces the committed bundle byte-for-byte after stripping (sha256 0630038c6ab4c200), which is the evidence that no shipped behaviour moved. --- CHANGELOG.md | 74 +++++++++++++++++++++++++++++++---------------- README.md | 34 +++++++++++++++------- src/cli.ts | 23 ++++++++++----- tests/cli.test.ts | 30 ++++++++++++++++--- 4 files changed, 114 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2801bd3..f56c269 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,29 +3,58 @@ ## Unreleased — documentation corrections to the multi-tag work Wording-only corrections to claims landed by #34/#35. No behaviour change: the -only non-comment source edit is one added `list --help` line, and the removal -semantics, exit codes and messages are untouched. +only non-comment source edit is one added `list --help` line, plus one added test +assertion; the removal semantics, exit codes and messages are untouched. - The `untag` whole-value-versus-split exclusivity is **per raw `-t` value**, not "one shape per run". Four sites said per run (`README.md` twice, `src/cli.ts` - twice); repeated `-t` still accumulates, so on an item carrying both shapes - `untag -t "a,b,c" -t a` removes 2 and `-t "a,b,c" -t a -t b -t c` removes 4 in a - single run. The re-run contract itself is true and unchanged (`removed: 1`, then - `removed: 3`, then exit 1) — only its stated reason was wrong, and the reason is - what a reader would reimplement from. + twice); repeated `-t` on `untag` still accumulates, so on an item carrying both + shapes `untag -t "a,b,c" -t a` removes 2 and `-t "a,b,c" -t a -t b -t c` removes + 4 in a single run. The re-run contract itself is true and unchanged + (`removed: 1`, then `removed: 3`, then exit 1) — only its stated reason was + wrong, and the reason is what a reader would reimplement from. (Repeated `-t` on + `list` narrows instead; that has not changed.) - Corrected the justification for quoting names in the partial-miss `untag` - message. The cited case — `(not found: p, q)` colliding with one tag literally - named `p, q` — is **unreachable**: a comma-bearing value only enters the removal - set via the whole-value branch, which requires the tag to be stored, so it is - found by definition and never appears in `not_found`. The reachable - justification is whitespace-bearing names (`-t "p q"` → `(not found: p q)`), plus - symmetry with the failure message, where commas in *stored* tags make the - quoting load-bearing for real. + message, at three sites (`README.md`, `src/cli.ts`, `tests/cli.test.ts`). The + cited case — `(not found: p, q)` colliding with one tag literally named `p, q` — + is **unreachable**: a comma-bearing value only enters the removal set via the + whole-value branch, which requires the tag to be stored, so it is found by + definition and never appears in `not_found`. Because every entry is comma-free + the `", "` join is injective, so a plain space is a legibility problem, not a + collision. The load-bearing reachable case is whitespace `trim()` does not + strip: `-t $'p\nq'` yields `not_found: ["p\nq"]`, which joined raw would break + the single-line message in two. Now covered by a test assertion. - Documented an undocumented flag precedence on `list`: when both `--archived` and `--include-archived` are passed, `--archived` wins (archived items only) in either order. Documented, not changed. -- Corrected the `verify:generated` citation under "inventory paths block fix" — - see that entry for what the verifier actually checks. +- Corrected the `verify:generated` citation under "inventory paths block fix". + `scripts/verify-generated-artifacts.mjs` only (a) `git diff --exit-code`s + `bin/knowledge-mcp.js` and `dist`, and (b) greps four generated files for two + stale Windows-path patterns. It never compares a bundle against its source, so + its exit 0 says the *untouched* artifacts are untouched — measured: it exits 0 + with `src/cli.ts` diverged from `bin/knowledge.js`, and exits 0 even with + `bin/knowledge.js` corrupted outright. To check bundle/source sync, rebuild the + bundle to a temp outfile with the `build` script's command and compare against + the committed file after per-line trailing-whitespace stripping (see + `scripts/strip-generated-trailing-whitespace.mjs`), and read the lockfile caveat + below first. + +Two pre-existing problems were found while verifying this and are **not** fixed +here, because both need their own change and review: + +- **`bun run verify:generated` is red on `main`**, independently of this entry. + The script rebuilds first, and the rebuild does not reproduce the committed + `bin/knowledge-mcp.js` or `dist/index.js`: `package.json` is inlined into the + bundles and gained a `files` entry in #33 that the committed mcp bundle predates, + and the zod codegen in `dist/index.js` has drifted. `bin/knowledge.js` is not + affected (it is `--minify`ed). Only `bun scripts/verify-generated-artifacts.mjs` + on its own exits 0. +- **There is no committed lockfile**, so `bun install --frozen-lockfile` exits 0 + while pinning nothing. `@hasna/events` is bundled into `bin/knowledge.js` (it is + not in the `--external` list) and is declared `^0.1.3`, so the rebuild-and-compare + check below is dependency-state-dependent: 0.1.14 reproduces the committed bundle, + 0.1.13 does not. Check `node_modules/@hasna/events/package.json` before concluding + a bundle is out of sync. ## 0.2.91 @@ -86,15 +115,10 @@ self_hosted/cloud (api) mode, where it disagreed with `knowledge paths`. - test(knowledge): `tests/cloud-inventory.test.ts` now asserts `inventory.paths` equals `service.paths()` for all four path fields, that the `/v1` URL never appears in the paths block, and that it is reported on `legacy_store.path`. -- Rebuilt generated bundles so shipped artifacts carry the fix. Note what - `verify:generated` does and does not prove: `scripts/verify-generated-artifacts.mjs` - only (a) `git diff --exit-code`s `bin/knowledge-mcp.js` and `dist`, and (b) greps - four generated files for two stale Windows-path patterns. It never compares a - bundle against its source, so its exit 0 is evidence that the *untouched* - artifacts are untouched — not that a rebuilt bundle matches the source it was - built from. For that, rebuild the bundle to a temp outfile with the `build` - script's command and diff it against the committed file after per-line - trailing-whitespace stripping (see `scripts/strip-generated-trailing-whitespace.mjs`). +- Rebuilt generated bundles so shipped artifacts carry the fix. `verify:generated` + passing is **not** evidence that a rebuilt bundle matches its source — see + "documentation corrections to the multi-tag work" for what that check does and + does not prove, and for the check that establishes bundle/source sync. ## Unreleased — Search overhaul, Stage 2 (Postgres full-text parity) diff --git a/README.md b/README.md index aa9577b..791df4a 100644 --- a/README.md +++ b/README.md @@ -322,9 +322,10 @@ falling back to the split names *only* when no stored tag equals the whole value Both rules are right for their own command. `untag` writes, so it must take one shape per `-t` **value** — that is exactly what makes its documented "re-run to clear the split -names" behaviour true. The exclusivity is per raw `-t` value, **not** per run: repeating -`-t` still accumulates, so `untag -t "a,b,c" -t a -t b -t c` removes four tags from an item -carrying both shapes in a single run. `list` only reads, so matching both shapes destroys +names" behaviour true. The exclusivity is per raw `-t` value, **not** per run: repeating `-t` +on `untag` still accumulates, so `untag -t "a,b,c" -t a -t b -t c` removes four tags from an +item carrying both shapes in a single run. (Repeating `-t` on `list` narrows instead — see +the `-t` row above.) `list` only reads, so matching both shapes destroys nothing, while matching only one would hide items from the very query used to find them. Items written before the multi-tag parse fix can carry a single literal `"a,b,c"` tag, which none of the split names equals, so a split-only filter does not merely miss the damaged @@ -429,7 +430,8 @@ The exclusivity is scoped to one `-t` value, **not** to the whole run. Repeated accumulates, so on an item carrying both shapes: ```bash -# stored tags ["a,b,c","a","b","c"] +# each line run against this same starting state, not as a sequence: +# stored tags ["a,b,c","a","b","c"] knowledge untag --id <id> -t "a,b,c" # removed: 1 (whole value only) knowledge untag --id <id> -t "a,b,c" -t a # removed: 2 knowledge untag --id <id> -t "a,b,c" -t a -t b -t c # removed: 4 (both shapes, one run) @@ -465,12 +467,24 @@ On the **partial-miss** path the justification is different, and it is *not* the case. A `not_found` entry can never contain a comma: a comma-bearing value only enters the removal set through the whole-value branch, which requires the stored tags to already contain it — so it is found by definition — and every other entry comes from splitting on -commas. `(not found: p, q)` therefore always means two names, and one missing tag literally -named `p, q` is **unreachable**. What is reachable is **whitespace-bearing** names: `-t "p q"` -yields `not_found: ["p q"]`, and unquoted that prints `(not found: p q)`, which gives the -reader no marker of where one name ends — one tag named `p q` reads exactly like a pair. -Quoting makes each name's extent explicit and keeps the two messages symmetric, so a reader -does not have to know which of the two lines happens to be ambiguity-proof. +commas. One missing tag literally named `p, q` is therefore **unreachable**, and because +every entry is comma-free the raw `", "` join is *injective*: `["p","q"]` prints `p, q` and +`["p q"]` prints `p q`, which are different strings. A plain space is a **legibility** +problem, not an ambiguity. + +The reachable case that does make quoting load-bearing is **whitespace the parser does not +strip**. `trim()` only removes the ends, so a tab or newline inside a name survives: + +``` +-t $'p\nq' -> not_found: ["p\nq"] + quoted: Removed 1 tag from k_x (not found: "p\nq") <- one line + unquoted: Removed 1 tag from k_x (not found: p + q) <- message split in two +``` + +Joined raw, that name breaks the single-line message and the tail reads as separate output. +`JSON.stringify` escapes it. Quoting also keeps the two messages symmetric, so a reader does +not have to work out which of the two lines happens to be collision-proof. ### delete ```bash diff --git a/src/cli.ts b/src/cli.ts index dac1e29..3cdbc7b 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -1802,7 +1802,9 @@ async function run(argv: string[]): Promise<void> { // A raw value matches an item when the stored tags contain the WHOLE value OR all of // its comma-split names — a UNION. This is deliberately NOT the rule `untag` uses: // there a whole-value hit short-circuits (`continue`), so the two shapes are mutually - // exclusive per RAW -t VALUE — not per run, since repeated -t still accumulates there. + // exclusive per RAW -t VALUE — not per run, since `untag` still accumulates across + // repeated -t. (Repeated -t here in `list` NARROWS instead: 3 -> 2 -> 1 matches as + // values are added. Do not read "accumulates" as describing this command.) // // Each rule is right for its own command. `untag` mutates, so it must take one shape // per -t value — that exclusivity is exactly what makes its documented "re-run to clear @@ -1965,13 +1967,18 @@ async function run(argv: string[]): Promise<void> { // Quoted, but NOT for the comma reason the failure message above has: `notFound` can // never hold a comma. A comma-bearing value only reaches `remove` through the // whole-value branch, which requires `stored.has(whole)`, so it is found by definition - // and filtered out of `notFound`; every other entry is a comma-split part. So - // `(not found: p, q)` always means two names, and ONE tag literally named `p, q` is - // unreachable here. The reachable ambiguity is WHITESPACE: `-t "p q"` gives - // `notFound: ["p q"]`, and unquoted that prints `(not found: p q)` with nothing to mark - // where the single name ends. Quoting fixes that and keeps the two messages symmetric, - // so no reader has to work out which line happens to be ambiguity-proof. `not_found` in - // the JSON was already unambiguous; this is the human line catching up. + // and filtered out of `notFound`; every other entry is a comma-split part. ONE tag + // literally named `p, q` is therefore unreachable here — and since every entry is + // comma-free, joining on ', ' is injective, so a plain space is a legibility problem + // rather than a collision (`["p","q"]` -> `p, q`, `["p q"]` -> `p q`, distinct strings). + // + // What makes the quoting load-bearing is whitespace the parser does NOT strip: `trim()` + // only touches the ends, so `-t $'p\nq'` yields `notFound: ["p\nq"]` and joining raw + // would break this single-line message in two, with the tail reading as separate output. + // Tab is the same. JSON.stringify escapes both, and a test pins it. Quoting also keeps + // the two messages symmetric, so no reader has to work out which line happens to be + // collision-proof. `not_found` in the JSON was already unambiguous; this is the human + // line catching up. const missed = notFound.length > 0 ? ` (not found: ${notFound.map((tag) => JSON.stringify(tag)).join(', ')})` : ''; const result: Record<string, unknown> = { ok: true, item, removed, message: `Removed ${removed} tag${removed === 1 ? '' : 's'} from ${item?.id ?? current.id}${missed}` }; if (notFound.length > 0) result.not_found = notFound; diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 578585f..0f44180 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -719,10 +719,19 @@ describe('knowledge cli', () => { expect(partialHuman.exitCode).toBe(0); expect(decode(partialHuman.stdout)).toContain('not found: "nope"'); - // Two missing names must not render identically to ONE missing tag literally named - // `p, q`. Joined raw, both print `(not found: p, q)` — the same ambiguity the all-miss - // failure message above already fixed, which the success line must not reintroduce. - // The JSON `not_found` array was never ambiguous; this pins the human line. + // This pins the QUOTING of the unmatched names, not a comma collision. There is no + // comma collision to fix: a comma-bearing value only enters the removal set via the + // whole-value branch, which requires the tag to be stored, so it is found by definition + // and never reaches `not_found`. One missing tag literally named `p, q` is unreachable, + // and since every entry is comma-free the raw `', '` join is injective — `["p","q"]` + // prints `p, q` and `["p q"]` prints `p q`, which are different strings. + // + // The quoting is still load-bearing, for names carrying whitespace the parser does not + // strip: `trim()` only removes the ends, so `-t $'p\nq'` stores nothing but yields + // `not_found: ["p\nq"]`, and joined raw that would split the single-line message across + // two lines. Tab behaves the same. `JSON.stringify` escapes both, which is why the + // assertion below is on the quoted form. See the separate whitespace case at the end of + // this test; here the pair is what pins the delimiter itself. seed('k_partial_two', ['alpha']); const partialTwo = runCli(['untag', '--id', 'k_partial_two', '--store', store, '-t', 'alpha', '-t', 'p,q']); expect(partialTwo.exitCode).toBe(0); @@ -730,6 +739,19 @@ describe('knowledge cli', () => { expect(twoOut).toContain('(not found: "p", "q")'); expect(twoOut).not.toContain('(not found: p, q)'); + // The reachable reason the quoting exists: `trim()` strips only the ends, so a control + // character inside a name survives into `not_found`. Joined raw, that newline would + // break this single-line message in two and the tail would read as separate output. + // Quoted, the whole name stays on one line as `"p\nq"`. + seed('k_partial_ws', ['alpha']); + const partialWs = runCli(['untag', '--id', 'k_partial_ws', '--store', store, '-t', 'alpha', '-t', 'p\nq', '--json']); + expect(partialWs.exitCode).toBe(0); + const wsOut = JSON.parse(decode(partialWs.stdout)); + expect(wsOut.not_found).toEqual(['p\nq']); + expect(wsOut.message).toBe('Removed 1 tag from k_partial_ws (not found: "p\\nq")'); + expect(wsOut.message.split('\n')).toHaveLength(1); + expect(storedTags('k_partial_ws')).toEqual([]); + // The human success line must carry the count, so it cannot read the same for 1 and 0. seed('k_human', ['alpha', 'beta', 'gamma']); const human = runCli(['untag', '--id', 'k_human', '--store', store, '-t', 'alpha', '-t', 'beta']); From 3f8fb58fd2774bf7e3d96dd4f52225807975e5ed Mon Sep 17 00:00:00 2001 From: Andrei Hasna <andrei@hasna.com> Date: Tue, 28 Jul 2026 00:52:56 +0300 Subject: [PATCH 3/3] docs(cli): the split-only list -t failure is corpus-dependent, not always a swap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 13 ++++++++++++- README.md | 12 ++++++++---- src/cli.ts | 11 +++++++---- tests/cli.test.ts | 11 +++++++---- 4 files changed, 34 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f56c269..57571a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ Wording-only corrections to claims landed by #34/#35. No behaviour change: the only non-comment source edit is one added `list --help` line, plus one added test -assertion; the removal semantics, exit codes and messages are untouched. +case (six `expect()` calls, 860 -> 866 in `tests/cli.test.ts`); the removal +semantics, exit codes and messages are untouched. - The `untag` whole-value-versus-split exclusivity is **per raw `-t` value**, not "one shape per run". Four sites said per run (`README.md` twice, `src/cli.ts` @@ -27,6 +28,16 @@ assertion; the removal semantics, exit codes and messages are untouched. - Documented an undocumented flag precedence on `list`: when both `--archived` and `--include-archived` are passed, `--archived` wins (archived items only) in either order. Documented, not changed. +- Made the consequence of a split-only `list -t` conditional instead of universal, + at three sites (`README.md`, `src/cli.ts`, `tests/cli.test.ts`). All three said + the defect "returns a DIFFERENT item ... at `total: 1` and exit 0" outright. It + only does that when the corpus *also* holds an item carrying the three names + separately; with the glued item alone it returns `total: 0` at exit 0. Measured + by removing the whole-value branch from the `list` predicate and running both + corpora: `total: 0` / no ids with the glued item alone, `total: 1` / the other + item's id once a split-shape item exists. Both outcomes are silent, so the point + stands — but which one occurs is a property of the corpus, not of the query, and + a swap has to be constructed rather than assumed. - Corrected the `verify:generated` citation under "inventory paths block fix". `scripts/verify-generated-artifacts.mjs` only (a) `git diff --exit-code`s `bin/knowledge-mcp.js` and `dist`, and (b) greps four generated files for two diff --git a/README.md b/README.md index 791df4a..63e3ec6 100644 --- a/README.md +++ b/README.md @@ -328,10 +328,14 @@ item carrying both shapes in a single run. (Repeating `-t` on `list` narrows ins the `-t` row above.) `list` only reads, so matching both shapes destroys nothing, while matching only one would hide items from the very query used to find them. Items written before the multi-tag parse fix can carry a single literal `"a,b,c"` tag, which -none of the split names equals, so a split-only filter does not merely miss the damaged -item: it answers with a *different* item that carries the three names separately, at -`total: 1` and exit 0, giving the operator no signal that the result changed. The union -therefore finds both shapes in one query: +none of the split names equals, so a split-only filter never matches the damaged item — and +what it returns *instead* depends on the rest of the corpus, silently either way. With +nothing else matching it answers `total: 0` at exit 0, an empty result for an item that is +demonstrably there. If some **other** item carries the three names separately, it answers +with *that* item at `total: 1` and exit 0 — a different item from the one asked for, and +nothing in the output says the answer changed. Which of the two you get is a property of the +corpus, not of the query, so neither can be relied on as a signal. The union therefore finds +both shapes in one query: ```bash # returns items tagged with the literal "a,b,c" AND items tagged a + b + c separately diff --git a/src/cli.ts b/src/cli.ts index 3cdbc7b..9750339 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -1811,10 +1811,13 @@ async function run(argv: string[]): Promise<void> { // the split names" behaviour true, and a test pins it. `list` only reads, so matching both // shapes destroys nothing, while narrowing to one would hide items. An item damaged by // the multi-tag defect carries one literal `"a,b,c"` tag that none of the split names - // equals, so a split-only filter does not merely fail to find it — it returns a - // DIFFERENT item that carries the three names separately, at total: 1 and exit 0, with - // nothing to tell the operator the answer changed. The union lets one query find both - // shapes, which is the one job `list -t` is needed for here. + // equals, so a split-only filter never matches it — and what it returns INSTEAD depends + // on the rest of the corpus, silently either way. Measured with the whole-value branch + // removed from the predicate below: total: 0 at exit 0 when the glued item is the only + // candidate, and total: 1 at exit 0 naming a DIFFERENT item once some other item carries + // the three names separately. Which one you get is a property of the corpus, not of the + // query, so neither is usable as a signal. The union lets one query find both shapes, + // which is the one job `list -t` is needed for here. // // So do not "align" the two: making this an exclusive match would break `list`, and // dropping `untag`'s `continue` would break its re-run contract. diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 0f44180..ced1d7b 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -822,10 +822,13 @@ describe('knowledge cli', () => { expect(err).not.toContain('not in [iapp,integrations,architecture]'); }); - // `list -t` is the read-side twin of the untag defect above, and it fails worse than an - // empty result: split-only filtering returns a DIFFERENT item that carries the three - // names separately, at total: 1 and exit 0, so the command used to FIND remaining glued - // items silently reports a confident wrong answer instead. + // `list -t` is the read-side twin of the untag defect above. Split-only filtering never + // matches the glued item; what it returns instead depends on the corpus. The fixture below + // deliberately supplies the worse case — `k_control` carries the three names separately, so + // split-only answers total: 1 with a DIFFERENT item at exit 0, and the command used to FIND + // remaining glued items reports a confident wrong answer rather than an empty one. Without + // such a control item the same defect merely returns total: 0 at exit 0. Both are silent; + // the swap is the one a fixture has to construct, so this test constructs it. test('list -t matches a tag value whole before splitting, so glued items stay discoverable', () => { const dir = mkdtempSync(join(tmpdir(), 'ok-list-glued-')); const store = join(dir, 'db.json');