[malibio]

Closes #3718

What changed

The task spec Play applies only to a task that links a spec. Moving a task to in_progress is rejected when the task links at least one spec and none of them has spec_status approved (ADR-097 §1). A task that links no spec is an ordinary to-do and starts with nothing set on it (§6).

requires_spec is deleted. No replacement field, no compatibility read.

An extension field named after a declared field is refused on write. custom:status (or org: / plugin: + any declared field name) on a core type is rejected, naming the declared field. A rule reads an extension field by its bare name and the declared field always wins that name, so such a field could be stored and never read.

Node type sequence

• Kind and category: task, flat core type, category unchanged.
• Field removed: requires_spec (boolean, default true). Task now declares seven fields.
• Relationships, structural rules, participation rules: none added or changed.
• Rule changed: the seeded Task spec Play. Rule id is now reject-starting-a-task-whose-spec-is-not-approved; conditions are node.status == 'in_progress', has(node.spec) && size(node.spec) > 0, and !node.spec.exists(s, has(s.spec_status) && s.spec_status == 'approved').
• Layers touched: schema (core_schemas.rs), wire type and typed update (nodespace-types), proto, daemon, app-lib, generated TypeScript (bun run gen:types), the hand-written TypeScript around it, dev-proxy.
• Docs: the task sheet in components/node-types.md is updated in the docs working tree and is pushed to docs main just before bun run merge. ADR-097 is already on docs main.

Other changes

• Rejection message and the two seeded skills (implementing-a-task, breaking-a-plan-into-tasks) name no marker. The next step in the message is to ask the user to approve the spec.
• Ready tasks comment updated; the query itself is unchanged and follows the rule through the permitted filter.
• Test fixtures that set requires_spec: false only to be startable no longer set it. The spec_gated_task test helper is gone: every test task is a plain task.

Tests

• a_task_with_no_spec_starts_with_nothing_set, a_task_with_a_draft_spec_and_no_plan_cannot_start, a_task_with_several_specs_starts_when_one_is_approved, and the Ready queue test (offers a task with no spec, withholds one whose spec is a draft).
• an_extension_field_named_after_a_declared_field_is_rejected: create and update, all three prefixes, and the same name accepted on a type that declares no such field.
• Mutation-checked in both directions: with the old "every task" condition restored, the no-spec and Ready queue tests fail; with a draft spec counted as approved, the draft, several-specs, switched-off and Ready queue tests fail; with the shadow check removed, its test fails.
• bun run test:changed passes (frontend, scripts, browser, types:check, node-types:check, Rust workspace).

Worth a reviewer's attention

• The shadow refusal is on the write path only. A schema edit can still declare a field named custom:status on task (existing tests cover that path), and a write that sets it is now refused. Refusing the name at schema-edit time is a non-goal of the issue.
• The refusal checks core types' buckets, as the ADR states. A user-defined subtype's own bucket is open and is not checked.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3725

• Review Type: Initial Review (no prior reviews or comments on the PR; head 5600ed0fc)
• Method: read the full diff against origin/main, the surrounding code (crud.rs, node_service/mod.rs, playbook/cel.rs, playbook/graph_resolver.rs), ADR-097, the node type sequence, and the uncommitted task sheet in the docs working tree. No tests were run for this review; bun run test:changed was reported as passing on the branch. Findings marked "from reading" were not confirmed by execution.

Summary

The change does what the issue asks and is a clear net improvement: one workflow's marker leaves the core task type, the rule narrows to tasks that link a spec, and every layer of the node type sequence is touched (schema, wire type, typed update, proto, daemon, app-lib, generated TypeScript, hand-written TypeScript, dev-proxy, tests, and the sheet). No compatibility read, no migration, no literal node_type comparison, no lint suppression. The CEL conditions are correct. The shadow refusal sits beside reject_undeclared_core_keys in the one shared write sequence, so it covers the same write paths that check does. One edge of the shadow refusal is worth closing or consciously deferring (finding 1).

Requirements Check

