Publish the node configuration schema in the contract - #342
Conversation
This comment has been minimized.
This comment has been minimized.
`RegisterRequest.config` is `dict[str, Any]`, so the generated contract described a node's configuration as a free-form object: fifteen fields and every one of their bounds were absent, where the hand-written 1.1.1 published them. The bounds were still enforced, only undiscoverable, so a client generated from the published file sent an unvalidated blob and learned the rules from 400s. The field stays untyped. A Pydantic model there would answer 422 ahead of the handler, putting a config-shaped refusal in front of identity resolution and making the difference between it and a 403 an oracle for which node identities exist. WithJsonSchema replaces what is published and leaves validation alone, so the shape reaches the document while the refusal stays behind identity resolution, and PUT /v1/nodes/config carries the same object in its openapi_extra. That also retires its cross-reference to a rationale which only ever existed in a code comment. Built from _NUMERIC_BOUNDS and _REQUIRED rather than written beside them, so twelve of the fifteen cannot drift from the checks that enforce them. The callsign and the two beam fields are checked against their own literals, so those are pinned against validate_config at the boundary itself, with nextafter either side of every published bound. NODE_API_VERSION stays at 1.1.3. The server accepts and refuses exactly what it did before this commit, so there is no change for a version to describe. The cost is recorded beside the constant: two documents now carry 1.1.3, the later a superset of the earlier, and a client cannot tell from the version alone which of the two it was generated from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a0be90a to
d7901be
Compare
This comment has been minimized.
This comment has been minimized.
Ten findings from review of the previous commit. The two that were defects rather than tidying: config_json_schema returned the _UNTABLED_PROPERTIES dicts by reference, so three of the fifteen properties were the module constant itself, shared by every call, while the twelve built by _numeric_property were fresh. Both callers hand their copy to a framework that mutates schema dicts in place, so one operation's published bounds could have rewritten the other's. Deep-copied on the way out, and the asymmetry that hid it is gone. The version note claimed the second 1.1.3 document was a superset of the first. It is not: `config` went from an object with no required keys and additionalProperties true to fifteen required fields with unknown keys forbidden, and its generated type from Config to NodeConfig. The document narrows, to what this server has always enforced, and renames. That is the honest statement of what carrying one version over two documents costs, and it is what the note says now. The rest: - The schema is hoisted into one NodeConfig component by generate_openapi, at the layer that already shapes the document, rather than inlined twice. Pydantic still cannot emit the $ref, but the generator can, and a substitution that fails by doing nothing is pinned by a test asserting both bodies are the reference. 103 lines of duplication leave the contract. - _numeric_property guards both ends against a non-finite bound, not just the high one. An unbounded low would have published a literal no JSON parser reads. - _REQUIRED derives from the two tables instead of restating three field names. - The boundary test parametrises off the published schema rather than a hand-written list, so a field added to either table is covered by the time it reaches the document. - numeric_branch is defined once, beside the builder whose output it reads, and selects the number branch by type rather than by position. - The untyped-config rationale is cited from routes/node_register.py rather than restated in three places. - A docstring paragraph left ragged by the previous commit is rewrapped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @Babissimo's task in 2m 34s —— View job Reviewing PR #342
Review: Publish the node configuration schema in the contract (#342)This PR was already reviewed once (see the earlier comment on this thread). Since then, commit Both prior findings are resolved
Fresh pass over
|
1 participant
Closes 86cb6d7he.
Why
RegisterRequest.configisdict[str, Any], so the generated contract describeda node's configuration as a free-form object. Fifteen fields and every one of
their bounds were absent, where the hand-written 1.1.1 published them. The bounds
were still enforced, only undiscoverable: a client generated from the published
file sent an unvalidated blob and learned the rules from 400s.
This is also what #278 ran into. Making the six coordinate fields nullable moved
services/node_config.pyand thenode_configstable, neither of which thegenerator can see, so its whole contract diff was the version line.
Published without being enforced
The field stays untyped, and that part is not negotiable: a Pydantic model there
would answer 422 ahead of the handler, putting a config-shaped refusal in front
of identity resolution and making the difference between it and a 403 an oracle
for which node identities exist.
WithJsonSchemareplaces what is published and leaves validation alone, so theshape reaches the document while the refusal stays where it was, behind identity
resolution and inside
validate_config. The guard on that ordering, intests/test_node_register.py, is unchanged and green.PUT /v1/nodes/configcarries the same object through itsopenapi_extra. Thatretires its "free-form here for the reason given on that endpoint", which pointed
at a rationale that only ever existed in a code comment.
What stops it drifting
config_json_schemabuilds twelve of the fifteen from_NUMERIC_BOUNDS, thesame table
validate_configloops over, so those cannot disagree. The callsignand the two beam fields are checked against their own literals, so they are
pinned separately, against
validate_configat the boundary rather than againstthe table:
math.nextaftereither side of every published bound, so a value thedocument permits is one this end accepts. Verified by breaking
beam_width_deg's published maximum to 300 and watching that test fail.Inline on both operations rather than one named component. Pydantic resolves
every
$refit emits against its own definitions, so a$refout ofRegisterRequest.configraisesKeyErrorfor a schema that is not one of itsmodels.
Also here
maximum. Their ceiling ismath.inf, which isnot a JSON Schema bound, and emitting it would state a limit the server does
not have.
contract with no route touched.
The version stays at 1.1.3
backend/routes/nodes.pyhad reserved a minor bump for publishing this schema.It is not taken: the server accepts and refuses exactly what it did before, so
there is no change for a version to describe.
The cost is real and is recorded beside the constant rather than left implicit.
Two documents now carry 1.1.3, the later a superset of the earlier, and a client
cannot tell from the version alone which of the two it was generated from. That
is survivable here because the difference is additive and no node reads the
config schema at runtime, but it is the reason the note is there.
Verification
Backend suite green,
pre-commit5/5, contract regenerated and--checkclean.No node-side change is needed: a minor bump adds description, and every config
the fleet sends today still validates.
🤖 Generated with Claude Code