fix(registryctl): unify authored nullability on the type union - #630
fix(registryctl): unify authored nullability on the type union#630jeremi wants to merge 3 commits into
Conversation
A structured output now declares nullability the same way at every type. Object and array outputs use `type: [object, "null"]` and `type: ["array", "null"]`, which is what scalar outputs already used, and the separate `nullable:` key they carried is removed. That key shipped on composites only and never applied to scalars, so one grammar had two ways to say the same thing depending on the type in hand. The key is a released surface: it went out in 0.16.0, so removing it is a break and carries a `BREAKING:` entry with migration steps in the crate changelog, per the pre-1.0 rule in the API stability reference. Delete `nullable: true` and pair the declared form with `"null"` in the `type` union; delete `nullable: false` outright, since a form without `"null"` is already non-nullable. An authored project that still carries the key is rejected by `registryctl.authoring.yaml.unknown_field`, naming the offending pointer and the union syntax that replaces it, rather than by a generic whole-document schema error. Malformed recursive declarations now address their own nested location. The output validators compile each `$defs` branch standalone and select the branch the `type` discriminator names, so a failure inside `outputs.parents.items.fields.identifier.schema` reports that pointer instead of collapsing to the whole document. When no authored path can be recovered the diagnostic degrades to the previous whole-document form. Boundary-parity tests cover the authored declaration through Relay and Notary: escaped control characters and non-ASCII strings and keys, exact bounded integers, nullable and required nested values, byte caps at N and N-1, and the platform depth, field, item, and expanded-node limits. Two things the ticket raises are deliberately not changed. The aggregate composite byte budget stays explicit and required rather than gaining a derived default, because a silent default would weaken a safety control that the ticket also asks to keep independent. The `nullable` field on the compiled Relay and Notary contracts is a different, internal wire surface and is untouched. The `docs/site` files are regenerated configuration reference data, produced by `node scripts/generate-authoring-reference.mjs`, not hand-edited. Closes #573 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0433a8a909
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Changed |
There was a problem hiding this comment.
The reviewed commit message contains no Signed-off-by: trailer, so it does not satisfy the repository's mandatory DCO policy and will be rejected by any DCO gate; recreate the commit with the author sign-off before merging.
AGENTS.md reference: AGENTS.md:L70-L70
Useful? React with 👍 / 👎.
| "parents": { | ||
| "type": "array", | ||
| "nullable": false, | ||
| "type": ["array", "null"], |
There was a problem hiding this comment.
This example previously declared nullable: false, but replacing that with type: ["array", "null"] changes the output to nullable—the opposite of the migration documented in the changelog. Users copying the canonical schema example will therefore allow null unexpectedly; migrate this declaration to plain type: "array" as was done for the nested non-nullable object.
Useful? React with 👍 / 👎.
| ) -> Vec<ProjectAuthoringDiagnosticAddress> { | ||
| for cause in error.chain() { | ||
| let message = cause.to_string(); | ||
| let Some(candidate) = message.split_whitespace().next() else { |
There was a problem hiding this comment.
When lowering emits an output-specific error such as outputs.parents: structured outputs require capability.script, taking the first whitespace-delimited token leaves the trailing colon in outputs.parents:. The segment predicate then rejects the token and returns no address, so structured outputs used with an HTTP capability—and the other colon-formatted output errors in authoring_contract.rs—still produce only a whole-document diagnostic instead of the output address this new recovery path is intended to provide.
Useful? React with 👍 / 👎.
… the example Two review findings, both correct. The authored-path recovery only fired for messages that end the path at a space, like `outputs.parents.maxLength is required`. The validators also punctuate with a colon, as in `outputs.parents: structured outputs require capability.script`, and the trailing colon failed the segment grammar check, so those errors silently fell back to a whole-document diagnostic. Nine such messages in `authoring_contract.rs` were affected, including both structured outputs on an HTTP capability. Trailing separators are now stripped before the check. The canonical `integration.schema.json` example migrated `nullable: false` to `type: ["array", "null"]`, which makes it nullable: the opposite of what it declared, and the opposite of the migration the changelog documents. The nested object in the same example was migrated correctly, so the example also disagreed with itself. It is now plain `type: "array"`. This is the example implementers copy first. Nothing validated those examples before; the documentation pipeline only recorded whether examples were present. Every published authoring schema's example is now checked against its own schema. That check would not have caught this inversion, since both spellings are schema-valid and only the intent differs, but it closes the wider hole. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
|
Both P2s were right and are fixed in FixedStrip separators before parsing output diagnostic paths — correct, and a good catch on a path I had already flagged as the soft spot in this change. The recovery only fired when the authored path ended at whitespace, as in Trailing separators are now stripped before the grammar check. Three tests pin it: the colon form, the whitespace form, and a message that names no output path recovering nothing. The first was red before the fix and the other two were already green, which is the right shape. Keep the migrated example non-nullable — correct, and worse than the comment says. I audited every other Worth naming: nothing validated these examples. The documentation pipeline only recorded whether an DeclinedAdd the required DCO sign-off — This is the third time across two PRs that this finding has been raised against a commit that is in fact signed. On #626 it was raised twice, each time citing a hash that does not resolve in this repository. Here the hash is right and the premise is still wrong. The policy is enforced by a green job; if a future round raises it again, please check that job first. Coming next in this PRJeremi has decided to break the remaining inconsistency rather than carry it into 1.0. |
A claim value now declares nullability the same way as every other authored declaration. Write `type: [string, "null"]` for a nullable claim value and `type: string` for a required one; the separate `nullable:` key it carried is removed. That key shipped with the project authoring workflow in 0.10.0 and was the last place on the authored surface that spelled nullability a second way, so scalar outputs, structured outputs, entity fields, and claim values now all read the same. The key is a released surface, so removing it is a break and carries a `BREAKING:` entry with migration steps in the crate changelog, per the pre-1.0 rule in the API stability reference. Delete `nullable: true` and pair the declared type with `"null"` in the `type` union; delete `nullable: false` outright, since a type without `"null"` is already non-nullable. An authored project that still carries the key is rejected by `registryctl.authoring.yaml.unknown_field`, naming the offending claim pointer and the union syntax that replaces it, rather than collapsing to a whole-service schema error. The union type is the existing tuple idiom, parameterized rather than copied. `AuthoredObjectSchemaType` and `AuthoredArraySchemaType` become aliases of one generic `AuthoredNullableType<T>` that claim values reuse with their own type enum, so there is one nullability code path with three users instead of three near-identical enums. Claim values keep their own type enum because they admit `date`, which no output scalar does, and widening the output scalar enum would have loosened four unrelated typed surfaces past their published schemas. The retired-key diagnostic signal is likewise extended, not duplicated: `retired_nullable_key` carries which authored surface was found instead of a bare flag, and the recovered validation location is shared between the integration output union and the project service union. The removed claim-value `nullable` field had no readers. It never reached Relay or Notary compilation, and no authored fixture declares a claim value, so no generated product input changes. The build output digest moves only because the reviewed field-knowledge revision covers the published schema paths, which is the review gate working as designed. The `nullable` field on the compiled Relay and Notary contracts is a different, internal wire surface and is unchanged. The `docs/site` generated reference data is produced by `node scripts/generate-authoring-reference.mjs`, and `dto-shape-contract.v1.json` by the ignored `regenerate_dto_shape_contract_from_five_rust_roots` test; both are byte-identical on a second run. The `docs/site` reference test counters were stale from the preceding structured-output commit, which regenerated the data without moving them, and are corrected to the generated values here. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
jeremi
changed the title
1 participant
Closes #573.
Three commits, one ticket. The extra two are a review round and a scope decision Jeremi made mid-flight, both described below.
What changed
One nullability idiom, everywhere an implementer writes. Scalar outputs, structured outputs, entity fields, and claim values now all declare nullability the same way: the JSON-Schema-native
typeunion[<form>, "null"]. Two surfaces spelled it a second way with a separatenullable:boolean, and both are removed.type: [object, "null"],type: ["array", "null"].project.schema.json:type: [string, "null"].After this,
rg '"nullable"'across all three published authoring schemas returns nothing.Diagnostics that address the failing path. The output validators compile each
$defsbranch standalone and select the branch the author's owntypediscriminator names, so a failure insideoutputs.parents.items.fields.identifier.schemareports that pointer instead of collapsing to a whole-document schema error. The same recovery is now shared with the projectservicesunion, which is how a retired claim-value key reports/services/<id>/claims/<id>/value/nullablerather than addressing the whole service.A retired
nullable:key gets a cause and remediation naming the union syntax that replaces it, per surface, not a generic unknown-field message.Boundary-parity coverage. Authored declaration through Relay and Notary: escaped control characters and non-ASCII strings and keys, exact bounded integers, nullable and required nested values, byte caps at
NandN - 1, and the platform depth, field, item, and expanded-node limits.Published examples are now validated. Nothing checked them before; the documentation pipeline only recorded whether an
examplesblock was present. Every published authoring schema's example is now checked against its own schema. All five pass.Compatibility
Both keys have shipped, so both removals are breaks, not pre-release cleanup. Each carries the obligation the pre-1.0 rule in
api-stabilityattaches: aBREAKING:entry with concrete migration steps incrates/registryctl/CHANGELOG.mdunder[Unreleased]. There are two such entries.nullable:nullable:project.schema.json; v0.9.0 and earlier have no project-authoring schemas at allAn earlier revision of this description said the claim-value key shipped in v0.13.0. That was wrong, and it was wrong in the brief that produced the work; the scan above is the corrected answer and is what the changelog and commit message record.
Migration is a delete, identically on both surfaces:
nullable: true→ delete the key and pair the declared form with"null"in thetypeunion.nullable: false→ delete the key. A form without"null"is already non-nullable.An authored document that still carries either key fails with
registryctl.authoring.yaml.unknown_field, naming the offending pointer and the replacement syntax. Neither key is silently ignored.Blast radius: the only in-tree carrier of the output key was the OpenCRVS authoring fixture, fixed here. No authored fixture declares a claim
value:block at all, so the claim-value break has no in-tree carrier. Thenullablefield on the compiled Relay and Notary contracts is a different, internal wire surface and is untouched; the ~700nullable:occurrences undercrates/registry-relay/are all that surface.Scope decision taken mid-flight
The original scope was outputs only, and this description previously flagged the claim-value key as an open call for 1.0: accept a second idiom into the freeze, or schedule it separately. Jeremi's answer was to break it now for a cleaner and more consistent API, so it is folded in here rather than deferred.
Folding it in cost one commit and produced a simplification rather than a widening.
AuthoredObjectSchemaTypeandAuthoredArraySchemaTypeare now aliases of one genericAuthoredNullableType<T>, so there is a single nullability code path with three users instead of three near-identical enums.retired_nullable_keybecameOption<RetiredNullableSurface>instead of a barebool, which makes the per-surface message selection exhaustively checked by the compiler. Claim values keep their own type enum because they admitdate, which no output scalar does; widening the output scalar enum would have loosened four unrelated typed surfaces past their published schemas.Deliberately not changed
The aggregate composite byte budget stays explicit and required. The ticket asks to consider a derived default when omitted. A silent default would weaken a control the same ticket asks to keep independently useful, and it would be a new behavior rather than a removed inconsistency.
max_bytesremains required on object and array outputs.One tradeoff worth naming
authored_output_addressesrecovers the failing authored path by reading the leading token of the validator's error message. That is string recovery over an error message, which is not the sturdiest contract, and a review round proved the point: the recovery only fired when the path ended at whitespace, so the ninebail!sites that punctuate with a colon were silently degrading to whole-document diagnostics. Trailing separators are now stripped before the grammar check, and three tests pin the colon form, the whitespace form, and the no-path case.It remains guarded: the token must start with
outputsand every segment must match the authored path grammar, and when nothing matches the diagnostic degrades to the previous whole-document form rather than inventing a pointer. Making the validators return a structured path instead would touch the recursive output validators throughoutauthoring_contract.rs, which is more churn than better pointers justify right now. If you would rather have the structured version, say so and I will do it as a follow-up rather than widen this PR.Verification
cargo fmt --checkclean.cargo clippy -p registryctl --all-targets -- -D warningsclean.cargo test --locked -p registryctlgreen, 29 test binaries, 0 failures.docs/site:npm testandnpm run checkboth green.node scripts/generate-authoring-reference.mjsreports 1838 paths across 7 schemas with no further diff, and theregenerate_dto_shape_contract_from_five_rust_rootsauthority test leaves the tree unchanged.path_count1829 → 1838, made of the output change (Property −2, Branch +4, ArrayItem +4, net +6, all onintegration) and the claim-value change (Property −1, Branch +2, ArrayItem +2, net +3, all onproject). Intent accounting agrees independently: assignments − reused = unique intents, and 1838 − 1298 = 540, 540 + 86 shared = 626 distinct, matching both the Rust and thedocs/sitetables.starter.content_digestchange is generator-derived and enforced: corrupting it failsall_advertised_starters_initialize_and_test_without_source_accessandevery_cataloged_supported_project_authoring_command_is_automated.check_and_build_produce_deterministic_product_inputsmoves only becausefield_knowledge_revisionis a serialized field of the promotion projection and the reviewed revision covers the published schema paths. No fixture declares a claim value, so no product input content changed.Note for the reviewer
Commit
0433a8a9regenerated thedocs/sitereference data without moving the hand-maintained counters indocs/site/scripts/authoring-reference-docs.test.mjs, so it fails CI in isolation. That was my miss: I ran the Rust checks but notdocs/sitenpm teston a commit that touched generated docs data. The counters are corrected inb629dea5and the branch tip is green. Say the word if you want the fix moved back into the commit that caused it.