Skip to content

fix(knowledge): honor import frontmatter identity - #2309

Merged
oceanwaves630 merged 3 commits into
mainfrom
codex/knowledge-import-frontmatter
Sep 11, 2026
Merged

fix(knowledge): honor import frontmatter identity#2309
oceanwaves630 merged 3 commits into
mainfrom
codex/knowledge-import-frontmatter

Conversation

@ohdearquant

Copy link
Copy Markdown
Owner

Summary

  • parse optional leading, delimiter-bounded YAML frontmatter and derive canonical atom identity from agreeing id, atlas_id, or atlas-id aliases
  • map name/title, tags, nested properties, and other structured metadata while storing and indexing only the post-frontmatter markdown body
  • preserve root-relative path identity and loose atlas_id: provenance for documents without canonical frontmatter
  • preflight final-slug collisions and refuse canonical identities already claimed by a different live slug; same-slug reimports update the existing UUID
  • amend ADR-048 and the public import documentation with the precedence, mapping, body, collision, and idempotence contract

Validation

  • cargo test -p khive-pack-knowledge --test import_integrity (13 passed)
  • cargo test -p khive-pack-knowledge (405 passed, 1 ignored)
  • cargo check --workspace --all-targets
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • deno fmt --check (206 files)
  • git diff --check

Closes #1984

@ohdearquant

Copy link
Copy Markdown
Owner Author

Deriving a record's identity from the file being imported is a trust boundary, and two things cross
it here.

A same-slug row with no identity claim is overwritten without any ownership check. The collision
preflight only fires in one direction (crates/khive-pack-knowledge/src/knowledge/sections.rs:785-813):
it builds slugs_by_identity from existing rows' recorded identity keys, then rejects the import
when some existing atom claims the same identity under a different slug:

if let Some(existing_slug) = existing_slugs.iter().find(|s| *s != &prepared.slug) { return Err(...) }

An existing row that simply occupies the target slug but has never recorded an identity claim never
enters that map, so the lookup misses and the import proceeds. upsert_atoms then resolves purely by
(namespace, slug) (crud.rs:142-150), finds the row, takes insert = false, and reuses its id. The
UPDATE at crud.rs:243 writes name=?1, content=?2, tags=?3, properties=?4 unconditionally — only
source_uri, source_type and finalized are COALESCE-guarded, and the importer supplies those
anyway.

Concretely: an atom exists at slug canonical-doc-42 with source_uri=file:internal.md and unrelated
content. An imported file declares id: Canonical.Doc-42, which normalizes to the same slug. Nothing
matches in the identity preflight, and the existing record's name, content, tags and properties are
replaced in place while keeping its UUID. The only scope check anywhere on this path is
namespace = token.namespace(). The two guards that are in upsert_atoms — the domain-mirror
collision and the soft-deleted-slug refusal — show the shape the missing check should take; neither
is a provenance check. Requiring an existing same-slug row to prove matching canonical provenance
before it can be updated, and rejecting otherwise, closes it.

A UUID-shaped canonical identity becomes a slug that knowledge.get can never reach. The derived
identity only ever becomes the slug (sections.rs:617-628); new rows still get new_id()
(crud.rs:168-176). But knowledge.get short-circuits on shape:

let resolved_id = if let Ok(uuid) = id.parse::<Uuid>() { Some(uuid.to_string()) } else { /* exact slug lookup first */ }

(crud.rs:504-507), and an id-shaped miss deliberately refuses to fall back
(crud.rs:614-619). So frontmatter declaring id: 123e4567-e89b-12d3-a456-426614174000 imports
successfully, stores under that slug with a different generated UUID, and then knowledge.get with
the declared identity returns NotFound.

What makes this a gap rather than a design choice is the branch immediately below it: the non-UUID
path performs an exact slug lookup before interpreting an all-hex value as a UUID prefix, with a
comment saying a registered all-hex slug must remain addressable. The hazard is already recognised
one branch away; a full canonical UUID just short-circuits past the logic written for it. Either
accept a canonical UUID as the primary key under the normal collision rules, reject UUID-shaped
identities at validation, or let an exact imported slug resolve before UUID interpretation — but the
current combination imports a record under an identity and then denies that identity resolves.

Two smaller things.

The frontmatter is deserialized whole into serde_yaml::Value and converted to owned JSON
(sections.rs:455-462) with no budget on the result. The caps at sections.rs:570-601 bound raw
bytes — 10 MiB per file, 256 MiB per request — not the alias-expanded value, and anchors and aliases
are materialized during deserialization. A file well under the raw cap can define one large anchored
mapping and reference it repeatedly, expanding through the JSON conversion, properties.extend and
the recursive check_json before anything is written. Either reject anchors and aliases for this
metadata format or add explicit node-count, depth and expanded-size budgets.

