feat(templates): lint keys the compiler ignores, at template compile (D67) - #554
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
angel-manuel
pushed a commit
that referenced
this pull request
Aug 12, 2026
`STOP` exists to stop the walk descending into author-supplied *data*, and every entry in it is a schema keyword. `Node::MapOf` was consulting it for the map's own keys too — but those are author-chosen names, so a request-body property legitimately named `default` or `enum` was never walked, while `collect_body_parameters` reads its extensions like any other's. A typo'd or misplaced extension inside such a property went unreported. Narrow, but the blind spot was at a position the compiler does read, which is the one place this lint must not be silent. Caught in review on #554. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hpwv2iQAGpmQykiZDgj86F
…539) A template document is an untyped `serde_json::Value` from `parse_yaml` to `compile_service`, so a key nothing reads is not an error — it is nothing at all. `services/metabase.yaml` carried `response_type: binary` on an operation for months: a real concept, in a position that never reads it, so a large export was buffered instead of streamed and the only evidence was the absent behaviour. D55 records the same shape from another angle. `openapi::lint_extensions` reports four classes, each asked *where* as well as *what*: an unknown `x-overslash-*` name, a known name at a position nothing reads it, an unprefixed alias where only the canonical spelling is read, and an unrecognized bare key at a position whose fields we enumerate. Neither motivating bug is a misspelling, so the last class is the load-bearing one — `response_type` is a `ServiceAction` field name, not an extension. Position comes from `openapi::ext`, which every extractor now reads through. `ext::get(obj, pos, ext)` carries a `debug_assert!` against the READS matrix, so a reader added without its entry fails in its own unit test rather than teaching the lint to warn about a correct template months later; `no_extension_getter_bypasses_the_accessor` holds the line. Warnings on every path, never errors: an error at `load_from_dir` would *skip* the template, and a missing service is worse than an ignored field, while an error on update would make an already-active stored template un-saveable by its owner. `shipped_services_lint_clean` is where that leniency is paid for. `template_resolve` lints stored rows into the resolution report the catalog already badges — the only place a template written before the lint is ever looked at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hpwv2iQAGpmQykiZDgj86F
…ry path SPEC §9 gains the four lint codes and the position/severity rules; TECH_DEBT records the one drift direction the accessor cannot close mechanically (a READS entry claiming a position no extractor reads makes the lint silent, and nothing detects that — the entries cite their readers, so it is reviewed, not proven). CHANGELOG is release-please generated, so the conventional commit on the feature carries it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hpwv2iQAGpmQykiZDgj86F
The misplaced-extension message read "it is read on an MCP tool and an MCP tool": `Pos::McpTool` and `Pos::McpToolDiscovered` describe identically on purpose, so the position list has to dedupe. And the editor's footer claimed "✓ Valid" for a document it had never checked — remote validation only runs on a document change, so an untouched draft showed the success state on arrival. That is the same overstatement as claiming "Valid" over a list of warnings, one step earlier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hpwv2iQAGpmQykiZDgj86F
`STOP` exists to stop the walk descending into author-supplied *data*, and every entry in it is a schema keyword. `Node::MapOf` was consulting it for the map's own keys too — but those are author-chosen names, so a request-body property legitimately named `default` or `enum` was never walked, while `collect_body_parameters` reads its extensions like any other's. A typo'd or misplaced extension inside such a property went unreported. Narrow, but the blind spot was at a position the compiler does read, which is the one place this lint must not be silent. Caught in review on #554. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hpwv2iQAGpmQykiZDgj86F
angel-manuel
force-pushed
the
feat/template-extension-lint
branch
from
August 12, 2026 17:15
9948ed0 to
61403ca
Compare
…silently
`compile_extension_actions` assembles extension operations into a synthetic
document where `paths.{path}.{method}` holds a single operation, so two actions
bound to the same method+path compile into one — and the loser is dropped with no
diagnostic. Which one loses depends on `HashMap` iteration order, so the same
stored delta could resolve differently between processes.
An error rather than a warning, unlike the rest of this branch: a key nothing
reads costs one field, this costs a whole action, and `validate_delta` is a
write-time gate with the author right there. Reported against the sorted pair so
the message is deterministic.
Found while checking a review claim that a lint dot-path could be misattributed
in this case. It cannot — `synthetic_to_key` and the operation map are overwritten
in lockstep, so a finding names whichever operation survived — but the silent drop
underneath the claim was real, and predates this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hpwv2iQAGpmQykiZDgj86F
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #539.
Why
A template document is an untyped
serde_json::Valuefromparse_yamlall the way tocompile_service, so a key nothing reads is not an error — it is nothing at all.services/metabase.yamlcarriedresponse_type: binaryonexport_queryfor months: a real concept, in a position that never reads it (the compiler derives the response type fromresponses:), so a large xlsx export was buffered againstmax_response_body_bytesand the only evidence was the absentprefer_streamhint. D57 fixed that instance. Nothing would have caught the next one.Neither motivating bug is a misspelling, and that decided the scope.
response_typeis aServiceActionfield name and D55'sresolve:is an alias, so a lint overx-overslash-*names alone would have caught neither — while a closed-world bare-key check alone would have missedx-overslash-downloadon an HTTP operation and every stray key at an open-world position. Each half misses the other's motivating case.What
openapi::lint_extensionsruns on the alias-normalized document at every validation entry point and reports four classes, each asked where as well as what:unknown_extensionx-overslash-*name nothing reads, with aclosest_matchsuggestionmisplaced_extensionunprefixed_alias_ignoredunknown_template_keyresponse_type:Position comes from
openapi::ext, and every extractor now reads through it.ext::get(obj, pos, ext)carries adebug_assert!against theREADSmatrix, so a reader added without its entry fails in its own unit test rather than teaching the lint to warn about a correct template months later.no_extension_getter_bypasses_the_accessorbans theobj.get("x-overslash-…")spelling inopenapi/production code. That mechanism earned itself during this PR: #550 landedx-overslash-iconondevmid-flight, and the guard failed on the unregistered reader on the first run after rebasing.Severity: warnings on every path, never errors
Both strict options are worse than the disease. An error at
registry::load_from_dirmeans the template is skipped — a stray key would remove a service where before it removed a field. An error on create/update would make an already-active stored org or user template un-saveable by its owner, on tenant data nobody can survey, over a key that was inert the whole time.So the teeth are in CI:
shipped_services_lint_clean, filtered onLINT_CODESrather thanwarnings.is_empty()so an unrelated warning can neither disarm nor break the gate. Andtemplate_resolvelints the stored document into the resolution report the catalog already badges — the only place a row written before the lint existed is ever looked at, which is how the affected tenant population becomes visible without a migration or a grandfathering table.The sweep
services/was already clean — a strict closed-world scan over all 23 shipped templates found zero findings, confirmed byshipped_services_lint_cleanpassing on its first run. The real offenders were elsewhere:dashboard/.../templates/new/+page.svelteseededx-overslash-prefixon atype: apiKeyscheme. D35 removed that key andextract_api_keyrejects it outright, so the default scaffold could not be saved at all. Nothing tested it, because it lived in a string literal in a component;scaffold_skeleton_is_valid_and_lint_cleannow reads the skeleton out of the Svelte source so the two cannot drift.APIKEY_HTTP_SEC_ALIASESsplit intoAPIKEY_SEC_ALIASES/HTTP_SEC_ALIASES:extract_http_authreads onlydefault_secret_nameandlabel, so normalizingtemplate:/secret_source:/optional:onto anhttpscheme only ever produced a canonical key nothing then read.normalize_body_properties_in— the alias walk never descended into a request body's schema, thoughcollect_body_parametersreads the canonical keys there. This is the HTTP twin of D55'sinput_schemafix, and a live behaviour change with zero shipped-template impact (all nine body-property annotations already use the canonical spelling).x-overslash-sql, a key that never existed (it issql-field).Path-item level stays un-extended on purpose:
risk:hoisted out of a method is not a concept, so the lint reporting it is the right outcome.Dashboard
Both surfaces were silent before. The editor footer claimed
✓ Validover a list of warnings; the draft page renderedimport_warningsand droppedvalidation.warningsentirely; the layer editor rendered warnings with nopath.The two fixes in 466fe09 came from looking at the screenshots: the message read "read on an MCP tool and an MCP tool" (two positions share a description on purpose), and the editor asserted
✓ Validfor a document it had never checked, since remote validation only runs on a document change.Notes for review
template_validation::validate_service_definition". That function takes the compiledServiceDefinition, where extension keys no longer exist, so the lint runs intemplate_validation's three YAML entry points instead — the same wrapper layer that already ownscheck_duplicate_operation_idsfor the same reason. Both consumers the issue names are reached: the shipped-template test and the user/org authoring path.discovered_toolssnapshot, a platform-action param, and an unrecognized security-schemetypeaccept unknown bare keys, because their sibling keys are vocabulary we do not own.data_fields_sharing_a_concept_name_are_not_flaggedpins the false positive that matters — a payload field genuinely namedriskortemplate. The gate caught one such bug in the lint itself before merge:x-overslash-mcp.auth.provideris a read field that shares a name with anoauth2scheme'sprovideralias, so a position's own fields now win over the extension vocabulary.TECH_DEBT.mdrecords the one drift direction the accessor cannot close: aREADSentry claiming a position no extractor reads makes the lint silent, and nothing detects that. Entries cite their reader lines, so it is reviewed, not proven./vet --agenticclean. 693 core unit tests + 13template_validationintegration tests pass;cargo clippy --workspace --all-targetsandsvelte-checkclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01Hpwv2iQAGpmQykiZDgj86F