[malibio]

Summary

• update_schema now holds a removal or identity rename that a saved query or play names, in core's write, so the CLI and every other caller meet what the agent's tool used to check alone. One agreement, force, covers active plays and these references, in one refusal whose first sentence carries the list (the failure notice a user sees keeps only that sentence).
• The agent tool drops its own guard (field_change_refusal) and its confirmed parameter, and declares force.
• The play check moved down beside it, after the hard refusals, so a caller is not asked to agree to a change that is then refused outright.
• The bound chat marks only core/system-protected fields built in. A user-protected field of a built-in type (task.priority, the dates) is described as changeable once what names it is dealt with, matching core and ADR-076.

Decision

The issue as first filed also asked core to refuse removal of any built-in field. Core's seeded User protection is deliberate (ADR-076; the shape check treats priority as the user's to remove), so that half was dropped and the issue narrowed. ADR-100's skill paragraph is updated to say so.

Tests

• schema_test.rs: a saved query holds a rename and a removal until force; a relabel and an unnamed field go freely; task.priority is held for a saved query while task.status is refused whatever is agreed.
• Agent tool test updated to force; bound-chat and describer tests updated; tool-surface golden updated for the parameter rename.
• bun run test:changed passes; clippy clean on core, agent and daemon.

Closes #3786

🤖 Generated with Claude Code

[malibio]

Code Review Summary

Verdict (self-authored PR, posted as a comment): net positive, no blockers; fix the one invariant gap below, then merge.

Moving the what-names-it hold into core's update_schema is the right place for it: the CLI and every other caller now meet the same refusal the agent tool used to apply alone, the agent keeps a single agreement (force), and the narrowing of the issue (no blanket refusal of User-protected seeded fields) is consistent with ADR-076 and is reflected consistently in the describer, bound chat, skill and ADR-100. The reordering of the play check is safe: nothing between the old and new position read affected/affected_names (the only later use is the result's affected_plays at the end, which is only reachable with force), and the check is a pure read.

Findings

Critical Issues

None.

Suggested Improvements

• packages/core/src/schema/mod.rs (hold block, comment "The title templates that name it are refused above"): the invariant "no hard refusal can follow a hold" is not fully true. Only a rename a title template names is refused in Phase 0. A removal of a field the title template names is refused only later, by SchemaNodeBehavior.validate_schema_node ("Schema validation failed"), after the hold. So a removal that a saved query and the title template both name asks the caller to agree via force, and then refuses it outright. Either add the removal case to the Phase 0 title-template check (cheap: template.contains("{field}") for remove_fields, ahead of the hold), or soften the comment. The first is better, since it is exactly the situation the PR set out to remove. The same applies to the other late refusals (Phase 1 rename failures, Phase 2 additions), which are unrelated to force and acceptable, but the comment "Last of the refusals ahead of Phase 1" should not be read as "before every refusal".
• packages/core/src/schema/mod.rs near line 25: the new FIELD_CHANGE_REFERENCES_SHOWN const was inserted between the /// Reserved core property names... doc comment and RESERVED_CORE_PROPERTIES, so rustdoc now attaches that sentence to the wrong item and RESERVED_CORE_PROPERTIES loses its doc. Move the const above the doc comment.
• Failure mode changed: the agent tool used to log a warning and let the change through when the reference lookup failed (fail open); core now returns an internal_error and blocks the change (fail closed) unless force=true. That is defensible for a destructive write and consistent with the impact-analysis check, but it is a behaviour change worth a sentence in the PR body, and there is no test for it.
• Test gap, ordering: test_update_schema_holds_a_user_field_of_a_built_in_type... asserts that removing task.status with force: true is refused, which would be refused regardless of order because force skips the hold. The ordering property ("a hard refusal is not preceded by a hold") is only pinned in the agent tool test (!contains("force=true") && contains("protected")). Add the unforced variant to the core test so the invariant is pinned where it lives.
• Test gap, composition: nothing in core's tests covers (a) a saved query and an active play in one refusal (first-sentence shape with the ", and it would affect N active play(s)" join), (b) a play reference found by find_field_references (only saved queries are exercised), or (c) the FIELD_CHANGE_REFERENCES_SHOWN cap and the "N more" entry. The first-sentence assertion splits on ". ", which a saved-query title containing ". " would also break; fine for a test, but it is worth knowing the "first sentence" contract is by convention only.

Behaviour notes (no change required)

• force semantics: callers that already passed force=true (core_type_registry_test, existing schema tests, any CLI/desktop caller) now also skip the saved-query hold, which is the stated intent. Callers that did not pass it and rename/remove a field a saved query or play names are newly refused; I found no non-test caller in cli/daemon/desktop that does so.
• A play that triggers on the type and names the field is reported twice in the combined message: once as a reference ("play ...") and once as an affected active play. Harmless, but noisy; dedupe by play id if the message ever gets long.
• Cost: find_field_references scans all schemas (with a resolve_type_chain each), all query nodes and all play nodes, and runs only when !force and a removal or identity rename is present, so additive calls and forced calls pay nothing. Acceptable for a rare, destructive, user-agreed operation. It builds references for every declared field of the schema, not only the changed ones; filtering by changed before scanning would cut the work, but is not worth doing now.

Nitpicks

• Nit: packages/core/src/schema/mod.rs, kind mapping _ => "play" hides any future ReferenceKind variant behind a wildcard; match Play explicitly and make TitleTemplate unreachable/filtered at the match.
• Nit: the force description in the agent tool ("naming a field that is gone or affect active plays") reads awkwardly; "or would affect active plays".

Docs consistency