The identity value itself is validated only as a non-empty trimmed string (sections.rs:363-385),
with no length or character policy before to_slug (sections.rs:618-626). Embedded newlines and
control characters are accepted: they become hyphens in the slug but survive verbatim in the stored
source_uri and properties.atlas_id (sections.rs:658-673). An identity approaching the file cap
is accepted and becomes the (namespace, slug) index key. No identity value reaches SQL as anything
but a bound parameter and none builds a filesystem path, so this is a contract bound rather than an
injection risk — but it should be a stated bound.

Worth keeping as-is: the alias agreement rule is genuinely well specified — id, then atlas_id,
then atlas-id; null treated as absent; non-strings and post-trim empties rejected; every non-null
value compared by exact trimmed equality with any disagreement rejected outright. There is no silent
winner among disagreeing claims, which is the failure this class usually has. One caller-facing trap
deserves documenting though: YAML scalar resolution runs before that rule, so id: 012345 stays a
string and is accepted while id: 12345 resolves to a number and is rejected. Callers need to know
quoting is required.

One backward-compatibility note that should be called out explicitly: a document whose body merely
begins with an exact --- line is now read as opening frontmatter. Without a later ---/... it
errors, where it previously imported as ordinary markdown; with one, those lines are stripped and the
remainder reinterpreted. Files with no frontmatter are unaffected. That class is a real break and is
currently undocumented.

The added tests cover the happy path, the different-slug identity refusal, in-batch collision, and
the unterminated-delimiter error. Nothing covers the same-slug overwrite above, disagreeing aliases,
UUID-shaped or oversized or control-character identities, anchors/aliases, duplicate keys, or a body
starting with ---. Also, malformed_frontmatter_is_rejected_before_any_write tests an unterminated
delimiter rather than malformed YAML syntax, so its name promises coverage the fixture does not
provide.

@ohdearquant

Copy link
Copy Markdown
Owner Author

One real failure; the other red rows are sibling matrix shards cancelled by fail-fast.

CI (macos-latest, shard 1/2) fails on
stores::note::tests::filtered_count_partitions_share_one_snapshot_during_concurrent_update
(-p khive-db --lib). A concurrent-update snapshot assertion is timing-dependent, so the first
question is whether it reproduces rather than what it means.

This branch is 10 commits behind main; merging up before re-running keeps the question anchored
to current code.

@ohdearquant

Copy link
Copy Markdown
Owner Author

The red CI gate looks ambient, not this change

CI (macos-latest, shard 1/2) fails one test:

test stores::note::tests::filtered_count_partitions_share_one_snapshot_during_concurrent_update ... FAILED
panicked at crates/khive-db/src/stores/note_tests.rs:676:10

That is a concurrency test over note-store snapshot partitioning in khive-db. This branch
touches no file under crates/khive-db/ — its changes are in khive-pack-knowledge plus the
manifest, lockfile and docs.

Stating the limit rather than overclaiming: an untouched crate is strong evidence but not
proof, since the branch does edit crates/Cargo.toml and crates/Cargo.lock, and a dependency
change can in principle move timing under a concurrent test. If a fresh trigger passes without
any change to the diff, that settles it as ambient.

@ohdearquant
ohdearquant marked this pull request as ready for review September 1, 2026 16:39

@ohdearquant ohdearquant left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.

Verdict on head 20dbc77: REQUEST-CHANGES, 2 blocking findings. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.

Two conflicts:

- `crates/khive-pack-knowledge/README.md`: main's verb table is taken, since it
  carries the corrected `knowledge.index` row (FTS rebuild is `kkernel reindex`
  only). Only the `knowledge.import` row is updated, to name frontmatter as well
  as path identity, which is what this branch changes.
- `crates/Cargo.lock`: resolved to main's and then regenerated against the merged
  manifests, which adds one line. `cargo metadata --locked` resolves cleanly at
  this tree, so `--locked` builds will not be asked to update it.
@oceanwaves630
oceanwaves630 merged commit 7ace8c6 into main Sep 11, 2026
29 checks passed
@oceanwaves630
oceanwaves630 deleted the codex/knowledge-import-frontmatter branch September 11, 2026 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

knowledge.import: identity from file basename causes collisions and duplicates; content captures frontmatter instead of body

2 participants