docs(specs): ratify the content-coding rule in S13 and S16, with tests

🎯 What

Settles the open content-coding decision for remote repositories: what a remote does when an upstream volunteers a Content-Encoding despite being asked for identity.

The decision has two halves, and they land in opposite directions.

🛡️ Half one — codings the client cannot decode

br, deflate, zstd, and multi-coding lists reach remote.Fetch still compressed. The shipped guard refuses them, and that guard was defensive rather than specified. This MR ratifies it, with the rationale, in S13 (virtual-remote-foundation).

Refusing is right because no cache row records a coding and forwardableHeaders omits Content-Encoding, so both caching and relaying would publish bytes that are not the content under an identity that does not describe them. The GitLab monolith's container Virtual Registry is the counterfactual: it sends no Accept-Encoding at all, relays the body byte-for-byte, and strips Content-Encoding on the way out through its response allow-list.

🔄 Half two — plain gzip, and a correction

S16 (container-remote) recorded S13's transparent gunzip as a defect that corrupts container digests, and gated an acceptance criterion on an S13-side switch to suppress it. That premise is false, and this MR corrects it.

A content coding is a transfer wrapper rather than part of the content, so an OCI digest names the decoded bytes. Gunzipping before hashing is what recovers the addressed bytes; the proposed switch would have failed pulls that succeed today.

Evidence, all of it checked rather than assumed:

  • Reproducibility. A digest over the coded form cannot serve as an address. gzip admits several compression levels and none of them is transmitted, so one representation hashes differently per upstream. Measured on a 249-byte manifest: sha256:9e70371… decoded, sha256:41120e2… at gzip level 9, sha256:1288292… at level 1. RFC 9530 section 6.5 makes the same point normatively.
  • A real image. library/alpine on Docker Hub: layers[0].digest is sha256:9824c27…, the sha256 of the 3,799,689 delivered bytes, while config.rootfs.diff_ids[0] is sha256:418dccb…, the sha256 of the 8,596,480-byte tar after gunzip. Both verified against the downloaded blob. The layer's own gzip is declared by media type (application/vnd.oci.image.layer.v1.tar+gzip) and sits inside the digest, so a Content-Encoding would be a second, outer wrapper, and removing exactly one wrapper is what yields the addressed bytes.
  • containerd. v2.0.0 sends Accept-Encoding: zstd;q=1.0, gzip;q=0.8, deflate;q=0.5 (core/remotes/docker/fetcher.go:271), decodes the coding itself (:328-355), and verifies the expected digest against the decoded stream (core/remotes/handlers.go:151 into plugins/content/local/writer.go:114-116). Its default arm errors on an unknown coding, which is the rule this MR ratifies over a wider set.
  • OCI specs. distribution-spec/spec.md contains the string encoding zero times, so it neither grants nor forbids this; image-spec/descriptor.md defines the digest as a hash of the content bytes, with size describing the same octets.

📦 Why the switch would also have broken npm

Worth stating because it is not a container-only consequence. The decode runs in the HTTP client, before Fetch sees the response, so a Fetch transform reads decoded bytes. Suppressing it would have handed S15's dist.tarball rewrite a gzipped packument: the rewriter finds no URLs to replace, streams the compressed body through untouched, and caches a packument still pointing at upstream tarball URLs. Silent, and exactly what that transform exists to prevent. S13 and S15 now both state the ordering explicitly, which neither did before.

📄 Changes

File Change
docs/specs/S13-virtual-remote-foundation.md New Content codings section (the three-case rule), a Resolutions entry with the rationale, three acceptance criteria, an Error Cases row, and a Follow-ups entry
docs/specs/S16-container-remote.md Identity-representation section, acceptance criterion (un-gated), error-case row, follow-up bullet, and resolution corrected. The S13 dependency cell is the union agreed with !1806 (merged); the #433 pointer on the deleted-Content-Length case is replaced by what the decode actually costs
docs/specs/S14-maven-remote.md Points at the shared rule; drops the "decoded entity rather than the transferred representation" caveat
docs/specs/S15-npm-remote.md Documents the Accept-Encoding: identity pin the code already sets, and records that the transform reads decoded bytes
docs/plans/2026-07-30-container-remote.md Drops the dependency entry and the e2e ledger row, and removes the overturned premise from four further places — Step 13's Gated entry, Step 12's pointer at it, the plan's canonical illustration of its gating discipline, and the rationale that cited the Maven comment this MR rewrites. Also drops the delivered cold-fill Content-Length gate from four sites
docs/roadmap/closed-beta.md S16 row no longer lists this as gating step 13, and drops the step 12 gate on #269 that the plan denies twice
internal/remote/fetch_test.go New composition test — see below
internal/remote/errors.go, internal/virtual/resolve.go Comment-only. Both said WinnerFetchFailedError reports a non-2xx status; it carries the upstream's own status for every response refused on something other than its status
internal/virtual/resolve_test.go New test pinning the per-flow divergence — see below
internal/format/maven/remote_upstream_test.go, internal/format/oci/remote_auth_test.go Comment-only. Both carried the rationale this MR deletes from their production twins
internal/remote/fetch.go, internal/format/maven/remote_upstream.go, internal/format/oci/remote_requestbuilder.go Comment-only. The first named this as an open decision; the other two claimed Accept-Encoding: identity suppresses Go's transparent decode, which DisableCompression: true on the transport already does

