[malibio]

Closes #3430

What changed

Deletes code that production never uses and the compatibility shims that read keys storage never holds.

Unused Rust (core)

• NodeBehavior::default_metadata() and every implementation, with the tests that only exercised it.
• The TextNode, DateNode, CodeBlockNode, QuoteBlockNode, OrderedListNode and CollectionNode wrappers and their test files. Nothing outside their own files referenced them.

Unused frontend

• The user and document plugin definitions (never registered; no backend type).
• embeddingVector and memberOf on the TypeScript Node (also embeddingVector on NodeUpdate and NodeWithChildren; the backend has no such field).
• icon and color on the TypeScript CollectionNode.

Compatibility shims

• toCamelCase() in query-node-model.ts and table-row.svelte. Both now read a field through the existing resolveFieldValue: a core type's declared field comes from its typed key (due_date → dueDate, decided by TYPED_CORE_FIELDS), and every other field comes from properties under its stored snake_case name. No spelling is guessed.
• The second read location in parse_rules_from_properties.
• The target_type action parameter check in play validation.

Two places this differs from the issue text

1. Play rules (item 7). The issue calls properties.play.rules the fallback. It is the stored location: a play's declared fields are bucketed under its type on write, validate_play_rules runs after bucketing, and node-types.md documents rules at play.rules. The read that storage never satisfies is the top-level properties.rules, so that is the one removed. Deleting the play.rules read instead would fail every persisted play with MissingField("rules"). In-memory test fixtures that activate a play directly were built in the top-level shape; they now use the stored shape (plays created through create_node still pass flat input, which the service buckets).
2. Query field lookup (item 6). "Use the stored name only" would break due_date / first_name / start_date columns on task, person and project, whose declared fields travel as typed camelCase keys. The lookup now goes through the declared typed-field registry rather than a name transformation, so custom fields are read by stored name only and core fields by their declared key.

target_type remains in the codebase as the schema relationship field and the query field; only the play action parameter check is removed.

Docs

No change needed in nodespace-docs: components/node-types.md has no icon/color rows for Collection and already documents rules at play.rules; no doc describes the removed wrappers or default_metadata other than ADR-086, which records the decision to delete them.

Testing

• bun run test:changed passes (frontend, scripts, browser, Rust workspace).
• bun run quality:fix clean.
• New tests: table rows render snake_case custom fields from properties, ignore a camelCase spelling, and render a core type's declared field from its typed key; filter evaluation covers the same three cases; parse_rules_from_properties rejects rules outside the play bucket.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3469

• Review Type: Initial Review
• Recommendation: REQUEST CHANGES (two small fixes; everything else is sound)

Summary

A 2,796-line deletion that does what the issue asks, and both deviations from the issue text are correct on the evidence. The Rust side is clean. The one real problem is in the frontend: swapping readFieldValue for resolveFieldValue in matchesFilter silently drops support for metadata filters, which the old lookup satisfied and the function's own doc comment still promises. One acceptance-criterion leftover (collection icon/color) also remains.

Requirements Check

#	Criterion	Status	Notes
1	Items 1–8 deleted with their tests; real-behaviour tests rewritten	✅	Items 6 and 7 deviate from the issue text; both deviations are right (see below). Deleted wrapper and default_metadata tests exercised only the deleted code.
2	No remaining reference	❌ partial	Clean for default_metadata, the six wrappers, the user/document plugins, embeddingVector, memberOf, toCamelCase, the top-level rules read, and the target_type action param. Collection icon/color is still read at core-plugins.ts:541-542.
3	Query table renders snake_case custom fields (test)	✅	Three new table-row.test.ts cases plus two in query-node-model.test.ts; I ran both files: 42 passed.
4	Companion docs	✅	No change needed. components/node-types.md has no icon/color rows and documents rules at play.rules (line 291). default_metadata appears in development/node-type-sequence.md only as a prohibition, and the wrappers only in ADR-086's record of the decision.
5	test:changed passes, quality:fix clean	Not re-verified	Per the review constraints I ran only the two targeted frontend files. No CI checks are reported on the branch.

The two deviations

Play rules (item 7) — correct. properties.play.rules is the stored location, and the top-level read was the one nothing satisfied:

• Create: insert_node_in_tx_no_invariant_dispatch runs normalize_flat_properties_to_namespace before validate_play_rules (crud.rs:335-357), so flat input is already in the play bucket when parsed.
• Update: validate_play_rules(&updated.properties) runs after the normalize, deep-merge and rebucket (crud.rs:1470-1501).
• Engine: load_active_plays, handle_play_created and handle_play_updated parse nodes fetched from the node service, i.e. storage shape. check_schema_change_impact (validation.rs:2014) does the same.
• Seeding (seed_core_plays_if_needed), methodology install and reset_seeded_play_to_default all write through create_node / update_node, which bucket.
• No production code in core, daemon, cli, agent or app-lib hands an in-memory play to activate_play; every such call is a test or a bench, and all were moved to the stored shape.

Deleting the play.rules read as the issue literally says would have failed every persisted play with MissingField("rules").

Query field lookup (item 6) — correct in approach, one regression. Reading through resolveFieldValue matches the wire contract (promoted_fields in nodespace-types/src/convert.rs and TYPED_CORE_FIELDS are keyed by exact type, and promotion is a move, so due_date on a task exists only as dueDate). "Stored name only" would have blanked those columns. Table columns come only from schema.fields (table-view.svelte:43-55) plus content, so no column is lost. Filters are the exception — see the first finding.

Code Review Findings

🟡 Important — metadata filters no longer evaluate

📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:202-207 (doc at :175)

matchesFilter sends every non-content filter through resolveFieldValue, which only knows schema fields. A metadata filter names a node column (created_at, modified_at, node_type, content, title — the backend allowlist at query_service/mod.rs:772), none of which is a schema field, so it now reads properties.<name> and gets null.

I ran a probe against the branch:

Filter (type: 'metadata')	Result on this branch
node_type equals 'invoice' on an invoice node	false
created_at gte '2026-01-01' on a node created 2026-10-01	false
title contains 'hell' on title hello	false
content contains 'hell'	false

On main the removed readFieldValue resolved these through the camelCase top-level key (node_type → nodeType, created_at → createdAt) or the same-named key (title, content); that is from reading the removed code, not from a run on main.

Effect: shouldShowCreatedNode returns false for any saved query carrying a metadata filter, so a node created by the CLI, the agent or another tab never live-appends to that view until the next query load. It fails closed, the same degradation parent/children filters already have, which is why this is not a blocker on severity alone. But the filters are reachable: the agent tool schema offers metadata for exactly these fields (agent/src/local_agent/tools.rs:658) and methodology install handles metadata/node_type filters on views (methodology/install.rs:604). The doc comment at :175 still says metadata filters are supported "fully", and no test covers them, which is how this slipped through.

Fix: give metadata its own branch with an explicit map mirroring the backend allowlist — created_at → node.createdAt, modified_at → node.modifiedAt, node_type → node.nodeType, content → node.content, title → node.title — and add a test per field. That is a declared mapping, not a spelling guess, so it stays within the intent of this PR. Principle: a shim removal must not change behaviour for inputs the shim was legitimately serving; the documented contract and the implementation must agree.

🟡 Important — collection icon / color still read

📁 packages/desktop-app/src/lib/plugins/core-plugins.ts:541-542

collectionNodePlugin.extractMetadata still returns icon: properties.icon and color: properties.color. The acceptance criterion says no remaining reference to collection icon/color, and the type no longer declares them. Both lines are also no-ops, since ...properties follows. Delete them (and consider whether the whole extractMetadata is needed: it reduces to { description, ...properties }, which is properties). Principle: YAGNI, and the issue's own "no remaining reference" criterion.

🟢 Suggestion — user / document type remnants outside the plugin file

📁 packages/desktop-app/src/lib/design/icons/registry.ts:92,130, packages/desktop-app/src/lib/design/components/base-node.svelte:158,179, packages/desktop-app/src/app.css:665,667

The plugin definitions are gone, but the icon registry still registers document and user icons, base-node.svelte carries mock autocomplete entries with those node types, and --node-document / --node-user tokens remain. The issue scopes item 3 to the plugin definitions, so this is not a criterion miss, but it is the same dead code by the same reasoning ("no backend type exists for either"). Worth folding in here rather than leaving it for later. Check noderef.css:304 first: --node-user is also used for a reference style there, so that token may have to stay.

🟢 Suggestion — test the regression class, not just the happy path

📁 packages/desktop-app/src/tests/components/query-node-model.test.ts

The new tests cover custom fields and typed core fields. Add metadata cases (with the fix above) and one case for a subtype row (a type that extends task, whose inherited fields arrive flat in properties because promotion is keyed on the exact type). The subtype case works today by my reading, but it is the next place this lookup will be wrong if TYPED_CORE_FIELDS and promoted_fields drift.

Nitpicks

