feat(knowledge): required description field, update detection, and governance taxonomy - #70
feat(knowledge): required description field, update detection, and governance taxonomy#70andrei-hasna wants to merge 4 commits into
Conversation
…ance taxonomy Owner directive 2026-08-05: every agent must be notified when knowledge is created or updated, with the TITLE and a BRIEF DESCRIPTION. The store held no description of any kind, so the notification had nothing to carry. Live row keys were exactly: archived, content, created_at, id, metadata, short_id, tags, tenant_id, title, updated_at, url, version. WHY REQUIRED RATHER THAN AVAILABLE. mementos already ships this exact kind of optional human-guidance field (when_to_use) and it is populated on ZERO rows; its optional summary sits at 6.4% of 6529 and is falling (July 70.0%, August 3.3%). An optional guidance field lands at zero — measured, not predicted. WHAT ENFORCES IT. ItemCreateInput and NoteInput are PLAIN TYPESCRIPT INTERFACES, erased at build time and enforcing nothing at runtime (zod is imported only by schema.js and mcp.js), so a type annotation would have been a comment. The runtime floors are an explicit guard in the Store create/update path — below the CLI, so SDK and MCP callers cannot route around it — and, for cloud, a Postgres CHECK constraint below the serve handler, the upsert branch, sync/outbox replay, backfill and psql. That is the same argument this repo already made for putting the version bump in a trigger. EXISTING ROWS ARE NOT REWRITTEN. The CHECK is added NOT VALID, so it binds every new INSERT and UPDATE while never being checked against the rows already stored; `description IS NULL` is the queryable mark of a legacy row. Exercised on a real Postgres (pglite): re-adding the constraint NOT VALID succeeds despite a violating row present, that row stays readable, and a new violating INSERT is still refused with SQLSTATE 23514. Update detection: `--sort updated`. updated_at was already on every row and transport; only the validator refused the key, which left "what changed?" unanswerable and blocked the change monitor. Taxonomy: two optional axes, reach (fleet|project|seat|self) and consequence (blocking|standing|reference), with closed vocabularies pinned in SQL and in one shared TypeScript module. They are deliberately NOT required and NOT defaulted onto the row: mementos' importance/category/scope are 400/400 populated and carry almost no information because a default did the work, so absence is preserved as signal and the defaults are a read-side notion. The version trigger's no-op guard learns the three new columns, or a description-only edit would take no version and no counter bump — invisible to the very update-detection surface this enables. Tests: 29 new, in-process plus five against a real Postgres, each with the negative and positive side exercised. Existing fixtures updated where a create now needs a description; the `add --help`, `add -h` and missing-args cases and the serve 403 authz probe are deliberately left without one, because each asserts behaviour that must not change. Task: b9f6c835 Agent: Augustus
The verify:generated gate (ci.yml:48) compares the committed bin/ and dist/ artifacts against what the current source builds. The description field, governance taxonomy and update-detection changes landed in src/ without a rebuild, so the gate failed: bin/knowledge.js carried 0 occurrences of description_required while src/knowledge-taxonomy.ts defines it. An agent running the shipped binary would not have had the feature. Rebuilt with the pinned toolchain. The gate warns that its byte comparison is bun-version-sensitive and that a rebuild must not be committed unless the local bun matches the pin; local bun is 1.3.14 and ci.yml pins 1.3.14 at lines 33 and 62, so that precondition is met. Agent: Augustus
`knowledge list --help` still advertised `--sort created|title` after this
branch added the `updated` key, so the per-command help contradicted the
general help. That is a discoverability failure of the exact capability the
branch exists to provide: an agent building the fleet change monitor reads
`list --help`, sees no `updated`, and concludes the CLI cannot answer "what
changed?" — which is the stale-capability shape where a doc tells an agent to
decline something that now exists.
Measured on the rebuilt binary: occurrences of "updated" in `list --help` went
from 0 to 2, and the usage line now reads `--sort created|title|updated`.
The two existing assertions (tests/cli.test.ts:370 and :405) use
toContain('--sort created|title'), which the widened string still satisfies —
verified by running them rather than by reading them: 3 pass, 0 fail.
Also names the --desc / --description distinction and states that the
description is returned by --json but is not a column in the table render.
Agent: Augustus
…ription on upsert Windows CI failed one test on this branch — "a stored item with no description is still listed and fetched" — with Expected length: 1, Received length: 0, in 1.75ms. Not a timeout: a real assertion failure, and one this branch introduced. Root cause: the fixture called Bun.write without awaiting it, so the store read raced the write. Measured on Linux, the race cannot manifest: the file is fully present immediately after the unawaited call (39 of 39 bytes), which is why the test passed on every POSIX runner and on every local run. Windows lost the race and read the store as empty, so the failure presented as "legacy rows are not readable" — a defect in the feature under test — when the fixture simply had not landed. It is the only unawaited Bun.write in tests/; the other 5 all await. Verification honesty: awaiting removes the race by construction, but the fix can only be confirmed by Windows CI. It is not verifiable on this machine, where the failure does not reproduce. Second, unrelated to the failure: `knowledge upsert --help` mentioned --description 0 times while its CREATE path requires it (measured: rc=1 for a new id, rc=0 for an existing one, which leaves the stored description alone). The help now states that asymmetry. `add --help` already mentioned it 3 times, so upsert was the odd one out. Agent: Augustus
|
[REVIEW-RELAY] NO_GO — #70 @ f30f3b1 — posted by the workflow HARVEST agent (wf_28d3406e-a9d, todos b9f6c835). The verdict is the workflow's adversarial reviewer's, relayed here because it was recorded on the task and in the workflow record but never anchored on this PR — which currently reads OPEN / MERGEABLE with 7/7 green CI, exactly the state that gets merged past. Do not merge until the P1 is fixed and the re-review passes (remediation cycle 1 of max 2). P1 (measured on both transports): local and Postgres disagree about editing a legacy (description-less) row.
Named remedy from the review (small, one cycle): when the stored row's description is null and the patch supplies none, throw Non-blocking follow-ups (P2/P3 — do not gate the re-review): version-snapshot INSERT does not retain Full evidence: todos b9f6c835 comments (transport probe outputs with controls) and knowledge items k_msg7ab4t_7vx6bs, k_msg7ak3l_sai2yz. |
|
[REVIEW] NO_GO — #70 @ f30f3b1 — lens: correctness+isolation+wiring, reviewer codewith-sol-reviewer (1 of 1) P0 — None found. P1 — P2 — P3 — None found. Could not verify the live production row count, a deployed hosted API, or external CI logs. I verified the exact head in the supplied worktree with isolated local JSON and migrated in-process Postgres (PGlite); the worktree remained clean. |
1 participant
Owner directive 2026-08-05: every agent must be notified when knowledge is created or updated, receiving the title, a brief description, and a prompt to read it if it matters.
The store held no description of any kind, so the notification had nothing to carry. Live row keys, measured:
archived, content, created_at, id, metadata, short_id, tags, tenant_id, title, updated_at, url, version. And--sortacceptedcreated|titleonly, so an edit could not be detected at all.This PR builds the three things that gap requires. It does not wire the notification itself — see "Not in this PR".
Why the description is REQUIRED, not merely available
This is the load-bearing decision and it is settled by measurement rather than taste.
hasna/mementosalready ships exactly this kind of optional human-guidance field.when_to_usehas existed for a long time and is populated on zero rows. Its optionalsummarysits at 6.4% of 6529 rows and is falling — July 70.0%, August 3.3%. An optional guidance field lands at zero. So this one is refused at the write path.The same data argues the opposite way for the taxonomy axes, which is why they are optional — see below.
What actually enforces it
The obvious enforcement point does not exist.
ItemCreateInputandNoteInputare plain TypeScript interfaces, erased at build time and enforcing nothing at runtime;zodis a dependency but is imported only byschema.jsandmcp.js. A type annotation alone would have been a comment — andmcp.js, a real writer with four create sites, is plain JavaScript that never sees the types at all.So there are two runtime floors:
ItemStorecreate/updateCHECKThis is the argument
db/pg-migrations.tsalready makes for putting the version bump in a trigger: "the writers of knowledge_items are already plural… and next month there will be another."Existing rows are not rewritten
The constraint is added
NOT VALID: it binds every new INSERT and UPDATE while never being checked against the rows already stored.description IS NULLis the queryable mark of a legacy row — no second column, no status enum, no backfill.Exercised against a real Postgres (pglite), not asserted from documentation:
NOT VALIDsucceeds with a violating row already present;description IS NULLfinds it;23514.One consequence, stated rather than left to be discovered: a
NOT VALIDcheck still fires when an existing violating row is updated. Editing a legacy item therefore forces a description at that moment — deliberate ("touch it, describe it"), and why the CLI carries its own guard whose message names the flag, so this surfaces as an instruction rather than a raw constraint violation.The taxonomy, and why these axes are optional
Two axes with closed vocabularies:
reach(fleet|project|seat|self) andconsequence(blocking|standing|reference).They are not required and not defaulted onto the row. mementos'
importance,categoryandscopeare 400/400 populated and carry almost no information, because a default did the work. On that same data the discriminator is deliberateness: rows at default importance carry a summary 0.1% of the time; rows at importance 10 carry one 83.6% of the time. Coverage is not the measurement; distribution is.So the defaults are a read-side notion: a lazy write lands quiet and searchable, only a deliberate write escalates, and "the author did not choose" stays countable. The escalation rate is then a health metric — 0% means the filter is inert, and very high means it is inert the other way.
Vocabularies live in one shared module and are restated in SQL only because a constraint cannot import TypeScript; a test asserts the two stay in step.
The trigger had to learn the new columns
The version trigger's no-op guard compared exactly
(title, content, url, tags, metadata, archived). Left alone, a description-only edit would produce no version row and no counter bump — invisible to the very update-detection surface this work exists to enable. The guard now includes all three new columns, with a test that a description-only edit bumps to version 2 and a negative control that an identical rewrite still does not.Tests
29 new, each with both sides exercised — 24 in-process, 5 against a real Postgres.
In-process tests were a deliberate choice: on this station the repo's subprocess-spawning CLI tests time out at their 5000ms budget regardless of correctness. Measured on unmodified
d689e45e: 15 failures in one run and 26 in the next, same code, load 31 on 20 cores. A test whose result tracks the box is not a regression test.Suite, like-for-like, knowledge env stripped:
The 16 remaining are all in the CLI-subprocess timeout population present at baseline. Each one named in a run but absent from the baseline list was re-run in isolation and passed — e.g.
storage repair-artifact-keysfailed at5070.27ms("timed out after 5000ms") and passes alone in 4.79s.tsc --noEmit: 0 errors. Staged secrets scan: 0 findings, with a positive control confirming the pattern fires on synthetic sentinels.Fixtures deliberately left without a description, because each asserts behaviour that must not change:
add --help,add -h, the missing-args['add','--json']case, and the serve 403 authz probe — that last one proves scope is refused before the body is validated, which is only meaningful while the body stays invalid.Behaviour changes reviewers should weigh
knowledge addnow fails without--description. Intended, and the point of the PR.NOT VALID-on-UPDATE consequence above).ok_importrefuses an export whose items predate the field, naming the item and how many imported before it. Import creates new rows, and the CHECK rejects a null description on INSERT regardless of application code — so permitting it here would only relocate the failure to a raw constraint violation with no item id in it. A real cost, recorded rather than hidden.--descriptionis not--desc.--descremains the sort-descending boolean. The parser matches tokens exactly and does no prefix matching, so they cannot be confused.Not in this PR
@hasna/eventsis wired (registerEventsCommands(..., { source: 'knowledge' })) and knowledge has emitted exactly one event ever, while todos pushes 29,766 through the same bus — so the path exists. But emitting from the write paths misses the server-side, psql and replay writers, and the durable form is a producer outbox fed by the same trigger that ownsversion. Doing it properly is its own change; doing it partially would let us claim "every write emits" when it does not.VALIDATE CONSTRAINTon the legacy rows, once the null set is small.reachfrom existingdirective-global/fleet*tags.Not checked
descriptionthe CLI did not expose. If it did, the migration story changes.Task: b9f6c835
Agent: Augustus
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.