[malibio]

Implements ADR-100 §3 to §5 for schemas.

What changes

• Describer (schema::description::describe_schema): the chat opens on the type's purpose, whether it is built in (closed) or the user's own (open), what it extends, then each field with type, allowed values, required, default and the type that declares it. Built from stored data, no model call.
• Skill: "Understanding and Changing a Type", procedure lane, pinned by the chat (not reached by search). It says which case the type is in before offering a change, guides a well-formed field, and tells the user what a change affects before writing.
• What names a field (schema::field_references): saved queries (filters, sorting, board grouping), plays that trigger on the type (trigger, conditions, actions) and title templates. A schema get_node carries it as field_references, with what was searched.
• Guard: update_schema refuses a rename or removal that something names, with the list in its first sentence, until it is repeated with force. This holds whichever skill led the turn.
• UI: the type's page offers Ask beside its view controls.
• Options: Explain this type, Add a field, Change a field, Add a value to a list, Remove a field.

Verification

• bun run test:changed passes (frontend, scripts, browser, types:check, node-types:check, Rust workspace); clippy clean on core, agent and daemon.
• Confusion matrix re-recorded on the locked embedding model; 40 live retrieval guards pass. "define a new type for vendors" now ranks Schema Creation second in raw retrieval (the new procedure skill is 0.04 above), but it is still routed to Schema Creation because a procedure skill never leads a request.
• Live, on the locked E4B model, machine calm and 0 Metal out-of-memory lines (live_type_chat_options, ignored by default):
  • the first reply for each of the five options writes nothing (15/15 across 3 reps), and asks or explains in words;
  • an agreed rename is refused with the saved query named and the field unchanged (3/3).
• Prompt dump captured: the skill text, field_references and the type are in what the model was sent.

Known gap

A bare one-shot "Rename state to status" (free text, not a picked option) is not led by the pinned skill (ADR-090 §5 leaves a free-text request to retrieval). On the locked model the first reply does not read the type and says nothing names the field. Nothing is written, and the refusal above names the saved query as soon as the user agrees. A fix that reached the first reply would be a change to how a pinned chat leads a free-text turn, which touches play and view chats too, so it is not made here.

Notes

• Added find_field_references to the extends-chain-blindness allowlist: it reads own fields on purpose, since only a type's own fields can be renamed or removed there.
• Docs: components/seeded-nodes.md updated (pushed to nodespace-docs).

🤖 Generated with Claude Code

[malibio]

Pragmatic code review: PR 3782

Review type: INITIAL (no prior reviews). Static read of the diff; no builds or tests run.

Requirements check (issue 3742)

• ✅ Type page offers the chat (Ask on the default mode of query-node-viewer.svelte, gated on the schema existing); opens on a description, then options with "Explain this type" first.
• ✅ Explaining writes nothing (skill + live test; not re-run here).
• 🟡 Built-in type says what cannot change and offers only prefixed fields/added values, but the describer mislabels the user own prefixed fields on a built-in type as built in (finding 1).
• ✅ Saved queries, plays and title templates that name a field are listed before a rename or removal (schema read carries field_references with searched; update_schema refuses until force).
• ✅ Shadowing refusal is core existing check; skill states it accurately.
• ✅ Skill is pinned (Procedure role, participant table), not found by search.
• ✅ Prompt dump and live results reported in PR body (ignored tests, not re-run); seed count, goldens, retrieval table, chain-blindness allowlist updated.

AGENTS.md checks (literal node_type comparisons, lifecycle reads, compat code, issue numbers, console logging): clean.

Findings

🟡 Important

1. User own prefixed field on a built-in type is described as "Built in: it cannot be changed or removed."
   📁 packages/core/src/schema/description.rs:66-70, packages/daemon/src/services/bound_chat.rs (describe_schema_node built_in set)
   fixed is protection != User || owner is built in. The owner of custom:effort on task is task (core), so a field the user added is marked fixed. Core allows removing it (can_delete_field = User protection) and renaming between prefixed names (see test_update_schema_core_type_rename_cannot_drop_namespace_prefix). The opening message and then the skill would tell the user they cannot do what they can. Use protection alone for the per-field note (or exclude extension fields) and add a test with custom:effort on task; today only priority is asserted.

