Generate the OpenAPI specification from the firmware route table - #801
Generate the OpenAPI specification from the firmware route table#801chrisgleissner wants to merge 11 commits into
Conversation
…e environment Claude-Session: https://claude.ai/code/session_01BDjsfEkpZj5ZvrEbae6r9U
|
Hi @GideonZ, thanks for reviewing and approving this PR. It ensures that the programmatic description of our REST API is generated from the source code. This in turn ensures that the two can never again diverge. It also simplifies REST API evolution. Thanks |
barryw
left a comment
barryw
left a comment
There was a problem hiding this comment.
Christian, thank you for the careful implementation and especially for putting the generator under tests. I reviewed head 94e99bcd, ran make openapi_test locally (120 tests pass), and ran make openapi_check (both committed documents match). The overall source-adjacent approach looks strong.
I found a few cases where the generated contract can still become misleading or where the new explorer increases the trust placed in external code. I have included concrete reproductions and kept existing firmware behavior separate from changes introduced here.
1. External Swagger code runs with device-page privileges
api.html loads swagger-ui-dist@5 from UNPKG. A script loaded through src runs in the context of the current page and can do anything the page can do, even when fetched cross-origin (MDN). Here that includes invoking the local firmware API and observing an X-Password entered into Swagger.
There are two compounding details:
@5is a semver range rather than an immutable version. UNPKG documents that ranges resolve to the newest matching release (UNPKG).crossorigindoes not provide integrity or sandboxing. SRI is the browser mechanism that locks an external script to reviewed content (MDN SRI).
I realize that index.html already loads CDN JavaScript. The concern is more acute on an interactive API explorer because it deliberately exposes powerful operations and authentication. The smallest safe option is to serve the YAML without executing third-party code. If the embedded explorer is retained, could the dependency at least use an exact version plus an integrity hash, with a small static test preventing an unpinned script from returning?
2. The machine-readable security requirement is unconditional
The prose correctly says that X-Password is required only when a password is configured, but the generated root declares only NetworkPassword. In OpenAPI this means that the scheme is required globally. The OpenAPI 3.1 specification explicitly says that optional security is represented by including an empty Security Requirement Object, {}, as an alternative (OAS 3.1).
Would this describe the actual conditional behavior more accurately?
security:
- {}
- NetworkPassword: []The existing prose can continue explaining when the server actually demands the header. A unit assertion on the generated root would keep the machine-readable and human-readable descriptions aligned.
3. Product equivalence currently compares source filenames, not compiler definitions
PROFILES supplies only an empty U2 definition set and U64=1, while compiled_sources checks only whether each target Makefile names the same route files. The real targets use differing definitions such as U2, U2P=1, U2P=2, U64=1, U64=2, RISCV, and NIOS.
I confirmed that the current API_CALL registrations are gated only by U64, so the two documents generated today appear correct. The risk is to the stronger guarantee that firmware and contract cannot diverge: a future call under #if U2P, #if RISCV, or #if U64 == 2 can pass the current check.
A minimal adversarial fixture demonstrates this: adding OPTIONS += -DBIG to the fixture target Makefile while leaving the profile definitions empty causes the generator to omit the call that the target compiles, and generation still succeeds.
Could the gate compare active call tables for each actual target definition set, or at minimum reject conditional identifiers that the profile has not explicitly modeled? A fixture with identical route filenames but different active calls would make a good regression test.
4. Duplicate document keys can silently replace earlier declarations
There are several fail-open collision paths in document.py:
- path and query declarations are converted directly to dictionaries;
- response media entries overwrite the same status/content-type pair;
- a repeated path plus method overwrites the previous operation.
Against the exact PR head I confirmed:
duplicate path+method -> only the replacement operation remains
duplicate PARAM -> the replacement declaration remains
duplicate RESPONSE -> the first description remains with the replacement schema
All three builds succeed. These look best handled by rejecting the second declaration, matching the existing duplicate-BODY and duplicate-operationId behavior. Three small fixture tests would close the gap.
5. CI checks freshness and custom unit tests, but not OpenAPI validity
The workflow runs only make openapi_test && make openapi_check. The standard validator and generated-client checks live in the hardware-oriented E2E suite, so they are not part of this PR build gate.
The documents may be valid today, but a future generator change can produce fresh, unit-tested YAML that is invalid under OpenAPI 3.1. Could the host-only validation and client-generation portions be split into a CI target? Even running only openapi-spec-validator in CI would independently verify the artifact that gets uploaded.
6. Existing firmware issue exposed by the documentation: 203 versus 204
This is not introduced by this PR. The generator accurately repeats existing firmware wording: routes.h emits HTTP/1.1 203 No Content, and the new description repeats it. RFC 9110 defines 203 as Non-Authoritative Information and 204 as No Content (203, 204).
I would treat the firmware status change as a separate compatibility decision. For this PR, it may be worth calling 203 a legacy/non-standard response rather than presenting 203 No Content as standard HTTP semantics.
Small fail-closed parser checks
Two low-cost validation cases also reproduced on the PR head:
_as_declared("boolean", "ture") -> False
string_literal("\x41") -> "x41"
The first silently turns a typo into a valid value. The second silently changes a valid C hexadecimal escape instead of decoding or rejecting it. Rejecting unsupported values/escapes would be safer than generating a plausible but incorrect contract, and each case is a very small unit test.
Thanks again for building this around deterministic generation and tests. The comments above are aimed at preserving the strong no-divergence guarantee as the API evolves, not at changing the overall design.
|
Thanks for the very thorough review @barryw . It is much appreciated. I will address your findings. |
Derive each target's macros from its own makefile, refuse a second declaration, pin the explorer's third-party code, and validate the documents in CI. Claude-Session: https://claude.ai/code/session_012Cpx38cC2t4pK9c73GnP1A
|
Thanks @barryw. Every reproduction in your review was correct against
1. External Swagger code runs with device-page privilegesFixed, and further than the minimum you asked for.
The policy is what bounds the damage if the pinned bytes are ever served by something other than unpkg:
I verified all of this in a browser against a local server, rather than only asserting it in a test:
That last case also confirms the recorded digest independently: Chromium reported the computed digest as The static test you asked for is I did consider dropping the explorer and serving only the YAML. The explorer is 1.6 KB of the 126 KB the updater grows by, and it is what makes the document usable on a device with no internet-connected tooling, so pinning it seemed the better trade. The policy above is what makes me comfortable keeping it. 2. The machine-readable security requirement is unconditionalFixed as you suggested. The generated root is now: security:
- {}
- NetworkPassword: []The prose is unchanged. Writing that turned out to need a fix in the YAML writer first, which is finding 9 below: an empty mapping inside a list was being written as the quoted string 3. Product equivalence compares source filenames, not compiler definitionsFixed, and by removing the hand-written definition list rather than extending it.
What that reads from the real targets: A call under Your adversarial fixture is now the regression test. Two more cases came with it, both of which the old check would also have passed:
The comparison is over the full registration and the full directive list, not just the set of routes. 4. Duplicate document keys can silently replace earlier declarationsFixed. All three of your cases are now refused, and I found a fourth of the same kind:
Rewriting 5. CI checks freshness and custom unit tests, but not OpenAPI validityFixed. There is a new Following your note, and after a round on this, it runs openapi_validate:
@$(PYTHON) -c "import openapi_spec_validator" 2>/dev/null || { ...how to install...; exit 2; }
@$(PYTHON) -m openapi_spec_validator --validation-errors all `$(OPENAPI) paths`The file list comes from Three deliberate choices:
Red and green, on the committed documents: The generated-client checks stay in the E2E suite, since generating a client is slow and the suite already does it against a device. 6. 203 versus 204I agree this is a firmware compatibility decision and not one for this PR, so the firmware is unchanged and the document now says what the status actually is. The Responses section reads:
The reason phrase in Small fail-closed parser checksBoth fixed.
From my own review of the branchFive more, same class as the ones you found. 7. An empty mapping in a list was written as a quoted string. 8. A control character in a key produced YAML that will not parse. A 9. One 10. Nothing held the embedded document to the product that embeds it. Which document an updater writes to 11. Verification
One thing I have not done: |
The image cannot create a virtual environment and refuses a write to its own packages, so install to a directory and reach it by PYTHONPATH. Claude-Session: https://claude.ai/code/session_012Cpx38cC2t4pK9c73GnP1A
The build image has neither pip nor ensurepip, so run this one step in a stock Python image, and hold it to the make target with a test. Claude-Session: https://claude.ai/code/session_012Cpx38cC2t4pK9c73GnP1A
|
One correction to finding 5 above, and how it ended up. I wrote that the new CI step installs the validator into its own virtual environment. That is not what it does now, because it could not: the build image cannot install a Python package at all. Two pushes found this out, which is worth recording since it constrains anything similar in future.
So the docker run --rm -v $GITHUB_WORKSPACE:/mnt/project:rw --workdir /mnt/project \
--user $(id -u):$(id -g) -e HOME=/tmp python:3.12-slim /bin/sh -c \
"pip install --quiet --disable-pip-version-check --root-user-action=ignore \
--target /tmp/openapi-packages -r tools/openapi/requirements.txt && \
PYTHONPATH=/tmp/openapi-packages python -m openapi_spec_validator \
--validation-errors all \$(python tools/openapi/generate.py paths)"Nothing else in the build uses that image, and no other step depends on this one. That image has no
CI on |
2 participants
Closes #800.
The REST API now has a machine-readable description that the build produces from the firmware itself, one document per product family. Both are committed, both are attached to every CI build, and every device serves its own copy at
/openapi.yamlwith a Swagger UI page at/api.html.What the build produces
doc/api/rest_api_openapi_u2.yamldoc/api/rest_api_openapi_u64.yamlmake openapiwrites them,make openapi_checkrebuilds them in memory and fails when the committed files differ, andmake openapi_testruns the generator's own tests.openapi_checkis a prerequisite of every firmware target and a CI step that runs before the builds, so a route change that is not reflected in the document stops the build the way an over-size image does. The two files are also uploaded as a newopenapi_<version>build artifact.Where the description lives
Every REST call is registered through the
API_CALLmacro insoftware/api/routes.h. That registration already carries the verb, the route, the command, the query parameters and which of them are required. The parts a route cannot know, the prose and the response shapes, are written in anAPI_DOCblock directly above it:API_DOCis defined as#define API_DOC(...)and nothing else. Because its parameters do not appear in the replacement list, the preprocessor never expands what is inside it either, soSUMMARY,PARAMand the rest need no definitions and cannot collide with anything else in the tree. The compiler still checks that the block is a balanced token sequence with terminated string literals.The only other change to the running firmware is one
#includeline inroutes.h. No handler, no route table and no runtime behaviour is touched.The workflow, and who keeps the documents current
The author does. Changing the REST API is three steps:
API_CALL, or theAPI_DOCblock above it, or both.make openapi. It reads the route sources and rewritesdoc/api/rest_api_openapi_u2.yamland..._u64.yaml. It needs no device, no toolchain and no packages: a few hundred milliseconds of standard library Python.Forgetting step 2 is caught rather than shipped, because a firmware build will not complete without it:
openapi_checkis a prerequisite of every firmware target, so a localmake u64fails the same way CI does.CI does not write them, it re-derives them and compares.
make openapi_checkrebuilds both documents in memory from the sources in the checkout and compares the rendered bytes against the committed files, failing and naming the stale one. Comparing rather than overwriting is deliberate: a step that regenerated and committed would hide the author's mistake instead of reporting it.It runs in two places, so a stale document is caught whether or not CI is involved:
Check OpenAPI Specificationstep, before the firmware builds;Makefile.What that looks like when an author edits a committed document by hand and forgets to regenerate:
and when they are current, which is what CI prints:
Why the documents are in git
Two things follow from the documents being versioned beside the code rather than produced and thrown away.
The contract changes visibly. A pull request that adds a parameter, changes a status code or renames a route shows that in the diff of
doc/api/, next to the code that caused it. Reviewing an API change stops depending on somebody noticing the implication of aroute_*.ccedit.Compatibility becomes checkable. Two committed revisions of the same file are what every OpenAPI diff tool takes as input, so
oasdiff,openapi-diffor Redocly'sdiffcan say whether a change is breaking, against any earlier commit or tag. Nothing in this PR wires that up, but the input it needs now exists and is trustworthy, which it would not be for a document maintained by hand.What the generator refuses
tools/openapi/generate.pycross checks the two halves before it writes anything. Each of these stops the build:API_DOCblock, or a block with no call;BODYwhere the call has no body handler, or a body handler with noBODY;{placeholder}with noPATH_PARAM, or aPATH_PARAMno path uses;operationIdused twice, a schema name that does not exist, a tag that is not defined;Marking the calls that need care
Nothing is omitted: all 53 registrations appear in the document for the product that serves them, and the generator fails the build if one is missing. Hiding a call would only make an agent guess. Three kinds of call are marked instead, each declared at the call site and validated:
GET /v1/helpis registered and was never implemented, so it carriesdeprecated: trueand says why. Every generator marks the method deprecated.debugreg,measureandheapare all underDiagnostics, described as hardware debugging rather than as a way to drive the machine.x-ultimate-caution, naming what those are from a closed vocabulary the generator enforces:destructive,machine-state,persistent,power,diagnostic,idempotent. The names follow the shape of MCP tool annotations, so a server wrapping this API maps them onto its own.One
CAUTIONdirective produces both renderings, so they cannot disagree:The prose is what a person reading the operation in Swagger UI or Redoc sees, above the Try it out button. The field is what an agent gates on. The document explains the vocabulary in its own description, so a consumer does not have to find this pull request to know what a token means.
Two documents, derived rather than declared
Which calls a product serves is decided at compile time by two things, and both are read from the tree rather than restated:
machine:debugregis in the Ultimate 64 document only;route_streams.ccis in the Ultimate 64 makefiles only and the cartridge document has no/v1/streamspaths at all.The generator also requires every makefile within a family to compile the same set, so a product drifting away from its family is a build failure rather than a wrong document.
Nothing reaches the firmware image
The prose has no use at run time and the U2 application partition has little to spare. Two pieces of evidence:
g++ -Eover a file containing anAPI_DOCblock emits zero occurrences of its text, and the block compiles clean under-Wall -Wextra -pedantic.stringsover the three application ELFs built from this branch finds zero occurrences of the documentation text:The device serves its own contract
The document rides in the updater and is written to
/Flash/htmlnext toindex.html, soGET /openapi.yamlreturns the contract for the firmware that device is running.GET /api.htmlis a 1.6 KB page that loads Swagger UI from a CDN and points it at the local document; when the browser has no internet it shows a link to the raw file instead. Its request interceptor rewrites the host of every call to the device the page was served from, so Try it out talks to that machine rather than to the example host in the document.index.htmlgains one link to it. The static file server already serves that directory on every product, so httpd is unchanged.Verified in a browser against an Ultimate 64 Elite with both files in place: Swagger UI renders all 58 operations in the nine tag groups, with no console errors. The device serves the document as
text/plain, because the static file server's type table has no.yamlentry, and Swagger UI parses it regardless.Cost, measured:
The updater images grow by the size of the document plus the 2 KB explorer page. Measured on the CI artifacts of the commit before this work and the current head:
update.u2rupdate.u2pupdate.u2lupdate.u64update.ue2update.cfwThe tightest is the Ultimate II, which links into a 4 MB window and is now at 1.92 MB. The runtime application is not affected, and the
app_spacegate measures that image, not the updater.The Ultimate II was checked specifically, because it is the product with the least room. It has networking through a USB AX88772 Ethernet adapter, and
usb_ax88772.cc,httpd.ccand the static file middleware are all in its source list, withWeb Remote Control Serviceenabled by default. CI builds and links all five products with the document embedded, the Ultimate II included.What it looks like on the device
All four are an Ultimate 64 Elite on firmware built from this branch, serving its own document.
GET /openapi.yaml. The header explains what the document is, where it came from, and what the caution vocabulary means, so a reader who arrives with nothing else has everything they need.GET /api.html. Operations grouped by tag.GET /v1/helpis struck through, which is Swagger UI renderingdeprecated: true.An operation that has consequences shows what they are above the Try it out button, and the shared 403 appears with its example.
Try it out, against the device that served the page. The request URL is
http://u64/v1/info, not theultimate64default in the document, because the page rewrites the host to wherever it came from. The response is that machine answering, alongside the example the document declares.How it is verified
Against the standard. Three independent checks, all clean on both documents:
Spectral's first run found 130 real problems, which are fixed here: examples were serialised as strings of JSON rather than as values, and two configuration response schemas put
additionalPropertiesacross anallOfwhere it also caught theerrorsarray. Generating a client also showed the server URL repeating the/v1that the path keys already carry, which would have made every generated client build/v1/v1/....Against a real generator.
openapi-python-clientturns each document into a typed client package. Theopenapi-contractsuite does this during the run and then calls the device through the generated code:Against real device traffic.
tests/lib/openapi_contract.pychecks each answer against the document: that the status code is declared for that operation, and that a JSON body validates against the declared schema, usingopenapi-schema-validatorfor the OpenAPI 3.1 dialect. The check sits intests/lib/rest.py, which is the only HTTP client the suites use, so./run-tests --validate-openapiapplies it to every existing suite without a suite being changed. It is off by default; the new suite turns it on for itself.Run against an Ultimate 64 Elite on firmware built from this branch:
That second run put every response those two suites received through the validator, 58 checks in all, without either suite being changed.
The check costs 0.08 to 0.13 ms per response with the validators cached.
In CI. The build for this branch is green, and the
openapi_<version>artifact it produced is byte identical to the two committed documents.Both products, through their own generated clients. A client was generated from each document and pointed at the matching device, an Ultimate 64 Elite and an Ultimate II+L, both on firmware 3.15. Every call answered as its document says. The two SDKs differ in the way that matters: the Ultimate II package has 54 operations and no
streamsmodule and noread_debug_register, the Ultimate 64 package has 58 and both. The device agrees:/v1/machine:debugregand/v1/streams/video:stopanswer 200 on the Ultimate 64 and 404 on the cartridge.Unit tests. 120 tests under
tools/openapi, run bymake openapi_test, covering the C++ reading, the#ifevaluation, the pairing rules, every refusal listed above, the YAML writer, and the shape of the real documents. 16 more intests/lib/openapi_contract_test.py, registered as the device-freeopenapi-validatorsuite, pin the validator's verdicts so that a validator which accepts too much or refuses too much is caught before it runs against hardware.Using it
Three host packages are added to
tests/requirements.txt:openapi-python-client,openapi-schema-validatorandopenapi-spec-validator, plusPyYAML. They are needed by the E2E gate only. The firmware build needs nothing that is not already there: the generator is standard library only and writes its own YAML.Limits
/Flash/htmlis written by the updater, so a device that had only its application replaced, which is what a JTAG load does, keeps the document from whichever release last ran the updater. The suite reports that as a warning naming the cause rather than as a failure.GETandPOST /v1/machine:input, andPUT /v1/machine:poweroff. The firmware registers them and answers 501 with a reason, which is more use to a client than a 404, so the cartridge document describes them as refusals with no success response. Spectral'soperation-success-responsewarns about exactly those three, which is it reporting the firmware's shape rather than a defect in the document. Everything else is clean at hint severity.GET /v1/helpis registered by the firmware and does nothing useful. It is documented as what it is, because leaving it out would mean the document no longer matches the route table.