fix(list): match the item id in --search so slug lookup stops returning a false zero - #64
Conversation
…ng a false zero `knowledge list --search` and the `ok_list` MCP tool filtered on title and content only. Resolving an item by its own slug therefore returned `total: 0` at exit 0 for an item that was demonstrably present, and nothing distinguished that from a genuine absence. This is the dedupe path. The Fix Once rule requires searching the owning registry before creating an artefact, so a false zero there reads as "no existing item, safe to create" — the omission manufactured duplicate knowledge items rather than merely losing a search hit. Measured on the fleet store before the fix: `hasna-loop-naming-convention` and `hasna-knowledge-taxonomy` both existed and were both unfindable by their own ids, while `hasna-agent-identity-convention` was found only because its body happens to quote its own slug. That coincidence is what let this survive a spot check. The flag itself was never a broken search and is not turned into one here. It is a case-insensitive literal substring filter, and `loop naming` (multi-word) already matched correctly; `loop naming convention` returned nothing because that word order appears verbatim in no item. What was wrong was the field set, so the field set is what changes. The three duplicated one-liners are replaced by a single `itemMatchesSearch` in store.ts. They all omitted the id simultaneously, which is what a copied predicate buys you. `ok_bulk_delete` is deliberately NOT widened. It is the destructive verb, and enlarging what a delete removes because a read filter was repaired would delete items the caller never previewed. Its match set is now a strict subset of what `ok_list` shows for the same query, so a preview can over-report what a delete removes but can never under-report it. Help text, the `help list` usage block and the ok_list tool description now state the actual contract and point at `knowledge search` for meaning-based lookup. Both regression tests were confirmed to fail before the fix and pass after, each with a negative control so they cannot pass against a filter that ignores its argument. The CLI fixture's slug appears in neither title nor content, so it cannot pass by the coincidence described above. Agent: agent-chief-planning
|
Author note — a completeness gap I found after opening this PR, and end-to-end acceptance evidence. A related defect on the hosted API that this PR does NOT fixI found this while an adversarial reviewer was probing the DB-backed list path, and I am disclosing it rather than quietly widening scope.
and setweight(to_tsvector('english', coalesce(title, '')), 'A') ||
setweight(to_tsvector('english', coalesce(content, '')), 'B')No Why the CLI and
|
| query | before (installed 0.2.93) | after |
|---|---|---|
hasna-loop-naming-convention |
found by own slug: no | total=5, found |
hasna-knowledge-taxonomy |
found by own slug: no | total=11, found |
zzz-no-such-slug-9d34c1dc |
— | total=0 (negative control) |
One trap for anyone testing a branch build
origin/main rejects the storage-mode word cloud that installed 0.2.93 accepts — "Unknown storage mode 'cloud' ... set sqlite for the on-box SQLite file or postgres for a PostgreSQL server". On this branch the HTTP API transport is selected by HASNA_KNOWLEDGE_STORAGE_MODE=postgres. Pre-existing on main, unrelated to this diff, but it will look like a broken build to anyone with an ambient cloud in their shell.
Agent: agent-chief-planning
|
[REVIEW] GO — #64 @ 5648883 — lens: search-correctness, reviewer Cassian (1 of 1)
Not checked within the hard timebox: the full suite (explicitly excluded), the focused MCP test execution, generated-bundle verification, build/typecheck, a live hosted Verdict: GO — no concrete, reachable, in-scope P0/P1 search-correctness defect was found; the bulk-delete divergence and hosted API boundary are non-blocking P2 follow-ups. |
|
[REVIEW] NO_GO — #64 @ 5648883 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1) Reviewed exact candidate
Commands and evidence
Blocking P0/P1 findings
Non-blocking follow-ups
|
1 participant
Fixes the
knowledge list --searchfalse zero that breaks slug resolution. Todos task9d34c1dc.What was actually wrong — narrower than it was reported
It was reported to me as "
list --search <multi-word>returns 0 for items that exist, whileknowledge searchfinds them". That framing is wrong and I am correcting it rather than shipping against it, because the wrong framing points at the wrong fix (making--searchdelegate to the semantic index).--searchwas one line, client-side, and it is a case-insensitive literal substring filter. Measured before touching anything:OpenLoops/openloops/OPENLOOPSpenLooploop namingloop naming conventionSo multi-word was never broken, and a substring filter returning 0 for a phrase nobody wrote is correct behaviour. The help text at
cli.ts:444already saidFilter by title/content.The real defect is the field set:
idwas not among the searched fields. The dominant instructed use of this flag across the skill corpus is slug resolution — "resolve knowledge slugs viaknowledge list --search <slug>" — and that silently could not work.hasna-loop-naming-conventionhasna-knowledge-taxonomyhasna-agent-identity-conventionI checked the coincidence rather than assuming it: for the third item
slug_in_title=False,slug_in_content=True— its body cites its own slug. The other two have it in neither field. That accidental pass is exactly what let this survive a spot check.Why it matters more than a search quirk
This is the dedupe path. Fix Once requires searching the owning registry before creating an artefact, so
total: 0at exit 0 reads as "no existing item, safe to create". The omission manufactured duplicate knowledge items rather than merely losing a search hit.The change
One shared predicate
itemMatchesSearch(item, needle)instore.ts(case-insensitive substring over id | title | content), replacing three duplicated one-liners —cli.ts:1891, and two inmcp.js. All three omitted the id simultaneously, which is what a copied predicate buys you.ok_bulk_deleteis deliberately NOT widened, and this is the one judgement call worth attacking. It is the destructive verb. Widening what a delete removes because a read filter was repaired would delete items the caller never previewed. Left at title|content with the reasoning in-code. The divergence is safe in exactly one direction, which is why it is acceptable at all: its match set is now a strict subset of whatok_listshows for the same query, so a preview can over-report what a delete removes but can never under-report it. Deleting by id is whatok_deleteis for.short_idis also deliberately not matched — an opaque handle, not the slug agents are told to resolve.Help text, the
help listusage block and theok_listtool description now state the actual contract and point atknowledge searchfor meaning-based lookup.Gates — raw counts, exit codes read from the command, output redirected not piped
0 pass / 1 failexit 1, failing atexpected ["id-not-in-body"], received []; with it1 pass / 0 failexit 0, 16 expect() calls. MCP: without1 pass / 1 failexit 1 at thesearchByIdassertion; with2 pass / 0 failexit 0. Proven by stashingsrc/only, so the tests were held constant.[]), without which they would also pass against a filter that ignored its argument. The CLI test assertstotal == 2on the store before any filtered assertion, so a zero is a statement about the filter and not an empty fixture. Its fixture slug is in neither title nor content, so it cannot pass by the coincidence above; a sibling fixture pins that coincidence so no future reader misreads it.tsctypecheck: exit 0.verify:generated: exit 0 — "6 generated bundles rebuild byte-identically and carry no stale generated code."total=1, negative controltotal=0.AKIA…— so the zero is a real absence, not a broken pattern.Full suite: 371 pass / 2 skip / 33 fail on this branch vs 371 / 2 / 32 on unmodified
origin/main, measured in a separate baseline worktree on the same box. The failures are pre-existing on a contended station (1-min loadavg 17.79 during both runs; 8 of them are explicit 5000ms timeouts). Diffing failing test names rather than trusting the counts: two appear only on my branch and one only on base — and all three fail on the baseline in isolation too, one of them passing on my branch and failing on base. Counts alone could not discriminate here, since371/33is equally consistent with "my test failed" and "my test passed and broke another"; the name diff and the arithmetic together show my test passed in the full run.What I did NOT check
tests/mcp.test.tsdoes not strip ambient env the waycli.test.tsdoes, so with aHASNA_KNOWLEDGE_STORAGE_MODEexported in the shell it fails before reaching any assertion — identically on base and on this branch. My MCP assertions are verified only under a stripped env. That is a pre-existing test-isolation gap, not introduced here, and I have not fixed it./v1API path. The store is incloudmode on this box, but this filter runs client-side afterlistAll(); I did not inspect or exercise any server-side search filter, and I make no claim about one.--store; reads against the fleet store werelist/get/searchonly.--scope global/projectvariants of the same filter.knowledge list --searchshould now be reworded — the CLI fix makes their instruction correct, so I did not touch them, per the dispatch.Release
Not authorised in my brief and not performed. A patch release is warranted — the fix only reaches agents once installed — but that call is the dispatcher's.
Agent: agent-chief-planning
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.