2. The tool guard runs before core validations, so it asks for force on changes core will refuse anyway.
   📁 packages/agent/src/local_agent/tools.rs (field_change_refusal call before handle_update_schema); packages/core/src/schema/mod.rs:2893-2925, 3086-3092
   Removing/renaming a Core-protected field (e.g. task.status, named by saved queries) returns "ask whether to go ahead ... force=true", then after the user agrees core rejects it as immutable. That contradicts "explained, not attempted" at the tool layer, and core own comment states the principle ("force cannot make the name legal, so the caller is not asked for it first"). Skip non-User-protected fields in the guard, or run it after core protection checks (best: put it beside check_schema_change_impact in core).

3. One force now means two things.
   📁 tools.rs (def_update_schema, guard), packages/core/src/schema/mod.rs:3149-3180
   Core params.force already bypasses the active-play impact guard (ADR-079) but was absent from the tool definition, so the local agent could not set it. Now the same boolean, described as agreeing to the field-reference list, also silently waives the play-impact refusal the user was never shown (on task a core play always triggers). Compose both lists in one refusal, or give the new parameter its own name. Also: force is advertised on every update_schema from any skill, so the model can self-confirm; fine as an advisory guard but say so. The check living only in the agent path means CLI/other callers get no warning; moving it into core next to the play-impact check would give one enforced check and one force (and fix 2).

🟢 Suggestions

4. Guard fails open silently (tools.rs, .ok()? in field_change_refusal). A lookup error (one broken chain aborts the scan via ?) lets the rename through with no log. Log a warn like the read path; consider failing closed.
5. Scan cost (field_references.rs:76-84): per schema get_node and per guarded write it runs get_all_schemas + resolve_type_chain per schema + loads all queries and plays. Fine today; one reverse walk of extends edges would avoid N chain resolutions.
6. Refusal list uncapped while the read is capped at 30 (FIELD_REFERENCES_SHOWN); cap and say "and N more".
7. Skill wording (understanding-a-type.md): "a template that names the field blocks it" - I found no core check that blocks the write; only this guard holds it, and force clears it. Say "is reported and held until confirmed". Other claims (built-in closed, prefixed fields, extensible values, removal leaves stored values, custom:status beside status refused) match core.
8. searched scope: discloses the related-filter gap; also name that play rules whose selector is not a target_type are not matched, and that whole-word text matching can flag a mention in a message string (safe-direction false positive).
9. Live test (live_type_chat_options.rs): sensible and ignored. The first test asserts store outcomes (robust). The rename test needs 3/3 reps where the model both calls update_schema and names "Open tickets" in prose, which is nondeterministic by construction; the deterministic half is already covered by the tools.rs unit tests, so assert the tool result and only log the prose, or accept a threshold. OPENING duplicates describer output and can drift.

Nitpicks

• Nit: tools.rs ~5075-5096: the comment says a schema node "emits no nodeType", but the new is_schema_read depends on nodeType being present. The comment is stale; fix or delete.
• Nit: description.rs standing() uses is_core while fixed uses protection/owner; one source of truth for "built in" avoids finding-1 drift.

Notes on the specific concerns

• Play/query chats: no regression seen; describer boxed-future change is mechanical and tests converted correctly.
• Frontend Ask: correct; error text distinguishes view vs type; both cases tested.
• Known gap (bare free-text rename): agree with the reasoning; the tool refusal makes the write safe.

Recommendation: COMMENT (REQUEST CHANGES on findings 1 and 2; 3-9 discretionary)

Well-scoped and tested; meets the acceptance criteria. I would approve once 1 (wrong model-facing statement) and 2 (guard asks for a force core never honors) are fixed or justified. Posted as a comment because GitHub blocks self-review.

