Skip to content

Refuse a schema field named after a declared field behind a prefix (closes #3726, refs #3728) - #3746

Merged
malibio merged 4 commits into
mainfrom
issue-3726-3728-shadowed-field-docs-inventory
Oct 12, 2026
Merged

malibio merged 4 commits into
mainfrom
issue-3726-3728-shadowed-field-docs-inventory

Conversation

@malibio

@malibio malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

Closes #3726. Refs #3728.

Refuse a schema field named after a declared field behind a prefix (#3726)

A schema edit could declare custom:status on task, beside status. 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.

  • One check, validate_no_shadowed_extension_fields (packages/core/src/schema/mod.rs). A field named an extension prefix + X is refused when X is 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_schema runs it over the new schema's fields and its extends parent, before the schema node exists.
  • update_schema runs it once, before any mutation, over the field list the call would leave (rename_fields, remove_fields and add_fields applied) against the parent the call would leave. That one call covers add_fields, rename_fields and an extends re-target, and a call that also removes or renames the declared field away stays legal. It runs ahead of the play impact check: force cannot make the name legal, so the caller is not asked for it first.
  • The write path's exemption is removed (reject_shadowed_extension_fields no longer skips a key the schema declares), with the test that pinned it.
  • The three friendly-name disambiguation tests no longer build their label collision from custom:status beside status.

org:custom:x beside a declared custom:x is not refused by the check (it reads as custom: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, and custom:effort accepted 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 via extends; no schema left behind), and a new parent. Each refusal asserts the schema is unchanged.
  • Each new test was run with the check disabled and fails.
  • bun run test:changed run locally (see the comment below for the result).

Docs

The extension-field sentence in the task sheet of components/node-types.md is 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 across decisions/, architecture/, components/, development/ and operations/, with a rule for strategy/ and archived/.

🤖 Generated with Claude Code

@malibio

malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Pragmatic Code Review: PR #3746

Review Type

Initial review (no prior reviews or comments on the PR). Reviewed origin/main...HEAD: commits fb378b7c7 and 1f678a214. Review by reading; no tests were run by the reviewer.

Summary

The change does what #3726 asks. One function, validate_no_shadowed_extension_fields, is called from create_schema and once from update_schema ahead of every mutation, and it reuses extension_field_bare_name and resolve_field_owners instead of defining the prefixes or the chain walk again. The projection of the final field list (renames, then removals, then additions) matches the order Phase 1 and Phase 2 apply them, and the new parent is validated before its chain is read. The tests assert the message, the unchanged schema and untouched node data.

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)

  • ✅ add_fields with custom:status on task is refused, naming status and task
  • ✅ Same for org: and plugin:, and for a name an ancestor declares (user subtype of task)
  • ✅ rename_fields to such a name is refused (before Phase 1; node data asserted untouched)
  • ✅ create_schema refused for own X + custom:X, and when it extends a type declaring X (no schema left behind)
  • ✅ A new parent is refused when a prefixed field is named after a field the new chain declares
  • ✅ custom:effort on task is still accepted, default applied on create
  • ✅ org:custom:x beside custom:x is not refused (tested against the function directly, since the field-name grammar refuses a second prefix first)
  • ✅ Schema unchanged after each refusal (assert_edit_refused compares fields and extends)
  • ✅ Write-path exemption removed, and an_extension_field_the_schema_declares_is_still_written removed
  • ✅ The three friendly-name tests no longer use custom:status / custom:due_date beside the bare name
  • ➖ "Each new test fails with the check removed": stated by the author, not re-run here. By reading, each new test asserts a refusal the check alone produces, and the direct-call test pins the double-prefix filter.
  • ➖ quality:fix / test:changed: run by the main agent, not by this review.
  • ❌ Documentation: the issue lists the extension-field sentence in the task sheet of components/node-types.md. On nodespace-docs origin/main (fetched during this review) line 182 still reads "is refused by a validated write" with no mention of a schema edit. See finding 3.

#3728 (docs inventory) has no code in this PR and was not reviewed.

Code Review Findings

1. 🟡 Important: an ancestor that gains the bare name leaves a descendant's prefixed field shadowed, and the subtype then refuses writes

📁 packages/core/src/schema/mod.rs:2953-2982 (Phase 0 block), packages/core/src/services/node_service/crud.rs:3371-3394

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:

  1. create_schema Gadget { size }; create_schema Widget extends: gadget { custom:color, default "red" }. Both legal.
  2. update_schema gadget add_fields: [color]. fields_after for gadget holds no prefixed name, so the check returns early and the edit lands.

Widget now declares custom:color under a chain that declares color. The same state is reachable with rename_fields on the ancestor (size to color) and by re-targeting a mid-chain type onto a parent that declares color.

Before this PR that field was unreadable but writable. With the exemption gone, reject_shadowed_extension_fields refuses the key in Widget's bucket, so by the reasoning of the deleted test (a declared default "does not turn every create into a refusal") every Widget create that takes the default is refused, as is any validated write of a Widget node that already stores the key. The new doc comment in crud.rs ("No schema can declare such a name either") is then not true. Reasoned from the code; not executed.

This is the retroactive-ancestor gap ADR-078 already leaves open for redeclaration (playbook/validation.rs:3891-3899), so it is a known class, but here it now ends in a refused write rather than a silent one.

Recommendation: in the same Phase 0 block, for each unprefixed name the edit introduces on this schema (an add_fields name, a rename destination, or, on a re-target, the new chain's field names), refuse when a schema whose chain contains this one declares <prefix> + that name. validate_type_system_changes already walks subtypes with load_parent_map + type_chain. Principle: an invariant the write path relies on must be enforced at every edit that can break it, not only the edits that name the offending field.

2. 🟢 Suggestion: add_field_values in a re-targeting call materializes a field the projection does not see

📁 packages/core/src/schema/mod.rs:3256-3276 against :2954-2974

Phase 2 resolves inherited fields for add_field_values from schema.extends (the parent being left), and extend_inherited_field pushes the inherited field into the schema's own list. The Phase 0 projection omits that. A single call on a user subtype of task with extends: "asset", add_fields: [custom:status] and add_field_values: [{ field: "status", ... mapsTo }] projects [custom:status] against asset (no status) and passes; Phase 2 then materializes status from task, and the re-target's validate_no_field_redeclaration against asset passes too. The schema ends with its own status and custom:status. Not executed; structural-rule checks on the re-target may refuse some instances of it.

Outside a re-target the materialized name is already in the inherited owners, so the projection is right. Recommendation: resolve add_field_values' inherited fields against the parent the call leaves (materializing from a parent being left is questionable on its own), or add the targeted inherited names to fields_after.

3. 🟡 Important: the node-types.md sentence is not on the docs remote

📁 nodespace-docs components/node-types.md:182 (origin/main)

The PR body says the sentence was updated. origin/main of nodespace-docs still describes the write-side refusal only. node-types:check compares type lists and field tables, so the gate will not catch it. Push it with the merge, as AGENTS.md requires for a type-sheet change.

4. 🟢 Suggestion: a generic node update of a schema node still writes fields unchecked

📁 packages/core/src/services/node_service/crud.rs:156-180, :1440-1441, :3031-3051

children, parent and context paths are refused on the generic update path ("can only be changed with update_schema"); fields is merged through. That path predates this PR and skips every update_schema check, not just this one, so it is not a regression, and I did not trace whether another guard stops it for a core schema. Worth a look given the removed exemption now assumes update_schema and create_schema are the only routes.

5. Nit: validate_extends_target now runs up to three times per call

📁 packages/core/src/schema/mod.rs:2844, :2970, :3394

A call with rename_fields and extends validates the target in the rename block, again in the new block, and again at the re-target, each loading the schema and the parent map. Computing the trimmed, validated parent_after once ahead of the rename block and using it in both Phase 0 places would remove one read and one copy of the explanation (DRY). Pure reads, so harmless.

6. Nit: the owner named for a materialized field

📁 packages/core/src/schema/mod.rs:1516-1519

Own fields are consulted before the inherited owners, so a subtype that has materialized status through add_field_values is named as the declaring type rather than task. Accurate enough; mentioning only.

What is right

  • The check is over the result of the call, so "rename the declared field away and take its name" and "re-parent and drop the field" stay legal, both tested.
  • Placement ahead of Phase 1 and ahead of the play check is correct: force cannot make the name legal, and a refused call leaves no rename committed.
  • No compatibility code, no migration, no lint suppression, no issue numbers in comments. No node type, field or relationship changes, so the node type sequence does not apply.

Recommendation: APPROVE

Every code acceptance criterion holds and the change is a clear net improvement. Fold in finding 1 (it is the same-shaped sibling of the bug this PR fixes, and the removed exemption depends on it) and push the docs sentence (finding 3) before merge; 2 and 4 are judgement calls for /address-review.

@malibio malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@malibio

malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review (4659b22)

Addressed

  • Finding 1 (an ancestor that gains the bare name leaves an extending type's prefixed field shadowed, and its writes refused): the check now looks down the chain too. An edit that would put X above a type declaring custom:X — by add_fields, by rename_fields, or by a new parent that declares X — is refused on the edited type, naming the extending type and its field. Test: a_declared_field_is_refused_above_an_extension_field_of_the_same_name (all three routes, the extending type's default still written, an unrelated name still added).
  • Finding 2 (add_field_values in a re-targeting call declares a field the projection did not see): the projected field list now counts a field declared that way. Test: a_field_declared_by_extending_its_values_counts_under_a_new_parent, confirmed to fail without the fix.
  • Finding 3 (the node-types.md sentence): prepared; it is pushed to nodespace-docs with the merge.

Skipped

  • Finding 4 (a generic update of a schema node writes fields unchecked): it predates this change and skips every update_schema check (redeclaration, protection, name grammar), not this one in particular. Closing it means refusing fields on the generic path as children and parent are, which is its own change.
  • Nit 5 (validate_extends_target runs up to three times): pure reads; hoisting it would reorder the errors of calls that have no rename.
  • Nit 6 (the owner named for a field declared through add_field_values): the schema does declare the field, so naming it is accurate.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: finding 1 added new validation logic (a walk over the extending types), not a trivial edit.

@malibio

malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Pragmatic Code Review: PR #3746

Review Type

Re-Review. Covers only 4659b2295 (the first review covered fb378b7c7 and 1f678a214). Review by reading; no tests were run by the reviewer.

Previous Review Summary

  • Previous recommendations: 6 (findings 1 to 6)
  • Addressed: 3 (finding 1 and finding 2 in code; finding 3 is the docs sentence, pushed to nodespace-docs with the merge and not counted against the code)
  • Not addressed: 3 (finding 4, nits 5 and 6), each skipped with a reason I accept
  • New issues found: 1 Important, 2 Suggestions

Verdict on the two fixes

Finding 1: closed for the case it described. validate_no_shadowed_extension_fields now walks get_subtype_closure(schema_id) and tests each extending type's extension fields against owner_of, which answers from the edited schema's projected field list and then from the parent it would have after the call. That covers all three routes:

  • add_fields: the new name is in fields_after.
  • rename_fields: the destination is in fields_after (renames are projected first).
  • extends re-target: inherited_owners is resolved from parent_after, so a name the new chain declares is found, and the message names the type that declares it (asset), not the edited one.

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 add_field_values is added to the projection when the schema does not already hold it, so in a re-targeting call the name is seen although the new chain does not declare it. For a legal call the extra entry changes nothing: outside a re-target the name is already in the inherited owners, and for an own field it is already in the list. I found no legal edit it refuses.

Skips. Finding 4 (generic update of a schema node writes fields unchecked) predates the PR and bypasses every update_schema check; refusing fields there is its own change. Nit 5 and nit 6 are fine as left.

Cost. One recursive closure query plus one get_schema_node per extending type, per create_schema / update_schema. For create_schema the closure of a type that does not exist yet is empty. Schema edits are rare and the read is pure. No concern.

Code Review Findings (new commit)

1. 🟡 Important: the mirror case is still open: a declared X below a type that declares custom:X

📁 packages/core/src/schema/mod.rs:1529-1566, packages/core/src/services/node_service/crud.rs:3371-3373

The check now covers two of the four placements of the pair in one chain:

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:

  1. update_schema task add_fields: [{ name: "custom:effort", default: "m" }]. Legal (custom:effort on task is the acceptance criterion for an accepted name).
  2. create_schema Bug extends: task fields: [effort]. A bare name is legal on a user-defined type. read_names of 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_closure are reused).
  • The refusal stays ahead of Phase 1 and of the play check, so nothing is committed before it.
  • assert_edit_refused_with keeps 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 malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@malibio

malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed re-review (839989e)

Addressed

  • Finding 1 (the mirror case: a declared X below a type that declares custom:X, and custom:X above a declared X of an extending type): the check is now symmetric. It gathers every field the edited schema would hold or inherit, and refuses the pair when both names are in that set, or when one is there and the other is on a type that extends the schema, in either direction. Test: a_declared_field_is_refused_below_an_extension_field_of_the_same_name (create_schema, add_fields, rename_fields, the reverse order, a new parent, and a Chore create that still takes custom:effort's default from task).
  • Finding 2 (an add_field_values name that exists nowhere was treated as declared): the name is counted only when the parent being left declares it, and only in a call that takes a new parent, so a missing field keeps its own "not found" error.
  • Finding 3 (the re-target message): the refusal now reads "'X' cannot be a declared field of the type 'A' in the same chain as 'B'", which holds whichever edit put the two together.

Skipped: nothing new.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the check was restructured, not patched.

@malibio

malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Pragmatic Code Review: PR #3746

Review Type

Re-Review (second). Covers only 839989efd. Review by reading; no tests were run by the reviewer.

Previous Review Summary

  • Previous recommendations (first re-review): 3 (one Important, one Suggestion, one Nit)
  • Addressed: 3
  • Not addressed: 0
  • New issues found: 0 blocking, 1 optional nit

Verdict on the three fixes

Finding 1 (the mirror case): closed. validate_no_shadowed_extension_fields now builds two sets, each entry a field name with the type that declares it:

  • here_and_above: the edited schema's projected fields, plus every field of the chain it would extend after the call;
  • below: the fields of every type that extends the edited schema.

It refuses a pair found in here_and_above against itself, in below (extension side) against here_and_above, or in here_and_above (extension side) against below. That covers the four placements:

custom:X on X on Comparison
the edited type the edited type or an ancestor here_and_above x here_and_above
an ancestor the edited type here_and_above x here_and_above
an extending type the edited type or an ancestor below x here_and_above
the edited type or an ancestor an extending type here_and_above x below

By entry point:

  • create_schema: own fields against the declared parent's chain, both directions; the closure of a type that does not exist yet is empty.
  • add_fields / rename_fields: fields_after holds the added name and the rename destination, so either name of the pair is seen whichever one the edit brings.
  • extends re-target: inherited is resolved from the parent the call would leave, so the new chain is compared with the schema's own fields and with its extending types, in both directions (a type with no fields of its own, moved under a parent declaring custom:size while a subtype declares size, is caught by the third comparison).

Finding 2 (an add_field_values name that exists nowhere): closed. The projection now takes the field from resolve_effective_fields(old_parent) and only when the call has add_field_values, a current parent and a new extends. This matches Phase 2, which resolves inherited fields from schema.extends before the re-target block runs and matches by addition.field. A name no ancestor declares is not added, so its own "not found" error is kept. Outside a re-target the name is already in the inherited set, so leaving it out changes nothing.

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.

  • Sibling subtypes. below is never compared with itself, so A { custom:x } and B { x }, both extending the edited type, are not paired. Siblings of the edited type are in neither set. Every type in below does share a chain with the edited type, so each cross-set comparison is between two types of one chain.
  • Two prefixes, one read name (org:estimate beside custom:estimate, the issue's non-goal). shadowed needs a declared-side name equal to the unprefixed read name estimate; neither field has it. Still legal.
  • org:custom:x beside custom:x. org:custom:x reads as custom:x, which is itself a prefixed name, so it returns None; custom:x reads as x, which nothing declares. Still legal, and the filter is the same one as before, now inside shadowed.
  • An own field materialized by add_field_values with no re-target. The name then appears twice in here_and_above (own, and inherited). Duplicates on the declared side only matter if an extension field reads as that name, which is the refused state anyway. The own entry comes first, so the type named is the edited one, which does declare it.
  • A pair wholly above, or wholly within here_and_above on two ancestors. Compared, but it can exist only if an earlier edit was let through; with every schema-edit route now checked it is not reachable through create_schema / update_schema. No seeded core schema declares a prefixed field.

Are the messages true?

Case Branch Text True
custom:X on edited type, X here or above first "'custom:X' cannot be declared: 'X' is a declared field of the type '' ... Use 'X', or give the extension field another name" yes
custom:X on an ancestor, X on edited type second "'X' cannot be a declared field of the type '' in the same chain as '' ..." yes
custom:X on extending type, X here second names the edited type and the extending type yes
same, through a new parent second names the new parent's declaring type yes
custom:X on edited type, X on extending type first "'X' is a declared field of the type ''" yes; see nit

Code Review Findings (new commit)

Nit: "Use 'room'" is not available when the declared field is on an extending type

📁 packages/core/src/schema/mod.rs:1591-1595
Severity: Nit. Principle: an error message that reaches an agent as a tool result should only offer actions the caller can take.

Adding custom:room to task while chore declares room takes the first branch, whose advice is "Use 'room', or give the extension field another name." room is a field of chore, not of task, so the first half does not apply to the type being edited. The statement of fact and the second half of the advice are correct, and the caller is told what to do. Optional; not worth another round.

What is right

  • One function still serves both entry points, and the four placements fall out of three calls to one small matcher instead of four hand-written loops.
  • No second definition of the prefixes or the chain walk: extension_field_bare_name, resolve_field_owners and get_subtype_closure are reused.
  • The refusal is still ahead of Phase 1 and of the play check, so nothing is committed before it.
  • The new test covers create_schema (and that no schema is left behind), add_fields, rename_fields, the reverse order, a new parent, and that a chore create still takes custom:effort's default from task. The two existing message assertions were updated with the wording.
  • The crud.rs doc comment now matches what the schema edit enforces.
  • No compatibility code, migration, lint suppression or issue numbers in comments. Cost is unchanged from the previous commit: one closure query and one schema read per extending type, per schema edit.

Not verified here

  • No tests were run. The author reports the two touched test groups pass (223) and the full Rust tier was still running when this was written.
  • The node-types.md sentence lives in nodespace-docs and is pushed with the merge; not counted.
  • The generic node-update path that writes a schema node's fields unchecked (first review, finding 4) is unchanged and remains outside this PR.

Recommendation: APPROVE

The check is complete for the four placements across create_schema, add_fields, rename_fields and a re-target, refuses none of the legal edits examined, and its messages are true. The one nit is optional.

@malibio malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@malibio

malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed second re-review

The second re-review approved with one optional nit and no code changes are made for it.

Skipped

  • Nit ("Use 'room'" is not an option when the declared field is on an extending type): the statement of fact and the other half of the advice are right, and the case needs the error to know which side of the edited type the declared field is on. Left as is.

Checked

  • The reviewer's one assumption, that no seeded core schema declares a prefixed field, holds: core_schemas.rs has no custom:, org: or plugin: field name.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: no code changed since the commit the approving review covered (839989e).

@malibio

malibio commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Merge queue: #3746 was taken out of the queue — the merge gate failed on it, on top of main. Reproduce by merging or rebasing origin/main and running bun run test:changed, fix, push, and re-run bun run merge 3746.

✓ skill installer script (tsc) 1s
▶ types:check (generated TypeScript drift) (timeout 15m)
▶ test (frontend, Happy-DOM) (timeout 15m)
  ✓ types:check (generated TypeScript drift) 12s
▶ rust:lint (clippy, whole workspace) (timeout 60m)
  ✓ rust:lint (clippy, whole workspace) 39s
▶ compile Rust test binaries (timeout 60m)
  ✓ test (frontend, Happy-DOM) 146s
▶ test:scripts (tooling) (timeout 10m)
  ✓ compile Rust test binaries 118s
▶ compile nodespaced and the Tauri-seam test binary (timeout 45m)
  ✓ test:scripts (tooling) 38s
▶ test:skill (skill package) (timeout 5m)
  ✓ test:skill (skill package) 3s
  ✓ compile nodespaced and the Tauri-seam test binary 24s
▶ skill:check (SKILL.md drift) (timeout 20m)
  ✓ skill:check (SKILL.md drift) 1s
▶ node-types:check (published node-types.md vs. the core schemas) (timeout 20m)

$ bun run scripts/check-node-types-doc.ts --at "05e2f7c430936bd2f0e7c3fd07308d838f67b2e9"
❌ components/node-types.md at 05e2f7c in /Users/malibio/nodespace/nodespace-docs and the core node types disagree (8):
  - line 115: the registry table lists `issue` under "Core subtypes", and the code registers no such type of that kind
  - line 115: the registry table lists `software-project` under "Core subtypes", and the code registers no such type of that kind
  - line 182: `issue` has a field table, and the code registers no such core type
  - line 230: `software-project` has a field table, and the code registers no such core type
  - line 161: the `task` schema declares `pull_request`, and its sheet's field table doesn't list it
  - line 161: the `task` schema declares `commits`, and its sheet's field table doesn't list it
  - line 214: the `project` schema declares `repository`, and its sheet's field table doesn't list it
  - line 214: the `project` schema declares `checkout_path`, and its sheet's field table doesn't list it
A change to a core type updates its sheet in the same change (the node type sequence). Fix whichever side is wrong.
If this merge changes no core type, it did not cause the difference: the published sheet is ahead of main
while a type change lands. Change neither side, and re-run `bun run merge` once that change has merged.
error: script "node-types:check" exited with code 1
[stage exited with code 1]

  full output: /var/folders/6c/mkxnmpkx5zqc9jwwy20tjkyr0000gn/T/nodespace-gate-logs/_gate-merge-2026-10-11T20-57-15-675Z/node-types-check-published-node-types-md-vs-the-core-schemas.log

✗ node-types:check (published node-types.md vs. the core schemas) failed — merge blocked.
  Fix the failure, push, and re-run: bun run merge <PR#>

malibio and others added 4 commits October 12, 2026 08:46
…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>
@malibio
malibio force-pushed the issue-3726-3728-shadowed-field-docs-inventory branch from 839989e to dedc256 Compare October 12, 2026 06:46
@malibio
malibio merged commit c2d88af into main Oct 12, 2026
@malibio
malibio deleted the issue-3726-3728-shadowed-field-docs-inventory branch October 12, 2026 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refuse a schema field named after a declared field behind a prefix (custom:status beside status)

1 participant