Leave the built-in schemas out of the default Knowledge search scope (closes #3565)

[mstomar125]

Summary

Closes #3565 (release blocker), with the acceptance criteria as revised on the issue.

Since #3538 embeds every schema for fetch-by-task, the ~40 built-in schemas ("Checkbox", "Horizontal Line", ...) flooded untyped search: nodespace search "Hall Nine" returned the note plus 13 schemas. In v0.3.3 the core schemas were never queued for embedding, so this never showed up.

The default Knowledge scope now leaves out built-in schemas only:

• KNOWLEDGE_CORE_TYPES keeps Schema. ADR-029 §5 keeps schema in Knowledge, so a user-defined schema such as "Venue" is still found there by meaning.
• NodeEmbeddingService::matches_scope now takes the node. For Knowledge it refuses a built-in schema, identified by the new schema_node::is_core_schema: an exact schema node whose row carries isCore. That is the flag from_storage reads into SchemaNode::is_core, and the one search_ops already uses to build user_types.
• The filter happens in the scope check, not by un-embedding, so the vectors stay for find_skills and --type schema.

Unchanged:

• --type schema / node_types: ["schema"]: named types replace the default scope (should_skip_scope_filter).
• find_skills: it searches the schema type directly and never consults the scope.
• The everything scope (desktop search box), Custom and Conversations.

Docs: NodeSpaceAI/nodespace-docs#100 adds one data-layer.md clarification. ADR-029 is not edited, and the earlier amendment PR (nodespace-docs#99) is closed.

Tests

• Unit (test_knowledge_scope_admits_user_knowledge_and_excludes_system_types):
  • a user-defined schema is in Knowledge and a built-in one is not;
  • a non-schema node carrying isCore is unaffected;
  • Everything keeps built-ins.
• Gate-run integration (embedding_service_test::test_search_semantic_default_scope_leaves_core_schemas_out_and_keeps_user_schemas):
  • untyped "*" returns the note and the user-defined Venue schema, and no built-in schema;
  • node_types: ["schema"] returns checkbox and Venue.
• Live, real model (default_scope_schema_search_live_test, #[ignore] like find_skills_schema_discovery_test):
  • untyped "Hall Nine" and "marketing budget" rank the note first, with no built-in schema;
  • untyped "venue capacity" returns Venue;
  • --type schema returns checkbox and Venue;
  • find_skills finds Venue from a query that doesn't name it, so the name-match backstop can't be what satisfies the assertion.

Mutation checks (each failed as noted, then restored):

• M1, built-in schemas no longer excluded: unit, gate-run and live all fail. The live test shows untyped "Hall Nine" returning 13 schemas.
• M2, every schema excluded (the earlier version of this PR): unit, gate-run and live fail ("a user-defined schema must be in the default scope"; Venue missing).
• M3, named types no longer replace the scope: gate-run and live fail.
• M4, find_skills returns no schema hits: the live test fails.
• M5, is_core_schema ignores the node type: the unit test fails.

Checks:

• All 107 affected core tests pass with --run-ignored all.
• cargo clippy -p nodespace-core --all-targets -D warnings is clean, rebased on main after Fix clippy collapsible_match errors on main #3571.
• bun run test:changed: result to follow in a comment.

🤖 Generated with Claude Code

[mstomar125]

Pragmatic code review (medium depth): fresh reviewer agent, .claude/agents/pragmatic-code-reviewer.md

Code Review Summary

This is a small, well-scoped fix that meets the revised acceptance criteria for #3565. The core-schema exclusion sits in the scope check (matches_scope) rather than in the embedding step, so find_skills and --type schema keep their vectors. ADR-029 §5's allow-list is left alone (KNOWLEDGE_CORE_TYPES still contains Schema), and Everything, Custom and Conversations behave as before.

What I checked:

• Every matches_scope call site. I grepped all packages in core, plus the sync, cloud and core-org-readonly checkouts. Its only production caller is search_ops.rs:757, and the only others are the unit tests. The new &Node signature is used correctly.
• Identity source. schema_node::is_core_schema (schema_node.rs:135-145) does the same test as from_storage's flag("isCore") (schema_node.rs:57). That flag feeds user_types (search_ops.rs:739-748) and assert_schema_deletable (relationships.rs:2808-2816). It uses is_exactly, so there is no literal node_type comparison in src/.
• Property hydration on every leg. The semantic leg loads nodes through get_nodes_by_ids, which runs SELECT * FROM node (nodes.rs:819). The keyword leg uses query_nodes_simple, which goes to store.query_nodes, also SELECT *. The enumerate leg uses query_nodes, the same SQL. So isCore is always present when the scope check runs.
• CLAUDE.md rules. No issue numbers in code comments. The literal node_type == "schema" checks appear only under tests/, which the literal-comparison check exempts. Comments explain why.
• Docs PR Implement NodeBehavior trait system for extensible node types #100. The data-layer.md text matches the code: core schemas are left out, user-defined schemas stay in, and --type schema reaches the built-in ones.
• Tests. The unit test pins the user-vs-built-in split, the non-schema node that carries isCore (M5) and Everything. The gate-run test pins the untyped and typed enumerate paths. The live test's assertions should be stable:
  • "Hall Nine" and "marketing budget" each appear literally in their note, so the BM25 title leg backs the ranking.
  • The find_skills query closely paraphrases Venue's description.
  • It is #[ignore], so it cannot flake the merge gate.

The PR body says bun run test:changed results will follow in a comment. None has been posted yet, so check for it before merging.

Verdict: APPROVE. The two improvements below are worth doing (the first is cheap), but neither blocks merge.

Findings

Critical Issues

None found.

Suggested Improvements

• packages/core/src/models/schema_node.rs:135-145 and packages/core/src/services/node_service/crud.rs:557-562, plus packages/core/src/db/sqlite_store/relationships.rs:2811-2815: the PR adds a third copy of the "is this a core schema" test instead of reusing an existing one.

  • NodeService::is_core_schema already does exactly this test (is_exactly(Schema) && as_bool() == Some(true)), and its doc already says it is "the same test the store's delete refusal applies".
  • assert_schema_deletable writes the same test out inline.
  • The four places that must agree are the delete refusal, ensure_schema_core_status_unchanged/ensure_not_creating_core_schema, from_storage/user_types, and now the search scope. Two of them guard security-relevant things: core-type deletion and core-status immutability.
  • Now that a crate-visible schema_node::is_core_schema exists, make the private NodeService::is_core_schema call it (or delete it), and have assert_schema_deletable call it too. That way the guards cannot quietly drift apart (DRY / single source of truth). It is a 10-line change.

• packages/core/src/ops/search_ops.rs:271-319 (enumerate) and search_ops.rs:660-663,754-775 (over-fetch, then post-filter): the dropped core schemas now use up the 3x over-fetch window, so a search can return fewer results than limit even when more matching nodes exist.

  • Enumerate leg. With an untyped empty or "*" query and the default limit of 20, the effective window is 60. enumerate_nodes runs SELECT * FROM node LIMIT 180 with no ORDER BY (nodes.rs:1976-1990), so rows come back in insertion order, and keeps the first 60. NodeService::new seeds the core schemas first (node_service/mod.rs:1599), so they fill the front of that window and are then dropped. The CLI's default query is "" (cli/src/commands/search.rs:37), so a bare nodespace search returns only what fits in the remaining slots.
  • Semantic leg. The PR body says "Hall Nine" matched 13 schemas above the threshold. A caller passing limit: 5 gets a 15-row window, and up to 13 of those rows are now discarded.
  • Why it doesn't block. Before this PR those slots were filled with schemas, so this is still a clear improvement, and the same limitation already exists for system types.
  • Test gap. The gate-run test sets untyped.limit = Some(1000) (tests/it/embedding_service_test.rs:2190), so it never runs at the default limit.
  • Suggested follow-up issue. Move the Knowledge scope, including the isCore exclusion, into the fetch itself: per-type with_node_type fetches as the typed enumerate path already does, or a SQL exclusion. Or at least add a test at the default limit.

Nitpicks

• Nit: packages/core/src/services/embedding_service.rs:901-902: "A search that names schema replaces this scope and returns them all." This holds only when no scope is requested. With an explicit scope: "knowledge" plus node_types: ["schema"], should_skip_scope_filter returns false (search_ops.rs:60-62) and the built-in schemas are still dropped. Suggest: "A search that names schema and requests no scope…".
• Nit: packages/core/src/services/mod.rs:83: the SearchScope::Knowledge doc ("KNOWLEDGE_CORE_TYPES plus user-defined types") was not updated to mention that built-in schemas are excluded. This is the first place a reader looks.
• Nit: packages/core/src/services/embedding_service.rs:42-43: "which also leaves the built-in schemas out of schema" reads awkwardly. Suggest "…which also leaves out the built-in schemas, though schema is listed here."
• Nit: packages/core/src/services/embedding_service.rs:904: the intra-doc link from the pub fn matches_scope points into a pub(crate) mod schema_node (models/mod.rs:22), so cargo doc raises rustdoc::private_intra_doc_links. It has no effect today because CI does not run rustdoc, but plain backticks would avoid it.
• Nit (follow-up only; this is Michael's lane, packages/agent): packages/agent/src/local_agent/tools.rs:1089 describes knowledge as "searches text/header/code/schema nodes". That was already out of date (no task/date/project/person or user types) and now also misses the built-in schema exclusion. Worth a note to Michael, not a change in this PR.

🤖 Generated with Claude Code

[mstomar125]

Address review (commit d17612f)

Addressed: 2 improvements and 4 nits

• One definition of the core-schema test.
  • schema_node::is_core_schema is now the only one. The private NodeService::is_core_schema is deleted.
  • ensure_schema_core_status_unchanged, ensure_not_creating_core_schema and the store's assert_schema_deletable now call the shared helper. Its doc names all four users.
  • The semantics are identical: is_exactly(Schema) and isCore == true.
  • The guard tests pass: 219 in the schema:: module plus schema_core_status_cannot_change_through_generic_updates, including test_delete_core_schema_refused and test_delete_refused_when_subtree_contains_a_core_schema.
• Built-in schemas eating the over-fetch window. I verified this before fixing it.
  • The local agent's search_semantic tool defaults to limit 5 (DEFAULT_SEMANTIC_LIMIT), a 15-row window, while a fresh database has 24 embedded built-in schemas.
  • When the Knowledge scope applies, search_semantic now fetches limit * 3 + <number of built-in schemas>. At most that many rows can be dropped as built-in schemas, so the rest of the window is never smaller than before they were embedded.
  • The schema list that builds user_types is now read before the fetch and also supplies that count, so there is no extra query.
  • New gate-run test test_search_semantic_small_limit_reaches_past_the_built_in_schemas: an untyped limit-2 search must still return the user's note behind the seeded schemas. Mutation-checked: without the widening it returns [].
  • On the enumerate-leg concern at the default limit, I probed a fresh database (28 rows: 24 core schemas first, the note last). The untyped default-limit enumerate now returns the note. Before this PR it returned 20 rows that started with the schemas.
• Nits.
  • SearchScope::Knowledge doc now mentions the built-in schema exclusion.
  • matches_scope doc now says "names schema and requests no scope".
  • KNOWLEDGE_CORE_TYPES wording fixed.
  • Replaced the intra-doc link into the crate-private schema_node with plain backticks.

Skipped: 2

• Moving the whole Knowledge scope into the fetch (per-type fetches or a SQL exclusion for system types too). This is the existing TODO(perf) in semantic_search_nodes, and it needs node_type stored on the embedding rows. That's beyond a release-blocker fix. The widening above bounds the part this PR introduces. Revisit if system types such as skills or tools crowd small-limit searches.
• packages/agent/src/local_agent/tools.rs:1089 scope description. That's Michael's lane, so I'm flagging it to the coordinator and not changing it here.

Checks

• 170 affected core tests pass with --run-ignored all, including the live real-model test and find_skills_schema_discovery_test.
• cargo clippy -p nodespace-core --all-targets -D warnings is clean, and cargo fmt --check passes.
• bun run test:changed is next.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: The fetch window is a logic change in search_semantic, and the shared helper now backs the security-relevant core-schema delete and core-status guards. Both should get fresh eyes even though the guard semantics are unchanged.

🤖 Generated with Claude Code

[mstomar125]

Re-review of d17612f (medium depth): fresh reviewer agent, .claude/agents/pragmatic-code-reviewer.md

Re-Review Summary

Commit d17612f is a good follow-up. It resolves both improvements and four of the five nits, and it gives sound reasons for the two items it skips. I checked the new commit only, against PR head d17612f7 (confirmed with gh pr view), reading the surrounding code. I did not compile or run anything.

• Shared core-schema helper: semantics are exactly the same.
  • The old NodeService::is_core_schema used as_bool() == Some(true) and the old inline check used unwrap_or(false). The helper schema_node::is_core_schema (schema_node.rs:142-149) uses unwrap_or(false). For an Option<bool> these give the same answer.
  • assert_schema_deletable still returns early for non-schema nodes (relationships.rs:2808-2810).
  • The DB trigger schema_core_status_fixed (db/schema.rs:529-531, json_type = 'true') also accepts only a JSON boolean true, so the Rust guards and the SQL backstop still agree.
  • A grep for Self::is_core_schema, NodeService::is_core_schema and .is_core_schema( across packages/ finds no remaining callers.
• Wider fetch window: correct for every scope a caller can request.
  • Default Knowledge, no types named: the scope applies and N built-in schemas are added to the fetch.
  • Default Knowledge with node_types: the scope filter is skipped, so N is 0 and user_types is empty, the same as before. The multiplier stays 3x because scope_filters is still true, which was already the case.
  • Explicit scope: "knowledge" with node_types: the scope applies and N is added. That only over-fetches, which is harmless.
  • Everything and Conversations: N is 0, so the window is unchanged.
  • Custom: callers cannot reach it, because parse_scope maps only knowledge, conversations and everything (search_ops.rs:213-222).
  • Enumerate path: the widening carries through per_query_limit = min(1000, 3·effective) and .take(effective) (search_ops.rs:277,319). Once the limit reaches about 334, the 1000-row ENUMERATE_FETCH_CAP binds exactly as it did before, so nothing gets worse there.
  • Early returns: both CollectionNotFound returns (:576, :630) come before the old and the new load position. The only path that now pays for the schema load without using it is a failed semantic fetch when include_title_matches is off (:748), and that cost is negligible.
  • Counting only is_core is right. That is exactly the set of schemas matches_scope drops. System types are out of scope, and the author says so explicitly.
• One real side effect. The widening is multiplied inside semantic_search_nodes and then by the 10x KNN over-fetch, which lowers the request limit at which an existing sqlite-vec hard cap starts making searches fail (Improvement 1).
• The new test does catch the bug, but it can fail without any regression. The claim that it "can only stop being sensitive, never falsely fail" does not hold (Improvement 2).
• Housekeeping before merge:
  • The PR is CONFLICTING against main @ d7d2f69d (Let rules and queries read a checkbox's checked state; let update and set-status name the version they change #3573). The only overlapping line is in packages/core/tests/it/main.rs: both sides add a mod line at the same spot, so keeping both resolves it.
  • The bun run test:changed results have still not been posted.

Verdict: APPROVE. I'd make Improvement 1's clamp (a one-line change) in this PR, because this PR lowers the failing limit. Nothing blocks merge.

Prior findings

• Improvement: third copy of the core-schema test. Resolved. The private method is deleted, and crud.rs:47-48, crud.rs:550 and relationships.rs:2811 call the shared helper with the same semantics (verified above).
• Improvement: dropped built-in schemas use up the over-fetch window.
  • The window part: resolved. limit * 3 + built_in_schemas (search_ops.rs:693-697) caps how many rows the schemas can take, at the node level. search_embeddings ranks and truncates per node, not per chunk (embeddings.rs:500-501).
  • Moving the scope into the fetch: reasonably skipped. It needs node_type stored on the embedding rows, which is the existing TODO(perf) at embedding_service.rs:964-966.
  • The test gap: a small-limit gate test was added (see Improvement 2).
• Nit: matches_scope doc on "names schema". Resolved.
• Nit: SearchScope::Knowledge doc. Resolved.
• Nit: KNOWLEDGE_CORE_TYPES wording. Resolved.
• Nit: intra-doc link into the crate-private schema_node. Resolved.
• Nit: agent tools.rs:1089 scope description. Reasonably skipped (Michael's lane, flagged to the coordinator).

New findings

Critical Issues

None found.

• Node-type rules: the new code compares types with CoreNodeType::Schema.is_exactly and with s.is_core. It adds no literal node_type comparisons in src/, no lifecycle_status reads, no new property keys and no compatibility code.
• Security guards: the delete refusal and core-status guards behave exactly as before.

Suggested Improvements

• [Improvement] search_ops.rs:693-697 feeding embedding_service.rs:971 and embeddings.rs:441: the extra rows bring a pre-existing sqlite-vec limit into play at smaller limit values.

  • The cap. sqlite-vec 0.1.9 (locked in Cargo.lock:5841) rejects any KNN query with k > 4096: #define SQLITE_VEC_VEC0_K_MAX 4096, error "k value in knn query too large" (sqlite-vec.c:7111-7116).

  • How k is built. search_embeddings uses k = limit × EMBEDDING_KNN_OVERFETCH (10) with no clamp. semantic_search_nodes multiplies by 3 again when node_types or property_filters are set.

  • Request limits that fail, with N = 24 built-in schemas:

    Search	Max working limit before	Max working limit now	Newly failing
    No filters, Knowledge scope	136	128	129-136
    With property_filters, default scope	45	37	38-45

  • Who can hit it. Neither the CLI's --limit nor the daemon puts an upper bound on the limit, and search_ops accepts up to 1000. So nodespace search "x" --filters … --limit 40 worked before this PR and now fails with "Search failed". The GUI path (include_title_matches) would instead quietly fall back to keyword-only results.

  • Root cause. The bug already existed: limits 137-1000 fail on main today. This PR moves the threshold down, and it will keep moving as more core types ship.

  • Fix. Clamp in search_embeddings: let k = (limit * EMBEDDING_KNN_OVERFETCH).max(limit).min(VEC0_K_MAX); with VEC0_K_MAX = 4096 next to EMBEDDING_KNN_OVERFETCH. It is one line, and it also fixes the existing failures at higher limits.

• [Improvement] tests/it/embedding_service_test.rs:2236-2256: the gate test can fail even when the widening works.

  • Two ways it can fail with the widening intact:
    1. person is in KNOWLEDGE_CORE_TYPES, so it already takes one of the two result slots. Any further Knowledge-type node seeded before the note pushes the note out.
    2. The window is N+6 rows and the note sits at index N+3, leaving 2 rows of slack. Three more non-Knowledge seeds push the note past the window, for example more core Plays.
  • How it can go quiet instead: if seeding order changes or enumerate_nodes gains an ORDER BY, the test passes with or without the widening, without any warning.
  • Suggested fix (cheap):
    • Compare the limit-2 result with the first two ids of the same untyped search at limit 1000.
    • Add a precondition that the first 3 × limit rows of a raw query_nodes enumerate are all built-in schemas, so a loss of sensitivity fails loudly instead of passing quietly.

Nitpicks

• Nit: models/schema_node.rs:139-141. "The one definition of the test … so they cannot disagree" overstates it. from_storage still does its own flag("isCore") (schema_node.rs:57), and that feeds SchemaNode::is_core, which search_ops, schema/mod.rs and skill_ops read. The SQL trigger (db/schema.rs:529-531) is a necessary mirror of the same test.
  • Option 1: in from_storage, use let is_core = is_core_schema(&node);.
  • Option 2: reword the doc to name the trigger as the SQL copy that must stay in step.

🤖 Generated with Claude Code

[mstomar125]

Address re-review (commit dec2bb6, rebased onto main d7d2f69)

Addressed: 2 improvements, 1 nit, the merge conflict

• sqlite-vec k cap.
  • Verified: search_embeddings used k = (limit * 10).max(limit) with no clamp, and sqlite-vec 0.1.9 refuses k > 4096 (SQLITE_VEC_VEC0_K_MAX).
  • Added VEC0_K_MAX = 4096 next to EMBEDDING_KNN_OVERFETCH and clamped k to it. This undoes the lower failing limit this PR introduced, and also fixes the limits above ~136 that already failed on main.
  • New store test test_search_embeddings_large_limit_is_held_to_the_knn_cap: a limit-1000 search succeeds and finds the node. Mutation-checked: without the clamp it fails with k value in knn query too large, provided 10000 and the limit is 4096.
• Small-limit test robustness. test_search_semantic_small_limit_reaches_past_the_built_in_schemas now:
  • asserts its precondition, that the first 3 × limit raw rows are built-in schemas, so a seeding change fails it loudly instead of leaving it vacuous;
  • compares the limit-2 result with the first two results of an exhaustive (limit-1000) search, so extra Knowledge-type seeds can't fail it falsely.
  • Mutation-checked: without the widening, the comparison fails.
• Nit: "one definition" overstated. from_storage now reads isCore through schema_node::is_core_schema, so every Rust reader shares the test. The doc names the schema_core_status_fixed trigger as its SQL mirror.
  • Mutation-checked: if from_storage ignores isCore, the built-in count drops to 0 and the small-limit test fails.
• Conflict with Let rules and queries read a checkbox's checked state; let update and set-status name the version they change #3573. Rebased. The only conflict was the two mod lines in tests/it/main.rs; I kept both. The PR is MERGEABLE.

Checks

• On the rebased branch, 455 core tests pass with --run-ignored all: db::sqlite_store::, schema::, the guard tests, the search suites, the live real-model tests and Let rules and queries read a checkbox's checked state; let update and set-status name the version they change #3573's derived_attributes_test.
• cargo clippy -p nodespace-core --all-targets -D warnings is clean.
• bun run test:changed passed on d17612f (rust workspace nextest, 212s). It is running again on dec2bb6, and I'll post the result.

Re-Review Decision

Decision: RE-REVIEW NEEDED (focused)

Rationale: The k clamp is a logic change in the store's KNN query. It is the reviewer's suggested one-liner and mutation-tested, but it gets a quick look per the rule that logic changes are re-reviewed. The rest is test hardening and a no-op refactor.

🤖 Generated with Claude Code

[mstomar125]

Focused re-review of dec2bb6 (medium depth): fresh reviewer agent, .claude/agents/pragmatic-code-reviewer.md

Focused Re-Review Summary (dec2bb6)

This commit resolves all three items from the second review, and the rebase onto d7d2f69d is clean. I checked dec2bb6c only, against PR head dec2bb6c (confirmed with gh pr view), reading the surrounding code. I did not compile or run anything.

• The clamp is correct, and k can never exceed sqlite-vec's cap.
  • embeddings.rs:441 computes (limit * 10).max(limit).min(VEC0_K_MAX), and .min comes last, so k <= 4096 for any limit.
  • sqlite-vec 0.1.9 rejects only k > SQLITE_VEC_VEC0_K_MAX (sqlite-vec.c:7111-7112, strict >), so k = 4096 itself is accepted.
  • When limit > 4096: k becomes 4096, which is less than limit. The query succeeds, and scored.truncate(limit) has no effect. The result is at most the distinct nodes found in 4096 chunks. That is a graceful degradation where this used to be a hard error.
  • Production does reach limit > 4096: search_ops accepts a limit up to 1000, which becomes limit*3 (+N) and then x3 in semantic_search_nodes when filters are set, about 9000 in total.
  • This is the only vec0 KNN query. search_embeddings_by_node_type is a plain scan with no k, so it is unaffected.
  • The doc on VEC0_K_MAX ("passes it from a limit of 410") is right: 410 × 10 = 4100.
• The from_storage change does nothing new, as intended. from_storage returns early for any non-schema node (schema_node.rs:37-42). After that point, is_core_schema(&node) reduces to the same properties.get("isCore").and_then(as_bool).unwrap_or(false) as the old flag("isCore"). It borrows node before the ..node move.
• No banned patterns. The only new literal node_type == "schema" is under tests/. There are no lifecycle_status reads and no compatibility code.

Verdict: APPROVE. The two nits below are optional.

Prior findings (second review)

• [Improvement] sqlite-vec k > 4096 cap: Resolved.
  • The clamp order is correct, and the new store test fails without the clamp.
  • See Nit 1: the test does not pin the order of the two calls.
• [Improvement] Small-limit test robustness: Resolved.
  • Vacuity guard: the precondition uses the same unordered SELECT * FROM node scan as enumerate_nodes; only the LIMIT differs. So if the first 6 rows are built-in schemas, the un-widened window drops all of them. small then comes back [], which cannot equal expected because expected.len() == 2 is asserted. The test therefore cannot pass vacuously while the precondition holds.
  • Failure mode 1 (extra Knowledge seeds) is gone. expected is now taken from the exhaustive run.
  • Residual: failure mode 2, extra non-Knowledge seeds, still has the same slack. A failure there would be a real symptom, not a false one (Nit 2).
• [Nit] "One definition" overstated: Resolved. from_storage now goes through the shared helper, and the doc names the schema_core_status_fixed trigger as the SQL copy that must stay in step.
• Merge conflict with Let rules and queries read a checkbox's checked state; let update and set-status name the version they change #3573: Resolved correctly.
  • tests/it/main.rs keeps both lines, in alphabetical order.
  • git diff d7d2f69d..dec2bb6c --stat touches only the PR's 11 files, so none of Let rules and queries read a checkbox's checked state; let update and set-status name the version they change #3573's changes were reverted.
  • Main has since moved to e593c48a (Put a shipped change to a customised seed to the user instead of only logging it #3572). It touches tests/it/main.rs and sqlite_store/mod.rs, but neither hunk is next to this PR's, so I expect no textual conflict.
  • Put a shipped change to a customised seed to the user instead of only logging it #3572's core-play seeding still produces one play node, so the small-limit test's row layout does not change.

New findings

Critical Issues

None found.

Suggested Improvements

None.

Nitpicks

• Nit 1: the store test does not pin the order of .max and .min.
  • At limit = 1000, a swapped .min(VEC0_K_MAX).max(limit) still gives 4096, so the test passes.
  • Above a limit of 4096, the swapped version yields k = limit, and the query fails again. Production reaches that range (about 9000 with filters).
  • Fix: use limit = 5000, which pins both the clamp and the order.
• Nit 2: the assertion message can blame the wrong cause.
  • Non-Knowledge seeds can still fail the test with the widening intact. That failure would be a real product symptom: system types crowding a small window.
  • The message, though, says "not stop at the built-in schemas".
  • Fix: something like "…not stop at rows the scope drops (built-in schemas or system types)".

🤖 Generated with Claude Code

[mstomar125]

Address focused re-review (commit 86fa937)

Addressed: both nits

• Nit 1. test_search_embeddings_large_limit_is_held_to_the_knn_cap now searches with limit 5000, past the cap itself, so it pins the order of .max(limit) and .min(VEC0_K_MAX). Mutation-checked: with the swapped .min(VEC0_K_MAX).max(limit) it fails with k value in knn query too large, provided 5000 and the limit is 4096.
• Nit 2. The small-limit test's message names both kinds of row the scope drops: built-in schemas or system types.

Checks on the final tree (86fa937, MERGEABLE against main)

• bun run test:changed: passed. test:scripts 33s, types:check 4s, rust:test (Rust workspace, nextest) 128s.
• cargo clippy -p nodespace-core --all-targets -D warnings: clean.
• Affected core suites with --run-ignored all: pass, including the live real-model tests.

Mutation summary across the PR (every one fails a test)

Mutation	What fails
M1: built-in schemas not excluded	unit, gate-run, live
M2: every schema excluded	unit, gate-run, live
M3: named types don't replace the scope	gate-run, live
M4: find_skills schema hits dropped	live
M5: isCore read on any node type	unit
M6: no window widening	small-limit
M7: no k clamp	store test
M8: from_storage ignores isCore	small-limit
M9: clamp order swapped	store test

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Both changes are test-only (a test input and an assertion message), and both are mutation-checked. All three reviews returned APPROVE.

🤖 Generated with Claude Code