[malibio]

Automated pragmatic-code-review: COMMENT, requesting changes on findings 1 and 2 (posted as a comment, GitHub blocks self-review). Full report is in the preceding PR comment.

[malibio]

Address review: summary

Addressed

• 🟡 1: a user's prefixed field on a built-in type (custom:effort on task) is no longer described as "Built in". The note now applies to protected fields and to unprefixed fields a built-in type declares. Test added.
• 🟡 2: the update_schema guard no longer holds a built-in field. The schema write refuses it whatever is agreed, so the user is not asked to agree to it first. Test added (task.status, named by a saved query, gets the schema write's own refusal).
• 🟡 3: the guard's agreement is its own confirmed parameter, consumed by the guard and not passed on. The schema write's force (the play-impact waiver) is not in the tool's parameters, so the model is not offered it by this change.
• 🟢 4: the guard logs a warning when it cannot read the schema or look a field up, then does not hold (an unreadable schema should not block a change the write itself validates).
• 🟢 6: the refusal's list is capped at 10, with the rest counted.
• 🟢 8: searched says that a rule that selects by saved query is not matched and that a mention of the name inside a message is counted.
• Nit: the stale "schema emits no nodeType" comment is corrected.

Skipped

• Moving the check into core, beside the play-impact check (🟡 3, "best"): wider than this issue, and it would change update_schema for CLI and other callers. Worth doing if those callers need the warning; the guard is advisory for now, as the reviewer notes.
• 🟢 5, scan cost: premature at current type counts; revisit if a schema read is measured slow.
• 🟢 7, skill wording "a template that names the field blocks it": core's own test (test_update_schema_remove_field_referenced_by_existing_template_rejected) asserts a removal is rejected while a title template names the field, so the claim holds.
• 🟢 9, live rename test: it is deterministic now (the failure notice carries the list), 3/3 on the locked model. OPENING duplicating the describer's output is kept, so the live test needs no daemon.
• Nit, standing() vs fixed using different sources: they answer different questions (is the type built in, is this field fixed).

Checks: bun run test:changed passes (frontend, scripts, browser, types:check, node-types:check, Rust workspace); clippy clean.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the guard's behavior and a tool parameter changed (force → confirmed, built-in fields skipped, list capped), which is the part the review was most concerned with.

[malibio]

Pragmatic code review: PR 3782 (re-review of 26e40fc)

Review type: Re-Review. Static read of git diff 5e5b5540a..26e40fcc1; no builds or tests run.

Previous review summary

• Previous recommendations: 9 findings + 2 nits
• Addressed: 1, 2 (with a caveat, see A), 3, 4, 6, 8, stale-comment nit
• Skipped with justification: 5 (scan cost), moving the guard into core, 9 (live test), standing()/fixed nit
• Partly right: 7 (skill wording, see B)
• New issues: 2 (both 🟡, neither introduced by this diff alone)

Verification of the author's claims

• confirmed is stripped before core: yes. UpdateSchemaParams is #[serde(deny_unknown_fields)] (packages/nodespace-types/src/schema_ops.rs:150), and parse_update_schema_params deserializes the raw args, so a leftover confirmed would be a hard "unknown field" error. The tool removes it from args before the guard and before handle_update_schema, on both the held and the confirmed path. Good. No stale force remains in seeds, bound_chat or the live test.
• Golden diff: only the force to confirmed parameter, re-sorted alphabetically. Nothing else changed.
• is_prefixed vs has_namespace_prefix (packages/core/src/schema/mod.rs:283): byte-for-byte the same logic.
• Fail-open on read errors: acceptable, now logged. The schema write validates independently, and an unreadable schema failing closed would block changes core would accept. One residual: a single broken chain inside find_field_references aborts the whole scan and lets the change through (logged). Fine for an advisory guard.
• Capped list: correct, "N more" appended after the whole-sentence list.

Findings

🟡 A. The guard's premise "a built-in field is refused by the schema write itself" is only true for Core/System protection

📁 packages/agent/src/local_agent/tools.rs (field_change_refusal, the retain on SchemaProtectionLevel::User), packages/core/src/models/core_schemas.rs (task priority: protection: User), packages/nodespace-types/src/schema.rs:762
Core's Phase 0 (mod.rs:2893-2925) only rejects non-User fields. task.priority is User-protected, so core allows removing it, or renaming it to a prefixed name. The guard filter therefore behaves correctly: it asks before the write core will perform, and task.status gets core's own refusal. But the claim the describer and the skill make ("Built in: it cannot be changed or removed", description.rs fixed, skill line 9 "explained and not attempted") is false for priority, and the new unit test pins that claim with a default-protection field. The guard's code comment repeats it. Either make core refuse declared fields of a core type (a core decision, needs the node-type sequence/ADR), or have fixed follow protection != User alone as finding 1 originally suggested, and reword the skill to "protected fields". Not blocking: the model is told it cannot, and the guard still holds if it tries.

🟡 B. A rename of a field a title template names is not blocked by core: it commits, then errors

📁 packages/core/src/schema/mod.rs:3262-3305 (Phase 1 rename persists), 3646-3695 (template validated afterwards), packages/core/src/services/node_service/schema.rs:928 (rename_schema_field rewrites only fields)
The author's claim holds for removal: test_update_schema_remove_field_referenced_by_existing_template_rejected asserts it, and removal is validated before anything is written. It does not hold for rename. rename_schema_field migrates data and rewrites the field list but not title_template; validate_schema_node then runs on the re-fetched schema, after the rename was persisted. The result is an error returned with the rename already applied across every node and the template naming a field that is gone. Core's own comment at mod.rs:3199-3203 names this exact hazard for the prefix check. This is pre-existing and no test covers it, but this PR now routes users to it: after the user agrees (confirmed=true) the tool result is an error that reads as "nothing happened". The skill's sentence is scoped to removal, so it is accurate as written; consider adding "a rename that a template names is applied and then refused, so also rewrite the template" or, better, a core pre-check beside Phase 0 that rejects a rename whose from a template names. Folding that in is small and worth it.

🟢 C. Duplicate helper

📁 packages/core/src/schema/description.rs is_prefixed
Identical to has_namespace_prefix, which is private to schema/mod.rs and therefore visible to the child module as super::has_namespace_prefix. Delete the copy (DRY; two definitions of "a user's own field" will drift). Duplication is tolerable for merge, but it is a one-line fix.

🟢 Nit

• Nit: field_change_refusal rereads the schema (get_schema_node) and then find_field_references reads it again per schema; minor, covered by the skipped scan-cost item.
• Nit: the tool no longer advertises force, but core still honours it if a model sends one (UpdateSchemaParams.force is a known field). Pre-existing; the play-impact waiver is therefore still self-confirmable, as the first review noted for an advisory guard.

Skipped items

• Moving the guard into core: justified for this issue (changes CLI semantics); worth a follow-up only with B, since the rename pre-check belongs in core anyway.
• Scan cost: agreed, premature.
• Live test: the failure notice now carries the list, so asserting it is reasonable; not re-run here.

Recommendation: APPROVE

The previous Critical-class concerns (mislabelled user field, asking for a force core will not honour, one force meaning two things) are fixed and tested, and the confirmed plumbing is sound. A and B are pre-existing core/describer gaps worth folding in but do not make this change a net negative.

[malibio]

Automated pragmatic-code-review (re-review): APPROVE. Findings 1-4, 6, 8 and the stale-comment nit are addressed; confirmed is stripped before core's deny_unknown_fields parse; golden diff is the parameter rename only. Two non-blocking notes (a User-protected declared field such as task.priority is described as unchangeable but core allows it; a rename of a template-named field commits before core's template validation errors) are in the preceding comment. Posted as a comment because GitHub blocks self-review.

