[malibio]

Closes #3509

What changed

search_nodes used to fetch a page, drop every conversation from it, and fetch again with a page four times larger when a full page came up short. It now leaves conversations out in the statement, so limit rows come back in one fetch.

• governance::excluded_types_sql is public. It gives a NOT IN (SELECT node_type FROM type_ancestry ...) condition, so a subtype of an excluded type is excluded with no change to the caller.
• Each layer of both engines has an _excluding variant that takes the excluded types: SqliteStore::query_nodes_excluding (the title stem fallback included), NodeService::query_nodes_excluding, node_ops::query_nodes_excluding, QueryService::execute_excluding and query_ops::execute_query_excluding. The existing functions call them with nothing excluded.
• GraphToolExecutor::run_node_query passes CoreNodeType::AiChat to both engines. CONVERSATION_FETCH_HEADROOM, CONVERSATION_FETCH_GROWTH, NodePageQuery, fetch_node_page and the refetch loop are gone.
• A search scoped to a conversation type is refused with the same message as before.

The exclusion is a function argument, not a field on QueryDefinition, NodeQuery or QueryNodesInput. The first two are wire shapes, and the existing test file builds a QueryNodesInput literal that the issue requires to pass unchanged.

search_semantic needed no change: chats are not embedded, so it has no conversation rows to drop.

Tests

• The existing tests in search_nodes_excludes_conversations.rs pass unchanged. One test is added there for the title stem fallback.
• Two tests are added in query_service_test.rs: the condition sits in the WHERE clause ahead of LIMIT, and excluded rows do not count against the limit.

Still to do

