Repository navigation
Refuse a schema field named after a declared field behind a prefix (closes #3726, refs #3728) - #3746
Conversation
Pragmatic Code Review: PR #3746Review TypeInitial review (no prior reviews or comments on the PR). Reviewed SummaryThe change does what #3726 asks. One function, Two schema-edit routes still reach the state the PR says can no longer exist, and the documentation sentence the issue lists is not on the docs remote. None of them breaks an acceptance criterion as written; the first matters because the write-path exemption was removed on the premise that no schema can hold such a field. Requirements Check (#3726)
#3728 (docs inventory) has no code in this PR and was not reviewed. Code Review Findings1. 🟡 Important: an ancestor that gains the bare name leaves a descendant's prefixed field shadowed, and the subtype then refuses writes📁 The check looks at the edited schema's own fields against its ancestors. It never looks down. On user-defined types, where bare names are legal:
Widget now declares Before this PR that field was unreadable but writable. With the exemption gone, This is the retroactive-ancestor gap ADR-078 already leaves open for redeclaration ( Recommendation: in the same Phase 0 block, for each unprefixed name the edit introduces on this schema (an 2. 🟢 Suggestion:
|
malibio
left a comment
There was a problem hiding this comment.
Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.
Addressed review (4659b22)Addressed
Skipped
Re-Review DecisionDecision: RE-REVIEW NEEDED Rationale: finding 1 added new validation logic (a walk over the extending types), not a trivial edit. |
Pragmatic Code Review: PR #3746Review TypeRe-Review. Covers only Previous Review Summary
Verdict on the two fixesFinding 1: closed for the case it described.
Types between the edited schema and the extending type need no look: this edit cannot give them a field. The test exercises the three routes, asserts the schema is unchanged after each, that the extending type's default is still written, and that an unrelated name is still added. Finding 2: closed. A field named by Skips. Finding 4 (generic update of a schema node writes Cost. One recursive closure query plus one Code Review Findings (new commit)1. 🟡 Important: the mirror case is still open: a declared
|
custom:X on |
X on |
Checked |
|---|---|---|
| the edited type | the edited type or an ancestor | yes (first loop) |
| an extending type | the edited type or an ancestor | yes (new loop) |
| an ancestor | the edited type | no |
| the edited type | an extending type | no |
The first loop reads only the edited schema's prefixed names; the new loop reads only the extending types' prefixed names. Neither asks whether a bare name the edit introduces sits under a prefixed field an ancestor declares, or whether a prefixed name the edit introduces sits above a bare field an extending type declares. validate_no_field_redeclaration does not catch it either: effort and custom:effort are different names.
A route built from the issue's own accepted example:
update_schemataskadd_fields: [{ name: "custom:effort", default: "m" }]. Legal (custom:effortontaskis the acceptance criterion for an accepted name).create_schemaBugextends: taskfields: [effort]. A bare name is legal on a user-defined type.read_namesof Bug's fields is empty, Bug has no extending types, so the call passes.
The same state is reached in the other order (Bug { effort } first, then custom:effort added to task: owner_of("effort") looks at task's fields and ancestors only, and the walk reads only extending types' prefixed names), by rename_fields to effort on Bug, and by re-targeting a type that declares effort under a parent that declares custom:effort.
The write path does not make this distinction. reject_shadowed_extension_fields (crud.rs:3382-3403) scans every bucket in the chain against owners resolved over the whole chain, so for a Bug node the task bucket's custom:effort is refused because effort is owned by bug. By the same reasoning as the first review's finding 1, a Bug create that takes the default is then refused, and custom:effort is unreadable on every Bug. Reasoned from the code, not executed.
This gap predates the commit (the first review's finding 1 named only one of the two mirror cases, so that is on the review, not the author). It is raised now because the commit's own comments assert it is closed: the function doc says the check "looks down the chain as well as up", and crud.rs now says a schema edit refuses the name "whichever of the two names it would add". Neither holds yet.
Recommendation: make the check symmetric over the chain the edit would leave. In the first loop's place, also refuse when a bare name in fields equals the read name of an extension field the ancestors declare (the ancestor field list is already returned by resolve_field_owners_or_error, element .0, with its owner in .1). In the subtype loop, also refuse when a bare field of the extending type equals the read name of an extension field in fields or in the new parent's chain. One test with the four placements would pin it. Principle: an invariant the write path relies on has to be enforced at every edit that can break it; a half-symmetric check leaves the write-side refusal as the place users find out.
2. 🟢 Suggestion: an add_field_values name that exists nowhere is treated as declared
📁 packages/core/src/schema/mod.rs:3005-3012
The projection adds addition.field whether or not any type declares it. A call on a schema holding custom:colour with add_field_values: [{ field: "colour", ... }], where no colour exists, is refused with "'colour' is a declared field of the type ''", and the same call on an ancestor of that schema gets "'colour' cannot be declared by the type ...". The call was going to fail anyway ("Field 'colour' not found"), so nothing legal is refused, but the message states something false and hides the real cause. Adding the name only when the parent being left resolves it (schema_before.extends) would keep the message true. The comment above the loop also says "Under a new parent", while the loop runs for every call.
3. 🟢 Nit: the re-target message advises the wrong action
📁 packages/core/src/schema/mod.rs:1558-1563
For a re-target the text reads "'colour' cannot be declared by the type 'asset' above 'widget' ... Use another name, or rename 'custom:colour' on 'widget' first." asset already declares colour; what is refused is placing gadget under it, and "use another name" is not an option the caller has. The second half of the advice is correct, and the owner named is the right one. Optional.
What is right
- One function still serves both entry points; no second definition of the prefixes or of the chain walk (
extension_field_bare_name,resolve_field_owners,get_subtype_closureare reused). - The refusal stays ahead of Phase 1 and of the play check, so nothing is committed before it.
assert_edit_refused_withkeeps the "schema unchanged" assertion on the new cases.- No compatibility code, migration, lint suppression or issue numbers in comments.
Recommendation: REQUEST CHANGES
Findings 1 and 2 of the first review are closed as written, and the skips are reasonable. One change is asked for: finding 1 above, the mirror of the case this commit fixed, reachable from the issue's own custom:effort on task example and ending in the same refused write. It is a small extension of the two loops already there, and belongs in this PR rather than a new issue. Findings 2 and 3 are optional.
malibio
left a comment
There was a problem hiding this comment.
Automated pragmatic-code-review (re-review): REQUEST CHANGES (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.
Addressed re-review (839989e)Addressed
Skipped: nothing new. Re-Review DecisionDecision: RE-REVIEW NEEDED Rationale: the check was restructured, not patched. |
Pragmatic Code Review: PR #3746Review TypeRe-Review (second). Covers only Previous Review Summary
Verdict on the three fixesFinding 1 (the mirror case): closed.
It refuses a pair found in
By entry point:
Finding 2 (an Finding 3 (the re-target message): closed. "cannot be a declared field of the type 'A' in the same chain as 'B'" is true for every route. Does the restructured check refuse a legal edit?I looked for one in each case asked about and found none.
Are the messages true?
Code Review Findings (new commit)Nit: "Use 'room'" is not available when the declared field is on an extending type📁 Adding What is right
Not verified here
Recommendation: APPROVEThe check is complete for the four placements across |
malibio
left a comment
There was a problem hiding this comment.
Automated pragmatic-code-review (second re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.
Addressed second re-reviewThe second re-review approved with one optional nit and no code changes are made for it. Skipped
Checked
Re-Review DecisionDecision: NO RE-REVIEW NEEDED Rationale: no code changed since the commit the approving review covered (839989e). |
|
Merge queue: #3746 was taken out of the queue — the merge gate failed on it, on top of main. Reproduce by merging or rebasing |
…loses #3726) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he same name - A type extending the edited one may already declare custom:X; adding or renaming a field to X, or taking a parent that declares X, is refused. - A field declared by add_field_values counts in the projected field list, so it is seen when the same call leaves the parent it came from. Skipped: fields written through a generic update of a schema node skip every update_schema check, not only this one, and predate this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The check compared an extension field with declared fields on the same type or above it, and a declared field with extension fields of extending types. It now also refuses a declared field below a type that declares the extension field, and an extension field above a declared field of an extending type, so no schema edit leaves a field a write would refuse. A field counted from add_field_values is taken from the parent being left, and only in a call that takes a new parent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
839989e to
dedc256
Compare
Closes #3726. Refs #3728.
Refuse a schema field named after a declared field behind a prefix (#3726)
A schema edit could declare
custom:statusontask, besidestatus. A rule or a query reads an extension field by its bare name and the declared field wins that name, so the field could be written and never read. The node write already refused the name (ADR-097 §6); a schema edit now refuses it too.validate_no_shadowed_extension_fields(packages/core/src/schema/mod.rs). A field named an extension prefix +Xis refused whenXis declared unprefixed in the schema's own fields or anywhere in the chain it extends. The error names the declared field and the type that declares it, and says what to do next.create_schemaruns it over the new schema's fields and itsextendsparent, before the schema node exists.update_schemaruns it once, before any mutation, over the field list the call would leave (rename_fields,remove_fieldsandadd_fieldsapplied) against the parent the call would leave. That one call coversadd_fields,rename_fieldsand anextendsre-target, and a call that also removes or renames the declared field away stays legal. It runs ahead of the play impact check:forcecannot make the name legal, so the caller is not asked for it first.reject_shadowed_extension_fieldsno longer skips a key the schema declares), with the test that pinned it.custom:statusbesidestatus.org:custom:xbeside a declaredcustom:xis not refused by the check (it reads ascustom:x). The field-name grammar refuses a second prefix before a schema edit reaches the check, so that case is tested against the function directly.Tests
core_type_registry_test:add_fields(all three prefixes, an ancestor's field, the subtype's own, both names in one call, andcustom:effortaccepted with its default),rename_fields(refused with node data untouched; legal when the declared field is renamed away in the same call),create_schema(own fields; inherited viaextends; no schema left behind), and a new parent. Each refusal asserts the schema is unchanged.bun run test:changedrun locally (see the comment below for the result).Docs
The extension-field sentence in the
tasksheet ofcomponents/node-types.mdis updated in nodespace-docs to say a schema edit refuses the name too.Inventory of nodespace-docs (#3728)
No code in this repository: the inventory is a working document in nodespace-docs (
development/docs-inventory.md), one row per page or section acrossdecisions/,architecture/,components/,development/andoperations/, with a rule forstrategy/andarchived/.🤖 Generated with Claude Code