• Nit: 📁 packages/core/src/playbook/validation.rs:2080-2082 — the comment now wraps awkwardly ("no node_type/relationship_type to / reference a schema through"); reflow it.
• Nit: 📁 packages/desktop-app/src/lib/types/typed-core-fields.ts:89 — typedCoreField matches the wire name as well as the storage name, so a column or filter named dueDate still resolves on a task. Pre-existing and not changed here, but the PR description's "no spelling is guessed" is slightly stronger than the code.

Out of scope, noted for the record

packages/nodespace-types/src/convert.rs:215-219, 264-279 still reads the "legacy dueDate" spelling from stored properties. That is compatibility code for an older shape, but it belongs to the task-defined-once work the issue lists as a non-goal, so it does not count against this PR.

What I verified and what I did not

• Verified by grep across packages/ (excluding target/, node_modules/): every "no remaining reference" claim in the table above.
• Verified by reading: every production caller of parse_rules_from_properties and activate_play; column and filter production for the query table; the three deleted-test groups.
• Ran: table-row.test.ts and query-node-model.test.ts (42 passed); a probe script for the metadata filter cases.
• Not run: the Rust suites, test:changed, quality:fix (review constraints).

[malibio]

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

[malibio]

Review feedback addressed

Addressed

• 🟡 metadata filters no longer evaluate — matchesFilter now resolves a metadata filter through an explicit map of the node columns the backend allows (created_at, modified_at, node_type, content, title). A name outside the list has no value, matching the backend, which rejects it. Tests cover each column and that a metadata filter does not read a schema field.
• 🟡 Collection icon / color still read — removed the collection plugin's extractMetadata altogether. It reduced to properties, which is what the registry returns when a plugin defines none.
• 🟢 Test the regression class — added the metadata cases above and a case for a type extending task, whose inherited field arrives flat in properties.
• Nit: reflowed the comment in playbook/validation.rs.

Skipped

• 🟢 user / document remnants in the icon registry, base-node.svelte mock data and CSS tokens — the issue scopes item 3 to the plugin definitions. --node-user and --node-document are used by reference styles in noderef.css, and the icon registry and mock entries are design-system surface with their own tests, so removing them is a styling change rather than a deletion of unused definitions. Left for a change that owns that surface.
• Nit: typedCoreField also matching the wire name — pre-existing and unchanged by this PR.

Verification: frontend unit suite 299 files / 5526 tests passed; quality:fix clean.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the metadata-filter fix adds new logic to filter evaluation rather than only deleting code.

[malibio]

Pragmatic Code Review — PR #3469 (Re-Review)

• Review Type: Re-Review (scope: 2d12a130..b90a3377)
• Recommendation: APPROVE

Previous Review Summary

	Count
Previous recommendations	6 (2 Important, 2 Suggestions, 2 Nits)
Addressed	4 (both Important, the test Suggestion, the comment-reflow Nit)
Not addressed, skip accepted	2 (user/document remnants; typedCoreField wire-name Nit)
New issues in this commit	1 Suggestion, 1 Nit. Neither blocks.

Both Important findings are fixed correctly. The new commit touches four files and stays within the review's scope.

Status of each previous finding

Finding	Status	Notes
🟡 metadata filters no longer evaluate	Fixed	METADATA_FIELDS holds exactly the five names in the backend allow-list (query_service/mod.rs:772), each mapped to the key it travels under on the wire Node.
🟡 Collection icon / color still read	Fixed	collectionNodePlugin.extractMetadata is removed. No icon/color read remains in core-plugins.ts.
🟢 Test the regression class	Fixed	Five metadata cases and a subtype case added.
Nit: comment reflow in validation.rs	Fixed
🟢 user / document remnants (icons, mock data, CSS tokens)	Skipped, reason sound	The issue scopes item 3 to the plugin definitions. --node-user / --node-document are still used by noderef.css and pinned by node-accent-colors.test.ts, so removing them is a design-system change, not a dead-code deletion.
Nit: typedCoreField also matches the wire name	Skipped, reason sound	Pre-existing and untouched by this PR.

What I checked

Removing collectionNodePlugin.extractMetadata is safe. The only consumer is pluginRegistry.extractNodeMetadata (plugin-registry.ts:448-458), which returns node.properties || {} when a plugin defines none. The removed function returned { description, icon, color, ...properties }, which is properties with up to three extra keys set to undefined. No test or component reads the collection plugin's extractMetadata directly.

