Skip to content

feat(cli)!: derive omnigraph embed vectors from the schema's @embed declarations - #952

Draft
ragnorc wants to merge 5 commits into
ModernRelay:mainfrom
ragnorc:claude/embed-schema-authority
Draft

ragnorc wants to merge 5 commits into
ModernRelay:mainfrom
ragnorc:claude/embed-schema-authority

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Stacked on #929. Until #929 merges, this diff also shows its commits. The change here is the last commit, 3f437a66; review that one alone with this compare view.

What & why

omnigraph embed filled vectors from a hand-written spec ({"dimension": N, "types": {"T": {"target": "embedding", "fields": ["title"]}}}, or a seed manifest's embeddings: entry). After #929 that spec restates exactly what the schema already declares: embedding: Vector(N) @embed("title", model="…") gives the target, the source, the dimension and the model. Nothing compared the two, so:

  • a spec naming body for a schema declaring @embed("title"), or a wrong dimension, silently wrote vectors a text nearest() cannot match;
  • unknown spec and manifest keys were ignored (the CLI seed fixture carried an embeddings.model: that did nothing);
  • the recorded @embed(model=…) was never compared with the provider model the pipeline resolves, although the query path refuses that mismatch (resolve_nearest_query_vec).

This PR makes the schema the only statement of what to embed:

  • --schema <schema.pg> replaces --spec. Every @embed declaration in the catalog (NodeType::embed_sources) is a vector the run fills, with that declaration's source property and its Vector(N) dimension. A type can now carry several vectors of different dimensions, which the spec could not express (one global dimension, one target per type).
  • Seed manifests name the schema with schema:. embeddings: is gone, and the manifest now refuses keys it does not read (deny_unknown_fields, nested sections included).
  • One model rule for both sides. EmbeddingConfig::embeds_with_model is the comparison the query path already made; resolve_nearest_query_vec now calls it, and omnigraph embed calls it for every selected vector with a recorded model, before it reads input or creates output.
  • More loud failures: a --type naming a type with no @embed vector; a schema with none; and, by line, a selected record without source text whose vector is a required Vector(N) (a load would refuse that record anyway; --clean is unchanged).
  • JSON output: vectors (each processed vector's type, property, source, dimension) and schema replace dimension, which has no single value once a schema declares several vectors.

Why refuse the old spec instead of validating it

Validating a spec against the schema keeps two sources of truth plus a comparison, and the spec format cannot even describe valid schemas. --spec is removed outright, following the precedent of --allow-data-loss (scripts get clap's unknown-argument error; the breaking note says what to do). #929 already changes the spec format in a breaking way for v0.13.1, so users migrate once if both land in that release; for that reason this PR also drops the "fields must list one property" sentence from #929's breaking note, which would otherwise contradict this one.

Why a schema file, not the graph's accepted schema

embed is a local-plane command (RFC 0010/0011): it reads and writes files and takes no graph address. RFC 0015 plans omnigraph embed --graph <scope> as a reconciler that writes to the graph, so a read-only graph flag now would collide with it. Seed files are often prepared before init, when no graph exists. omnigraph schema show prints a graph's accepted schema for anyone without the file; the docs say so.

Backing issue / RFC

Checklist

  • Change is focused (one logical change: the schema becomes the offline pipeline's authority)
  • Tests added/updated for behavior changes
  • Public docs updated: docs/user/search/embeddings.md (offline pipeline, seed manifests, the model and required-vector refusals), docs/dev/execution.md, skills/omnigraph/ (SKILL.md, references/search.md, references/commands.md); changelog embed-schema-authority.breaking.md and .fixed.md
  • Reviewed against docs/dev/invariants.md: invariant 8 (integrity failures are loud: model mismatch, unknown manifest keys, and a required vector with no source now fail instead of passing silently) and 12 (one source of truth: the spec that restated the schema is removed). No write-path, publication or storage change.

Local verification

  • Baseline before the change: cargo test -p omnigraph-cli --locked --bin omnigraph embed: 9 passed; cargo test -p omnigraph-cli --locked --test integration -- cli_data::embed_ cli_data::optimize_json_succeeds_on_local_graph: 5 passed.
  • cargo test -p omnigraph-cli --locked: unit 147 passed (6 ignored), integration 295 passed (8 ignored). The first run failed 57 server-backed tests because a -p omnigraph-cli run does not build omnigraph-server; after cargo build -p omnigraph-server --locked, those 57 passed with --exact.
  • cargo test -p omnigraph-engine --locked --test search nearest_string_: 4 passed (the query path's model checks through the shared rule).
  • Red checks: with check_model disabled, a_run_embeds_each_declared_source_at_its_declared_dimension fails; with the required-vector refusal disabled, a_required_vector_without_source_text_is_refused fails. Both pass restored.
  • cargo clippy -p omnigraph-cli -p omnigraph-engine --all-targets --locked -- -D warnings -W clippy::dbg_macro: clean. cargo fmt --all --check: clean.
  • python3 scripts/check-docs.py, bash scripts/check-agents-md.sh, typos on the changed files, python3 scripts/release_notes.py preview --working-tree: clean; the notes render.
  • Not run locally: the canonical workspace graph and the GQT corpus. The engine change is the extracted comparison above, and GQT refuses @embed schemas, so no case reaches this code; CI runs both.

Notes for reviewers

  • Test tier: GQT refuses @embed schemas because they resolve a provider from the process environment, so the regression tests are in-source in crates/omnigraph-cli/src/embed.rs. run_embed_job now takes the client as a parameter, so a whole run is tested against temp files with a mock client that resolves an explicit model: each stored vector must equal embed_query_text(source, N) at the declared dimension, and a provider resolving another model must be refused before the output file exists. One CLI test (cli_data::embed_reads_vectors_from_the_schema_and_refuses_another_model) covers the env-to-refusal wiring and that --spec is no longer accepted.
  • Seed fixture: it now writes schema.pg, and its pre-existing vector is 4-dimensional; it was 2-dimensional under a 4-dimension spec, which a load would have refused.
  • --type refusal is a behavior change: before, a --type naming a type the spec did not cover was a silent no-op.
  • --spec error text: clap's "a similar argument exists: '--select'" tip on the removed flag is unhelpful; I left it rather than add a hidden flag only to refuse it, consistent with --allow-data-loss.
  • Parsing: the schema goes through parse_schema + build_catalog (admission mode), the same path as lint --schema.

ragnorc and others added 5 commits October 9, 2026 14:49
The offline pipeline embedded `type: <Type>\n<field>: <value>` while a text
`nearest($v, $q)` embeds `$q` verbatim, so a record whose source equaled the
query text did not rank at distance zero: unrelated vectors under the mock
provider, a systematic skew under a real model.

`@embed("source")` declares `vector = embed(source)` with one String source
(RFC 0015), and a query string carries no type or field to template, so the
pipeline is the side that changes: it now embeds the source value unchanged,
which is what the query path already does. The spec's `fields` must name
exactly that one property; a record with an absent, null or blank source gets
no vector instead of one embedded from its type name, and a re-embed removes
such a record's stale vector; a non-string source is refused.
The no-source path of `omnigraph embed` kept the target whenever
`embedding_missing` held, which is also true for an empty list: a record with
a blank source and `embedding: []` came out unchanged, with
`cleaned_records: 0`, and its load then failed with "expects 4 dimensions,
got 0". Selecting which records need embedding is not the same contract as
deciding what a record without source text may keep.

The no-source path now removes whatever the target holds (a stale vector, a
null, an empty placeholder) and reports `Removed` exactly when it removed
something, which `--clean` already counts the same way. The existing
`a_record_without_source_text_keeps_no_vector` test covers all three target
values; before this change it failed on the `[]` case with `Unchanged`
versus `Removed`.
…d` declarations

`omnigraph embed` took a hand-written spec (`{"dimension": N, "types": {"T":
{"target": …, "fields": […]}}}`, or a seed manifest's `embeddings:` entry)
that restated what `embedding: Vector(N) @embed("title", model="…")` already
declares, and nothing compared the two. A spec naming another source or
dimension silently wrote vectors a text `nearest()` could not match, unknown
spec keys were ignored, and a recorded `@embed` model was never checked
against the provider, although the query path refuses that mismatch.

The command now takes `--schema <schema.pg>` (a seed manifest names it with
`schema:`) and fills every `@embed` vector the catalog declares, with that
declaration's source and dimension. A type may carry several vectors of
different dimensions, which the spec could not express. `--spec` and the
manifest's `embeddings` entry are removed, and the manifest refuses keys it
does not read.

Before reading input or creating output, a run refuses a recorded model the
provider does not resolve, through `EmbeddingConfig::embeds_with_model`,
which `resolve_nearest_query_vec` now uses too. It also refuses a `--type`
without an `@embed` vector, a schema without one, and, by line, a selected
record without source text whose vector is a required `Vector(N)`. JSON
output reports `vectors` and `schema` in place of `dimension`.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Recommendation: request changes before merge for the one P2 finding below. This is a COMMENT review because the authenticated account is the PR author.

Reviewed 3f437a6670a60c176968138134959681546b0a50 against its parent 3721e4640c0944c0b45e3cd830b851b8daa103a6 in a clean isolated checkout. This covers the final schema-authority commit. The prerequisite #929 is now merged.

The old file pipeline repeated the schema in an embedding spec. The two could disagree about the source, dimension, or model. This change reads targets from the compiler catalog, checks recorded models, and generates each declared vector. Users must replace --spec and the manifest's embeddings section with a schema path. It remains a file preparation command. Loading the result remains a separate graph operation.

Required change

The model check couples every vector on a node to one provider, even though the schema permits different models per property. Both model choices then refuse, and --type cannot select between properties. The inline comment gives the reproduction and the required regression. Preserve schema authority while allowing a run to select the declared vector it will generate. Do not restore a second source/dimension/model mapping or silently ignore a mismatched target.

Correctness, tradeoffs, and liability

The central fix is sound: remove duplicate declarations at the CLI boundary. The catalog-derived plan covers each node's declarations and dimensions. It uses the same model comparison as text nearest. Strict manifest decoding closes the ignored-key class. Required-vector refusal agrees with the loader.

This removes a configuration format and its drift. It adds an in-memory target plan, a small public model-comparison method, and a changed JSON summary. It adds no durable state, queue, or publication rule. Those costs are justified. Five future vector options can derive from the catalog instead of adding five parallel spec fields. The unresolved target-selection gap would otherwise force users to maintain reduced schema copies, which recreates the drift this change removes.

The intended workload is offline seed preparation. The loop streams records but still makes serial provider calls for each vector it generates. More declared targets can mean more calls. I did not measure throughput. The caller must supply the intended schema file. Existing vectors remain trusted in fill-missing mode; this is not a provenance audit or automatic re-embedding on source changes.

I surveyed the other 29 open PRs. The relevant connections are #793's vector-space contracts, #884's compiler-owned type expansion, #951's proposed views, and the skill edits in #811/#892. Keeping this work in the catalog and offline CLI fits those boundaries. Merge the overlapping skill edits carefully. The planner, merge/cache, deployment/admission, GQT corpus, and release/documentation work does not require another embedding authority. RFC 0015's future graph reconciler can reuse the same declarations and provider rule without moving network calls into ordinary writes.

I consulted the repository guides and the complete relevant Lance type, array, vector-search, and index documentation. I checked stock Lance 11.0.0 source (ab6b5bbe46009ed78746b444df8db59a8bc5d842). OmniGraph maps vectors to FixedSizeList<Float32>; Lance's scanner checks query dimensions. Neither check establishes an embedding model's identity. That remains OmniGraph's responsibility. The current Lance 14 beta does not justify a substrate dependency for this CLI fix. No storage or deployment behavior changes here.

Local validation and before/after proof

All Cargo commands used RUSTUP_TOOLCHAIN=1.97.1, CARGO_TARGET_DIR=/tmp/review-928/target, and RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'.

Command Parent Reviewed head
cargo test -p omnigraph-cli --locked --bin omnigraph embed 9 passed 14 passed
cargo test -p omnigraph-cli --locked --test integration cli_data::embed_ 4 passed 5 passed
cargo test -p omnigraph-engine --locked --test search nearest_string_ Not run 4 passed
cargo test -p omnigraph-cli --locked --test integration cli_data::embed_review_ 4 behavioral failures 4 passed
cargo test -p omnigraph-cli --locked --bin omnigraph review_seed_source_refuses_an_unread_model 1 behavioral failure 1 passed

The last two rows used temporary tests in the existing CLI owners. The four CLI cases were embed_review_schema_owns_source, embed_review_schema_owns_dimension, embed_review_recorded_model_mismatch_refuses, and embed_review_required_vector_without_source_refuses. The parent wrote the wrong source vector, wrote eight dimensions instead of four, accepted another model, and installed a row without its required vector. The fifth test showed that the parent accepted an unread sources.model key. All five passed on the reviewed head.

The same test source and behavioral assertions ran on both commits. One compatibility branch selected the parent's --spec interface or the head's --schema interface from embed --help. An unchanged command would only prove an unknown-flag failure. No compile, setup, skipped, or zero-test result counts as this proof. GQT cannot exercise the file CLI, and its runner explicitly refuses @embed schemas, so the existing Rust owners are appropriate. All temporary source/test edits were restored. No changes were pushed.

cargo fmt --all --check, git diff HEAD^ HEAD --check, changed-file typos, python scripts/check-docs.py (253 files), and bash scripts/check-agents-md.sh passed. The macOS linker emitted a compact-unwind size warning, but tests ran.

CI and limits

The exact head has green Fix Regression Gate and PR Title checks. These are not runtime test evidence. I found no workspace/GQT build checks on this draft head. I did not run the full workspace, live provider calls, cloud tests, or performance measurements. No separate optional finding.

Comment on lines +195 to +198
fn check_model(&self, config: &EmbeddingConfig) -> Result<()> {
for target in self.types.values().flatten() {
if let Some(recorded) = &target.model
&& !config.embeds_with_model(recorded)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[P2] Let a run select one declared vector when a type uses several models

This loop requires every vector on a node to use the same provider model. The schema permits vec_a: Vector(4)? @embed("text", model="model-a") and vec_b: Vector(4)? @embed("text", model="model-b") on one Doc. With one row containing text: "hello", embed --schema schema.pg --input raw.jsonl --output out.jsonl --type Doc refuses vec_b under model-a and refuses vec_a under model-b. I reproduced both with the mock provider on this head. Changing --type cannot help, and the check also blocks a missing vector when the other model's vector is already present. On parent 3721e464, two runs with a spec selecting each target populated both properties.

Please preserve the ability to process an individual schema-declared target. Its source, dimension, and recorded model must still come from the catalog. A target selector can do that without restoring the parallel spec or adding automatic provider dispatch. Extend the existing CLI embedding case with two differently modeled vectors on one node: run each target with its matching model, preserve the other vector, and refuse a wrong model for the selected target. The invariant belongs at the selected vector, not at the entire node type.

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.

3 participants