feat(validator): enforce header names, allowReserved placement and non-empty server enums - #452
Merged
Merged
Conversation
…n-empty server enums
Six of the eleven missed negatives in A2, chosen because none of them
needs machinery oastools does not have. The OAS 3.2 negative suite moves
from 18/29 to 23/29, the first movement on that axis in this project.
Positives hold at 34/37 and no corpus verdict changes.
Header names must be RFC 9110 tokens. The rule covers header map keys and
a header parameter's name, both of which are field names on the wire.
allowReserved is rejected where in and style do not permit it, and on a
Header Object, where no version permits it at all.
A server variable enum that is present but empty permits no value, so not
even the required default could satisfy it.
Every one of the three turned out to be version-scoped, and two changed
between versions:
- the RFC 9110 token definition does not exist in the 3.1 schema, so
the header-name rule is 3.2+ only
- allowReserved permits in: query only at 3.1, and widened at 3.2 to
in: path, in: query, and in: cookie with style: form
- the server variable enum gained minItems: 1 at 3.1
A single ungated rule would have produced a new false positive in one
direction or a miss in the other, which is exactly the defect class #433
fixed. #440 is the issue that makes this class representable rather than
checked by hand each time.
The rules hook into schema_traversal.go rather than individual call
sites, so they inherit the structural reachability that walk exists to
provide: a header is a header wherever it occurs. Asserted across all six
positions rather than assumed, since #423 was the case where a correct
rule simply never ran in most places its object appears.
specBaseURL is extracted from validateOAS3, which held the version to URL
switch inline and so kept it out of reach of traversals that carry no
baseURL parameter. That is why the first draft of this change hardcoded
3.2 URLs. The convention now has one statement: cite the document's own
version when a rule varies by version, and oas32SpecRef when a rule
exists only at 3.2.
parser.Parameter.AllowReserved gains a doc comment recording where it
diverges from the specification. The rule is about the key's presence
rather than its value, and a bool cannot express presence, so an illegal
`allowReserved: false` decodes the same as an absent field. The OAI
fixture that uses false is therefore still uncaught. Changing the type
would break the v1 API.
Refs #434
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe validator now applies version-specific serialization constraints, validates header names and ChangesOAS 3 validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAPIDocument
participant Validator
participant schemaTraversal
participant serializationConstraints
OpenAPIDocument->>Validator: provide OAS document and version
Validator->>schemaTraversal: traverse parameters and headers
schemaTraversal->>serializationConstraints: validate header names and allowReserved
serializationConstraints-->>Validator: return validation metadata and specification reference
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
+ Coverage 86.80% 86.89% +0.09%
==========================================
Files 203 205 +2
Lines 29344 29415 +71
==========================================
+ Hits 25472 25561 +89
+ Misses 2557 2542 -15
+ Partials 1315 1312 -3
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@validator/oas3.go`:
- Around line 151-167: Update the empty-enum condition in the server-variable
validation to enforce the OAS 3.1 rule for unrecognized versions as well: apply
it when the version is invalid or at least parser.OASVersion310, while
preserving the existing nil-versus-empty enum distinction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b435b70-01fd-42f2-8fc1-b9aaebe94f8e
📒 Files selected for processing (7)
parser/parameters.govalidator/oas3.govalidator/schema_traversal.govalidator/serialization_constraints.govalidator/serialization_constraints_test.govalidator/spec_refs.govalidator/spec_refs_test.go
…-enum rule The empty server enum check required a recognized version before it would fire, while the other three version gates in this change treat an unrecognized one as in scope. That matches oas32TraversalApplies, which states the reasoning: a document the parser could not classify is still checked. Not reachable today. ParseBytes rejects an unknown version string and ValidateParsed rejects an unset OASVersion, so nothing arrives at the validator unclassified. It is consistency rather than a live defect, and worth correcting because the four gates are written separately and OAS 3.3 is in development. Extracted as emptyServerEnumApplies beside the other gates so all four can be read together, with a test covering each. Verified the test is not vacuous by reverting the gate, which fails the empty_server_enum_rule subtest. Also removes project narrative from four comments. Code comments document the code; the significance of a change belongs elsewhere. Each kept its technical half and dropped the rest, with #423 and #439 replacing the retellings. Refs #434
erraggy
added a commit
that referenced
this pull request
Aug 2, 2026
PR #452 merged as 6f94167. main is at 34/37 positive and 23/29 negative, re-measured after the merge rather than carried over from the branch. Negatives moved for the first time in this project. Adds docs/plans/handoff.txt, a fresh-session entry point: the standing rules a cold session must know before touching anything, where main stands, what is next and why, and the context that took a full session to learn. The state file remains the authority; the handoff says so and defers to it. Also reorders the session log back to newest first. Successive insertions had scrambled it, so the oldest entry was leading.
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
1 participant
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.
Part of #434. 🎯
Takes the six of A2's eleven missed negatives that need no machinery oastools does not already have, leaving the five mutual-exclusion rules (which do) for their own change.
The OAS 3.2 negative suite moves from 18/29 to 23/29, the first movement on that axis in this project. Positives hold at 34/37, and no corpus verdict changes across all ten specs.
The three rules
nameheader-object-name,parameter-object-header-nameallowReservedrejected whereinandstyledo not permit it, and on a Header Object where no version permits itheader-object-allowReserved,parameter-object-cookie-allowReservedenumthat is present but empty permits no value, so not even the requireddefaultcould satisfy itserver_enum_emptyAll three are version-scoped, and two changed between versions
This is the part worth reviewing carefully, because getting it wrong in either direction reproduces the defect class #433 fixed.
allowReservedon a Parameterin: queryonlyin: path,in: query,in: cookiewithstyle: formallowReservedon a Header Objectenumnon-emptyminItemsThe
tokendefinition does not exist in the 3.1 schema at all, and theallowReservedpermitted set widened in 3.2. Enforcing 3.1's rule everywhere would reject valid 3.2 documents; enforcing 3.2's would accept invalid 3.1 ones. Each was checked against both the published schema and the prose before being written, since the schema is explicitly not the source of truth.For 3.0 the rules are deliberately not enforced: its schema lists
allowReservedas a plain Parameter property with no conditional, and its prose describes effect rather than validity.#440 is the issue that makes this class representable instead of hand-checked every time.
Reachability
The rules hook into
schema_traversal.gorather than individual call sites, so they inherit the structural reachability that walk exists to provide: a header is a header wherever it occurs.Asserted rather than assumed, across all six positions a header or parameter can occupy:
components.headers, response headers inline and in components, encoding headers, operation parameters, and path-item parameters. #423 was the case where a correct rule simply never ran in most of the places its object appears, so a comment claiming reachability is not worth much without a test behind it.Incidental: one standard, one duplication removed
The first draft hardcoded
v3.2.0.htmlURLs, which is off-convention. The codebase already had a documented standard, but the version-to-URL switch lived inline invalidateOAS3and so was unreachable from any traversal without abaseURLparameter.specBaseURLis now extracted andValidator.specRefwraps it, using theoasVersionfield that exists so "version-sensitive checks can consult it without plumbing through every call". The convention now has one statement:oas32SpecRef) when a rule exists only at 3.2, which is whatoas32.goalready did.Verified: a 3.1 document gets
v3.1.0.html#parameter-object, a 3.2 document getsv3.2.0.html#parameter-object. The 74 existingbaseURLsites are untouched, since they already follow the standard.A known gap this does not close
fail/parameter-object-header-allowReserved.yamlusesallowReserved: **false**and remains uncaught, which is why this is 23/29 and not 24/29.The rule is about the key's presence:
unevaluatedProperties: falserejects it whatever it holds. Butparser.Parameter.AllowReservedis a plainbool, and a bool has two states where three are needed (absent, present-and-true, present-and-false).Explodebeside it is a*boolfor exactly this reason;AllowReservedwas not given the same treatment and has been public since close to the type's creation, so changing it now would break the v1 API.The field gains a doc comment recording the divergence. Addressing it properly belongs to a v2 driven by the published OAS artifacts rather than hand-transcription.
Testing
make checkgreen at 10,511 tests. New coverage:validator/serialization_constraints_test.go: each rule across 3.0, 3.1 and 3.2 in both directions (accepting where permitted, rejecting where not), the JSON decode path for the enum rule since nil-versus-empty is decode behaviour and parser keeps separate implementations, error detail (path, field,SpecRef) rather than only message presence, and the six-position reachability guardvalidator/spec_refs_test.go: every version mapping plus both fallback paths, and thespecRefaccessorVerified: negatives 18/29 → 23/29, positives unchanged at 34/37, corpus error counts identical across all ten specs.