[malibio]

Closes #3441

What changed

task is a flat concrete core type (ADR-086 §4). This PR gives it one definition and one write path, and makes project share its priority scale.

One definition. packages/core/src/models/task_node.rs, its test file and models/priority.rs are deleted. TaskNode, TaskNodeUpdate, TaskStatus and the shared Priority enum live only in nodespace-types; core re-exports TaskNodeUpdate, TaskStatus and Priority. TaskPriority is gone: Priority (with its rank and NODE_TYPES) moved to nodespace-types and backs both TaskNode.priority and ProjectNode.priority.

One write path. NodeService::update_task_node now lowers the typed update into the generic update pipeline, the same way update_person_node and update_project_node do, and returns the stored Node. Removed with it: update_task_node_in_tx, validate_task_status, get_task_node, and the store's update_task_node_with_version_check_in_tx / node_to_task_node. Status and priority are validated by the pipeline's enum check.

The response is the stored node. The daemon's update_task_node answers with node_response(node) instead of rebuilding a node from core's copy, so lifecycle_status and extension properties survive.

The typed update carries the task schema's fields only.

Field	Change
priority	Uses the clearable deserializer: absent = unchanged, null = clear
content	Removed from TaskNodeUpdate, the proto request and the TypeScript type
properties	Removed from UpdateTaskNodeRequest (the daemon ignored it)

For properties the issue asked to choose between dropping it and honouring it. I dropped it: extension fields go through the generic update, which is the one write path they already have. Naming content or properties in a typed task update is refused rather than dropped: TaskNodeUpdate is deny_unknown_fields (the Tauri command), and the dev-proxy's PATCH /api/tasks/:id returns 400.

No fields, relationships or structural rules were added or changed. No migration.

Behaviour to know about

A cleared field is now stored as null in the task bucket instead of having its key removed. This is what the shared pipeline already does for person, project, the CLI and the agent, and every reader treats null as absent. The new daemon test pins the stored shape.

Tests

• nodespace-types: priority: null clears, absent leaves unchanged, content / properties are rejected, the patch carries only named fields; project priority is the shared enum.
• Core service (typed_update_tests): writes fields and leaves others; null clears priority and dates; an archived task with a custom: field keeps both in the response; a priority outside the vocabulary is rejected; stale version conflicts; another type and an empty update are refused.
• Core: the existing title_template and invariant-rule / event tests for update_task_node now run against the shared pipeline.
• Daemon: the RPC response is the stored node (archived, extension field kept, priority cleared); an absent priority is kept.
• Frontend: the patch builder carries schema fields only; unknown body keys are named.

Run locally: nodespace-types + nodespace-core + nodespace-daemon under nextest (2982 passed), bun run test (301 files, 5559 tests), bun run quality:fix clean.

Docs

