[malibio]

Closes #3449. Implements ADR-086 §11 and §4 ("Structured") for play and query.

What changed

One RelationshipPath, compiled to SQL. nodespace-types gains RelationshipPath (a list of hops: a relationship name, built-in, schema-declared or reverse, optionally open-ended). Query relationship filters, a play's for_each and the dot-paths in play conditions all use it.

• ops::path_ops resolves a path's names against the schemas (one resolver, same order the CLI's relationship reads use).
• db/sqlite_store/relationship_paths.rs is the one compiler: a CTE per hop, each a correlated subquery that looks edges up by endpoint (never a JOIN against relationship), WITH RECURSIVE for open-ended hops, rows keyed by root, all roots bound as one JSON array. Query-plan tests pin the endpoint-index form.
• GraphResolver no longer walks hop by hop: it hands whole runs of hops to the store, for every root at once. A scheduled scan resolves each condition path for all selected nodes in one statement and passes the result to the work items it enqueues.
• Query relationship / related filters take path and compile through the same compiler, walking backward from the anchor so the subquery runs once per statement. They now accept schema-declared and reverse names, several hops, and open-ended hops.

Typed play rules. RuleDefinition, a tagged Trigger, a tagged Action with one params struct per action (deny_unknown_fields), Selector, PlayFields, PlayNode and PlayNodeUpdate are defined once in nodespace-types; core's ParsedRule is the compiled form. A rule with an unknown trigger, action or param does not decode, so it does not save. Open JSON remains only for properties and edge_data.

Selectors. A trigger's select replaces node_type: a query's target_type and filters inline, or { "query_id" } naming a saved query. A scheduled scan runs it as one query through QueryService.

Typed update, end to end. update_play_node in core → UpdatePlayNode proto → daemon → Tauri → dev-proxy → adapters → store, lowering into the shared update pipeline. TypeScript is generated.

Node type sequence

• play: structured core type, now WireShape::Typed { update: true }. Fields unchanged (rules, description); rules is now RuleDefinition[] with the typed shapes above. Typed update: rules (replaced whole), description (tri-state).
• query: structured. QueryFilter loses relationshipType / relationshipName and gains path (RelationshipPath). No new schema fields.
• No new relationships or structural rules. No data migration: reset the database.
• Docs, same change: nodespace-docs 40193d3 (architecture/relationship-paths.md, playbook-system.md, playbook-engine.md, components/query-node.md, node-types.md, typed-relationships.md).

Decisions worth a reviewer's eye

1. A graph_event selector is a bare type. Filters and saved-query selectors are rejected on a graph_event trigger at save time: events are matched in memory by type, and invariant rules inside the triggering transaction, so there is no query to run. The issue scopes query selection to scheduled triggers; the docs say so explicitly.
2. Resolved paths are never stored. Names are validated at save time and resolved again whenever a query or play runs. A play's condition paths are resolved from the concrete node's type, so a declared hop after a built-in one (node.child_of.project) is a second statement for the whole set, not one per node.
3. QueryService now matches subtypes of target_type through the ancestry table (as query-node.md already said it did). Needed so a selector over task still scans issue nodes, as the old type scan did. This also changes nodespace query --type task.
4. The model-facing filter input changed: relationship_type (parent/children/…) and relationship_name are replaced by path in the agent tool schema, the CLI's --filters, and the skill guidance. The prompt golden is regenerated. No model eval was run for this.
5. A scan's condition paths are read when the scan runs, so a node late in a large scan is evaluated against the graph as it stood when the schedule fired. Action bindings and for_each still resolve live.
6. Folded in: validation accepts a core node field (id, content, …) as the end of a path on any node, matching what the resolver already does; update_node no longer takes lifecycle_status (typed params; ADR-087).

Acceptance criteria

