fix(compilers/openapi): warn on a duplicate server name

[fuad-daoud]

Summary

OpenAPI 3.2 gives a Server Object a name, "an optional unique string to refer to the host designated by the URL". Two entries of one servers list could declare the same name, both lowered, and nothing said so (#631). This reports it.

• New code openapi/duplicate-server-name, a warning, beside openapi/duplicate-parameter in the duplicate family.
• Load checks every servers list the source writes, lowered or not: the document's, each path item's (under paths, webhooks, components/pathItems or a callback, and the fields written beside a path item $ref), and each operation's. An entry whose name an earlier entry of the same list already has is reported once, at its own name key (…/servers/<i>/name), naming the first holder's. Both entries are kept; nothing is dropped or merged.
• Names compare as written. An entry with no name, or an empty one, holds none, and a URL-derived hint is the lowering's, not a name the document wrote. Names equal only once canonicalized (prod/Prod, prod-eu/prod_eu) are not reported: an emitter uniquifies them, as it does a hint collision.
• Uniqueness is per list. A path item's or an operation's servers replace the ones above them, so one name may recur across lists.
• The check runs on every version. 3.0 and 3.1 do not define name, but the parser reads it and serverName lowers it, so the IR carries the repeat either way; load already warns that the version does not define the field (openapi/later-version-construct).

Why load, in the walk the parameter rule already makes

• The rule is about the source, and walkSource is the one pass that visits every list where the source writes it, whether or not it lowers, never through a $ref. openapi/duplicate-parameter checks its lists there for the same reason (openapi: a $ref'd repeat in a path item's parameters can go unreported #840).
• Path-item and operation servers lower to no ir.Server (they are kept under Unmodeled), so a check in the lowering of the document's list, where this rule first sat, cannot reach them.
• Both rules read the same owners, so the parameter check's walk becomes listRepeats, which hands each owner's lists to a rule per list: paramRepeats (unchanged) and serverRepeats. A walk of its own would cost about a million allocations on an 11 MB description; the shared walk allocates exactly what the parameter check did alone on it.
• Not pass/validate: a pass sees the IR, where path-item and operation servers are raw YAML and every finding carries NoSource, so it could neither read those lists nor site a finding at the source's name key.

Why a warning

The 3.2 name row says "unique" with no RFC 2119 keyword, while the spec's uniqueness rules for a parameter list, operationId and tag names are MUSTs. So this sits with openapi/duplicate-enum-value (JSON Schema's SHOULD) rather than with openapi/duplicate-parameter. A document that exits 0 on main still does under the default --fail-on error; --fail-on warning makes the repeat fatal.

Overlay attribution

Each repeat is credited to the document that wrote that entry, as any finding at a pointer is. Crediting the overlay whenever it wrote only the earlier entry was considered and rejected: with a base that repeats a name itself and an overlay renaming an earlier entry to it, that reports the overlay's entry twice and the base's own repeat never.

Deliberately not done

• A list only a $ref reaches, under an x-* key or in another document, is not checked, since walkSource never follows a $ref. The parameter rule misses the x-* case too; openapi: a list only a $ref reaches is never checked for repeats #880 covers both, and listRepeats and the code's doc say so.
• Tag-name uniqueness, a MUST nothing checks, is openapi: two tags with the same name compile clean #881; listRepeats is where it would go.

Test plan

• go test ./compilers/openapi/internal/load -run Repeats. TestServerRepeats_ReportsEveryListTheSourceWrites: the document's list (two entries; three, one report per repeat; an alias; a merge key; an unnamed entry ahead keeping source indices), a path item's and its operation's lists, a component path item nothing mounts, a webhook, a component callback, a path item two paths mount (reported once, where written), the fields beside a path item $ref mounted again by an alias (reported once), and a 3.1 document. TestServerRepeats_ReportsOnlyANameOneListRepeatsExactly: distinct names, entries with no name or an empty one, and one name in the document's, a path item's and two operations' lists. The parameter tests are unchanged.
• go test ./compilers/openapi -run 'DuplicateServerName|Overlay': openapi: two servers with the same name compile clean #631's repro compiled end to end, both servers lowering with their names; hint/hint, name/hint, case and separator collisions, each unreported and each first proving its lowered words really collide; and overlay attribution: an appended repeat credited to the overlay, a base's own repeat beside an overlay's rename credited to the base, and two appended entries.
• testdata/openapi/duplicate_server_names.yaml repeats names in the document's list, a path item's and an operation's, under the harness order-invariance sweep; go run ./cmd/morphic-harness reports it ok.
• Mutations, each planted and reverted, each reddening a test: placing the document's list from the walk's / site; checking the fields beside a $ref at every position an alias reaches, or at none; letting an empty name claim; overwriting the first holder; skipping the document's, a path item's or an operation's list; skipping inline path items; error severity; the short-list guard off by one; comparing canonical words; siting at the entry rather than its name; dropping the first holder from the message; the walk's panic label; and crediting the overlay as above. Reading the document's list through GetServers rather than the field survives, as it must: the server that stands in has no name.
• morphic compile on openapi: two servers with the same name compile clean #631's repro exits 0 with the warning at /servers/1/name, and 1 under --fail-on warning.
• Real-world differential at 933b507: the OpenAPI 3 descriptions under the reference corpora (1,840 files) give byte-identical IR, exit codes and diagnostics on main and this branch, since none repeats a server name; a positive control of three that do differs only by the new warnings.
• Locally at 933b507: make fmt vet build, golangci-lint v2.14.0 (0 issues), make coverage (every statement covered), make bench-smoke. make fuzz is left to CI.

Closes #631

[OmarAlJarrah]

One declaration mounted twice (a YAML alias such as - *s, or a merge key <<: *base that carries name) is reported here as an error, and the alias row in meta_test.go pins that. The document wrote the name once, so "the document's own claim is contradictory" (the diag.go rationale) does not hold, and "the name identifies two hosts" is false when both mounts have the same URL.

#502 settled this class for operationIds: a declaration mounted more than once is a warning (DuplicateOperationID), and only a second declaration that writes the id again is an error (ConflictingOperationID). operationIDClaims / byDeclaration already separate the two by shared node. This loop treats every repeat as the second kind, so a spec that exits 0 on main now exits 1 with no --fail-on setting that avoids it (the CLI only accepts error or warning).

Probed on this branch: x-base: &b {name: prod} with two servers using <<: *b reports /servers/1/name, exactly as two separately written entries do. Suggest grouping by declaring node as byDeclaration does, with mounts of one declaration a warning and separate declarations an error.

[fuad-daoud]

Agreed, fixed in b23b370. Every repeat is now a warning, which is the #502 precedent for one declaration mounted more than once. The message no longer claims the name identifies two hosts: it now reads "a consumer keying servers by name cannot tell them apart", which holds for a mount as much as for two declarations. A merge-key row (<<: *b carrying name) joins the alias row, and both expect the warning.

[OmarAlJarrah]

name is only defined on the Server Object from 3.2, but this runs for every version. Probed on this branch: a 3.0.3 or 3.1.0 document with two servers carrying name: prod exits 1 with openapi/duplicate-server-name, while a single name on the same document is silent (no unknown-key report, since the parser reads the field). The doc comment on DuplicateServerName in diag.go cites OAS 3.2.1 §4.5.1, which that document is not written against.

Because --fail-on only offers error and warning, such a document has no way to compile with exit 0. The PR description lists the sub-3.2 read as an observation only, but this change is what turns it into a refusal. Either gate the rule on a 3.2 document or downgrade it when the version does not define the field, and add a row for a 3.1 document either way so the choice is pinned.

[fuad-daoud]

Kept on every version, deliberately: the parser reads name on 3.0 and 3.1, and serverName lowers it, so the IR carries the colliding names whatever the version. As a warning it no longer refuses such a document under the default --fail-on. A 3.1 row in the reported table pins the choice, and the serverName comment now says the field is defined by 3.2 but read on every version (b23b370, trimmed in 63384ae). The citation in diag.go is gone with the rewrite.

[OmarAlJarrah]

The rule keys on the raw string, but the identity emitters consume is Naming.Canonical. Probed: prod / Prod / prod-eu / prod_eu / https api example com v 1 / https_api_example_com_v_1 pairs each lower to one canonical name and produce no diagnostic. The stated reason for the error (an emitter naming one client per server cannot stay unambiguous) applies to those pairs just as much, so the rule catches only the spelling-identical case.

That may be the right scope (the spec's uniqueness is case-sensitive), but then the comment on serverName ("Two declared names colliding are the document's error") overstates it. It should say exact-string collisions, and the canonical-collision case should be stated as tolerated or reported.

[fuad-daoud]

Kept exact strings, since the spec's uniqueness is case-sensitive, and now pinned (b23b370): the not-reported table has prod/Prod and prod-eu/prod_eu rows, each asserting that the canonical names really do collide, so keying on serverName(s).Canonical reddens both. The serverName comment now says names that differ only once canonicalized are tolerated like URL hints, and only exact repeats are reported.

[OmarAlJarrah]

The diagnostic's source is whichever source owns the repeat's position, and the message names the first claimant by pointer alone. This test only covers the overlay appending the repeat. If the overlay instead rewrites an earlier entry to collide with a later base entry (target: $.servers[0], update: {name: prod} over a base of dev then prod), the finding is credited to the base document at /servers/1/name and says it duplicates /servers/0/name, which the base wrote as dev. Probed on this branch: Provenance.Source == 0.

The collision was introduced by the overlay, so a reader opening spec.yaml cannot find the duplicate. Worth either crediting the source of the first claimant when it differs, or naming it in the message, and adding this ordering as a second overlay case.

[fuad-daoud]

Fixed in b23b370. When only the earlier claim came from the overlay, the overlay made the two collide, so the report sits at that entry with Provenance.Source == 1 and names the base's entry in the message. The test is now a three-case table: the overlay appends a repeat, the overlay renames an entry ahead of the base's claimant (your $.servers[0] ordering, reported at /servers/0/name), and the overlay writes both entries (reported at the later one, as in a single document). Dropping the swap reddens the second case, and swapping without checking the repeat's source reddens the third.

[OmarAlJarrah]

This nil guard is dead: (*soa.Server).GetName returns "" on a nil receiver, so a nil entry already falls through to the name == "" continue. Mutation check on this branch: deleting these three lines leaves DuplicateServerName|LowerServers green, so TestDuplicateServerName_NilEntryKeepsSourceIndices does not pin the guard. The only thing it does pin is that i is the range index. The guard and the comment that mentions nil entries can go, with the test kept for the index claim and its doc reworded to say that.

[fuad-daoud]

Removed in b23b370. The check now runs inside the lowerServers loop, after the nil continue that lowerServer already needs. TestDuplicateServerName_NilEntryKeepsSourceIndices stays for the source-index claim, and its doc now claims only that.

[OmarAlJarrah]

This is a second walk over the same list that rebuilds each server's pointer from scratch (ids.Ptr("servers", strconv.Itoa(i), "name")) when the loop above already builds ids.Ptr("servers", strconv.Itoa(i)) for lowerServer. The two spellings of the server pointer can now drift apart. Doing the check inside the existing loop and extending the server's pointer, as serverVariables does with sptr + ids.Ptr("variables", name), avoids the second pass and the second pointer derivation.

[fuad-daoud]

Folded into the existing loop in b23b370: serverNameClaims.claim takes the server's sptr and sites the report at sptr + ids.Ptr("name"), so there is one walk and one pointer derivation.

[OmarAlJarrah]

"Collisions this compiler makes, like DuplicateStatusKey's or a URL hint's, only warn" is not accurate for the hint half: a URL-hint collision reports nothing at all (TestDuplicateServerName_NotReported asserts that, and serverName says an emitter uniquifies). Only DuplicateStatusKey warns. Suggest "warn or are tolerated silently", or just drop the clause.

[fuad-daoud]

Right, a hint collision is silent. The comment is rewritten in b23b370 to say that names equal only once canonicalized, and URL hints, are tolerated because an emitter uniquifies them.

[OmarAlJarrah]

This row never asserts that the declared name really equals the other entry's derived hint, which is the guard the neighbouring hintsCollide row added so it cannot pass vacuously. It currently holds only because CanonicalWords("https://api.example.com/v1") happens to be https_api_example_com_v_1 (I checked), and a change to the hint grammar would turn this into a row that tests nothing. Assert doc.Servers[0].Name.Hint == doc.Servers[1].Name.Canonical here.

[fuad-daoud]

Added in b23b370. The row guard is now a collide function picking the two lowered words a row claims are equal. This row asserts Servers[0].Name.Hint == Servers[1].Name.Canonical (non-empty), so a change to the hint grammar reddens it instead of leaving a row that tests nothing.

[OmarAlJarrah]

Findings that have no line in the diff:

• The merge commit e912f7e carries Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> and a Claude-Session: https://claude.ai/code/... trailer. Neither belongs in this repo's history, and the repo's squash setting is COMMIT_MESSAGES, so both would be pasted into the squashed commit body on main. The branch cannot be rewritten, so the squash body has to be set by hand at merge time.
• The description has no closing keyword. Issue openapi: two servers with the same name compile clean #631 is the report this resolves, so without Closes #631 it stays open after the merge. The test row "the issue's repro" and the commit bodies refer to "the issue" without a number.
• The "Why error, not warning" paragraph says a caller who wants only the report without the refusal has --fail-on warning. That is the opposite of what the flag does (cmd/morphic/compile.go: warning fails on warnings and errors). With an error-severity rule no caller can get the report without exit 1, which is the downside of the choice, not the escape hatch.
• docs/ir-design.md (the OpenAPI mapping row, "servers+variables (3.2 named) → Servers") records the sibling diagnostics such as duplicate-status-key and invalid-method-key beside their lowering. This one is not mentioned there.

[OmarAlJarrah]

Follow-up notes from a second pass over the final state (head e912f7e with current main merged in). The full suite passes on that merge, and the new functions are at 100% coverage.

Correction. My inline comment on the version gating said the error text cites OAS 3.2.1 §4.5.1. It does not: the diagnostic message carries no citation, and only the doc comment on DuplicateServerName in diag.go does. That comment is edited; the finding itself stands.

1. The severity rests on wording the spec does not make normative.
The name row (3.2.0 and 3.2.1, §4.5.1) reads "An optional unique string to refer to the host designated by the URL." It carries no RFC 2119 keyword. The uniqueness MUSTs in the document are on operationId ("The id MUST be unique among all operations described in the API"), tag names ("Each tag name in the list MUST be unique") and patterned fields. The diag.go comment and the description justify an error by analogy to ConflictingOperationID, whose spec text is a MUST, and "the document's own claim is contradictory" is stronger than "optional unique string" supports. #631 asks for "a diagnostic" and is P4, and tag-name uniqueness, the nearest rule the spec does make a MUST, is still unreported. Either make this a warning, or say in the description why a descriptive word earns an error that --fail-on (which only accepts error or warning) cannot turn off.

2. The title and body do not mark the exit-code change.
A document that exits 0 on main now exits 1: any repeated name, including 3.0 and 3.1 documents (see the version-gating comment). #548 (fix(compilers/openapi)!: own the operationId uniqueness rule) is the closest precedent. It carried ! and a ## Breaking section for this class of change, and it fixed "a YAML alias exits 1 where the $ref form only warns" (#502) as a defect, which is the aliased-server case here. This PR is fix(...) with no Breaking section and no Closes #631.

3. The exact-string choice is not pinned.
Replacing s.GetName() with serverName(s).Canonical as the key in duplicateServerNameDiags leaves go test ./compilers/openapi -run 'DuplicateServerName|LowerServers|ServerName|TestCompile_Overlay' green, so nothing distinguishes "compares the raw string" from "compares the canonical name". prod / Prod and prod-eu / prod_eu lower to one canonical name and are silent today. Whichever way it is decided, a row in the reported or not-reported table pins it. (Deleting the nil guard also leaves the suite green; see the inline comment.)

4. Two citations in the description do not point where they say.
pass/validate.go:969-971 does not exist at e912f7e: the file is 920 lines, and the helper that sets NoSource is at 902-908. operations.go:773-800 is the status-key code; the path-item servers keeping is applyPathServers, at operations.go:689. The 442-470 citation for operation servers is right. Naming the function would outlast the line numbers.

5. The overlay test claims more than it checks.
TestCompile_OverlayIntroducingADuplicateServerNameCreditsTheOverlay says "The base's own server keeps the document as its source", but no assertion reads that, and ir.Server carries no provenance to read it from. Drop the sentence, or assert it somewhere it can be observed.

6. A neighbouring comment contradicts the rule.
The existing comment on serverName says the declared name is "only OpenAPI 3.2 has one", while the new rule runs on every version. Gating the rule on 3.2, or saying why it is not gated, would make the two agree.

[fuad-daoud]

Thanks for both passes. Everything is addressed in b23b370 and 63384ae (the latter keeps the serverName comment under the doc-comment cap); the inline threads each have a reply.

Severity (second pass, 1). Now a warning. The 3.2 name row carries no RFC 2119 keyword, unlike operationId's MUST, and an alias or merge key repeats a name the document wrote once, so the ConflictingOperationID analogy did not hold. The diag.go comment now states this reasoning instead.

Breaking (2). With a warning, a document that exits 0 on main still exits 0 under the default --fail-on error (probed with the CLI: exit 0 by default, exit 1 under --fail-on warning). So no ! and no Breaking section. The title is now fix(compilers/openapi): warn on a duplicate server name.

Exact strings (3). Pinned by the prod/Prod and prod-eu/prod_eu not-reported rows (details on the meta.go thread).

Citations (4). The description now names applyPathServers, applyOperationServers and the NoSource helper in pass/validate.go instead of line numbers.

Overlay sentence (5). Dropped. The test is now a three-case table, including your renamed-earlier ordering, and every case asserts what its name claims.

Version gating (6). Kept on every version, because the lowering uses name on every version. The serverName comment and a 3.1 row now say and pin that.

--fail-on. You are right that the description had it backwards. It now says --fail-on warning is how a caller makes this fatal, and the default leaves it a report.

Closing keyword. Closes #631 is in the description.

ir-design.md. The OpenAPI mapping row now records the duplicate-server-name warning beside the servers lowering.

Merge commit trailers. Noted: the squash body will be set by hand at merge time.

testdata/openapi/duplicate_server_names.yaml also puts the warning under the harness order-invariance sweep (a second entry, an alias, a merge key, and a case-only variant that stays silent).