feat(npm): share one read surface across the npm remote routes (S15 step 11)
🎯 Summary
Adds the surface all three kind=remote read routes share: the upstream status mapper, the metadata serve helpers, the error codes, the instrumented dispatch slots, and the S15 spec amendment that authorizes them.
🧩 What it decides
One mapper, one axis of variation, and it is narrow. Only remoteUpstreamStatusMapping's 404 arm varies by route, and it varies by one thing: the not-found code, package_not_found on the packument and file_not_found on the tarball. Everything else is shared-identical rather than shared-parameterized. remoteReadPolicy's other field, retryAfterSeconds, is the same derived value in every composition; it travels in the policy because only the composition root reads the interval it comes from, not because a route may pick it. And in writeRemoteProxyError the cap-breach, unavailable, and single-flight-timeout arms write an identical status, code, and message, differing only in their log string. A reader looking for per-route variation should look at notFoundCode and stop.
Retry-After is derived, not declared. remote.RetryAfterSeconds (merged in !1678 (merged)) turns health_check.scheduled_interval into the value S13 requires, and only the composition root reads that interval. The three constants the parallel attempts grew, one 5s and two 30s, are deleted rather than reconciled.
The validator describes the bytes served. The packument path rewrites every dist.tarball in stream, so relaying the upstream ETag would advertise a validator for a document the client never received. A cache serve derives a strong one from the content address instead; a live upstream stream sends none, because the fill has not committed when the headers are written. remote.ServeResult grows an additive BlobRef to carry that address, populated by Standalone.openCached, the one producer of a cache-source result.
The options instrument what they are given. The seam and its labels are unexported, so a composition root cannot apply them, and a live remote route that skipped them would emit neither the request metric nor the completion log line its hosted twin emits.
Callers land later. These helpers are reached only from their own tests until Step 12, which is what keeps them out of unused — measured, not assumed.
📐 Governing ADRs
- ADR-009 API design — conforms.
upstream_rejectedjoins the envelope set for a relayed non-4044xx, and the spec amendment records the propagation rule and the status set. - ADR-008 Content-addressable storage — conforms. The strong validator is derived from the blob's content address rather than the upstream's own, so it describes the bytes served.
- ADR-005 Artifact delivery mode — partial, pre-existing deviation. The shared cache serve reaches the redirect branch on the instance default, but no npm read path threads the per-namespace override ADR-005 requires be "always available":
npm.Resolutioncarries no field for one, whilemaven.Resolution.DeliveryModeOverridedoes. This MR inherits the gap from the hosted npm reads rather than widening it; closing it is an npm-wide change across hosted and remote together. Tracked in #708 (closed). - Mirror checked with
./scripts/adr-freshness.sh: current. One internal ADR is never mirrored and was not read.
✅ Testing
New: remote_read_errors_test.go and remote_document_serve_test.go. Two pin the parameters this step introduces (..._NotFoundCodeComesFromThePolicy, ..._RetryAfterComesFromThePolicy); TestServeRemoteUpstream_AdvertisesTheCallersValidator hands the result and the caller different validators so it cannot pass by coincidence.
Added in review: TestWriteRemoteProxyError_UnavailableUpstreamIsUnavailable covers the ErrUpstreamUnavailable and ErrRemoteUnhealthy arms, which had none; TestWriteRemoteProxyError_LogsNoCredentialOnAnyArm plants a credential in a *url.Error behind each sentinel and asserts on the captured log record, verified failing on every arm with the redaction reverted; TestRequestCodeLabels_ExcludesUncountedCodes guards the codes no instrumented route emits. The Retry-After assertions moved from NotEmpty to the policy's own value.
go test ./internal/format/npm/... ./internal/remote/... ./internal/metrics/... green. golangci-lint run 0 issues on all three changed packages.
📏 Diff size
~1,659 reviewable LOC (added lines against main), past the 500 line in docs/dev/development-model.md. It grew from ~1,316 in review: the added tests and the repaired comments account for the difference.
| Group | LOC |
|---|---|
| Production Go | 597 |
| Tests | 1,006 |
| Docs (spec, metrics catalog) | 56 |
Why not split. Correcting an earlier claim here, which a reviewer was right to challenge: the two halves are not mutually entangled. remote_document_serve.go never calls into remote_read_errors.go, and the only cross-file call runs the other way, writeRemoteProxyError's default arm into the ~15-line writeRemoteProxyServerError. A three-way split (shared primitives, then each half) was available, and nothing in the code forced this shape.
What argues for one MR is the review, not the dependency graph. Both halves are dead until Step 12, so an earlier landing puts half the surface on main with no caller and no second reader for a whole step, and the two halves are read against each other: the serve file's validator rule and the mapper's relay rule are the two answers to "what may this proxy tell a client about the upstream", and the spec amendment states both. Splitting would have been cheap; it would have bought a smaller diff and a worse review, and the LOC table below is the trade the split would have moved.
🧪 E2E scenario impact
None. No route is mounted, so no journey changes.
Related to #700 (closed)