[malibio]

Address review (re-review round): summary

The re-review was APPROVE with two non-blocking findings and a nit.

Addressed

• 🟡 B: renaming a field the stored title template names now fails before anything is written, with the order that works (a template that does not name it, then the rename, then the template with the new name). Found while testing: core cannot rename and reset the template in one call, because the rename validates against the template already stored, so the check looks at the stored template only. Test added in schema_test.rs; the type skill says the same.
• 🟢 C: the describer calls core's has_namespace_prefix instead of a copy.

Not changed

• 🟡 A: a User-protected declared field of a built-in type (task.priority) is not refused by core today. The chat's wording follows ADR-086 (a built-in type is closed), so it says those fields cannot be changed; making core enforce that is a core decision, not something to encode here by describing the gap. Worth a core change if ADR-086's rule is meant to hold at the write.

Checks: bun run test:changed passes (frontend, scripts, browser, types:check, node-types:check, Rust workspace); clippy clean on core, agent and daemon.

Re-Review Decision

Decision: RE-REVIEW NEEDED (narrow)

Rationale: a validation was added to core's update_schema write path. The delta is one pre-check in schema/mod.rs, its test, a one-line skill sentence and the helper swap.

[malibio]

Automated pragmatic-code-review (narrow re-review): APPROVE

