[fuad-daoud]

Summary

{type: oauth2, flows: {}} is valid OpenAPI, since every member of the OAuth Flows Object is optional. It lowers to an oauth2 scheme with no flows and nothing reports it (#646). The scheme states no way to obtain a token, yet a requirement naming it keeps a live AuthID for a mechanism no emitter can implement.

Rule. An oauth2 scheme that lowers no flow and names no oauth2MetadataUrl gets one openapi/oauth2-no-flow warning at its declaration, unless its flows fails the loader's REQUIRED-field rule, which reports it instead.

One reporter per spelling. The parser library's REQUIRED-field rule fires on !core.Flows.Present || Flows == nil. That covers flows absent, not an object, or holding a flow that fails to decode: {implicit: null} leaves the object non-nil but not Present. flowsMissing restates that condition over the fields the library reads, and the lowering stands aside wherever it holds, so the rule's error stands alone. A plain "flows is nil" check would not do: it gives {implicit: null}, {implicit: []} and the like an error and a warning for one defect. No spelling probed reaches the Flows == nil half with the pinned library: every Present flows object decoded to a non-nil value. It is kept so the restatement stays the library's own condition.

Why the rule stays split, and not the design #803 sketches. #803 proposes handing the whole rule to the compiler: drop the library's finding in the loader and report every spelling here, with an absent flows as an error. That needs a loader-side predicate that recognizes the library's finding by its node. The predicate #803 sketches ("a mapping with type: oauth2 and no flows key") misses {implicit: null}, whose flows key is present. Matching that spelling would mean re-deriving the library's decode rules in the loader. It would also make flows the one REQUIRED field of a security scheme whose loader finding is dropped in favour of the compiler's. Every other one keeps the loader's finding: apiKey name and in, http scheme, openIdConnectUrl, and each flow's URLs and scopes. And it would change the absent spelling's diagnostic code. Deferring through the library's own condition gives each spelling one reporter, with severity following validity: absent is an error, empty is a warning. No existing diagnostic code changes. What #803 would still add is a JSON pointer on the absent spelling's report. That is a question about every loader finding, not this one, so #803 stays open for it.

Placement. The report sits at decl, not entry. A flowless declaration is a fact about the text where its fields are written, and a $ref entry holds no flows. The message names no entry, so the copies drawn by each alias of one declaration are identical and compile.Diags keeps one. A declaration in another document has no pointer here, so each $ref naming it carries its own copy at its entry, as every finding placed at decl does today (#874).

Interned, not refused. #294 refused an entry naming no mechanism because ir.AuthKind has no value for "the document did not say which mechanism". Interning it would have asserted that the API is authenticated by nothing in particular. Here the IR states exactly what the document said, oauth2 with no flows, so nothing is misstated. Refusing would delete declared text (description, x-*) and drop every requirement naming the scheme, for a defect the IR can hold. The compiler already interns two equally unusable siblings, apiKey with no name and openIdConnect with no URL. The line it draws is the mechanism token (fillSchemeKind), and that line is unchanged.

Warning, not error. The shape is valid OpenAPI and the document lowers whole. This is the same class as ReservedHeaderName, InvalidMethodKey and EmptyEnum: the shape is represented exactly but unusable. An error would also stop harness.Check at the first error diagnostic and keep any fixture carrying the shape out of irverify. --fail-on warning is the caller's lever.

The metadata URL exempts the entry. In 3.2.0, oauth2MetadataUrl points at RFC 8414 authorization-server metadata, which states the endpoints. flows is REQUIRED, so {type: oauth2, flows: {}, oauth2MetadataUrl: …} is how a valid metadata-only scheme is written. The exemption follows the IR field, so a 3.1 document whose URL the parser reads is exempt too. An empty oauth2MetadataUrl: '' names no server and is reported.

Where it lives.

• compilers/openapi/internal/auth/auth.go: tokenless (an oauth2 scheme with no lowered flow and no metadata URL), flowsMissing (the library's condition) and oauthNoFlowDiag, called after the named guard in lowerSecurityScheme.
• compilers/openapi/internal/diag/diag.go: diag.OAuth2NoFlow and its design record.
• ir/auth.go: AuthKindOAuth2 said "with one or more flows". It now says Flows may be empty, since OAuth2MetadataURL can name the server instead and a source can state neither. docs/ir-design.md §9 says the same. The OpenAPI row records the rule beside the other "lowered as declared + warning" entries. No IR type changes, no Options switch, no irverify rule: nothing is inferred, and a flowless scheme is a problem in the source, while irverify reports compiler bugs only.

Test plan

• TestLowerSecuritySchemes_AnOAuth2SchemeWithNoFlowIsReportedOnce names, for each spelling, who reports it. Every row compiles through the real loader and identifies the library's finding by its rule and the field its message names.
  • The lowering, once, as a warning at /components/securitySchemes/s: flows: {} (the issue repro), a flows object holding only an x-*, one holding only a key that names no flow, and a 3.2 empty oauth2MetadataUrl.
  • The loader alone: flows absent, null, a sequence, and {implicit: null}.
  • Nobody: an empty flows object beside a metadata URL in 3.2 and in 3.1, one declared flow, one device flow, and an apiKey scheme carrying flows: {}. The last pins the kind guard.
  • Every row also pins that the scheme interns and the requirement naming it keeps a live AuthID.
• TestLowerSecuritySchemes_AFlowlessDeclarationIsReportedOnceWhereItIsWritten: two $ref aliases are declared before the flowless target they share. All three intern, and the declaration is reported once, at the target, with nothing at either alias.
• TestPresentFlows_ReadsEverySourceFlowField sets each flow field of soa.OAuthFlows alone, through reflection, and requires presentFlows to read it. The rule reads an empty lowered list as "declares no flow", which holds only while every flow field is read. A field the library gains would otherwise be lost and draw a false report.
• Conformance spec testdata/conformance/openapi/oauth2-no-flow.yaml, with its golden and table row. It holds flows: {}, a flows object with only an unknown key, two aliases declared before their target, and two silent controls: a 3.2 metadata-only scheme and a declared flow. morphic-harness passes on it, so the corpus oracles (order invariance included) reach the rule.
• Mutations, each planted on the final tree, observed and reverted. A no-op control stays green.
  • Delete the branch: the table, the placement test and TestConformance redden.
  • Drop the kind guard: the table's apiKey row reddens.
  • Drop the deferral to the loader: the table's loader rows redden.
  • Weaken the deferral to GetFlows() == nil: the {implicit: null} row reddens.
  • Drop the metadata exemption: the table and TestConformance redden.
  • Drop the flow count: the table, TestConformance and TestGolden redden.
  • Report at entry: the placement test and TestConformance redden.
  • Error or info severity: the table and TestConformance redden.
  • Drop the device flow from presentFlows: the reflection test reddens.
• Real-world differential: a local corpus of 1,840 OpenAPI 3 documents, compiled with main at bdbabdb and with this branch. The branch's IR and stderr equal main's once oauth2-no-flow diagnostics are removed, and every exit code matches. One document gains the warning: its flows object writes authorizationUrl, tokenUrl, refreshUrl and scopes directly, with no flow name around them.
• make gate (fmt, vet, lint at the pinned v2.14.0, build, coverage at 100%, fuzz, bench-smoke) passes on dcc6e28.

Deliberately out of scope

• A flow that is present but incomplete (missing tokenUrl, authorizationUrl or scopes). That is the loader's own finding, not what "declares no flow" means.
• An object the library never validates. A scheme a $ref reaches under an x-* key draws none of the findings the library's Validate makes, so an absent flows there is reported by neither side, as main already behaves. The same holds for every REQUIRED field of every object at such a location, so it is fixed in the loader, not here: openapi: an object a $ref reaches under an x- key is never validated #872.
• One report, located in that document, for a flowless declaration in another document that several $refs name. Every finding placed at decl repeats at each entry there today, so it is fixed in the placement, not here: openapi: a finding about another document's declaration repeats per $ref #874.
• A JSON pointer on the loader's report of an absent flows. That is openapi: the oauth2 flows rule is split between the loader and the compiler #803's remainder, and it concerns every loader finding.
• Whether a 3.0 or 3.1 document that writes oauth2MetadataUrl should be warned. That is openapi: a 3.2 field the parser models lowers silently in a 3.0 or 3.1 document #864; this rule reads the field as the parser does.
• An irverify rule. A flowless scheme is a problem in the source, and irverify's findings are compiler bugs only.

Closes #646.

[OmarAlJarrah]

Corrected after a second pass: I don't think this reaches the bar yet, contrary to my first summary. The verdict and the ordered list of gaps are in the PR comment below, and each point is detailed inline.

One item outside the diff: the merge commit 97b5fb2 carries an AI co-author trailer and a session URL in its body. The branch shouldn't be rewritten, so set the squash body explicitly at merge time rather than letting a COMMIT_MESSAGES paste carry it onto main.

[OmarAlJarrah]

The rule behind this message reports a valid, usable document. In OpenAPI 3.2.0, oauth2MetadataUrl is "URL to the OAuth2 authorization server metadata [RFC8414]", and RFC 8414 metadata states token_endpoint and authorization_endpoint. flows is REQUIRED in 3.2.0, so {type: oauth2, flows: {}, oauth2MetadataUrl: ...} is the only way to write a metadata-only scheme. Compiled under openapi: 3.2.0 it prints this warning and exits 0, and --fail-on warning then rejects a spec-valid document. #646's own premise is a scheme that "describes no way to obtain a token"; a metadata URL is one.

The PR body and diag.OAuth2NoFlow record that the URL deliberately does not exempt the entry. I disagree on the merits and would reverse it: a warning that fires on valid input that says how to get a token is a false positive, not a stricter lint. The fix is one more clause, scheme.OAuth2MetadataURL == "". On a prototype, a 3.2.0 metadata-only scheme is silent and flows: {} without the URL still reports.

The flows empty with a metadata url row would then become the silent control. It compiles under openapi: 3.1.0 today, a version that doesn't define the field, so it should compile under 3.2.0.

[fuad-daoud]

Agreed, and reversed. declaresNoFlow now returns false when oauth2MetadataUrl is set, so a 3.2.0 metadata-only scheme is silent, and flows: {} without the URL still reports. The flows empty with a 3.2 metadata url row now compiles under openapi: 3.2.0 as the silent control, and the new conformance spec carries the same control (metadataOnly). Dropping the exemption reddens both. b87282c

[OmarAlJarrah]

This sentence is more than imprecise; its premise undercuts the placement rationale. The loader's finding does carry a site: morphic validate on {type: oauth2} prints absent.yaml:7:7: ... securityScheme.flows is required for type=oauth2, a Position with no pointer, and #511 made line:col a first-class locator beside the pointer. So for the absent spelling "the loader cannot place it" is false, and the new warning is a second report of a defect the loader already reports as an error.

That leaves one defect with two diagnostics, two severities and an exit code that depends on the spelling: absent exits 1, flows: {} exits 0. That is the opposite of the PR body's "one rule for every spelling". The spellings are not alike in the spec. Absent violates a REQUIRED field, so an error is right and the loader supplies it. flows: {} is valid but unusable, so a warning is right and only the compiler can see it. The repo's precedent when a rule could be checked in both stages is compilerOwned (#502): one report, not two.

The consistent design reports only what the loader cannot see: ss.GetFlows() != nil && len(scheme.Flows) == 0. GetFlows() is nil for an absent key and non-nil for {}, so the lowering can tell them apart even though the IR cannot (the PR's stated reason for one rule). Prototype result: absent draws only the loader's error, and {} and a flows object holding only unknown keys draw the one warning. The bigger alternative is owning the whole rule in the loader (drop the library finding, report every spelling pointed). That needs a node-based predicate, since the library error has no structured field identity, and I would file it separately.

[fuad-daoud]

Agreed: the loader does site the absent spelling, by line and column (absent.yaml:6:8: securityScheme.flows is required for type=oauth2), so the old sentence was wrong. The warning now fires only for what the loader cannot see, ss.GetFlows() != nil with no lowered flow. {type: oauth2} now draws only the loader's error, while {} and a flows object holding only unknown keys draw the one warning. diag.OAuth2NoFlow's record is rewritten to match. Moving the whole rule into the loader is left as the separate change you describe. b87282c

[OmarAlJarrah]

nit: "reported before the lines below, which is why those append into diags" describes the diff (only preserveUnreadFields changed from assignment to append) and reads as an ordering requirement the code doesn't have. "Reported rather than refused, see oauthNoFlowDiag" is the part that earns its place.

[fuad-daoud]

Reworded to just "Reported rather than refused; see oauthNoFlowDiag." b87282c

[OmarAlJarrah]

The new report reads an empty lowered flow list as "the document declares no flow", which holds only while presentFlows lists every flow field of soa.OAuthFlows. It does today: the five fields in v1.25.2 are all read, and I checked. But soa.SecurityScheme is held to its source struct by TestMechanismFieldNames_AccountForEverySourceField, and nothing holds presentFlows to soa.OAuthFlows.

If a library bump adds a flow field, a scheme declaring only that flow lowers to empty Flows. Before this PR that flow was dropped silently. Now it is dropped with a false "oauth2 scheme with no flow". A reflection test over soa.OAuthFlows beside the sibling guard would close it.

[fuad-daoud]

Added TestPresentFlows_ReadsEverySourceFlowField in auth_internal_test.go, beside the sibling guard. It walks soa.OAuthFlows by reflection, sets each *soa.OAuthFlow field alone and requires presentFlows to return it. Any other field has to be listed as not-a-flow (only Extensions is). Commenting out the password candidate reddens it with "presentFlows does not read the Password flow". b87282c

[OmarAlJarrah]

The Kind == ir.AuthKindOAuth2 half of the new condition has no row of its own: every interned row in this table is oauth2. I planted if len(scheme.Flows) == 0 { (guard dropped) at this head. Exactly one test in the auth package reddens, TestLowerSecuritySchemes_ARefdEntryRecordsFieldsWhereTheyAreWritten, and only because its exact diagnostic list gains a warning for a mutualTLS alias. Six TestConformance subtests redden too (security-schemes, security-or-and, extension-promotion, extensions-x, neutral-naming, empty-names).

So the guard is held, but incidentally, and the PR's mutation list doesn't include this one. It also shows in the list's "fire only for absent flows" bullet, which credits ARefd... with catching that mutation. With the Kind guard kept (len(scheme.Flows) == 0 replaced by ss.GetFlows() == nil), ARefd... stays green and only the three oauth2 rows redden. ARefd... reddens only when the whole condition, Kind guard included, is replaced, so what it catches there is the missing guard.

One interned non-oauth2 row (apiKey or http) asserting messagesAt(diags, diag.OAuth2NoFlow) is empty would pin the guard where the rule lives.

[fuad-daoud]

Added a row of its own: an apiKey scheme carrying flows: {}. With the new GetFlows() != nil half, a plain apiKey could not reach the guard, so the row has to write a flows object. Dropping the Kind guard now reddens exactly that row of TestLowerSecuritySchemes_AnOAuth2SchemeWithNoFlowIsReported. The PR body's mutation list is corrected too. b87282c

[OmarAlJarrah]

nit: "beside a document that declares nothing wrong with them" isn't accurate, since two of the three entries are defective (the flowless oauth2 entry and the ghost $ref). "which is now reported" is change history rather than what the test pins. Further down, "the loader's other spelling" is confusing, because the entry here, {type: oauth2}, is the absent spelling itself. Something like "pins three entries a document can carry: an oauth2 scheme with no flow, reported at its own entry and interned anyway; ..." avoids both.

[fuad-daoud]

Reworded along the lines you suggested: "pins three entries a document can carry: an oauth2 scheme with no flows object, interned with nil flows and reported only by the loader, …". The "loader's other spelling" passage is gone. That entry is the absent spelling, and the test now asserts it draws no oauth2-no-flow report. b87282c

[OmarAlJarrah]

"The IR states exactly what the document said" is the argument for interning, but ir/auth.go:16 still documents AuthKindOAuth2 as "OAuth 2.0 with one or more flows". After this PR a flowless oauth2 scheme is a deliberately interned, documented shape, so that comment is now imprecise about a shape the compiler is designed to produce (and has produced silently since #4). Nothing in docs/ or irverify states or checks a flow count, so I found no emitter-facing contract broken. But a reader of ir.AuthKindOAuth2 is told the opposite of what this record says. Amending that line ("with zero or more flows; none when the document declares none, which a compiler reports") would keep the two in step.

[fuad-daoud]

Amended as suggested: AuthKindOAuth2 is now "OAuth 2.0 with zero or more flows; none when the document declares none, which a compiler reports." b87282c

[OmarAlJarrah]

The placement contradicts a rule this function states and a sibling test pins. lowerSecurityScheme's doc says entry is where the document names the scheme and places "anything said of the entry as a whole", while decl is "where the fields live". TestLowerSecuritySchemes_ARefdEntryRecordsFieldsWhereTheyAreWritten asserts "nothing is reported against a position the alias does not hold" and "two aliases of one declaration report it once", through the dedup on the whole diagnostic value in compile.Diags.

A flowless declaration is a fact about the declaration's text. This reports it at each alias's {$ref: ...} object, which holds no flow either way, and embeds the alias name in the message so the dedup cannot collapse the copies. One defect with N aliases gives N+1 warnings, and none is at the place to edit. The cited #107 concerns fabricated hoist pointers and doesn't mention security schemes.

Report at decl with a message that doesn't embed the entry name (the preserveUnreadFields messages don't either), and the copies dedup. Prototype: a target plus two aliases gives one warning at /components/securitySchemes/target, a three-deep alias chain gives one at the final declaration, and an unaliased scheme reports where it did before. TestLowerSecuritySchemes_AFlowlessRefEntryIsReportedAtEachEntry would then pin the rule as the function's doc states it. Switching the call to decl alone reddens only that test, so the PR's tests already pin the current choice.

[fuad-daoud]

Agreed. It's now reported at decl, and the message names no entry ("oauth2 security scheme declares no flow: no token endpoint is stated for it"), so the copies dedup. A target with two aliases gives one warning at /components/securitySchemes/target, and a three-deep alias chain gives one at the final declaration. The test is now TestLowerSecuritySchemes_AFlowlessDeclarationIsReportedOnceWhereItIsWritten, with both aliases declared before the target. Switching the call back to entry reddens it and TestConformance. The #107 citation is dropped from the code and the PR body. b87282c

[OmarAlJarrah]

Nothing the repo's sweeps reach exercises this rule, so everything outside this package's unit tests is blind to it. I deleted the feature (if false && ... on the new branch, which compiles) and ran ./compilers/openapi, ./internal/harness/..., ./ir/..., ./pass/... and ./cmd/.... All stayed green, including TestConformance, the goldens and TestHarness_InRepoCorpus. Compiling the 154 specs under testdata/, none draws openapi/oauth2-no-flow; the four that declare type: oauth2 all declare a flow.

CLAUDE.md names the way to cover a new construct: add a spec the sweep reaches, so the no-panic, irverify, round-trip, determinism and order-invariance oracles run over it, and a golden pins the diagnostic's code, pointer and severity byte for byte. The PR's test plan checks the existing fixtures and leaves testdata/ alone, which is the gap. A conformance spec (testdata/conformance/openapi/: .yaml, .golden.json and a table row) should hold flows: {}, a flows object with only unknown keys, an aliased flowless scheme with the alias declared before its target (the order that would be wrong under a canonical-site bug), and a metadata-only 3.2.0 scheme as the silent control. I ran the harness on probes of these shapes and they pass, so adding them should be safe.

[fuad-daoud]

Added testdata/conformance/openapi/oauth2-no-flow.yaml with its golden and a table row (assertOAuth2NoFlow). It holds flows: {}, a flows object with only an unknown key, two aliases declared before their flowless target, and two silent controls: a 3.2.0 metadata-only scheme and a declared flow. morphic-harness passes on it. With the feature deleted (if false && …), TestConformance now reddens along with the unit tests. b87282c

[OmarAlJarrah]

Verdict: not yet at the bar. The core is sound: flows: {} deserves a diagnostic, a warning is the right severity, interning rather than refusing is right, and the PR merges cleanly with current main with the full test suite green. It falls short of the best version in four ways, in order of weight. Each is detailed inline.

1. False positive on a valid 3.2 document (auth.go:207). A metadata-only scheme warns that it "states no token endpoint", although oauth2MetadataUrl is the RFC 8414 metadata that states it. Exempt it.
2. Duplicates the loader for the absent spelling (diag.go:252). One defect gets an error and a warning, and the exit code depends on the spelling. Report only the case the loader cannot see: flows present, declaring no flow.
3. Placement contradicts lowerSecurityScheme's own rule (auth.go:122). N aliases give N+1 reports on {$ref: ...} objects, where the existing precedent is one report at the declaration.
4. No sweep reaches it (auth_test.go:504). Deleting the feature leaves every corpus-level suite green. Add a conformance spec and golden.

Smaller: the IR's "one or more flows" text now misdescribes a shape this PR deliberately interns (diag.go:255), nothing holds presentFlows to the upstream flow fields (auth.go:231), the Kind guard is pinned only incidentally (auth_test.go:575), and three wording nits.

I prototyped 1 to 3. The whole production change is:

if scheme.Kind == ir.AuthKindOAuth2 && ss.GetFlows() != nil &&
	len(scheme.Flows) == 0 && scheme.OAuth2MetadataURL == "" {
	diags = append(diags, oauthNoFlowDiag(c, decl))
}
// oauthNoFlowDiag message: "oauth2 security scheme declares no flow: no token endpoint is stated for it"

Results against it: a target plus two aliases gives one warning at the target, a three-deep chain gives one at the final declaration, absent flows draws only the loader's error, {} and a flows object with only unknown keys give one warning, and a metadata-only 3.2.0 scheme, a declared flow and an apiKey scheme are silent. The tests that pin the per-entry placement and the metadata row change with it, which is intended.

The PR body should also drop two claims that this changes: "one rule for every spelling", and the #107 citation for per-entry reporting.

[fuad-daoud]

Thanks for the thorough pass. All four gaps and the smaller notes are addressed in b87282c, each one probed before the change:

1. Metadata-only 3.2 scheme: exempt. A scheme with oauth2MetadataUrl is no longer reported. The table row compiles under 3.2.0 as the silent control, and the conformance spec carries the same control.
2. Duplicating the loader: only a flows object that is present but names no flow is reported now. {type: oauth2} draws just the loader's required-field error, so the two spellings no longer get two severities.
3. Placement: reported at the declaration, with a message that names no entry, so the copies dedup. A target with two aliases gives one warning at the target, and a three-deep alias chain gives one at the final declaration.
4. Sweep coverage: new conformance spec oauth2-no-flow, with its golden and table row. Deleting the feature now reddens TestConformance, and morphic-harness passes on the spec.

The smaller notes:

• ir.AuthKindOAuth2 now says zero or more flows.
• TestPresentFlows_ReadsEverySourceFlowField holds presentFlows to soa.OAuthFlows by reflection.
• An apiKey row with flows: {} pins the Kind guard.
• Both wording nits are reworded.

The PR body is updated: it no longer claims one rule for every spelling or cites #107, and the mutation list is corrected. On the merge commit trailers: noted, and the squash body will be set by hand at merge time.