fix(server): structured 405 on /metrics, allow HEAD, derive the route set

[mchmarny]

Summary

Closes items 6–9 of #2454 — the pkg/server conformance findings from the #2448 review. Items 1, 2, 4 and 5 are in #2450; item 3 is #2451; items 10 and 11 need no action yet.

Motivation / Context

#2454 consolidates review findings across #2436 and #2448. This PR takes the API-server group. Two of the four are defects I introduced in #2448 itself.

Fixes: N/A
Related: #2454, #2448, #2112, #2450, #2451

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• Build/CI/tooling

Component(s) Affected

• API server (cmd/aicrd, pkg/server)

Implementation Notes

Item 6 — plain-text 405 broke the package's own convention

getOnly called http.Error(w, "method not allowed", 405), writing a bare string. The seven other 405 sites in this package — including handleHealth and handleReady, registered on the same mux — return the structured envelope via WriteError. A client parsing the error received JSON from /health and plain text from /metrics for the identical condition. Now uses WriteError with ErrCodeMethodNotAllowed, matching its siblings.

Item 9 — HEAD, decided rather than left implicit

getOnly also narrowed /metrics to GET, which promhttp had not done. RFC 9110 §9.1 makes GET and HEAD mandatory for a general-purpose server, and some monitoring probes reach /metrics with HEAD — so #2448 introduced a behavior regression on a monitoring endpoint while trying to make the contract true.

readOnly allows GET and HEAD, and the spec declares head: alongside get: so contract and server agree.

The pre-existing HEAD rejection on /health, /ready, and the v1/v2 endpoints is left alone. It is a standing wart, but widening it as a side effect of a conformance test is the wrong way to decide it. It is now recorded in the test's doc comment so the next reader does not "fix" the assertion without realizing it encodes a deliberate deviation.

Item 7 — the route set could not see a future direct-mux route

registeredPaths derived from config.Handlers plus a hand-maintained systemRoutes list. A route added directly in New() — exactly where /health, /ready and /metrics already live — would appear in neither source and be invisible to all three conformance tests. A hole in a gate built to catch undocumented endpoints.

http.ServeMux exposes no way to enumerate its patterns, so Server.handle now records each pattern as it registers it.

Corrected after review: an earlier revision of this PR claimed the route set "cannot drift". It does not — routePaths only records registrations that go through s.handle, so a raw mux.HandleFunc in New is served, unrecorded, and invisible to all three tests. My mutation check used s.handle, which proved the recording path worked rather than that the bypass was caught. TestMuxRegistrationsGoThroughHandle now parses server.go with go/ast and fails, with file and line, if any mux.Handle/mux.HandleFunc call site sits outside the helper.

Item 8 — the name promised more than the oracle delivered

TestOpenAPISpecMethodsAreAccepted fails only on 405, so a declared operation whose handler 500s still passed. The narrowness is deliberate — asserting success codes would make it a fixture treadmill — so the fix is honesty, not a stricter oracle: renamed to TestOpenAPIDeclaredMethodsAreNotRejected, with the docstring stating what it does and does not cover.

Not addressed

• Item 10 ($ref-only or parameters-only path items misread as declaring no operations) — latent; all 10 spec paths use inline get/post.
• Item 11 (spec path relative to the go test CWD) — no go test -c or Bazel lane exists.

Both become real only under conditions that do not exist today, and guessing at the fix now would be speculative.

Testing

go test -race ./pkg/... ./cmd/...          # all pass
golangci-lint run -c .golangci.yaml ./...  # 0 issues
make lint-yaml check-docs-mdx              # OK

readOnly is at 100% coverage; pkg/server holds at 84.3%. docs/user/api-reference.md and docs/contributor/api-server.md now document HEAD /metrics, which the spec declared but the prose did not.

Mutation-verified, including the case the first attempt missed. Registering /debug via s.handle fails the path assertion; registering it with a raw mux.HandleFunc — which passed before review — now fails the source-level guard:

server.go:125:2: mux.HandleFunc registers a route without going through s.handle,
so it is absent from Server.routePaths and invisible to the OpenAPI route
conformance tests; use s.handle(mux, pattern, handler) instead