What this unblocks

S16 (container-remote) step 13 loses an external dependency, and its identity-representation criterion un-gates and can now be asserted with a real gzip fixture rather than skipped.

🧪 Review rounds, and what they changed

The new text went through five review rounds — a claim-by-claim audit against the code, then two independent passes, then a fourth pass over the delta the first three produced, then human and automated review on this MR. Between them they found 29 defects in this MR's own new text, all fixed here.

One pattern accounts for nearly all of them, and it is worth stating plainly: every defect was in prose describing the code, never in the code itself. The rule as implemented was correct from the first commit. What kept being wrong was the sentence describing it — which is the exact defect class this MR exists to remove, so the rounds were not wasted.

The instructive failure. An intermediate commit "corrected" the identity-pin claim to say the npm and container probes pin no content coding. That correction was wrong and made a true sentence false: the wired npm health sweep goes wire_remote.go:212npmremote.NewHealthSweepSourcenpmremote.NewUpstreamRequestBuilder (health_sweep.go:130) → upstream_request.go:290, which does pin it. The builder that omits it has no caller outside its own tests. The reversal commit records that, so the next reader who greps it learns it is unwired.

That sentence was then wrong a third time, because there are three probe paths and each version tried to say one thing about all of them: a resolution Probe composes the slice's own RequestBuilder and inherits the pin; Maven's and npm's health resolvers reuse that builder and carry it; the container health probe deliberately composes a bare unauthenticated HEAD of its own — its doc explains why — and pins nothing. It is now split by path.

The other recurring shape was writing about 200 where the code keys on Status/100 == 2 || Status == 304 (standalone.go:244). That made three separate claims too narrow at once, all fixed by widening the set and adding the Error Cases row it needed.

Also fixed across the rounds: an S16 sentence that contradicted S13 outright; security cover 14's "or similar"; a missed min() sweep in Resolutions; "drained and closed unread" contradicting the deliberate bounded drain, in two specs; a stale S16 dependency cell and a dangling "either"; a roadmap gate the plan denies twice; remote.Fetch naming a symbol that does not exist; a usableContentEncoding doc quantified over builders that do not all pin; two test comments still carrying rationale this MR deleted from their production twins; and a misuse of "converse".

Round five, from review on this MR, found four more, and the pattern held: every one a stale claim about the codebase rather than a defect in the rule.

  • The canonical gating illustration cited a gate that has lifted. FetchResult.ContentLength is on main, and Maven forwards it on both cold-fill paths guarded on >= 0 (remote_artifact.go:466, remote_metadata.go:355). It shipped as a typed result field rather than the forwardable-header allowlist exemption the plan waited on, so grepping for the exemption finds nothing left to land. Round four had replaced the overturned plain-gzip fixture with exactly this — one dead example swapped for another. The illustration now uses Step 15's Fetch coalescing, which that step already argues in its own Gated block. Three further sites in the plan carried the same dead gate and are fixed alongside it.
  • The S16 #433 pointer named an issue that does not track the case. #433 covers the expected digest on commit, the pre-commit Docker-Content-Digest cross-check, and Content-Length on ServeResult — not the length deleted along with a decoded coding. That is also not a gap but a permanent cost of the decode: gzip carries no usable declared output size, and deriving one would mean buffering the whole body, which the constant-memory contract rules out. Reworded to say so, link dropped.
  • The S13 dependency cell is now the union agreed with !1806 (merged), which rewrites the same cell for #433. Each MR strikes a different item off the same list of S13-side gaps, so resolving that conflict in favour of either side whole would reinstate the other's stale claim.
  • The virtual test carried an assertion that could not fail. require.NotErrorIs(err, ErrUpstreamUnavailable) cannot fire once errors.AsType has matched WinnerFetchFailedError, which is a bare struct with no Unwrap. Removed. Worth recording that the first replacement was equally vacuous — resolve.go:803 returns a zero ResolveResult alongside the error, so no assertion on the result adds to the type assertion either. Same shape as the SafeHeaders assertion round three removed from fetch_test.go.

Two tests, both mutation-verified

Prose drifts; a test does not. Both behavioral claims this MR ratifies are now pinned, and each was checked by breaking the code and watching the test fail — a passing new test proves nothing on its own.

1. The decode reaches the cache (internal/remote/fetch_test.go)

