feat(npm): the upstream assembly 3/3 (S31 plan: 11/19)
The stack
Step 11 split from !2194 (closed), in merge order. Each part compiles, carries its own tests, and is reviewable alone.
| # | MR | What | Reviewable LOC |
|---|---|---|---|
| 1 | !2196 (merged) (merged) | the two binding-missing orphan shapes, in internal/virtual and internal/remote |
1,045 |
| 2 | !2197 (merged) (merged) | the per-request memo adapters and the fall-through counter | 1,108 |
| 3 | !2198 (merged) | the loading slice that installs both | 2,028 |
!2194 (closed) is closed in favour of these. The production Go is byte-identical to what it carried: virtual_assembly.go, virtual_memo.go, metrics.go and the four foundation files are unchanged by the split, which only moved shared test fixtures into virtual_testsupport_test.go and split the one contract sidecar into virtual_memo_contract.md and virtual_assembly_contract.md so that no part cites a file a later part introduces.
The three are not equal in size, and cannot be without breaking something worth more. Part 3 is 49% of the stack (2,028 of 4,181) because its two remaining seams both resist a cut: shipping virtual_assembly.go without virtual_assembly_test.go would violate the tests-in-the-same-MR guardrail, and splitting the assembly itself passes a half-built slice across an MR boundary, which the plan's own Seam interrogation bullet rejects by name. Parts 1 and 2 are within 6% of each other.
Merge gate for the whole stack: !2191 (merged) corrects the plan in eight places where this implementation contradicts it. It has merged, along with parts 1 and 2, so this part now targets a main that carries all three.
Rebased onto main after parts 1 and 2 merged. The branch previously carried their commits, which is what made it conflict. Rebasing dropped those, and dropped three test functions this part's virtual_assembly_refusal_test.go duplicated once part 2's review moved them into virtual_memo_test.go: the health reader's nil-store and zero-value refusals, and a three-case NewVirtualRemoteFetcher table that main's TestNewVirtualRemoteFetcher_RefusesWiringDefects already covers with seven cases, including all three. No coverage is lost; the file is 76 lines shorter and now holds only the assembly's own faults, which is what its header always said it was.
What this part does
The loading slice: a virtual repository and a target become []virtual.UpstreamSpec plus a virtual.Coordinate. It is what installs part 1's constructors and part 2's adapters.
Four statements for the load and Phase 1, whatever the position count: the upstream-list read, the batched rules read, and one batched Lookup per upstream kind present. Nothing per position across those phases, which is the criterion this step exists for.
Phase 2 is not covered by that count, and the sidecar says so. Every probe builds its request through LiveUpstreamConfigSource, which reads its own position's row, so a cold read pays one statement per probed position. That read is the per-request credential materialization and cannot be batched ahead of the fan-out without holding every position's credentials for the whole request, which is the opposite of what the list read's projection buys. TestVirtualUpstreamAssembly_ColdReadPaysOnePerProbedPosition asserts the fixed part stays fixed at 2 and at 20 upstreams while the probed part grows, so the open seam is measured rather than hidden.
Two refusals, both per request rather than at construction:
- An unset
npm.public_registry_urlon a metadata read over a remote position.packumentBaseURLreturns a non-empty host-relative/{slug}/npm/{name}from an empty base, so nothing downstream catches it, and a fill would write host-relativedist.tarballvalues into thenpm_remote_metadata_filesrows the standalone remote route also serves. Per position, because the field is optional and a hosted-only deployment must keep booting. - A requested position the batched hosted lookup returned no verdict for. The zero
remote.LookupResultis a Miss, resolution may answer404for it, and npm caches a404as a hardE404it never retries, so a seam that dropped one id would make a published package permanently missing. The converse violation is already refused aserrVirtualHostedUnrequested.
Assemble returns the caller's context on every error path: the idiomatic ctx, upstreams, err := a.Assemble(ctx, req) otherwise nils it exactly where the next line reports the failure, and log.FromContext panics on a nil one.
The index-to-stored-position mapping rides alongside the built slice, because ResolveResult.Position is an index into that slice while the position column is unique but not contiguous, so the two disagree for exactly the lists an operator is debugging.
Size, and why this part is the big one
528 production, 1,397 test, 103 Markdown added (2,028 reviewable) — 49% of the stack. Past the 500-LOC ceiling, and the plan's Step 11 Seam interrogation bullet carries the argument: the list load, the kind classification and the UpstreamSpec construction share the loaded rows, so splitting them passes a half-built slice across an MR boundary. Two thirds of the added lines are tests, which the tests-in-the-same-MR guardrail requires here rather than in a follow-up.
What remains after this stack
- The
rapidproperty test is a plainrapid.Checkover a drawn slice rather thanrapid's state-machine form, which the plan anddocs/dev/go-testing.mdboth name. All three drawn dimensions and the lowest-position-success invariant are asserted, so the substance is covered and only the technique differs. - No counter yet for data-error exclusions, which is what would make part 1's three per-read ERROR lines safe to ration. It belongs with the resolution metric family rather than in
internal/virtual, where a limiter would ration the line for every format over the foundation. - The assembly-to-registry composition (someone handing in a per-request
FlightRegistry) cannot be tested until Step 14 exists to wire it. - Several data-error enumerations in
resolve.go, and the doc onremote.ErrAllUpstreamsIneligible, still list the two causes that existed before the binding shapes. Those blocks are 15 to 21 comment lines against caps of 1 and 3, so correcting one clause means deleting the block; the current list lives inorphan_upstream_shapes.md, which says so and why.
e2e scenarios and surfaces
No scenario added or affected: docs/testing/ has no npm virtual catalog and the plan assigns the harness and catalog entries to Step 19. Nothing in this stack is reachable from a route, so there is no client-visible behaviour to script yet. No migration, no SQL, no config or proto change, no route, status code, header or response shape, and no api/openapi or api/bruno change.
Related to #892 (closed)