feat(oci): upstream URL, segment safety, name transform (S16 step 3)
What this ships
S16 Step 3 — the pure, S13-independent building blocks for container remote repositories. No route is wired to any of it; Steps 12–16 compose it.
- Upstream URL construction (
remote_upstream.go) with per-segment path escaping, so a multi-segment image name stays multi-segment:library/nginxbuilds<base>/v2/library/nginx/manifests/…, neverlibrary%2Fnginx. Four endpoints — manifests, blobs,tags/list, referrers. - The outbound reference gate, applying S12's tag grammar to a non-digest
<reference>before it reaches an upstream URL or a cache lookup. It sits in the same file as the builder because it exists to guard the interpolation the builder performs. Asha256:-prefixed reference with a malformed hex portion routes to400 DIGEST_INVALIDthrough the samereadReferenceDigestInvalidhelper the hosted read path uses; everything else failing the grammar routes to the404 MANIFEST_UNKNOWNsentinel. - The
tags/listnamerewrite (remote_nametransform.go) as a constant-memoryio.Readerdecorator: the root object'snamebecomes the AR-facing path, every other byte is relayed verbatim. Pure bytes, no HTTP, which is why it lands here rather than in the handler step — the same place S15 puts itsdist.tarballtransform, for the same reason.
Two fuzz targets, both wired into fuzz:oci so they run coverage-guided rather
than replaying a seed corpus: FuzzOutboundUpstreamURL and
FuzzTagListNameRewrite. The job's changes: anchor gains
internal/remote/**/*, matching the anchor fuzz:npm and fuzz:maven already
carry, because the URL target reaches remote.ParseUpstreamBase and would fail
at fuzz setup if the shared gate's refusal set changed.
Size
The plan estimates Step 3 at ~880 lines "roughly half of it table-driven tests". That estimate is wrong by about 3.5x, and the actual diff is 3,109 insertions across 7 files — 1,734 non-blank, non-comment Go lines (936 production, 2,126 test raw). This is above the 500-line ceiling in development-model.md, so per that document: here is the justification rather than a split.
Measured against the two sibling building-blocks MRs with the same filter
(added Go lines, blanks and // comments stripped):
| MR | Reviewable Go | Raw insertions | Files |
|---|---|---|---|
| S14 maven Step 5 — remote building blocks | 1,533 | 2,415 | 7 |
| S15 npm Step 4a — remote building blocks | 1,387 | 2,446 | 6 |
| S15 npm Step 4b — remote rules and transform | 2,254 | 3,775 | 41 |
| S16 Step 3 (this MR) | 1,734 | 3,109 | 7 |
This lands about 13% above S14's directly analogous MR on reviewable Go, at the same file count, and below S15's larger half. The weight is test, not production: 2,126 of the raw lines are tests, against 936 of production, which is the ratio the outbound security boundary warrants — every rejected byte class gets a row, and the refusal tables are asserted twice, once against the gate directly and once through the builder that calls it. The test share grew after review: four guards in the transform turned out to be deletable with the suite green, and the structural-mismatch, skipped-value, and truncation-tail tables that pin them are the largest single addition.
Why one MR rather than 3a/3b. Two seams exist and neither helps:
- The file seam (URL builder + gate |
nametransform) is clean — the two halves share no symbol. But it splits into roughly 1,250 and 1,475 lines, so both halves stay 2–3x over the ceiling. It converts one warning into two. - The gate | builder seam inside
remote_upstream.gois the only cut that yields a sub-500 MR (~380 for the gate and its tables). It is also precisely the split S14's own size justification rejected — "splitting would put the URL builder and its rejection tables in separate MRs, which is where the review value is lowest" — and what this plan argues against in Step 3's Files entry: the gate is in the builder's file because it guards the builder's interpolation. Separating them across two MRs means reviewing an interpolation whose guard is not in the diff.
The transform itself is one 584-line byte state machine; any cut inside it ships a half-working scanner. So the honest options are one MR at ~1,555 reviewable lines, matching what both siblings did for this exact class of work, or three MRs of which two are still over the ceiling. This is the first.
Conformance tests
Not applicable to this MR. These are pure functions with no route wired to them — no client can reach any of it until Steps 12–16 compose it into the dispatcher. Per the plan, Step 18's hermetic end-to-end proxy harness is this slice's conformance deliverable for a proxy path, as it is in both siblings.
E2E scenario catalog
No change, and deliberately so. docs/testing/e2e/oci.md
lists "Virtual and remote (proxy/cache) repositories" under Out of scope until
the capability ships, and it stays there for this MR: nothing here is reachable
by a client, so no user journey changes. The plan assigns the catalog edit to
Step 18, which moves remote out of that list and adds pull-through scenarios
for a cache miss, a cache hit, a stale-tag revalidation, and a token-auth
upstream, with docs/testing/e2e/docker.md getting the Docker-client-driven
equivalents.
This is worth stating explicitly because it is not the answer S14 and S15 gave. Both said "no catalog exists for this format" — true for Maven and npm. An OCI catalog does exist, so the answer here is that the capability it gates has not finished shipping, and the step that finishes it owns the edit.
Notes for review
- Copy, not import. The transform copies S15's
TarballURLRewriterbyte state machine including its key-candidate and nesting-depth caps; the URL builder copies S14'sremote_upstream.go. Neither is imported: the linter allows it, ADR-023 does not. Nointernal/format/npm/...orinternal/format/maven/...import appears in any new file. - Not npm's escaping. npm's
Tarballescapes a scoped name as a single%2Fsegment, which is deliberately the opposite of what a multi-segment image name needs. The builder splits on/and escapes per component. ErrUnsafeUpstreamSegmentis coarse on purpose. It does not distinguishNAME_INVALIDfromDIGEST_INVALID, matching how S14 and S15 collapse error codes within one status class and split only across status classes. The plan's criterion table now assigns the remote arms' own digest check to Steps 11 and 14–16 rather than assuming S12 covers it — it does not, since S12's digest checks sit in the hosted per-action handlers, below where thekindbranch goes.- Allocation bound is tested, not asserted by comment. The oversize-name
test asserts only on emitted bytes, so a scanner that buffered the 4 MiB name
and dropped it at the closing quote would pass it.
TestTagListNameRewriter_BoundedAllocationmeasures gross allocation instead (5.4 KiB measured against a 64 KiB budget), followingTestBlobGetRangeBoundedAllocationin this package. Verified by mutating the drop path to buffer and release: the new test fails at 21 MB while the byte-level one still passes. - The nesting cap fails closed. Past
maxNameNestingDepththe scanner can no longer tell a root member from a nested one, so an open that would carry the document past the cap ends the stream withErrTagListNestingTooDeeprather than relaying a tag list whosenameit can no longer scope. Nesting to exactly the cap is still relayed, so the accepting side of the bound is unchanged. Reaching the cap takes 64 KiB of opening bytes, every one already relayed, so the refusal surfaces mid-stream: the client sees an interrupted200, the line S16 and S14 both draw once streaming has started. The sentinel is exported forerrors.Istriage — telling this refusal apart from a truncated body and a source read error — not for a status mapping. - npm's tarball rewriter still carries four shapes this file corrects. The
decode placement, the nil-receiver guard on
Transform, the one-shot output sizing, and the escape-encoded comment example were all corrected here and not ininternal/format/npm/npmremote, per the copy-do-not-import rule. Fixing npm is tracked in #505 and is a separatefix(npm):MR. The receiver guard is the one worth watching: npm'sTransformchecks the zero value (rw.arTarballPrefix == ""), not a nil receiver, and reading that field off a nil pointer is itself the panic site, so it catches&TarballURLRewriter{}and notnil. It is latent rather than live — nothing outside that package's tests calls the constructor until npm remote is wired.
Related to #288