components/node-types.md task and project sheets: NodeSpaceAI/nodespace-docs@c19fec0

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3470 (issue #3441)

Review Type: Initial Review (no prior reviews or comments on the PR; head 0e09abe1)

Summary

The change does what the issue asks. Core's TaskNode / TaskNodeUpdate / TaskStatus / Priority copies are gone, update_task_node is a four-line lowering into update_typed_fields like person and project, and the daemon answers with node_response(node). About 1,700 lines net are deleted, including a second version-checked store write and a second title/invariant/event pipeline.

One leftover on the frontend contradicts the new contract and should be fixed before merge (finding 1). Everything else is optional.

What I checked beyond the diff:

• update_typed_fields and update_with_version_check_returning_node_in_tx (crud.rs): the flat patch is bucketed, null passes check_field_value, the title is recomputed on any properties change, the event is buffered and invariants dispatch in the same transaction.
• No Rust reference to the removed symbols remains (TaskPriority, get_task_node, validate_task_status, node_to_task_node, TaskNodeUpdate::new, core's TaskNode).
• bun run scripts/check-node-type-literals.ts passes.
• cargo check -p nodespace-app-lib --tests compiles (that crate was not in the PR's listed local runs).
• I did not run any test suite.

Requirements Check

#	Acceptance criterion	Status	Notes
1	No task type defined in core/src/models/task_node.rs; core imports the nodespace-types ones	✅	File, its test file and models/priority.rs deleted. models/mod.rs re-exports TaskNodeUpdate, TaskStatus, Priority from nodespace-types.
2	Typed task update goes through the generic pipeline: title_template, invariants and events fire (test)	✅	update_task_node → update_typed_fields → update_node. Covered by the title_template tests in node_service/mod.rs and the invariant_task_update_* / reactive_task_update_* tests in playbook_invariant_integration_test.rs, now running against the shared pipeline.
3	Response keeps lifecycle_status and extension properties (archived task with a custom: field)	✅	Core: task_update_response_keeps_lifecycle_status_and_extension_fields. Daemon: update_task_node_response_is_the_stored_node.
4	priority: null clears; absent leaves unchanged (test)	✅	Tested at the type, core service, daemon RPC and Tauri contract layers.
5	TaskNodeUpdate has no content	✅ in Rust, proto and the TS type; ❌ one frontend leftover	See finding 1.
6	project.priority uses the shared priority enum	✅	ProjectNode.priority: Option<Priority>, ProjectNodeUpdate.priority: Option<Option<Priority>>; daemon and Tauri encode through optional_priority_clear / priority_clear.
7	Follows the node type sequence; node-types.md task and project sheets updated	✅	Both sheets in nodespace-docs describe the new typed update. No field, relationship or rule changed. "TypeScript regenerated" does not apply yet: no generator exists (#3440), and the hand-written TaskNodeUpdate was updated to match.
8	bun run test:changed passes; bun run quality:fix clean	Not verified	The PR lists nextest for three crates plus bun run test, not test:changed. nodespace-app-lib's contract test and the e2e adapter contract were not in that list; the merge gate runs them.

Code Review Findings

1. 🟡 Important: the frontend typed write path still accepts content for task

📁 packages/desktop-app/src/lib/services/shared-node-store.svelte.ts:810-823

 * typed update rejects), plus `content` for task, whose typed update also
 * carries content.
...
const TYPED_UPDATE_EXTRA_KEYS: Partial<Record<TypedNodeType, readonly string[]>> = {
  task: ['content']
};

Problem. typedUpdateKeys('task') still lets content through updateTypedNode(). That method applies the value optimistically, stages it in pendingTypedFields, and sends it to backendAdapter.updateTaskNode. The backend now refuses it: deny_unknown_fields in the Tauri command, a 400 from the dev-proxy. The result would be a local content change that never persists, plus a write-failure notification.

No typed caller reaches this today, because the TaskNodeUpdate interface has no content and updateNode() splits on typedCoreKeys. But updateTypedNode takes Record<string, unknown>, so the compiler does not protect it. The PR removed the test that exercised this (should update task content) and kept the mechanism.

Recommendation. Delete TYPED_UPDATE_EXTRA_KEYS, make typedUpdateKeys return writableTypedCoreKeys(nodeType) (or inline it), and fix the doc comment. Optionally add a test that updateTaskNode(id, { content } as never, …) is dropped with the existing "no typed fields" warning and never reaches the adapter.

Principle. CLAUDE.md: delete old patterns when replaced. ADR-086 §4: one typed write path carrying the schema's fields only.

2. 🟢 Suggestion: unknown keys are refused for task and query but silently dropped for person and project

📁 packages/nodespace-types/src/person.rs:31, packages/nodespace-types/src/project.rs:45, packages/dev-tools/src/dev-proxy.ts:571-620

TaskNodeUpdate and QueryNodeUpdate are deny_unknown_fields; PersonNodeUpdate and ProjectNodeUpdate are not. The dev-proxy's unknown-key guard (unknownTaskNodeUpdateKeys) exists only for the task route. A content or properties key in a typed person or project update is still a silently dropped write, which is the failure this PR removes for task.

Recommendation. Add deny_unknown_fields to the other two update structs and generalize the dev-proxy helper to take a field list. This is the same-shaped sibling of the fix here, so it fits in this PR; a separate change is also fine.

Principle. Consistency across the four typed updates; fail loudly instead of dropping a write.

3. 🟢 Suggestion: document that a cleared field is stored as null

📁 packages/daemon/src/services/node_service.rs:4613-4621 (the test that pins the shape); nodespace-docs components/node-types.md §1.2

A field cleared from the UI used to have its key removed from the task bucket; it is now stored as null. The PR description says so, and the readers I checked treat null as absent:

• interpolate_title_template_with_schema
• check_field_value
• QueryService::compare_priority_values
• the Exists filter (IS NOT NULL over json_extract)
• priority_prop
• the frontend's storageNodeToApiFields

One thing I did not verify: a user-written CEL condition has(node.priority) probably now evaluates true for a cleared field. The seeded Jira rules already guard with !has(x) || x == null, which suggests it does.

Recommendation. Add one sentence to node-types.md §1.2 (or data-layer.md): a cleared field is stored as null, and readers treat null as absent. Rule authors then know to write the == null guard.

Principle. A stored shape is a contract, and the docs repo is where it is stated.

Nitpicks

• Nit: 📁 packages/core/src/markdown/mod.rs:1920-1931. format_task_checkbox reads properties.task.status by hand. It is correct: both callers gate on CoreNodeType::Task.is_exactly, and stored nodes are always bucketed. A small task_status(&Node) -> TaskStatus in core, shared with any future reader, would keep the bucket layout in one place (ADR-086 §1 "Storage mapping: functions").
• Nit: 📁 packages/desktop-app/src/lib/services/adapter-core.ts:307-313. satisfies ReadonlyArray<keyof TaskNodeUpdate> checks that each listed name is a field, not that every field is listed. A field added to TaskNodeUpdate and missed here would get a 400 from the dev-proxy only. A Record<keyof TaskNodeUpdate, true> would make the list exhaustive at compile time.
• Nit: 📁 packages/nodespace-types/src/task.rs:18-24, packages/nodespace-types/src/priority.rs:29-35. FromStr (now Infallible) and from_value are two spellings of one conversion. The only remaining .parse() callers are the drift tests in core_schemas.rs; switching them to from_value would let both FromStr impls go.
• Nit: No test drives the refusal of content / properties at the Tauri command or the dev-proxy route. The struct-level and helper-level tests cover the logic, so this is a small gap.

Security / Performance

• Security: nothing new. The change narrows the typed input surface, and enum values are now validated by the schema check for priority as well as status.
• Performance: a typed task update now does a few more primary-key reads per write: the type pre-read, the seed-stamp lookup, the collection-collision pre-read and the unique-field detection. This is the same cost person and project already pay and is negligible on a local SQLite store.

Recommendation

APPROVE, with finding 1 fixed before merge. It is a three-line deletion and the only place the change contradicts its own contract. Findings 2 and 3 and the nits are optional.

[malibio]

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

[malibio]

Address review

Addressed

1. 🟡 Frontend typed write path still accepted content for task. TYPED_UPDATE_EXTRA_KEYS and typedUpdateKeys are deleted; updateTypedNode() now allows exactly writableTypedCoreKeys(nodeType).
2. 🟢 Unknown keys were refused for task and query but dropped for person and project. PersonNodeUpdate and ProjectNodeUpdate are deny_unknown_fields (with a test each), and the dev-proxy refuses unknown keys on the person and project routes through one shared helper, unknownTypedUpdateKeys(nodeType, body).
3. 🟢 A cleared field is stored as null. Documented in node-types.md §1.2 (NodeSpaceAI/nodespace-docs@ebd9823). I left out the CEL has() guidance: I could not verify how has() treats a null property, so the doc states only the reader behaviour that was checked.
4. Nit: the update field lists are exhaustive. Each is satisfies Record<keyof …Update, true>, so a field added to an update interface and missed in the list is a compile error.
5. Nit: one string conversion. The FromStr impls on TaskStatus and Priority are removed; from_value is the only spelling.

The helper's new test caught a bug in my first version of it: key in fields matched inherited keys such as toString. It now uses Object.hasOwn.

Skipped

• Nit: a shared task_status(&Node) reader in core. format_task_checkbox is the only reader of the bucket today; worth adding when there is a second.
• Nit: a transport-level test for the refusal of content / properties. The Tauri command takes an already-deserialized TaskNodeUpdate, so the refusal is the struct's, which is tested; the dev-proxy's is the helper's, which is tested.

Verification: bun run test:changed passed (frontend, scripts, browser, Rust workspace); bun run quality:fix clean.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: Finding 2 added behaviour the first review did not see: person and project typed updates now refuse unknown keys in the Tauri command and the dev-proxy, across Rust, the dev-proxy and the shared helper. The helper also had a bug in its first version. The changes are small, but they are new refusals on two more write routes, so a second look at the new commits is cheaper than a failed merge gate.

[malibio]

Pragmatic Code Review: PR #3470 (issue #3441)

Review Type: Re-Review (commits a5a610d0 and 909abf86, after 0e09abe1)

Previous Review Summary

	Count
Previous recommendations	7 (1 Important, 2 Suggestions, 4 nits)
Addressed	5
Not addressed (skipped with a reason)	2 nits; both reasons hold
New issues found	0 blocking; 2 optional suggestions

#	Previous finding	Status
1	🟡 Frontend typed write path still accepted content for task	✅ Fixed. TYPED_UPDATE_EXTRA_KEYS and typedUpdateKeys are deleted; updateTypedNode() allows writableTypedCoreKeys(nodeType) only. No reference to either name remains.
2	🟢 Unknown keys dropped for person and project	✅ Fixed. Both structs are deny_unknown_fields, each with a test; the dev-proxy refuses on all three per-field routes through one helper.
3	🟢 Document that a cleared field is stored as null	✅ Fixed in node-types.md §1.2 (nodespace-docs ebd9823). Leaving out the unverified CEL has() guidance is the right call.
4	Nit: field list not exhaustive	✅ Fixed. satisfies Record<keyof …Update, true> fails to compile on a missing or extra key.
5	Nit: FromStr and from_value duplicate each other	✅ Fixed. Both impls removed; the two callers use from_value.
6	Nit: shared task_status(&Node) reader	Skipped. Reason holds: format_task_checkbox is the only reader, so a helper now would be speculative (YAGNI).
7	Nit: transport-level refusal test	Skipped. Reason holds: the Tauri commands take the struct as a serde-deserialized argument, so the struct tests cover the refusal; the dev-proxy wiring is three one-line calls to a tested helper.

What I verified

• Every producer of a person or project update sends schema keys only.
  • Store: updateTypedNode() filters to writableTypedCoreKeys, and sendPendingTypedFields() sends exactly those staged fields.
  • Callers: person-schema-form.svelte keys by keyof PersonNodeUpdate; updateNode()'s typed/generic split sends only typedCoreKeys to the typed path.
  • HttpAdapter: the body is { ...update, version }, and the helper exempts version.
  • TauriAdapter: version is a separate command argument, so update holds fields only.
  • Rust: the only deserialization sites are the two Tauri command arguments. The daemon and the contract test build the structs from fields.
  • Tests and e2e: adapter-contract.e2e.ts sends lastName / email and status / startDate; no test or fixture passes content or properties to a typed update.
• The two key lists agree today. TYPED_UPDATE_FIELDS (dev-proxy refusal) and the writable keys of TYPED_CORE_FIELDS (store allowlist) hold the same keys for task, person and project, and both match the Rust structs' camelCase names.
• No FromStr caller is left. cargo check --tests passes for nodespace-types, nodespace-core, nodespace-daemon and nodespace-app-lib.
• The frontend typechecks. svelte-check reports 0 errors over 2,457 files. It does not cover packages/dev-tools, so the dev-proxy edits were checked by reading only.
• No caller relied on content in the typed task path. The only production callers of updateTaskNode are in task-schema-form.svelte, and they pass status, priority and dates.
• The query route still refuses unknown keys, at the daemon, where QueryNodeUpdate is decoded with deny_unknown_fields. "Every route" in the commit message is accurate.
• I ran no test suite.

Code Review Findings

No critical or important findings.

1. 🟢 Suggestion: two lists now name the same writable keys, with nothing holding them together

📁 packages/desktop-app/src/lib/services/adapter-core.ts:311-330, packages/desktop-app/src/lib/types/typed-core-fields.ts:35-53

The store decides what to send from TYPED_CORE_FIELDS; the dev-proxy decides what to refuse from the new TYPED_UPDATE_FIELDS. The new list is tied to the update interfaces at compile time; the older one is not. If a field is added to TYPED_CORE_FIELDS and not to the interface, the store sends a key that the dev-proxy and the Tauri command both refuse, and the user sees a write failure.

Recommendation. Add one assertion to adapter-core.test.ts: for task, person and project, writableTypedCoreKeys(type) returns the same keys as the helper accepts. That needs TYPED_UPDATE_FIELDS exported or a small accessor. Not needed for merge.

Principle. DRY: when a fact has to live in two places, a test should fail when they disagree.

2. 🟢 Suggestion: the person and project sheets don't state the refusal

📁 nodespace-docs components/node-types.md:155 (task), :168 (project), person sheet

The task sheet says a typed update naming content or an extension field is refused. The project sheet's "Typed update" line covers tri-state only, and the person sheet has no "Typed update" line. Since a5a610d0, the same refusal applies to both.

Recommendation. Move the sentence to §1.2 as a rule for every typed update, or repeat it on the two sheets.

Principle. node-types.md is the one per-type reference and changes with the behaviour.

Nitpicks

• Nit: 📁 packages/dev-tools/src/dev-proxy.ts:249. Parameters<typeof unknownTypedUpdateKeys>[0] works, but an exported PerFieldTypedUpdateType alias in adapter-core.ts would read better than deriving the type from the function's parameter list.

Security / Performance

• Security: the change narrows input on two more routes. Object.hasOwn closes the inherited-key hole the first version had (toString, constructor, __proto__ are now reported as unknown), and the test pins it.
• Performance: no change.

Recommendation

APPROVE. The one Important finding is fixed, the two Suggestions and two of the nits are addressed, and the reasons for the two skipped nits hold. The new refusals on the person and project routes break no existing producer. Both suggestions above are optional and need no further review.

[malibio]

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

[malibio]

Address review (re-review)

Addressed: all three items; nothing skipped.

1. 🟢 Two lists named the same writable keys with nothing holding them together. adapter-core.test.ts now asserts, for task, person and project, that the typed update's fields are exactly writableTypedCoreKeys(type) (the keys the store sends). typedUpdateFieldNames exposes the list for it.
2. 🟢 The person and project sheets didn't state the refusal. Both sheets in node-types.md now say the typed update carries the type's fields only and refuses content and extension fields (NodeSpaceAI/nodespace-docs@e06d63f).
3. Nit: the dev-proxy takes the exported PerFieldTypedUpdateType instead of deriving the type from the helper's parameter list.

Verification: bun run test:changed passed (frontend, scripts, browser, Rust workspace); bun run quality:fix clean; the dev-proxy bundles.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: One new test, one exported type alias and accessor with no logic change, and a docs sentence. No behaviour changed since the re-review's APPROVE.