#	Acceptance criterion	Result	Notes
1	Draft spec and no approved spec cannot start; one approved among several can	✅	a_task_with_a_draft_spec_and_no_plan_cannot_start, a_task_with_several_specs_starts_when_one_is_approved
2	A task that links no spec starts with nothing set	✅	a_task_with_no_spec_starts_with_nothing_set; every other fixture now starts plain tasks too
3	requires_spec appears nowhere in packages/	✅	Case-insensitive git grep for requires_spec, requiresSpec, spec_gated, "light lane" over the whole repo: no hits
4	Rejection message and the two seeded skills name no marker	✅	Message asks the user to approve the spec (ADR-097 §8); both skills reworded
5	Ready tasks offers a no-spec task and withholds a draft-spec one	✅	the_seeded_ready_queue_leaves_out_a_task_the_spec_play_would_refuse; query unchanged, comment updated
6	Prefixed name matching a declared field refused, naming the field; non-matching prefixed name accepted	✅	an_extension_field_named_after_a_declared_field_is_rejected (create, update, three prefixes, text negative); the accepted case stays covered by a_namespaced_key_and_a_bookkeeping_key_are_accepted. See findings 1 and 2 for the edges
7	gen:types output committed; types:check and node-types:check pass	✅	Three generated files changed consistently with convert.rs; checks reported passing via test:changed (not re-run here). The sheet is in the docs working tree and must be pushed just before bun run merge
8	Fixtures no longer set requires_spec: false	✅	Removed in core, agent, app-lib and frontend tests; spec_gated_task helper deleted
9	bun run quality:fix	✅ (as reported)	Not re-run in this review
10	bun run test:changed	✅ (as reported)	Not re-run in this review

Code Review Findings

🔴 Critical

None.

🟡 Important

1. A schema can still declare custom:status on task, and every write of that field is now refused
📁 packages/core/src/services/node_service/crud.rs:3371-3401 (with packages/core/src/schema/schema_test.rs:3738-3762, which adds exactly that field to task)

• Problem: reject_shadowed_extension_fields runs after defaults are applied (rebucket_and_validate_behaviors then the check, crud.rs:3254-3263). update_schema add_fields still accepts custom:status on task. From reading: if that added field carries a default, the default lands in the task bucket on every create and the new check refuses it, so every task create fails with "'custom:status' cannot be set" for a key the caller never sent. If it is required with no default, the type cannot be written at all. Without either, the schema simply declares a field no write can set. Before this PR that schema edit was harmless. The PR description flags the schema path as out of scope, and the issue lists it as a non-goal "if that turns out to be a separate path"; it is a separate path, but the write-side refusal is what turns it from odd into broken.
• Also: the check compares the stripped name with raw declared names, not with the names a rule reads. So org:custom:estimate beside a declared custom:estimate is refused although the two read as different names (custom:estimate and estimate), and org:estimate beside a declared custom:estimate is accepted although both read as estimate. Rare, but it shows the invariant is "no two keys with the same read name", which is easiest to hold at declaration time.
• Recommendation: refuse, in schema validation, an added or renamed field whose bare name matches another declared field in the chain, in this PR (it is a small check next to the existing reserved-name warning in schema/mod.rs). If it is deferred, record that decision and at least add a test that pins what happens to a task create when the schema declares custom:status with a default, so the failure mode is known rather than discovered.
• Principle: make invalid states unrepresentable at the earliest boundary; two validators (schema edit, node write) must agree on what is legal, or one accepts what the other can never satisfy.

🟢 Suggestions

2. The refusal skips a user subtype's own bucket, where the field is just as unreadable
📁 packages/core/src/services/node_service/crud.rs:3379-3382

• Problem: the loop skips non-core scopes. A flat custom:status on a node of a user subtype of task lands in the subtype's own bucket and is accepted, yet node_to_cel_value_at_scope takes unprefixed keys across the whole chain before any prefixed one (cel.rs:543-561), so it can never be read either. The neighbouring reject_derived_attribute_keys refuses its names "in every bucket of the chain, a user subtype's open one included" for the same reason. ADR-097 §6 says "on a core type", so this matches the letter of the ADR, and the PR description calls it out.
• Recommendation: drop the core-only filter so the check covers every bucket in the chain (owners already spans the chain), and add one case to the test using the Chore extends task fixture. If the narrower scope is intended, say why in the doc comment, since the stated reason ("could never be read") applies to both.
• Principle: a rule should follow its reason; least surprise for the user whose field is silently dead.

3. Two neighbouring read-name collisions the check does not reach (pre-existing, not introduced here; from reading)
📁 packages/core/src/playbook/cel.rs:564-590, packages/core/src/playbook/graph_resolver.rs:778-785

• A prefixed key stored beside the node's own bucket ({"task": {"status": "open"}, "custom:status": "x"}; normalize_flat_properties_to_namespace returns such a map as-is, crud.rs:3122-3126) is inserted by the flat branch with map.insert, which overwrites the bucketed status. There the extension field wins, the opposite of the premise in the new doc comment ("a declared field always wins that name"). reject_undeclared_core_keys has the same blind spot for keys beside the bucket.
• An extension field named after a relationship (custom:spec on a task) reads as the property node.spec, and enrich_context skips relationship resolution when a two-segment path is a property. That could mask the relationship the Task spec rule reads.
• Recommendation: neither blocks this PR. Confirm each with a short test; if they hold, the first is best fixed by refusing a non-bucket, non-_ key beside an own bucket in ensure_properties_are_buckets, and the second by adding relationship names to the names an extension field may not take.
• Principle: defence at the boundary should cover every location a value can be stored in, not only the common one.