• RelationshipPath in nodespace-types; query filters, for_each and CEL dot-paths use it
• One SQL statement per path, correlated subquery per hop, WITH RECURSIVE for open-ended; a scan resolves a path for all roots in one statement (query-plan and statement-count tests)
• Results keyed by (root id, path)
• GraphResolver no longer calls get_related_nodes
• Typed play rules; an unknown action param is rejected on save (a case per action)
• A scheduled trigger selects with inline type and filters or a saved query id
• Bundled playbooks and core plays use the typed shapes (skill references regenerated)
• Node type sequence followed; node-types.md play and query sheets updated
• bun run test:changed passes; bun run quality:fix clean

Testing

bun run test:changed passed on the merged tree (frontend, scripts, skill, browser, generated-types drift, Rust workspace). New tests cover the SQL compiler (including plans, cycles, archived nodes), path resolution, batched resolution and root keying, selectors through the cron runner and the save gate, the typed play update (write, clear, conflict, wrong type), and one end-to-end scheduled play selecting through a saved query.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3481

Review Type: Initial Review (no prior reviews or comments on the PR; all commits on the branch reviewed)

Summary. The design is sound and the PR is a clear net improvement: three name resolvers collapse into path_ops::resolve_hop, the SQL walk is written once, play rules are typed at the wire, and the resolver's batching keeps the old per-hop semantics. I compared graph_resolver.rs against the origin/main version case by case (single vs collection, missing vs unresolved, field-before-relationship, reverse-name narrowing, declared-many, caching) and found one narrow divergence (finding 7). Batching cannot hand one root another root's answer: rows are keyed by the start node of each run, start ids are deduplicated, and follow_run stops reading a run as soon as a hop is not a single node. No SQL injection or identifier-safety regression: every value in the new compiler is bound, the only formatted fragments are static column names and validated identifiers.

Two things should be fixed before merge: a sort regression caused by the new subtype matching (finding 1), and the open-ended walk, which does not terminate on a cycle the way its comment, its test and the docs say (finding 2).

Severity labels: Critical, Important, Suggestion. Each finding says whether it is verified (and how) or suspected.

---