TestMetricsMethodRejectionIsStructured pins the JSON envelope, the Allow: GET, HEAD header, and that HEAD /metrics returns 200 rather than 405.

Risk Assessment

• Low — Isolated change, well-tested, easy to revert

Rollout notes: Two behavior changes, both on /metrics and both corrective. HEAD /metrics returns 200 again, restoring the pre-#2448 behavior that #2448 removed. Non-GET/HEAD methods now return a JSON error envelope instead of a plain-text body, matching every other 405 in the package. Prometheus scraping is unaffected — it uses GET.

Checklist

• Tests pass locally (make test with -race)
• Linter passes (make lint)
• I did not skip/disable tests to make CI green
• I added/updated tests for new functionality
• I updated docs if user-facing behavior changed
• Changes follow existing patterns in the codebase
• Commits are cryptographically signed (git commit -S)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: bbe10c87-2d46-4c8a-ad23-2cc3282082af

📥 Commits

Reviewing files that changed from the base of the PR and between df5f207 and 9d2c50e.

📒 Files selected for processing (3)

• docs/contributor/api-server.md
• docs/user/api-reference.md
• pkg/server/openapi_routes_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The OpenAPI specification now declares HEAD /metrics. The server centralizes route registration and records registered paths. The metrics route accepts GET and HEAD and returns structured JSON 405 responses for other methods. Tests verify recorded routes and metrics method behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 9d2c5

The PR restores HEAD support and structured 405 responses for /metrics while tightening route conformance checks and updating documentation; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: almaslennikov

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description directly explains the /metrics behavior changes, route tracking, AST-based guard, tests, documentation, and remaining scope.
Title check	✅ Passed	The title clearly summarizes the main changes: structured 405 responses, HEAD support for /metrics, and route-set derivation.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/server-conformance-followups

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Coverage Report ✅

Metric	Value
Coverage	84.2%
Threshold	80%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-84.2%25-brightgreen)

Merging this branch will increase overall coverage

Impacted Packages	Coverage Δ	🤖
github.com/NVIDIA/aicr/pkg/server	84.34% (+0.03%)	👍

---

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/NVIDIA/aicr/pkg/server/server.go	82.05% (+0.23%)	156 (+2)	128 (+2)	28	👍

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

[mchmarny]

Both fixed in 9d2c50e30. The first one was a claim of mine that did not hold, and the way it failed is worth stating plainly.

1. MAJOR — you are right, and my verification was the problem

I reproduced it exactly as reported: a raw mux.HandleFunc("/debug", ...) in New is served, never recorded in routePaths, and passes every conformance test. The item 7 scenario was still open.

The reason I missed it is that my mutation used s.handle(mux, "/debug", ...) — the recording path. So I proved that recording works, not that bypassing recording is caught. Those are different claims and I asserted the stronger one.

http.ServeMux exposes no way to enumerate its patterns, so this cannot be verified at runtime. TestMuxRegistrationsGoThroughHandle parses server.go with go/ast and asserts every mux.Handle/mux.HandleFunc call site lives inside the handle helper. The raw-registration mutation now fails with the offending file and line:

server.go:125:2: mux.HandleFunc registers a route without going through s.handle,
so it is absent from Server.routePaths and invisible to the OpenAPI route
conformance tests; use s.handle(mux, pattern, handler) instead

I also narrowed the claim rather than leaving the stronger wording next to a weaker guarantee. The doc comment now says routePaths covers cooperating registrations and points at the source-level test for the rest. The PR description said "cannot drift" too — that was wrong and I have corrected it there as well.

2. MINOR — HEAD was in the contract but not the docs

Correct, and it is the half that users actually read. Adding head: to the spec left both prose sites advertising GET alone:

• docs/user/api-reference.md — heading is now GET and HEAD /metrics, stating that HEAD returns the same headers with no body and other methods get 405 with Allow: GET, HEAD
• docs/contributor/api-server.md — endpoint table row is now GET, HEAD and names readOnly

go test -race ./pkg/server/... passes, golangci-lint 0 issues, docs gates OK.

[github-actions]

🌿 Preview your docs: https://nvidia-preview-fix-server-conformance-followups.docs.buildwithfern.com/aicr