The new tests pin the fix and fail on the pre-fix code. I ran the file: 43 passed. By reading the pre-fix lookup (resolveFieldValue reads properties.<name> for a non-schema field), each of the five metadata tests fails on 2d12a130: the four positive cases get null, and the "does not read a schema field" case gets 'open' from properties.status. That matches the probe table in the initial review. The subtype test passes on both commits; it pins existing behaviour rather than the regression, which is what was asked for.

Frontend evaluation against the backend's SQL. I probed the edge cases on this branch:

Case	Frontend	Backend	Direction
title exists, title null / undefined	false	title IS NOT NULL is false	agrees
title contains x, title null	false	NULL comparison is false	agrees
created_at gte '2026-01-01'	string compare (value is not numeric)	text compare on a TEXT column	agrees
created_at gte '2026'	true (falls to string compare because the node side is not numeric)	text compare	agrees
content exists, empty content	false	'' IS NOT NULL is true	fails closed
Same instant in the other spelling (…Z on the wire, …+00:00 stored by to_rfc3339)	differs only when the two values tie to the second		negligible
title equals 'null', title null	true (String(null))	false	fails open, pre-existing in equals
node_type equals 'Invoice', no caseSensitive	true	= is always case-sensitive	fails open, pre-existing default

The first six rows are fine for a function whose job is deciding whether to live-append a node. The last two fail open, but both come from equals and the caseSensitive ?? false default, which predate this PR and apply to property and content filters too.

Code Review Findings

🟢 Suggestion — METADATA_FIELDS lookup reaches Object.prototype

📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:177-189

METADATA_FIELDS[filter.property]?.(node) on a plain object literal also resolves inherited keys. Probed on this branch with type: 'metadata':

property	exists result
constructor	true
toString	true
hasOwnProperty	true
__proto__	throws TypeError: … is not a function

The doc comment says a name outside the list "has no value", and that is not true for these names. Reachability is low: the backend rejects any metadata field outside its allow-list, so a query carrying one errors instead of settling, and shouldShowCreatedNode only evaluates filters when queryState === 'success'. I did not trace whether every path into the gate guarantees that, so I would close it rather than rely on it.

Fix: guard with Object.hasOwn(METADATA_FIELDS, filter.property) or make METADATA_FIELDS a Map. Add constructor to the "unknown column" test, which today only covers status. Principle: an allow-list must reject everything not listed; the documented contract and the implementation should agree.

🟢 Suggestion (pre-existing, author's call) — case-sensitivity default disagrees with the backend

📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:209; packages/core/src/services/query_service/mod.rs:803,810

The frontend defaults caseSensitive to false for equals, in and contains. The backend defaults contains to case-sensitive (unwrap_or(true)), and equals / in are a plain = / IN that ignore the flag. So with no flag set, the frontend matches where the backend does not, and a node can live-append to a view whose query excludes it until the next load. That is the direction the matchesFilter doc comment says to avoid.

This is not introduced here and affects property and content filters equally. I raise it because the new test matches title and content (value: 'acme inv' against title Acme invoice, no caseSensitive) now pins the divergent default for metadata filters. Either align the default in this PR, or change the fixture to a same-case value so the test does not assert behaviour the backend contradicts. Not blocking.

Nitpick

• Nit: 📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:182 — title: (node) => node.title has no trailing comma while the rest of the file uses them. quality:fix reportedly passed, so the formatter accepts it; cosmetic only.

What I verified and what I did not

• Ran: query-node-model.test.ts (43 passed); a scratch probe of the metadata edge cases above (outside the repo; the worktree is clean).
• Verified by reading: the backend allow-list and build_filter_condition; timestamp storage (to_rfc3339 into TEXT columns); every extractMetadata consumer; the full diff of the commit.
• Not run: the pre-fix commit against the new tests (failure on the old code is from reading it plus the initial review's probe); Rust suites; test:changed; quality:fix.

[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 feedback addressed

Addressed

• 🟢 METADATA_FIELDS lookup reaches Object.prototype — the lookup is guarded with Object.hasOwn, so constructor, __proto__ and the like have no value, as the comment says. The "unknown column" test now covers constructor and __proto__.
• 🟢 New test pinned the case-insensitive default — the title and content cases now compare same-case values.
• Nit: trailing comma on the last METADATA_FIELDS entry.

Skipped

• Changing the frontend's caseSensitive default to match the backend. It predates this PR and applies to property and content filters as well, so it is a behaviour change to filter evaluation rather than part of this deletion.

Verification: query-node-model.test.ts 43 passed; quality:fix clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the re-review approved; this commit is a one-line own-key guard plus test fixture changes, with no new logic paths.