Requirements Check (issue #3449)

#	Acceptance criterion	Status	Note
1	RelationshipPath in nodespace-types; query filters, for_each and CEL dot-paths use it	Met, with a deviation	Query filters carry RelationshipPath on the wire. A play's for_each and condition dot-paths are still strings; their segments go through the same resolve_hop and the same compiler, always as fixed hops (RelationshipHop::fixed), so a play cannot express an open-ended hop.
2	One SQL statement per path; correlated subquery per hop; WITH RECURSIVE; a scan resolves a path for all roots in one statement (tests, query-plan assertion)	Met, with caveats	It is one statement per run of hops per starting node type: a declared hop after a built-in or untyped hop starts a second statement (disclosed), and a scan whose nodes have several concrete types runs one per type. Plan and statement-count tests exist. The cycle test does not pin what it claims (finding 2).
3	Results keyed by (root id, path); never another root's answer (test)	Met	results_are_keyed_by_root_never_shared_between_roots, cache_does_not_leak_between_root_nodes, seeding_takes_only_the_roots_own_paths.
4	GraphResolver no longer calls get_related_nodes per hop	Met	No call remains in graph_resolver.rs.
5	Typed play rules; an unknown action param is rejected on save (test per action)	Met, with a gap	each_action_rejects_a_param_it_does_not_take and a_play_with_an_unknown_action_param_is_rejected_on_save cover all five actions. A selector's filters are the exception: QueryFilter accepts unknown keys (finding 4).
6	A scheduled trigger can select with QueryFields or a saved query id (tests)	Met	Inline is target_type + filters (not sorting/limit, which is reasonable). Cron-runner tests plus the save gate.
7	Bundled playbooks and core plays use the typed params	Met
8	Node type sequence followed; node-types.md play and query sheets updated	Met	Registry, behaviour, wire type, generated TypeScript, typed write path and tests are all present. The docs commit is 882f679 in nodespace-docs; the PR body cites 40193d3. query-node.md contradicts itself on stored filter key casing (finding 4).
9	bun run test:changed passes; quality:fix clean	Not re-run	Reported as passed by the author; I did not re-run suites.

---

Code Review Findings

1. Important: sorting a base-type query mis-orders subtype rows (regression from subtype matching)

packages/core/src/services/query_service/mod.rs:313-346 (with the type filter at :471-474)

_ => {
    let val_a = a.properties.get(&a.node_type).and_then(|ns| ns.get(field));
    let val_b = b.properties.get(&b.node_type).and_then(|ns| ns.get(field));

The type filter now matches subtypes, so a task query returns issue rows. SQL orders them by $.task.<field>, which is right, because an inherited field stays in the base type's bucket (db/schema.rs:63-66). The Rust re-sort that "has the final say" then reads each row's own-type bucket (properties.issue.<field>), finds nothing, and treats every subtype row as having no value. For priority it ranks them as unprioritized. Result: nodespace query --type task --sorting priority (and the agent's search_nodes) puts every issue first, in ascending order, whatever its priority. Before this PR the type match was exact, so node_type == target_type held for every row and the two passes agreed.

• Recommendation: read the sort value from the bucket the SQL used: properties[target_type] when target_type != "*", the row's own type only under the wildcard. Add a test that sorts a base-type query over a base row and a subtype row.
• Principle: two passes that must agree should be asserted to agree; the existing comment on compare_nodes_by_field already states this invariant for priority.
• Status: verified by reading (bucket layout confirmed by db/schema.rs:63-66 and by an_inline_selector_filters_in_the_query, which matches a sel_bug through $.sel_ticket.state). Not executed.

2. Important: an open-ended walk over a cycle laps until the depth cap; comment, test and docs say it does not

packages/core/src/db/sqlite_store/relationship_paths.rs:24-26, 188-205, 531-551

"{walker}({columns}, depth) AS (\
 SELECT {carry_p}j.value, 1 FROM {previous} p, json_each(({first})) j \
 UNION \
 SELECT {carry_w}j.value, w.depth + 1 FROM {walker} w, json_each(({again})) j \
 WHERE w.depth < {OPEN_ENDED_DEPTH_CAP})",

depth is part of the row, so UNION only removes duplicates at the same depth. On a cycle every lap produces new rows until depth 100. I ran the same statement shape in sqlite3 on a three-node cycle: the walker holds 100 rows, max depth 100, 3 distinct nodes. The test an_open_ended_walk_over_a_cycle_terminates_with_each_node_once says the walk must "end, not loop until the depth cap emits a row per lap"; it passes only because the outer SELECT DISTINCT hides the laps, so it would pass with UNION ALL too. Each walker row runs a correlated subquery, so a strongly connected component of S nodes costs about 100 x S lookups per root (per seed for a query filter) instead of S. mentions, blocks and relates_to graphs have cycles in practice.

• Recommendation: drop the depth column and the cap from the recursive CTE. With UNION on (root_id, node_id) the walk terminates on any finite graph by itself, the intermediate SELECT DISTINCT CTE becomes unnecessary, and OPEN_ENDED_DEPTH_CAP can go. If a cap is wanted for its own sake, keep it but fix the comment and relationship-paths.md ("UNION keeps a cycle from repeating rows"), and make the test assert the walker's row count.
• Principle: a regression test must fail without the property it names; KISS.
• Status: verified (ran the SQL).

3. Important: the scan's "one query" is 1 + N statements

packages/core/src/playbook/selectors.rs:113-123 calling packages/core/src/services/query_service/mod.rs:255-285

let node_ids = self.execute_query_for_ids(&built).await?;
...
for id in node_ids {
    if let Some(node) = self.store.get_node(&id).await? {

QueryService::execute runs SELECT * FROM node ..., keeps only the ids, then calls get_node once per id. That is pre-existing, but this PR puts it on the scheduled-scan path, where the old code made one query_nodes call, and the new doc comments say "One query, however many nodes and filters". A scan of N nodes now costs N + 1 statements before the path statements, which is the cost shape the issue set out to remove.

• Recommendation: hydrate nodes from the rows the first statement already returns (row_to_node), or drop the "one query" wording. Small change, removes the round trips for every query caller.
• Principle: N+1 avoidance; comments must describe what the code does.
• Status: verified by reading.

4. Important: a selector's filters accept unknown keys and use a different key casing from the rest of the rule

packages/nodespace-types/src/query.rs:74-78, packages/nodespace-types/src/play.rs:5-11, 107-114

#[serde(rename_all = "camelCase")]
pub struct QueryFilter {

play.rs states "Every type a user or agent authors rejects unknown fields" and "Rule keys are snake_case in storage and on the wire alike". A rule's select.filters[] is a QueryFilter: camelCase keys (nodeId, caseSensitive) and no deny_unknown_fields. An author who writes node_id or case_sensitive (the spelling the CLI's --filters, the agent tool, node-types.md and the example in query-node.md all use) has the key dropped. node_id then fails with "missing 'nodeId'", which is recoverable; case_sensitive: false is silently ignored and the filter runs case-sensitive. query-node.md says both that stored filter keys are snake_case (ADR-086 section 9) and that a stored filter spells the anchor nodeId.

• Recommendation: add deny_unknown_fields to QueryFilter now. Decide one stored spelling for filter keys; snake_case matches ADR-086 section 9, the execute input and the rest of a rule, and there is no data to carry forward. Fix the docs either way.
• Principle: the PR's own stated rule (typed rules reject what the engine never reads); one spelling per stored name.
• Status: verified by reading. The casing is pre-existing; embedding it in a play rule is new.

5. Important: unused public code

packages/core/src/playbook/selectors.rs:34, 97; packages/core/src/db/sqlite_store/relationship_paths.rs:64-74

SelectorError::is_genuine() and selector_target_type() have no callers (the latter is named only in a doc comment at types.rs:199). PathReach::reached() and hop_count() are called only from tests. They are pub, so the compiler does not warn.

• Recommendation: delete them, or make the test-only ones #[cfg(test)].
• Principle: YAGNI; the project rule is to delete unused code.
• Status: verified by grep.

6. Suggestion: scan-time resolution of condition paths (staleness trade-off)

packages/core/src/playbook/cron_runner.rs:186-205, packages/core/src/playbook/engine.rs:1364-1375

I checked the claim in the PR description: action bindings and for_each build a fresh GraphResolver (actions.rs:1466, 2118), so only conditions read the seeded cache. The scan already evaluated each node against its scan-time snapshot (trigger_node), so this widens an existing window to related nodes rather than opening a new one. The case to keep in mind is a rule whose action changes a node that later nodes' conditions read through a path (siblings rolling up to a shared parent): every sibling now sees the parent as it was when the schedule fired, so the action fires once per sibling instead of once. Bundled plays are not affected. It is documented in playbook-engine.md.

• Recommendation: acceptable as is. Add one test that pins the behaviour (two scanned nodes sharing a related node that the first one's action changes), so a later change to it is deliberate. ScanContext.paths also holds a cloned Node per root per path prefix until the last work item is processed; fine at current sizes, worth knowing for large scans.
• Status: verified by reading.

7. Suggestion: a reverse name shared by an untyped declaration and another declaration resolves differently than before

packages/core/src/ops/path_ops.rs:122-139

if let Some((source_type, rel)) = inbound.iter().find(|(_, rel)| rel.reverse_name == name) {

The old play resolver went through rel_ops::resolve_relationship_name, which drops an untyped declaration unless one of its edges reaches the node, and then takes the first match (rel_ops.rs:272-301). resolve_hop takes the first declaration with that reverse name, untyped or not, and narrows to its declarer. If an untyped declaration and a typed one share a reverse name and the untyped one sorts first, a node linked only through the typed one now resolves to Missing. follow_run's untyped && related.is_empty() check covers shape, not which declaration was chosen. This is the one place the two remaining resolvers (resolve_relationship_name for the CLI, resolve_hop for plays and queries) disagree.

• Recommendation: prefer typed declarations over untyped ones in resolve_hop's two inbound lookups, and consider having resolve_relationship_name delegate to resolve_hop so there is one precedence list.
• Principle: DRY; the PR's goal of one resolver.
• Status: suspected (read both resolvers; no test written). Narrow: needs a reverse-name collision involving an untyped declaration.

8. Suggestion: the frontend live-append gate still compares types exactly

packages/desktop-app/src/lib/components/query/query-node-model.ts:308-317

if (gate.targetType !== '*' && node.nodeType !== gate.targetType) return false;

The backend now returns subtypes for a typed query; this gate declines a newly created issue for a task view until the next load. It errs on the safe side, so it is not a correctness bug, but it no longer matches what the query returns. The doc comment above it still speaks of parent/children filters. matchesFilter also has no branch for a related filter, which falls through to the property comparison with no property; that predates this PR.

• Recommendation: use isA(node.nodeType, gate.targetType), return false for related, and update the comment.
• Status: verified by reading (the related fall-through is suspected, not traced through equals).

9. Suggestion: the model-facing filter change has had no eval

packages/agent/src/local_agent/tools.rs:751-762

relationship_type (an enum of four words) became path (a free array) with reversed-feeling semantics: path: ["child_of"] + node_id: X selects X's children. The description says so clearly, and AgentFilterItem rejects the old key outright. The PR description discloses that no eval was run.

• Recommendation: run eval:decisions (after checking the fixture age) before or right after merge, and watch for filters that send has_child where child_of is meant.
• Status: verified by reading.

Nits

• Nit: relationship_paths.rs:313-314 reads the appended columns as row.get(9) / row.get(10), which silently depends on node having nine columns. Select the two extra columns first, or derive the offset from one named constant shared with row_to_node.
• Nit: relationship_paths.rs:49-51 says a hop's nodes are "in edge insertion order", but the final statement has no ORDER BY, and a later hop can repeat a node when an earlier hop fans out and converges (has_child then child_of returns the parent once per child). The resolver never reads past a multi-node hop, so nothing breaks today; say so in the doc or add DISTINCT.
• Nit: a play that selects through { "query_id": ... } keeps no link to the query. Deleting the query, or changing its target_type, leaves the play validated against the old type and logging an error every time the schedule fires. ADR-086 section 11 chose the reference by id, so this is not a rule violation, but check_schema_change_impact has no counterpart for it. Consider refusing to delete a query a play references, or suspending the play.
• Nit: install.rs rewrite_node_type_filters does not descend into a related filter's nested filter.
• Nit: shared-node-store.svelte.ts:809 extends the hand-written TypedNodeType union with 'play'; it could be derived from the generated typed-core-fields list so the next typed type cannot be forgotten.
• Nit: the PR body cites docs commit 40193d3; the commit in nodespace-docs is 882f679.

---

What I checked and found sound

• Resolver semantics against origin/main: field-before-relationship at every hop including inside a run (follow_run re-checks own_value for index > 0), declared-many as a collection at any count, Missing vs Unresolved never folded together or cached, prefix caching and resume, reverse-name narrowing (now in SQL through the ancestry table).
• A multi-hop run in one statement when an intermediate hop reaches several nodes: the walk settles on that hop as a collection and never reads the later hops' rows, which are the only rows that would mix sources.
• Parameter binding: numbered placeholders throughout, so the nested filter's values, the anchor and the hop values cannot drift. A relationship filter nested in a related filter produces nested CTEs with the same names; I ran that shape in sqlite3 and the inner names resolve correctly.
• Participation goes through governance::visible_sql; no lifecycle_status read outside governance, no literal node_type comparison, no compatibility code, no lint suppression in the new files.
• Typed update wiring is complete across core, proto, daemon, Tauri, dev-proxy, both adapters and the store, and matches the query pattern.

---

Recommendation: REQUEST CHANGES

Required before merge: finding 1 (sort regression) and finding 2 (open-ended walk on cycles, plus the test and docs that describe it). Findings 3 to 5 are small and worth folding into the same pass. The rest can follow.

[malibio]

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

[malibio]

Review addressed (commit cd9f3c2b)

bun run test:changed passed on the result (frontend, scripts, skill, browser, generated-types drift, Rust workspace); bun run quality:fix clean. Docs follow-up: nodespace-docs 85fc5a4.

Addressed

1. Subtype rows mis-sorted in a base-type query. The Rust re-sort now reads the target type's bucket, the same one the SQL ordering reads (each row's own bucket only under *). New test sorting_a_base_type_query_orders_subtype_rows_by_their_value; reverting the fix makes it fail with ["m", "a", "z"].
2. Open-ended walk lapped a cycle until the cap. The walker no longer carries a depth column, so UNION ends the walk as soon as it finds no new node; the cap and its constant are gone. The cycle test now counts the walker's own rows (3 for a three-node cycle) instead of the distinct result. Module comment and relationship-paths.md corrected.
3. Scan was 1 + N statements. QueryService::execute loads the matched nodes with one batched get_nodes_by_ids, keeping the query's order. Comments and docs now say "one statement for the ids, one batched read".
4. Selector filters accepted unknown keys. QueryFilter is deny_unknown_fields. This immediately caught a stale test fixture still sending relationshipType, which had been silently ignored.
5. Unused code. Removed SelectorError::is_genuine and selector_target_type; PathReach::reached is a test helper and hop_count is gone.
6. Reverse-name collision with an untyped declaration. A declaration that names the type as its target now wins over an untyped one spelling the same name. New test in path_ops.
7. Frontend live-append comment. Updated: it no longer mentions parent/children, and says a subtype node appears on the next load.

• Nits: appended columns are read through a NODE_COLUMNS constant; the PathReach ordering comment now says what later hops do; rewrite_node_type_filters descends into a related filter's nested filter.

Skipped

• 4, key casing. A stored QueryFilter is camelCase today, inside snake_case rule JSON. Changing it needs separate storage and wire spellings for the query type and its frontend consumers, which is a change to query, not to this PR. The docs no longer claim otherwise in the sentence this PR added.
• 6, a test pinning scan-time resolution. The behaviour is stated in playbook-engine.md and the consequence is an idempotent repeat of an action; a test would pin the trade-off rather than guard a defect.
• 9, model eval for the path filter input. Not run. Flagged in the PR description for the merge decision.
• Saved-query integrity when a query is deleted or retargeted. A missing query is refused at save time and logged at scan time; a reference check on query deletion is a new rule, outside this issue.
• Hand-written TypedNodeType union. Pre-existing pattern, one line per typed type; the table-driven routing test fails if a typed type has no route.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: two of the changes alter logic rather than wording: the open-ended walk's SQL was restructured, and query execution now sorts from a different bucket and loads nodes through a different read. QueryFilter also became stricter on a shared wire type.

[malibio]

Pragmatic Code Review: PR #3481

Review Type: Re-Review (scope: commit cd9f3c2b, the only commit since the review of 6e8ef8bf)

Previous Review Summary

• Previous recommendations: 15 (nine numbered findings, six nits).
• Addressed: 9 in full (findings 1, 2, 3, 5, 7; the strictness half of 4; three nits), 1 in part (finding 8, comment only).
• Not addressed: 5, all with acceptable reasons, plus one nit not mentioned (the PR body's docs commit reference).
• New issues: 1 Important (docs only), 2 Suggestions, 3 nits. No new code defect found.

What I ran: .tools/bin/cargo-nextest nextest run -p nodespace-core --lib relationship_paths path_ops sorting_a_base_type_query (19 passed), and the new walk's SQL shape in sqlite3 on a cyclic graph. I did not re-run the full suites.

How each previous finding stands

#	Finding	Status	How checked
1	Subtype rows mis-sorted	Addressed	compare_nodes_by_field reads properties[target_type] for a concrete target and the row's own bucket under *, which is what resolve_field emits in both cases. The priority branch takes its values from the same closure and keeps the same Priority::applies_to(target_type) condition as resolve_order_field. The new test has the subtype row in the middle of the order, so the old code fails it in both directions.
2	Cycle laps to the depth cap	Addressed	The walker's row is (node_id) or (root_id, node_id), so UNION ends the walk once no new row appears. In sqlite3, a three-node cycle with a self-loop, two roots and two chained open-ended hops gave three rows per root per hop and terminated; a root with no edges gave none. The recursive arm references its table once; the correlated lookup reads only relationship. The test now counts the walker's own rows.
3	1 + N statements	Addressed	get_node and get_nodes_by_ids both build nodes with row_to_node from SELECT * FROM node, so no hydration is lost. Order comes from node_ids; ids are primary keys from a join-free select, so nothing is dropped or duplicated. Reads are chunked at 900 ids.
4	Filters accept unknown keys; casing	Strictness addressed, casing skipped	See finding A below for what the skip leaves behind. No producer breaks: the CLI, agent and frontend execute paths go through AgentFilterItem and build QueryFilter in Rust; the frontend's stored filters use the generated camelCase type; the methodology views construct the struct directly.
5	Unused public code	Addressed	No references remain to is_genuine, selector_target_type, hop_count or OPEN_ENDED_DEPTH_CAP; reached is test-only.
6	Test pinning scan-time resolution	Skipped, acceptable	Documented trade-off, no defect.
7	Reverse-name collision with an untyped declaration	Addressed	Typed declarations are tried first in both inbound lookups; the new test declares the untyped one first.
8	Frontend live-append gate	Comment only	Acceptable; see finding C.
9	No eval for the path filter input	Skipped, acceptable	A merge-time decision, disclosed in the PR body.
Nits	Column offsets, ordering comment, nested filter descent	Addressed
Nits	Saved-query integrity, TypedNodeType union	Skipped, acceptable	Out of this issue's scope.

Removing the depth cap is not a regression. The walk is bounded by roots times reachable nodes, and a hierarchy deeper than 100 is now walked in full instead of being cut off silently.

Findings

A. Important: the docs give a stored filter spelling that the code now rejects

nodespace-docs/components/query-node.md:43, nodespace-docs/components/node-types.md:288, against packages/nodespace-types/src/query.rs:77

Stored, their keys are snake_case like every nested stored value (`case_sensitive`, `node_id`; ADR-086 §9)

#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct QueryFilter {

A stored filter is written by to_properties_patch and read by QueryFields::from_properties, both through this struct, so the stored keys are caseSensitive and nodeId. Before this commit the documented spelling was dropped silently. Now QueryNodeBehavior::validate (packages/core/src/behaviors/mod.rs:1460) and a play's select.filters refuse it as an unknown field. That is the right behaviour, but an author following either doc gets an error. The docs follow-up (85fc5a4) removed the one sentence that gave the true spelling (A stored filter spells the anchor nodeId), so the accepted spelling is no longer documented anywhere.

• Recommendation: since the casing change is deferred, make the two docs say what the code does. Stored filter keys are camelCase (caseSensitive, nodeId), the execute input is snake_case, and both reject unknown keys. This is a docs-only change and does not need a code change in this PR.
• Principle: one spelling per stored name; docs describe the code as it is.
• Status: verified by reading the serde attributes and both read and write paths. Not executed.

B. Suggestion: no test pins the new strictness

packages/nodespace-types/src/query.rs:77

Nothing fails if deny_unknown_fields is removed from QueryFilter. The fixture it caught was a silent drop, which is the failure this guards against.

• Recommendation: add one assertion that a filter with an unknown key (case_sensitive, say) fails to decode, next to a_malformed_field_is_rejected_by_name, and one that a play whose select.filters carries one is refused on save.
• Principle: a guard without a test is removed by the next refactor.
• Status: verified by grep; no such test exists.

C. Suggestion: the updated live-append comment overstates what the gate declines

packages/desktop-app/src/lib/components/query/query-node-model.ts:308-321

 * the gate. The same goes for a node of a subtype of the target type: the
 * backend's query returns it, but the type's `extends` chain is not known
 * here, so it appears on the next load.

Two details are off. isA in $lib/types/core-node-types.ts does know the chain for core types, so a new issue could join a task view. And a related filter still has no branch in matchesFilter: it falls through to the property comparison with no property, and equals(undefined, undefined) is true, so a related filter built without a value passes every node, which contradicts "every node fails the gate". A filter decoded from the wire carries value: null and is declined, so the exposure is narrow.

• Recommendation: return false for related at the top of matchesFilter, and either use isA or say the chain is known only for core types.
• Status: verified by reading. Carried over from finding 8; not introduced by this commit.

Nits

• Nit: packages/core/src/services/query_service/mod.rs:516 still says execute pays to "get_node each one", and lines 258-260 describe a fetch strategy the method no longer uses.
• Nit: packages/core/src/methodology/install.rs:642-645: the nested-filter descent has no test. A related filter whose nested filter is a metadata filter on node_type would pin it.
• Nit: the sort test covers the generic branch. A task query over a task and an issue sorted by priority would pin the ranked branch as well; it shares the same bucket closure, so it is covered structurally today.

Recommendation: APPROVE

Both blocking findings from the first review are fixed, and the fixes are checked by tests that fail without them. Finding A is a docs correction in nodespace-docs and should land with the merge. B and C are optional.

[malibio]

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

[malibio]

Re-review addressed (commit 091a12d7)

bun run test:changed passed; bun run quality:fix clean. Docs: nodespace-docs 6d3eab9.

Addressed

• B, strictness had no test. a_filter_with_an_unknown_key_is_rejected (a stored query) and a case in the selector decode test (a play's select.filters) now fail if deny_unknown_fields is removed from QueryFilter.
• C, live-append gate. matchesFilter returns false for a related filter, with a test; the comment now says the gate compares the type exactly.
• Nits. Stale comments in QueryService::execute and count removed; the nested-filter re-key has a test.

Not changed

• A, docs give a stored filter spelling the code rejects. The docs state the decided design: ADR-086 §9 and node-types.md §1.2 say the keys of nested stored values, QueryFilter included, are snake_case. The code stores them camelCase, which predates this PR. Rewriting the docs to camelCase would contradict the ADR, so the docs keep the design and now also say an unknown key is rejected. The gap is in the code: QueryFilter needs a stored spelling separate from its wire spelling. That is a change to the query type and its frontend, and it is raised with the maintainer rather than folded in here.
• Ranked priority branch of the sort test. It reads through the same bucket closure the new test pins.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: this commit adds tests, removes stale comments, and adds one early return false in a frontend predicate for a filter type it could never evaluate. No backend logic changed since the approved commit.

[malibio]

Merge queue: #3481 was taken out of the queue — it conflicts with main in: packages/core/src/behaviors/mod.rs, packages/core/src/playbook/cron_runner.rs, packages/core/src/playbook/graph_resolver.rs, packages/core/src/playbook/validation.rs, packages/core/src/services/node_service/schema.rs, packages/daemon/src/services/node_service.rs, packages/desktop-app/app-lib/src/types.rs, packages/desktop-app/src/lib/types/generated/index.ts, packages/desktop-app/src/lib/types/generated/typed-core-fields.ts, packages/nodespace-types/src/convert.rs, packages/nodespace-types/src/core_type.rs, packages/skill/references/jira-playbook.md. Rebase onto origin/main, push, and re-run bun run merge 3481.