architecture/data-layer.md in nodespace-docs still describes the refetch. It is updated right after this lands.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3548 (issue #3509)

Review Type: Initial Review (no earlier reviews or comments on the PR)

Verdict: APPROVE. The code change is correct and a clear net improvement. Three follow-ups are worth doing before merge, none of them a code defect: one new test does not prove what it claims, the test file's comments still describe the removed refetch, and the docs criterion is open.

Requirements Check

#	Acceptance criterion	Status	Notes
1	Conversations excluded in the query, in one fetch, for keyword search and enumeration	✅	Both engines carry the condition in every statement they run (traced below). run_node_query makes one call with limit.
2	Resolved through the type registry; a subtype is excluded with no change; no literal type comparison	✅	NOT IN (SELECT node_type FROM type_ancestry WHERE ancestor IN ('ai-chat')), id taken from CoreNodeType::AiChat. check-node-type-literals.ts passes.
3	CONVERSATION_FETCH_HEADROOM, CONVERSATION_FETCH_GROWTH and the refetch loop removed	✅	Also NodePageQuery and fetch_node_page. No references remain in packages/.
4	Tests in search_nodes_excludes_conversations.rs pass unchanged	✅	All 9 pass. The existing tests are byte-identical; one test is added.
5	A conversation-scoped search is still refused with the same message	✅	Refusal block untouched; a_search_scoped_to_conversations_is_refused and every_chat_subtype_is_excluded pass.
6	architecture/data-layer.md describes where the exclusion happens	❌	Not done. Line 320 of that file still says "The rows are dropped after the fetch, so a page that conversations filled is fetched again, larger". See finding 3.

What I traced

Every statement carries the exclusion.

• SqliteStore::query_nodes_excluding: the mentioned_by join (suffix on n.node_type), the main statement (pushed into conditions before the WHERE is built), the id-chunked path (each chunk clones conditions after the push), and the stem fallback (all three SQL arms share governed).
• QueryService::execute_excluding: one statement, built by build_where_clause_with. The condition sits in WHERE, ahead of ORDER BY and LIMIT.
• NodeService::query_nodes_excluding with property filters fetches unpaged and limits in memory, after the SQL exclusion, so it is correct there too.
• Count paths (SqliteStore::count_nodes, QueryService::count / matches) do not take the exclusion. Nothing on the search_nodes path calls them: its count is the length of the returned list. Acceptable, with the caveat in finding 5.

A user-defined type extending ai-chat is excluded. The ancestry table is trigger-maintained: a schema node insert adds its self row, and an extends edge insert adds (descendant, ancestor) pairs against the registry-seeded core rows. db::schema::tests::a_subtype_of_a_core_type_resolves_to_it covers exactly this for a user type extending a core type, and passes.

A side benefit not in the PR description: in the stem fallback, chats no longer take slots in the candidate cap (TITLE_STEM_FALLBACK_CANDIDATE_CAP). The old post-filter could not fix a cap filled by chats.

The _excluding shape. Six variants across five layers is more surface than a field would be, but the constraints are real: NodeQuery is a generated wire type, QueryDefinition is the stored saved-query shape, and QueryNodesInput is built as a literal by the test that must pass unchanged. A field on NodeFilter (not a wire type) would remove only one of the six. The parameter is &[CoreNodeType], so nothing user-supplied reaches the inlined SQL. I would not change the shape.

Code Review Findings

🟡 1. The stem fallback test passes without the exclusion

📁 packages/agent/tests/it/search_nodes_excludes_conversations.rs:169-189

Problem. a_stem_matched_keyword_search_excludes_conversations searches with limit: 1. The fallback's candidates are ordered modified_at DESC, and seed_note_among_chats creates the note last, so the note is the first candidate. The note and all three chats match the same three stems (shared, data, layer), the rank sort is stable, and truncating to 1 returns the note whether or not the chats were excluded. If someone removes excluded from the fallback, this test stays green. (This is from reading the code; I did not mutate the source to confirm it, since the review is read-only.)

Recommendation. Drop "limit": 1 from this test (the default is 50). Then assert_eq!(types_of(..), vec!["text"]) fails as soon as a chat comes back. Confirm by temporarily removing excluded from the fallback and watching it fail.

Principle. A regression test must fail when the behaviour it names is removed. The fallback is a separate statement, which is exactly where a dropped condition would go unnoticed.

🟡 2. Comments in the test file still describe the refetch

📁 packages/agent/tests/it/search_nodes_excludes_conversations.rs:123-124, 145-148, 272-275

Problem. Three doc comments now describe behaviour that no longer exists:

• "The exclusion sits above both" (it is now inside both engines);
• "Dropping conversations after the fetch must not cost the caller the rows it asked for";
• "a page that came up short because chats filled it is fetched again, larger".

Recommendation. Reword them to say the exclusion is part of the statement. "Pass unchanged" in the issue is about the tests' code and assertions; the file is already edited to add a test, and a comment edit changes no test.

Principle. Comments explain why; a comment that describes a deleted mechanism misleads the next reader.

🟡 3. The docs criterion is open, and the PR closes the issue

📁 ../nodespace-docs/architecture/data-layer.md:320

Problem. Criterion 6 is unmet, and "Closes #3509" will close the issue on merge with it unchecked. The stated plan (update right after merge) is reasonable given the docs live in another repository, and this is not a node-type change, so no merge-gate check depends on the sheet.

Recommendation. I do not treat this as blocking the code. Make the docs edit part of the merge step itself, not a later task: one sentence replaced on line 320 (the condition is in the statement on both engines, resolved through the ancestry table; no refetch). Tick the criterion only once it is pushed.

Principle. Acceptance criteria are the definition of done; an auto-closed issue with an open criterion is how doc drift starts.

🟢 4. excluded_types_sql in governance names its one consumer

📁 packages/core/src/governance.rs:131-139

Problem. The module owns the participation rules the registry records. A caller-supplied exclusion is not one of those rules, and the new doc comment describes "the agent's node search" from inside core. The function is db::schema::is_not_a_sql plus an empty check, and is_not_a_sql is already public and used directly in nodes.rs.

Recommendation. Optional. Either keep it and make the doc comment generic (what it does, not who calls it), or call is_not_a_sql behind the store's own and_excluded_types helper and leave excluded_types_sql private.

Principle. Single responsibility; a lower layer should not document its upper-layer caller.

🟢 5. build_where_clause's stated invariant no longer covers build_query

📁 packages/core/src/services/query_service/mod.rs:493-534, 677-684

Problem. The doc on build_where_clause says it is "deliberately the only place" that decides which rows match, so that select and count agree. build_query now bypasses it, calls build_where_clause_with directly, and repeats chrono::Local::now().date_naive(). With a non-empty exclusion, execute_excluding and count select different rows. No caller pairs them today. There are also now three functions (build_where_clause → _on → _with) for one clause.

Recommendation. Optional. Add excluded to build_where_clause_on and drop _with, and add one line to the invariant comment saying the count has no exclusion variant.

Principle. KISS; keep a documented invariant true or amend it.

🟢 6. Nit: is_conversation_type loops over a one-element list

📁 packages/agent/src/local_agent/tools.rs:285-307

Fine as written, since it keeps CONVERSATION_TYPES the single source for both the refusal and the exclusion. No change needed.

Recommendation

APPROVE. Fix findings 1 and 2 in this PR (both are small test-file edits), and land the docs sentence with the merge.

Verified by running vs inferred from reading

Ran:

• cargo-nextest -p nodespace-agent --test it search_nodes_excludes_conversations::: 9 of 9 pass.
• cargo-nextest -p nodespace-core --lib for an_excluded_type_is_a_condition_of_the_select, excluded_rows_do_not_count_against_the_limit, a_subtype_of_a_core_type_resolves_to_it, a_type_rule_with_nothing_to_exclude_adds_no_condition: 4 of 4 pass.
• scripts/check-node-type-literals.ts and scripts/check-lifecycle-reads.ts: both pass.
• Grep for the removed symbols: none remain in packages/.
• Grep of data-layer.md: line 320 still describes the refetch.

Inferred from reading only:

• That every statement on both paths carries the condition.
• That the stem fallback test passes without the exclusion (finding 1). Not confirmed by mutation.
• That excluded_rows_do_not_count_against_the_limit would fail without the change (the note is created first, sorted newest first, limit 1).
• That a user type extending ai-chat is excluded end to end; the ancestry test I ran uses collection, not ai-chat.

Not run: the full Rust suite, test:changed, clippy.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed (61073b5)

Addressed

• 1 (stem fallback test). The test no longer passes a limit, so a chat the fallback let through is in the result. Checked by temporarily removing the exclusion from the fallback: the test failed, and passes again with it restored.
• 2 (stale comments). The three comments in the test file now describe the exclusion as part of the statement.
• 4 (governance doc). excluded_types_sql's doc no longer names a caller.
• 5 (where-clause invariant). build_where_clause takes the exclusion and build_where_clause_with is gone, so one function again decides which rows match. Its doc says that a count and a membership check pass no exclusion.

Not changed

• 6. No change needed, as the review says.

After merge

• 3 (docs). The data-layer.md sentence is pushed to nodespace-docs as part of the merge step. This session cannot run git in the docs repository while it is in the PR worktree.

Tests: the 12 agent search_nodes tests and the 81 core query-service and governance tests pass; clippy is clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The fixes are a test assertion, comments, and one private signature change in QueryService with no change to the SQL it produces.

[malibio]

Merge queue: #3548 was taken out of the queue — the merge gate failed on it, on top of main. Reproduce by merging or rebasing origin/main and running bun run test:changed, fix, push, and re-run bun run merge 3548.

▶ Merge gate: full pyramid on the rebased PR.
  stage logs: /var/folders/6c/mkxnmpkx5zqc9jwwy20tjkyr0000gn/T/nodespace-gate-logs/_gate-merge-2026-10-04T19-57-31-760Z

▶ quality:scripts:check (scripts/ lint + typecheck) (timeout 10m)
  ✓ quality:scripts:check (scripts/ lint + typecheck) 4s
▶ quality:design-tokens (design-token drift) (timeout 5m)
  ✓ quality:design-tokens (design-token drift) 0s
▶ check-version-sync (app version drift) (timeout 5m)
  ✓ check-version-sync (app version drift) 0s
▶ check-pro-boundary (ADR-081 code-boundary ratchet) (timeout 5m)
  ✓ check-pro-boundary (ADR-081 code-boundary ratchet) 1s
▶ check-node-type-literals (ADR-086 chain-aware type rules) (timeout 5m)
  ✓ check-node-type-literals (ADR-086 chain-aware type rules) 0s
▶ check-lifecycle-reads (ADR-087 one participation check) (timeout 5m)
  ✓ check-lifecycle-reads (ADR-087 one participation check) 0s
▶ skill installer script (tsc) (timeout 5m)
  ✓ skill installer script (tsc) 0s
▶ types:check (generated TypeScript drift) (timeout 15m)
▶ test (frontend, Happy-DOM) (timeout 15m)
  ✓ types:check (generated TypeScript drift) 1s
▶ compile Rust test binaries (timeout 60m)
  ✓ test (frontend, Happy-DOM) 146s
▶ test:scripts (tooling) (timeout 10m)
  ✓ test:scripts (tooling) 37s
▶ test:skill (skill package) (timeout 5m)

$ bun run --cwd packages/skill test
$ bunx vitest run
error: script "test" was terminated by signal SIGTERM (Polite quit request)
error: script "test:skill" was terminated by signal SIGTERM (Polite quit request)
[stage killed by SIGTERM]

  full output: /var/folders/6c/mkxnmpkx5zqc9jwwy20tjkyr0000gn/T/nodespace-gate-logs/_gate-merge-2026-10-04T19-57-31-760Z/test-skill-skill-package.log

✗ test:skill (skill package) failed — merge blocked.
  Fix the failure, push, and re-run: bun run merge <PR#>