[malibio]

Closes #3009, closes #3010, closes #2978.

Changes

#2978 — NodeFilter temporal fields. Deleted created_after / created_before / modified_after / modified_before and their with_* builders; nothing read them and NodeQuery had nowhere to put them. The doc example that advertised "tasks created in last 7 days" is gone too. Date-range querying stays with QueryService's metadata filters.

#3009 — QueryNodesInput.parent_id / root_id. Deleted both fields and the tracing::warn!-only handling. All 7 construction sites (agent tools, skill pipeline, prompt assembler, two integration tests) updated.

#3010 — property-filter fetch cap. Chose fix over delete: the in-memory property-filter branch in NodeService::query_nodes is the only implementation of ADR-078 base-scope maps_to value resolution (QueryService doesn't do it; extends_subtype_identity_test covers it), so deleting it would remove that behavior. Instead the 10,000-row cap is gone — the branch reads the whole type-scoped set, then filters and paginates in memory. This matches the collection-scoped path, which already loads the full member set.

A paged scan was considered and rejected: an empty page past the last row trips the store's title stem-match fallback, and id-scoped queries ignore limit/offset.

Tests

• New test_query_with_property_filters_sees_rows_past_ten_thousand: 10,000 non-matching tasks, then the single match. Fails on main, passes here.
• Pre-push gate (test:all + daemon build + e2e) green.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3039

Review Type: Initial review (the PR had no reviews or comments before this one)

Requirements Check

Issue	Suggested fix	Status
#2978	Delete the unused NodeFilter created_after/created_before/modified_after/modified_before fields and their with_* builders	✅ Fields, builders, the chrono import and test_node_filter_temporal are gone. The doc example no longer claims date-range support and points readers to QueryService metadata filters.
#3009	Delete QueryNodesInput.parent_id/root_id, or wire them into a real filter, and remove the warn-only handling	✅ Both fields and the two tracing::warn! calls are deleted. All 7 construction sites are updated (agent tools ×2, prompt_assembler, skill_pipeline test, event_emission_test ×2, object_valued_property_roundtrip_test).
#3010	Remove the silent truncation (flag/error), or delete the branch	✅ The cap is gone, so no result is silently truncated. The PR keeps the in-memory branch instead of deleting it. That choice holds up: build_scope_context / apply_property_filters is the only ADR-078 maps_to read-scope implementation, and extends_subtype_identity_test.rs uses it directly. There is a regression test for the lost-match case.

Leftover references to the deleted items: I searched packages/ and scripts/ (.rs/.ts/.svelte/.proto) and ../nodespace-docs for created_after|createdAfter|modified_*|with_created_*|with_modified_*|PROPERTY_FILTER_FETCH_CAP and for QueryNodesInput construction sites. I found no TS wire types, daemon, CLI or src-tauri usages. The only stale reference is the comment listed below.

Code Review Findings

🔴 Critical

None.

🟡 Improvements

• 📁 packages/core/src/services/node_service/query.rs:675: stale comment. The scope_context_tests module doc still justifies itself with "one DB round-trip per matched node — 10,000 of them at the fetch cap." That cap no longer exists. Now that the branch reads the whole type-scoped set, the per-row-I/O risk is larger, because it grows with the size of the type rather than stopping at 10k. Suggested rewording: "...one DB round-trip per row of the queried type, which is unbounded now that the whole type-scoped set is read." Principle: comments must match the code. A reader who sees a cap mentioned here will assume one still limits the loop.

• 📁 packages/core/src/services/node_service/query.rs:94-103: the unbounded fetch is acceptable, but record its failure mode. The PR is right that a silent cap is worse than an unbounded read: wrong answers are worse than slow ones. Today's exposure is also nil. Every production ops::query_nodes caller passes filters: None, and the agent's property-filter search goes through QueryService SQL pushdown. One edge the new comment does not mention: if node_type is None and property_filters is Some, this becomes a full-table scan loaded into memory. That is reachable through ops::query_nodes if QueryNodesInput { node_type: None, filters: Some(..) } is ever built. Blocking now would be YAGNI. Instead, name that case in the comment, e.g. "without a node_type, this reads every node". Then whoever next wires a caller into this path knows to route large or untyped property queries to QueryService. Principle: make the cost of a deliberate tradeoff visible where the code runs.

🟢 Nits

• Nit: 📁 packages/core/src/services/node_service/mod.rs:4962: the regression test is correct and well aimed. The lone match sits past the old boundary, and the assertion checks the exact id list. It took 3.45s for the pair in a warm debug build, which is fine. The 10_000 is now tied to a constant that no longer exists. That is acceptable for a regression test, and the doc comment already explains why it is 10,000.
• Nit: 📁 packages/core/src/ops/node_ops.rs:407: the query_nodes doc says "over-fetching". That describes the old collection-scoped over-fetch, not the current id-scoped read. This line was not touched by the PR, so it can be left as is. It is only worth a one-word fix while the file is open.
• Nit: 📁 packages/core/src/models/node.rs:239: the doc example is now titled "newest high-priority tasks" and backs that with OrderBy::CreatedDesc, which is accurate. It also no longer implies date-range filtering. Good.

Positives

• The commit is atomic and cohesive: three related dead or silently wrong filter paths are closed in one pass, and the diff is −94/+58.
• It follows CLAUDE.md: nothing is deprecated or given a compatibility shim, there is no allow(dead_code), and code comments contain no issue numbers.
• The replacement comment in query_nodes explains why limit/offset must be applied after filtering. That is the fact the old code got wrong.
• Keeping the in-memory branch rather than deleting it (as query_nodes silently drops property-filter matches beyond the first 10,000 rows of a node type #3010 offered) is correct. Deleting it would have removed the only ADR-078 scope-resolution read path.

Recommendation: APPROVE

This is a net improvement with no blockers. The stale "fetch cap" comment at query.rs:675 should be fixed before merge because it is a one-line edit. The untyped-scan note is optional hardening.

[malibio]

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

[malibio]

Addressed review in 1225254 (comment-only; fmt + cargo check clean, pushed with --no-verify as a docs-only change):

• ✅ 🟡 query.rs scope_context_tests doc: no longer cites the removed 10,000 cap; states the per-row I/O risk is now unbounded
• ✅ 🟡 query_nodes comment: names the worst case (no node_type → reads every node)
• ✅ 🟢 node_ops::query_nodes doc: dropped stale 'over-fetching'
• ⏭️ 🟢 test's 10_000 literal — its doc comment already explains it
• ⏭️ 🟢 node.rs doc example — reviewer confirmed accurate, nothing to change

Re-review: not needed — comments only, no logic change.