One of the three new acceptance criteria had no end-to-end coverage, and the gap was structural rather than a missing assertion: remotetest.FakeUpstreamDoer hands Fetch a response directly, so newCappedBody never runs and the decode chain is absent from every test in fetch_test.go. No assertion written against that fake could fail on a decode that did not happen — so the criterion whose failure mode is a cache blob full of compressed bytes was the one left unproven.

TestFetcher_Fetch_GzipUpstreamCachesDecodedBytes wires the real upstreamhttp.Client into a Fetcher against an httptest server that gzips. Both its assertions are verified by mutation, not by passing:

  • disable the gzip branch → fails on the outcome assertion, since the surviving header is then refused;
  • scrub Content-Encoding while leaving the body encoded (the dangerous shape, where the response claims decoded and is not) → fails on the byte assertions;
  • delete resp.ContentLength = -1 from markUncompressed → fails on the length assertion.

That third one replaced an assertion review found vacuous: SafeHeaders().Get("Content-Encoding") is identically empty for every input, because forwardableHeaders is a closed four-entry allowlist. It read as coverage of the scrub and provided none.

2. The per-flow divergence stays divergent (internal/virtual/resolve_test.go)

Three documents now assert normatively that virtual resolution surfaces WinnerFetchFailed rather than serving the cache — and nothing enforced it. The only virtual winner-failure test used a non-2xx, so unifying the two flows, the natural simplification for someone to attempt, left the suite green.

TestResolve_WinnerFetchRefusedCoding_WinnerFetchFailedNotFallback mirrors the NotImplemented propagation test beside it, stale row included so a fallback would be observable. Mutation-verified: routing the 2xx case to serveFallback — exactly that simplification — fails it with got <nil>.

↔️ Two flows, two answers, both intended

A refused coding is answered differently depending on the flow, and only the standalone answer was written down. Review read that as a defect in the resolver. It is not — the resolver is the one following the ratified rule.

  • Standalone (standalone.go:244) serves a cached copy, treating the refusal like a transport failure.
  • Virtual (resolve.go:801) surfaces WinnerFetchFailed{200} with no fallback.

The split is wider than the refused coding alone. standalone.go:244 keys on Status/100 == 2 || Status == 304, so a bodyless 204 or 206, and a 304 answered to a request that sent no validator, diverge identically. All four now share one Error Cases row and one statement of the rule.

The two are answering different questions. A standalone request names one upstream, so a refusal leaves the client nothing else the registry could serve and a cached copy beats an error. Virtual resolution has already run a selection: the winner answered its Probe and was committed to, so a coding it then volunteers is a deliberate act by a reachable upstream rather than an outage — the case the 5xx rule keeps visible rather than masking. Hiding it behind a stale copy would keep an operator from seeing a misconfigured upstream the resolver goes on choosing.

A Probe cannot pre-empt it: a HEAD has no body to refuse, and a coding on the HEAD would not bind the GET that follows. So the refusal is only ever discoverable after commitment, which is why it reads as a winner failure rather than an eligibility one.

This is now ratified rather than incidental — recorded in the Winner fetch failure section, a new Resolutions entry, the Content codings rule, the acceptance criterion, the Error Cases row, and both transport-failure enumerations.

Two Go doc comments carried the old, narrower story and are corrected with it. remote.WinnerFetchFailedError said it "reports … a non-2xx status", and its Status can be 200 on exactly this path — a caller mapping it verbatim would answer a client a success whose body it never received. resolve.go's fetchWinner said the same. Comment-only.

🔍 Deliberately out of scope

  • Content-Length on a decoded response. markUncompressed deletes it along with the coding, so a cold-fill blob GET against a gzipping upstream has no upstream length to forward and streams chunked, which conflicts with the OCI HEAD requirement. That is what the decode costs permanently rather than a tracked gap: gzip carries no usable declared output size, and deriving one would mean buffering the whole body, which the constant-memory contract rules out. The monolith's Workhorse cites the same failure for disabling compression.
  • Decoding the wider coding set. Deferred to #760, opened alongside this MR. No demand signal, and each codec adds decompression-bomb surface for upstreams that are already ignoring a SHOULD.

🧪 Validation

go build ./..., go vet, and go test on internal/remote/..., internal/virtual/..., internal/format/maven/..., internal/format/oci/... all pass, and golangci-lint reports 0 issues on those packages (run with --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false). markdownlint-cli2 and vale pass on every changed document. Rebased onto origin/main and re-validated on that base.

No production code changes. Every non-doc edit is a Go comment or a test, verified by filtering the diff's Go hunks for non-comment changed lines. Nothing this MR touches can change behavior. The commits keep their scopes separate, one per review round or concern, though squash collapses them on merge.

No ADR governs content codings — docs/adr/ mentions them nowhere. ADR-008 (content-addressable storage) is consistent with the rule and is cited by the S13 resolution, so no handbook amendment is needed.

Related to #397 (closed)

Edited by David Fernandez

Merge request reports

Loading
Loading