Narrow re-review of e3e22bb (previous: 6144483048)

Verdict: APPROVE. The rename-vs-stored-template finding is resolved. One doc-comment nit, nothing blocking.

Checked

• Placement: The pre-check sits after every validation (validate_type_system_changes, context paths, relationship removal, play impact, core-prefix, redeclaration) and before Phase 1. Nothing between schema_before and Phase 1 writes, so a refusal changes nothing.
• Snapshot and token form: schema_before.title_template is the stored template, which is what rename_schema_field -> update_node_unchecked_in_tx -> validate_templated_content validates at rekey time. The match is the exact {name} token. validate_template_tokens also takes the bytes between { and } verbatim (no trim, no dotted or nested form, defined set = the schema's own fields). So { first_name } is already refused as an undefined field at write time and cannot be stored; there is no spaced form to miss.
• No false blocks: from == to relabels are filtered. A rename of a field the template does not name passes. A subtype cannot rename a parent field, and a template only names its own schema's fields, so a parent's template never trips it. A call that sets title_template: "" together with a rename is refused, but that is not a regression: Phase 1 runs against the stored template, so it already failed after beginning the rekey. Core has no in-call ordering that makes it work, so the message's three-step advice is correct.
• Skill sentence and message: Both are accurate. An empty-string title_template clears, and the sequence in the message is the one the test exercises.
• super::has_namespace_prefix: A private fn in the parent module is visible to its child module, and the body is identical to the deleted is_prefixed (ns:bare, exactly one colon, both non-empty). Fine, and it removes a duplicate.

Findings

• 🟢 Nit 📁 packages/core/src/schema/schema_test.rs (test doc comment, ~line 321): "A call that also sets a template that uses the new name goes through" is not true and not tested. The stored template is what is validated at rename time, so a single call that renames and sets the new template is refused. The test only proves the three separate calls. Reword to "The sequence the refusal names (three calls) goes through" (accuracy of comments, why not what).
• 🟢 Nit 📁 packages/core/src/schema/mod.rs:3243: The pre-check runs after the play-impact check, so a caller without force on an affected play sees that error first, then this one. Harmless (this refusal is not forceable) but you could move it ahead of the impact analysis, next to the other force-independent Phase 0 checks.

Recommendation

APPROVE. The change is minimal, correctly scoped, and tested on both the refusal and the working sequence. Fix the test comment if convenient.

[malibio]

Address review (narrow re-review): summary

Addressed: the test's doc comment now says the rename goes through in three calls, which is what the test exercises (the previous wording claimed a single call worked).

Skipped: moving the title-template check ahead of the play-impact check. It is harmless: neither refusal can be forced past the other, and the order only changes which message a caller without force sees first.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the only change since the approving narrow re-review is a test comment. The reviewer's verdict on the code was APPROVE with no re-review needed.