ADR-100's skill paragraph, understanding-a-type.md, the describer text and the generated update-schema-params.ts doc comment all agree with the code. No node-type, field or relationship change, so node-types.md needs nothing. No remaining mentions of confirmed or field_change_refusal in the repo. Skill file size: the sequence change is a net small addition; confirm the CLI skill-size integration test still passes (it is at its cap per project notes), since understanding-a-type.md grew by roughly 150 characters.

Not merging; no re-review needed beyond the title-template ordering fix.

[malibio]

Address review: summary

The review was a net positive with no blockers and one invariant gap.

Addressed

• Invariant gap: a removal that the title template (as the call leaves it: the template it sets, else the stored one) names is now refused in Phase 0, ahead of the hold, with the way that works. A removal that a saved query and the title template both name no longer asks for force and then refuses. Test added, including the call that moves the template in the same call.
• Misplaced doc comment: the const is above the comment it displaced.
• Kind mapping names every ReferenceKind instead of a wildcard.
• Tool wording: "would affect active plays".
• Tests: an unforced core-protected removal is refused without a hold; a saved query and the seeded plays arrive in one refusal; the cap of 10 with the rest counted.

Noted, not changed

• Fail-closed lookup: if the reference lookup fails, core returns an internal error and the change is not applied unless force is given. The agent tool used to log and let it through. For a destructive write that is the safer side, so it stays; a failing lookup is not something a test can induce without a fault-injecting store.
• Cost: the scan runs only for an unforced removal or identity rename, and finds references for every declared field, not only the changed ones. Not worth narrowing until measured.
• Duplicate listing of a play that is both a reference and an affected play: harmless, and the one agreement covers both.
• No test of a play found as a reference in core's write: field_references.rs covers that lookup, and the composition test reaches it through the seeded plays.

Checks: bun run test:changed passes; clippy clean earlier on core, agent and daemon; tool-surface golden updated for the wording.

Re-Review Decision

Decision: RE-REVIEW NEEDED (narrow)

Rationale: a new refusal was added to core's update_schema write path (the template-named removal). The delta is one pre-check in schema/mod.rs, its tests, and a comment move; the rest is wording.

[malibio]

Re-review summary (commit 74827d5)

Verdict (self-authored PR, posted as a comment): net positive, no blockers. The earlier invariant gap is closed. One narrow over-refusal is worth a two-line fix before merge.

The previous round's findings are all addressed: the const sits above the doc comment it displaced, the kind match is explicit, the wording is corrected, and the new tests pin the ordering, composition and cap properties in core.

Findings

Critical Issues

None.

Suggested Improvements

• packages/core/src/schema/mod.rs (new Phase 0 check, ~L3217): the check looks only at remove_fields against the effective template, and ignores what the same call adds. update_schema applies removals first and then additions (fields.retain(...) then the duplicate check against the post-removal list), so remove_fields: ["state"] plus add_fields: [{name: "state", type: ...}] is a legal call that leaves {state} defined and passes validate_schema_node. That is how a templated field's type is changed. The new check now refuses it with "named by the title template ... cannot be removed". The sibling Phase 0 block a few lines below (the extension-field-name check) already computes fields_after (renames, removals, additions) for exactly this reason. Suggested fix: skip a removed name when params.add_fields or a rename_fields[].to supplies it, or reuse that fields_after and refuse a template token that is not in it. That also makes the check say what the validation will say, which is the invariant the comment claims. The user-level workaround today (clear the template, remove and re-add, set the template) works but is exactly the dance this PR set out to avoid.
• Test gap, same area: nothing pins the effective-template resolution beyond "stored template, field removed" and "call sets a template that moves off the field". Worth one test each for (a) clearing with title_template: "" plus a removal (persisted as Some(""), validated as empty, so it is accepted; the logic is right, it is just unpinned), and (b) the remove-and-re-add call above, once fixed.

Nitpicks

• Nit: the removal check uses the effective template (call's, else stored), while the rename check just above still uses only the stored one, so a call that renames a field and sets a template naming the new name is refused by the rename check but a removal with a same-call template is accepted. The comment on the rename check explains the rekey ordering, so this may be intended; a sentence saying why the two differ would save the next reader a trip.
• Nit: contains("{name}") matches what validate_template_tokens does for well-formed tokens (exact name, no trimming), so no false negatives that I could construct; a malformed {{name} is refused by either path with different wording, which is harmless.

Checked and fine

• Effective template: params.title_template.or(schema_before.title_template) matches the persisted value (params.title_template.clone().or(schema.title_template)), and "" replaces the stored template rather than falling through, so a clear is handled consistently.
• Ordering: Phase 0 refusal now precedes the hold, so a removal that a saved query and the template both name no longer asks for force and then refuses; the new test asserts !contains("force=true").
• Unforced core-protected removal is refused before the hold (test added), and the seeded plays plus a saved query arrive as one refusal (composition test asserts the shape).
• Cap test: 12 saved queries produce 10 listed and "2 more"; assertion counts listed definitions, not just the tail text.
• Tool-description change and regenerated stage2_tool_surface.golden agree.

[malibio]

Address review (re-review round): summary

The re-review found the invariant gap closed and one narrow over-refusal.

Addressed

• A removal of a templated field that the same call declares again, by add_fields or as a rename_fields destination, is no longer refused: the name is still declared afterwards, which is how a field's type is changed. Test added (remove and re-add state under a {state} template); clearing the template while removing is already pinned by test_update_schema_remove_field_and_clear_template_succeeds.

Not changed

• The sentence on why the rename check reads the stored template and the removal check the effective one: the rename is validated against the stored template at rekey time, the removal against the template the call leaves; the comments on each say so.
• The malformed {{name} wording difference: refused either way.

Checks: bun run test:changed passes.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the delta only narrows a refusal this PR added, by skipping names the call declares again, and adds its test. Nothing new is refused.