4. Test coverage of the shadow refusal
📁 packages/core/tests/it/core_type_registry_test.rs:447-489

• Covers create, update, all three prefixes and a negative. Not covered: an inherited declared field (a subtype node whose prefixed key names an ancestor's field, where the message should name the ancestor as owner), and an explicitly bucketed write ({"task": {"custom:status": "x"}}). One case each would pin the owners.get(bare) chain lookup.

Nitpicks

• Nit: 📁 packages/core/src/playbook/core_plays.rs:807-810: !message.contains("--property") is a loose stand-in for "names no marker". Asserting the message does not contain node update would say it more directly.
• Nit: 📁 packages/core/src/playbook/core_plays.rs:405-408: the third condition is safe without its own has(node.spec) only because conditions short-circuit in order (cel.rs:1097-1130) and a runtime error reads as "not met". The doc comment mentions the spec_status guard; one clause on the ordering dependency would help the next editor of the Play.
• Nit: 📁 packages/core/src/services/query_service/core_queries.rs:121-122: the reflowed comment leaves a short line ("however its checklist looks. The conditions"); rewrap.
• Nit: docs sheet (components/node-types.md, working tree): "refused on every write" is true of every path through rebucket_and_validate_with; the trusted import calls only rebucket_and_validate_behaviors and skips it, as it skips the closed-bucket check.

What is done well

• CEL conditions match ADR-097 §1 and §2: guarded on the claim only, has()-guarded spec_status, any-one-approved semantics, and a no-spec task falls out at the second condition.
• extension_field_bare_name replaces the boolean helper and is_extension_field_name delegates to it, so there is one definition of the prefix list.
• Removal is complete and symmetric across layers; Boolean promoted shape is still used by other types, so nothing is left dead.
• Tests assert the user-visible behaviour, and the PR reports mutation checks in both directions.

Recommendation

APPROVE, with finding 1 recommended before merge (or an explicit decision to defer it). Findings 2 to 4 and the nits are optional. Reminder: push the task sheet to docs main just before bun run merge.

[malibio]

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

[malibio]

Review addressed

Addressed

• Finding 1 (🟡), the regression part. Confirmed by test: with the schema declaring custom:status with a default, every task create was refused. The refusal now skips a key the schema itself declares, so a declared field is written like any other. New test an_extension_field_the_schema_declares_is_still_written, mutation-checked (it fails with the exemption removed).
• Finding 1, the name comparison. org:custom:x reads as custom:x, which a declared custom:x (read as x) does not shadow. A bare name that is itself prefixed is no longer compared.
• Finding 2 (🟢). The core-only filter is dropped: the name is refused in every bucket of the chain, a user subtype's open one included, as reject_derived_attribute_keys does. New test a_subtype_refuses_an_extension_field_named_after_any_field_in_its_chain covers an inherited field and one of the subtype's own, and checks the message names the declaring type.
• Finding 4 (🟢). Added the explicitly bucketed write ({"task": {"custom:status": …}}) and the inherited-field case.
• Nits. The marker assertion checks for node update; the rule's doc comment says why the last condition reads node.spec unguarded; the Ready tasks comment is rewrapped; the docs sheet says "refused by a validated write" (the trusted import skips this check as it skips the closed-bucket one).

Not done

• Finding 1, refusing the name at schema-edit time. The issue lists it as a non-goal when it is a separate path, and it is: create_schema, add_fields, rename_fields and the extends re-target each validate field names, and existing tests of friendly-name disambiguation are built on custom:status beside status. With the exemption above, a schema that declares such a field behaves as it did before this PR (stored and written, and not readable by a rule).
• Finding 3 (🟢), both parts. Pre-existing and outside this change: a prefixed key stored beside the node's own bucket, and an extension field named after a relationship (custom:spec). Neither was confirmed by a test here.

Verification: bun run quality:fix clean, bun run test:changed passes (frontend, scripts, browser, types:check, node-types:check, Rust workspace).

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: No critical finding. The changes narrow one check by two conditions and widen it to the buckets the reviewer named, each pinned by a new test, one of them mutation-checked. The rest is comments and a test assertion.