[malibio]

Closes #3483.

What changed

ADR-086 §9 says a nested stored value has one spelling, snake_case, in storage and on the wire. QueryFilter and SortConfig were the two nested types stored in the query bucket (and in a Play's selector) that did not follow it.

Kind and category: no type, field, relationship or rule is added. query is a structured core type; this changes the keys of two nested wire types it stores.

• QueryFilter: rename_all = "camelCase" is gone. The keys that change are caseSensitive → case_sensitive and nodeId → node_id, in a saved query, a Play's select.filters, and the generated TypeScript. The CLI's and the agent's filter input already used these spellings.
• SortConfig: rename_all is gone (its keys are single words, so no key changes) and it is now deny_unknown_fields.
• Frontend: buildExecuteQueryWire sends a definition's filters and sorting as written. The filter-key converter and encodeSortField (which turned modifiedAt into modified_at for five metadata columns) are deleted. A sort names its field by the stored name.
• Removed unused code: DEFAULT_QUERY and QUERY_TEMPLATE_EXAMPLES had no consumer. One template sorted by dueDate, a name the backend never matched.
• No fallback read of the camelCase keys. A database holding a saved query with caseSensitive or nodeId needs a reset: on read that query is served with the schema defaults (all types, no filters) and the decode error is logged, and a Play selector that executes it errors.
• Live-append gate: matchesFilter follows the backend's SQL. equals and in compare exactly, and contains is case-sensitive unless case_sensitive is false.

Not changed

• AgentFilterItem (the execute input) stays a separate type. It is not the same shape as QueryFilter: its type is optional and inferred, and its operator is parsed with model-facing errors. Its keys already matched.
• The AI chat message records (AiChatMessage and the records nested in it) are also camelCase; Store AI chat messages as ai-chat-message child nodes with real edges #3448 replaces them with message nodes.

Tests

• filter_keys_are_snake_case_stored_and_on_the_wire, a_filter_with_an_unknown_key_is_rejected (now covers caseSensitive and nodeId) and a_sort_item_with_an_unknown_key_is_rejected. All three fail with the rename_all restored and the deny_unknown_fields removed.
• the_execute_input_accepts_what_the_stored_types_serialize round-trips a stored filter and sort item through the execute input; agent_filter_item_reads_an_explicit_null_as_absent covers explicit nulls. Together they pin what lets the frontend send a definition unmodified.
• bun run test:changed passes: frontend, scripts, browser, generated-types drift, Rust workspace.

Docs

nodespace-docs a5848bf: naming-conventions.md gains the nested-stored-value rule and the one-spelling section; query-node.md no longer says filter keys are camelCase on the wire.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3485

Review Type: Initial review (no prior reviews or comments on the PR). Reviewed commit 3da1c3d0.

Recommendation: APPROVE. No blocking findings. One non-blocking test gap is worth closing in this PR (finding 1).

Summary

The change removes rename_all = "camelCase" from QueryFilter and SortConfig, makes SortConfig deny_unknown_fields, regenerates the TypeScript and deletes the frontend's key and sort-field converters. It adds no fallback read of the old keys. The diff is small and deletes more than it adds (125 added, 259 removed).

Requirements Check (issue #3483)

	Acceptance criterion	Evidence
✅	QueryFilter and SortConfig have no rename_all; both are deny_unknown_fields	Read packages/nodespace-types/src/query.rs:82 and :126
✅	caseSensitive / nodeId rejected; case_sensitive / node_id accepted, in a saved query and a Play's selector	Ran the tests (see below). One gap: no positive test of case_sensitive in a Play selector (finding 3)
✅	A sort item with an unknown key is rejected	Ran a_sort_item_with_an_unknown_key_is_rejected
✅	Generated TypeScript regenerated and committed; frontend uses snake_case keys	Ran bun run types:check: up to date, 77 files
✅	One definition if the execute input's filter item is the same shape	It is not the same shape, so two types is correct (see "Design calls")
✅	Bundled playbooks, seeds and fixtures use snake_case keys	Grep of packages/ and scripts/ finds no caseSensitive, no nodeId filter key and no camelCase sort field outside the tests that assert rejection
✅	Docs updated	Read nodespace-docs a5848bf (on origin/main): naming-conventions.md, query-node.md, node-types.md, node-type-sequence.md, creating-node-types.md
➖	bun run test:changed passes	Not re-run in full. I ran the targeted tests below; all pass

What I verified, and how

Ran:

• Frontend: adapter-core.test.ts and query-node-model.test.ts, 54 of 54 pass.
• bun run types:check: generated TypeScript matches Rust.
• nodespace-types: the three new or changed query tests and a_selector_is_inline_or_a_saved_query pass.
• nodespace-core: 44 lib tests (query_ops, the play selector and saved-query write-path tests, the relationship filter tests) and 35 integration tests in query_service_test pass.

Read:

• Leftover camelCase: none in the frontend, the dev-proxy, the Tauri command layer, the daemon, the CLI, the skill package or methodology/. The dev-proxy, Tauri commands and CLI forward filtersJson / sortingJson as opaque strings. The bundled playbooks build QueryFilter / SortConfig as Rust structs, so they have no key spelling to get wrong. linear.rs sorts by start_date.
• Sending filters unmodified is safe today. QueryFilter serializes exactly eight keys (resolved_path is #[serde(skip)] and absent from the generated type). AgentFilterItem accepts the same eight. The server sends property, value, case_sensitive and node_id as explicit nulls; each is #[serde(default)] Option<_> on the input, so null reads as absent. The old encoder already sent those four nulls.
• Removing encodeSortField breaks no real path. The only frontend caller of executeQuery with sorting is query-node-viewer.svelte:275, which passes the stored definition.sorting through. No component authors a sort (there is no column-click sort). resolve_field and compare_nodes_by_field match the snake_case metadata names. The agent tool schema and the CLI docs already use due_date / created_at.
• DEFAULT_QUERY and QUERY_TEMPLATE_EXAMPLES were unused. A search of origin/main finds only their definitions and one comment.
• The write path rejects the old keys. QueryFields::from_properties runs in behaviors/mod.rs:1446 and node_service/crud.rs:2525.

Inferred, not run: that the three new nodespace-types tests fail with the change reverted. I did not mutate the code (review only). By inspection they must: with rename_all restored, node_id is an unknown field, and without deny_unknown_fields the nulls key is dropped. agent_filter_item_reads_an_explicit_null_as_absent passes with or without the change, as the PR says: it pins existing behaviour.

Design calls

• Keeping AgentFilterItem separate is sound. Its type is an optional string with inference, including tolerating a node type written in the category slot, and its operator is a string parsed with model-facing errors. Merging it into QueryFilter would weaken the stored type or lose the lenient input.
• Deferring the AI chat records to Store AI chat messages as ai-chat-message child nodes with real edges #3448 is sound. I checked every rename_all = "camelCase" left in nodespace-types. The rest are top-level typed-node wire shapes (promoted fields, camelCase by convention) or SchemaField metadata (the documented exception). Only the AiChat* nested records violate the rule, and Store AI chat messages as ai-chat-message child nodes with real edges #3448 (open) replaces them with message nodes.

Findings

1. 🟡 Important (non-blocking): nothing asserts that AgentFilterItem accepts what QueryFilter serializes

📁 packages/core/src/ops/query_ops.rs:667 and packages/desktop-app/src/lib/services/adapter-core.ts:434

Problem. Before this PR the frontend encoder picked each key by name. Now whatever QueryFilter serializes goes straight into a deny_unknown_fields input, so the two structs must have the same key set. No test checks that. agent_filter_item_reads_an_explicit_null_as_absent lists the keys by hand, and the frontend test only checks that JSON passes through. If someone adds a key to QueryFilter and not to AgentFilterItem, every test still passes and every saved query in the viewer fails with invalid filters_json: unknown field.

Recommendation. Add a Rust test in query_ops.rs: build a QueryFilter with every optional field set (a related filter with a path and a nested filter that carries case_sensitive, plus a relationship filter with node_id), serialize it, deserialize as AgentFilterItem, run to_query_filter, and assert it equals the original. Do the same for SortConfig into AgentSortItem.

Principle. When two definitions must agree, test the agreement directly.

2. 🟢 Suggestion: a sort field in the wrong case still fails silently

📁 packages/core/src/services/query_service/mod.rs:630 (resolve_field)

Problem. A stored sort such as {"field": "modifiedAt"} or dueDate passes validate_identifier, resolves to a json_extract that is NULL for every row, and orders nothing. That is the "looks applied and is not" failure this PR removes for keys. It is not a regression: the CLI, agent and Play paths never translated the field, and the frontend authors no sorts. The deleted converter was the only thing hiding it, for five names, on one path.

Recommendation. Optional. On a query write, reject a sort field that is neither a metadata column nor a declared field of target_type, when the target is not *. Filter property names have the same gap, so this may be better as its own change.

Principle. Fail loudly on input that cannot have an effect.

3. 🟢 Nit: no positive test for case_sensitive in a Play selector

📁 packages/nodespace-types/src/play.rs:684

The test flips the rejected spelling to caseSensitive but never asserts that a selector with case_sensitive decodes. node_id in a selector is covered indirectly by a_selector_filter_with_an_undeclared_path_is_rejected. One extra serde_json::from_value::<Selector> assertion would cover the criterion directly.

4. 🟢 Suggestion (existing behaviour, outside this PR): the default for case sensitivity differs between frontend and backend

📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:216 and packages/core/src/services/query_service/mod.rs:811

matchesFilter defaults case_sensitive to false; the SQL path defaults it to true, and SQL equals is always case-sensitive. A node created outside the viewer can be added to a live result that the backend query would exclude, until the next reload. This PR only renames the key on that line. Noted because the PR's goal is one meaning per filter.

5. 🟢 Nit: PR description wording

The description says an old-shape saved query "fails to decode". On the read path query_node_to_value (packages/nodespace-types/src/convert.rs:552) serves it with schema defaults (all types, no filters) and logs to stderr; execution through a Play selector does error. Either way the answer is a database reset, so no code change is needed.

Checklist notes

• Architecture / node-type sequence: no type, field, relationship or rule is added. The wire type has one definition, the TypeScript is generated, there is no compatibility code, and node-types.md changed in the docs commit. None of the Critical triggers apply.
• Security: no new input surface. Sort fields and filter properties still pass the validate_identifier allowlist before reaching SQL text.
• Performance and dependencies: no impact.

[malibio]

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

[malibio]

Review addressed (commit 9d3a7ccf)

bun run test:changed passed on the result; bun run quality:fix clean.

Addressed

• 1, nothing pinned the execute input to the stored filter's keys. the_execute_input_accepts_what_the_stored_types_serialize serializes a QueryFilter with every optional field set (a related filter with a path and a nested filter, and a relationship filter with node_id), reads it as AgentFilterItem, converts it back and asserts equality; a SortConfig goes through AgentSortItem the same way. With node_id renamed on the input it fails with unknown field node_id.
• 3, no positive selector test. The selector decode test now asserts a filter with case_sensitive decodes.
• 4, case-sensitivity default differed between the live-append gate and the backend. matchesFilter now follows the SQL: equals and in compare exactly, and contains is case-sensitive unless case_sensitive is false. The gate declines more than before, which is its safe direction.
• 5, PR description. Corrected: an old-shape saved query is served with schema defaults on read and errors when a Play selector executes it.

Skipped

• 2, a sort field or filter property in the wrong case still sorts or filters nothing. Rejecting a name the target type does not declare is a new write-time rule, and it has to account for extension fields, subtypes and the * target. It is not a regression of this PR.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: three of the four changes are tests or wording, but matchesFilter changed behaviour: equals and in no longer honour case_sensitive, and contains defaults the other way.

[malibio]

Pragmatic Code Review — PR #3485

Review Type: Re-Review. Reviewed commit 9d3a7ccf only (git diff 3da1c3d0..9d3a7ccf, 4 files, 93 added, 20 removed).

Recommendation: APPROVE. No blocking findings. The commit does what the "Review addressed" comment says. The live-append gate now agrees with the backend on case; three narrow cases remain where it still accepts a node the backend excludes (finding 1), all older than this PR.

Previous Review Summary

	Count
Previous recommendations	5
Addressed	4 (findings 1, 3, 4, 5)
Not addressed	1 (finding 2, skipped with a reason; I agree, see below)
New issues found	3, none blocking (1 Important, 2 Suggestions)

Status of the previous findings

#	Finding	Status
1	Nothing pinned AgentFilterItem to QueryFilter's keys	Addressed. The new test passes and covers all eight serialized keys. One gap: finding 2 below
2	A sort field or filter property the type does not declare fails silently	Skipped. Sound call
3	No positive case_sensitive test in a Play selector	Addressed
4	Case-sensitivity default differed between gate and backend	Addressed for case. Mixed-type equals still differs: finding 1 below
5	PR description wording	Addressed (per the comment; I did not re-read the description)

What I verified, and how

Ran:

• query-node-model.test.ts: 45 of 45 pass.
• the_execute_input_accepts_what_the_stored_types_serialize, agent_filter_item_reads_an_explicit_null_as_absent, agent_filter_item_rejects_unknown_field (nodespace-core) and a_selector_is_inline_or_a_saved_query (nodespace-types): 4 of 4 pass under nextest.
• SQLite comparison semantics, with the system sqlite3 3.45.3 (not the libsql build the app bundles): json_extract of a number = '5' is false; IN ('5') is false; a TEXT column = 5 is true; NULL = 'null' is not true; LOWER('Élan') LIKE LOWER('%élan%') is false; INSTR(x, '') > 0 is true.

Read:

• build_filter_condition, build_content_condition, build_property_filter, build_metadata_filter and bind_json_value in packages/core/src/services/query_service/mod.rs.
• matchesFilter, shouldShowCreatedNode and their one production caller, query-node-viewer.svelte:204.
• AgentFilterItem, AgentSortItem, to_query_filter, and QueryFilter / SortConfig / SortDirection.

Inferred, not run:

• That the new Rust test fails when a key is renamed on one side. I did not mutate code (review only). By reading: both input structs are deny_unknown_fields, so it must.
• That libsql's LOWER is ASCII-only like stock SQLite's. I tested stock SQLite only.

Does the gate now agree with the backend?

Checked operator by operator. "Declines" means the gate says no where the backend says yes; that is the safe direction. "Over-accepts" is the unsafe one.

Operator	Filter type	Backend	Gate	Agreement
equals, in	property, same JSON type	json_extract(...) = ?, exact	===	Agrees. case_sensitive ignored on both sides
equals, in	property, number vs string	false (no affinity, so no conversion)	String(a) === String(b), true	Over-accepts (finding 1)
equals, in	metadata (content, title, node_type, timestamps)	TEXT column, so a bound number is converted to text	string comparison	Agrees
equals	any, subject unset	NULL = x is never true	String(null) === 'null' is true for the value "null"	Over-accepts, for that one literal (finding 1)
contains	property, content, metadata; case_sensitive absent or true	INSTR, case-sensitive	includes	Agrees. This is the default this commit fixed
contains	case_sensitive: false, ASCII	LOWER(...) LIKE LOWER(...)	toLowerCase().includes	Agrees
contains	case_sensitive: false, non-ASCII letters	SQLite's LOWER folds ASCII only, so É does not match é	Unicode fold, matches	Over-accepts (finding 3)
contains	subject is ''	INSTR('', '') > 0 is true	false	Declines (safe)
exists	property, metadata	IS NOT NULL, so '' exists	'' counts as empty	Declines (safe)
gt / lt / gte / lte	all	unchanged by this commit	unchanged	Not reviewed here

Callers and tests that relied on the old behaviour: none in production. matchesFilter is called only by shouldShowCreatedNode, which is called only by the viewer's subscribeAll handler. No frontend code authors a filter or sets case_sensitive. Two tests relied on the case-insensitive default (value: 'OPEN' on equals, value: 'acme' on content contains); both were updated in this commit, and the first now asserts the opposite for all three case_sensitive values.

Is declining the safe direction? Yes. A declined node is missing until the next query load, which runs the real SQL. An over-accepted node sits in a result the query excludes, and nothing removes it until a reload. Every change in this commit moves toward declining: equals and in lose the case fold, and contains defaults to case-sensitive. The commit cannot add a false positive.

Code Review Findings

1. 🟡 Important (non-blocking): equals and in still accept a number-vs-string match the backend rejects

📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:150-155

Problem. The new doc comment says equals is "Exact, as the backend's SQL = is". It is not, for a property filter with mixed types. The fallback is String(actual) === String(expected), so a stored amount: 500 matches the filter value "500". The backend compares json_extract(properties, '$.invoice.amount') = ? with a bound TEXT value. Neither side has column affinity, so SQLite does no conversion and the comparison is false (checked with sqlite3). The same holds for in, and for a stored true against the value "true".

A second case in the same function: when the subject is unset, String(null) is "null", so a filter value of "null" or "undefined" matches. In SQL, NULL = 'null' is never true.

The string comparison is correct for metadata filters, because those columns are TEXT and SQLite converts the bound number.

Impact is low. A model that writes "value": "500" against a number field gets an empty backend result, and then the viewer live-appends each new node whose amount is 500. This behaviour predates the PR; the commit only made the comment claim more than the code does.

Recommendation. Return false when the subject is null or undefined. For a property filter compare with === only. Keep the string comparison for metadata and content. Add a test for amount: 500 against "500". If this is left for later, soften the comment so it claims exactness for case only.

Principle. When two implementations must agree, the looser one should fail closed where it cannot match the stricter one.

2. 🟢 Suggestion: the new Rust test does not see a new key that is skipped when None

📁 packages/core/src/ops/query_ops.rs:672-705

Problem. The test covers every key QueryFilter serializes today: type, operator, property, value, case_sensitive and node_id are always written (as null when unset), and path and filter are set in the fixtures. A new plain Option field would be written as null and rejected by deny_unknown_fields, so the test fails as intended. SortConfig's fields are always written, so that half is tight.

The gap is a new field declared like path and filter are, with skip_serializing_if = "Option::is_none". The fixtures use ..Default::default(), so the new field is None, is not serialized, and the test passes while the input still rejects the key once a real filter sets it. The same spread hides a field added to both structs but not copied in to_query_filter.

Recommendation. Write the fixtures as full struct literals, with resolved_path: None and no ..Default::default(). Adding a field to QueryFilter then fails to compile in this test, which sends the author here to set it.

Principle. A test that pins a key set should fail when the set changes, however the new key is declared.

3. 🟢 Suggestion: contains with case_sensitive: false folds Unicode in the gate and ASCII in SQL

📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:161 and packages/core/src/services/query_service/mod.rs:822

toLowerCase() folds É to é; SQLite's built-in LOWER leaves it alone, so the gate matches Élan against élan and the SQL does not (checked with stock sqlite3; inferred for libsql). The gate over-accepts for non-ASCII text in a case-insensitive contains. This is arguably a backend defect, since the user asked for a case-insensitive match. No change needed in this PR; worth a sentence in the gate's doc comment so the remaining difference is recorded.

Scoping call on previous finding 2

Skipping it is sound. Rejecting an undeclared sort field or filter property at write time is a new validation rule, not a fix to this change. It has to resolve fields through the extends chain, allow extension fields and the * target, and decide what happens to a stored query when its type's schema later drops a field. That is a design decision with its own tests, and nothing in this PR makes the existing behaviour worse. The frontend converter this PR deleted hid the problem for five names on one path only.

Checklist notes

• Architecture / node-type sequence: no type, field, relationship or rule changes in this commit. None of the Critical triggers apply.
• Selector assertion (packages/nodespace-types/src/play.rs:681-689): correct. It decodes an inline selector with case_sensitive: false and asserts the value, next to the existing rejection of caseSensitive.
• Security, performance, dependencies: no impact. Test code and a pure predicate only.
• Docs: nodespace-docs does not describe the gate's case rules, so nothing there went stale.

[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

bun run test:changed passed on the result; bun run quality:fix clean.

Addressed

• 1, equals and in accepted a number-vs-string match and the text "null" for an unset subject. equals now returns false for an unset subject, compares a property filter's value by strict equality (500 is not "500"), and keeps the text comparison for metadata and content, whose columns are text. Two new tests cover the mixed-type, column and unset-subject cases. The doc comment now says what the function does.
• 2, the agreement test missed a key skipped when None. Its fixtures are full struct literals with no ..Default::default(), so a field added to QueryFilter stops the test compiling until it is set there.
• 3, Unicode fold. Recorded in contains' doc comment: case-insensitive matching folds every letter in the gate and only ASCII in SQL.

Skipped

Nothing from this round. Previous finding 2 (write-time rejection of an undeclared sort field or filter property) stays out of this PR, as the re-review agreed.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the one logic change is the reviewer's own recommendation, applied as written to a pure predicate. It can only make the gate decline more, which is its safe direction, and each new branch has a test. The rest is a test fixture and a comment.