feat(virtual): the binding-missing orphan shapes 1/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) the two binding-missing orphan shapes, in internal/virtual and internal/remote 1,333
2 !2197 (merged) the per-request memo adapters and the fall-through counter 1,108
3 !2198 (merged) the loading slice that installs both 2,104

!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 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.

Plan corrections: !2191 (merged) is merged, and it grew before merging to cover both binding-missing shapes, so the amendment this MR originally carried is superseded. The rebase onto main takes !2191 (merged)'s version of the plan whole, because where the two overlap !2191 (merged) is the better text: it measures Step 11 at 628 rather than estimating 575, renames the batching heading to "the foundation additions change no contract" and fixes the four anchors citing it, corrects the health column to NOT NULL and the adapter count to two, and carries five Step 11 Files bullets this branch never had.

What !2191 (merged) could not contain is re-applied on top, in 61a17744. Its figures were measured at edc6336d, the unsplit step-11 branch, which predates this MR's first review round, so seven of the ten restored passages describe code that round added: the kind guard on both constructors (upstream.go:192 and :203, where !2191 (merged) still says neither panics on an in-range kind), the both-repairs ERROR lines (resolve.go:982 and :995, where !2191 (merged) says the audit points at a row to restore), the runPhase1 case per shape, the distinct-and-appended reason assertions, and the sidecar's record of the four over-cap blocks. The other three are facts about the work: the spec's own enumeration is still a closed list of two against an implementation that takes four and is deliberately left uncorrected, Step 11 ships as three stacked MRs, and npm caches a 404 as a hard E404 it never retries. The Status table is untouched, so its single-writer rule still holds.

What this part does

The S13 foundation touch on its own. A virtual repository's upstream list can hold a row no read path can use; before this change one such shape had a constructor (the out-of-range kind) and two did not. Both of those carry an in-range kind, so NewUnexpectedKindUpstream panics on them by contract:

  • kind=2 with no npm_remote_repositories row. NewRemoteUpstream panics on the nil Cache it leaves.
  • kind=0 with no npm_repositories row. NewHostedUpstream takes it without complaint, which is the quieter failure: the position answers a Miss, an absence no upstream established, so resolution may answer 404 for a package a repaired row holds.

Fabricating an out-of-range kind to slip past the panic is worse than either, because the audit stream would then record unexpected_kind for a row whose kind is genuinely hosted or remote.

So: two constructors keeping the observed kind, two UpstreamExclusionReason values appended after ExclusionUnexpectedKind, two Reason values classified from their own marks before rules are consulted, and two loadAndFilter arms each with its own ERROR log naming the table to repair.

Each constructor panics unless the kind it is handed matches the join it names. The mark alone keeps the shapes apart, but it would let Kind() report the sibling kind, and the pair is what an operator reads: the line would say "row is remote but carries no remote binding" beside an upstream_kind of 0. Nothing downstream catches that, because an orphan is failed before any phase reads its kind, so the disagreement reaches only the log and the audit event, which are the two places the distinction exists for. The loading slice in part 3 calls both from inside a switch on that same kind, so the check costs it nothing.

Each ERROR line names both repairs the reason admits, not the likelier one, for the reason under Outstanding spec question below: an upstream naming a repositories row of another format reaches the same reason and is repaired by removing the upstream rather than restoring a row, and no field on the event can tell the two causes apart.

The three shapes need three distinct marks. EligibleUpstreams classifies on the presence of a mark alone, so two sharing one collapse into one reason with nothing failing, which is why the tests put all three in one list rather than asserting each alone.

The exclusion switch also gains a default arm, from the AppSec review. excludePosition fails the position before the switch runs, so a Reason outside the declared set spent it with no ERROR line, no audit event, and dataError left false, and a list whose every position went that way reaches serveNoWinner, which answers a 404 for an absence nothing established. exhaustive runs with default-signifies-exhaustive: false, so every declared value keeps needing its own arm and the new one covers only the value no constant names. It records the data error and error-logs exclusion_reason, and it emits no audit event because no remote.UpstreamExclusionReason can truthfully name the exclusion, which is why logUnusableForLookup is silent in the same stream. That is a fourth ERROR site, unreachable because it marks a defect in this package rather than because part 3 has not landed.

Additive: no existing exported signature, panic condition, or enum value changes meaning; the only new panic conditions are the kind checks on the two new constructors. UpstreamSpec gains an unexported orphan discriminator, Kind() and the internal repository-id accessor gain arms for it, and the zero-value test becomes a predicate over a field every constructor sets.

Nothing calls the constructors yet. Part 3 does. That is the cost of this cut, and it is the reason the plan originally kept the foundation touch inside one step. The three ERROR log sites are therefore unreachable on this branch, and orphan_upstream_shapes.md states its volume figures as what they will admit once part 3 lands rather than as what they emit today.

Outstanding spec question

An npm virtual upstream list can name a repositories row of another format: the composite FK constrains only the namespace and the list read applies no format predicate. Such a row has an in-range kind and no npm format child, which is byte-for-byte a binding-missing orphan, so it is excluded as a data error, and because a data error vetoes the not-found for the whole list one such upstream turns every otherwise-404 read of that virtual repository into a 503 (a 500 when it is the only position). Neither S31 nor S13 rules on it: both enumerate the data-error causes as a closed list naming a corrupt rule and an out-of-range kind.

internal/virtual/orphan_upstream_shapes.md documents both readings rather than special-casing either. The recommended answer is rejection at the association write path, where the format is known. That ruling is recorded as the plan's fourth Open Question, which !2191 (merged) added to main; this MR adds to it the reason the case cannot be narrowed silently, that npm caches a 404 as a hard E404 it never retries. Per guardrail 1 this is a ruling for the spec author, not a choice for this MR.

Size

Measured at the head commit: 431 reviewable production Go (242 added, 189 removed), 562 test (550 added, 12 removed), and 340 Markdown (324 added, 16 removed), for 1,333 reviewable in total.

The production figure is the reviewable one and it is under the 500 ceiling. The removals are large because the comment caps force a block a diff touches down to its cap, so widening four enumerations by two values each rewrote the blocks around them; orphan_upstream_shapes.md is where that argument went instead.

The Markdown splits 275 for orphan_upstream_shapes.md, which carries the argument once instead of in fragments across the four files, and 65 for the plan port described under Plan corrections above. A split would not help: the plan port gates the code by review, not by compilation, and the operator asked for both in one MR.

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.

Review round 1

Applied in 4a2e9708c and 275d99c7b (code) and 61a17744 (plan). The two code commits were rebased onto main; their pre-rebase shas were 2463a3389 and 8c3ef37b9.

Finding Fix
The merged plan ratifies one shape, this MR ships two, and its "four edits" and "only internal/virtual and internal/remote edits" claims are falsified in three places Ratified on main by !2191 (merged), which now carries both shapes, eight edits, the ## Research Findings correction and the fourth ## Open Question. This MR keeps that text and ports the ten passages it predates, per Plan corrections above
The two constructors accept any kind, while orphanBindingRow.kind's comment claims it is "in range and correct" and the sidecar said a disagreeing kind is accepted Each constructor panics unless the kind matches its join; field comment corrected; four new panic subtests covering the sibling kind and an out-of-range one
TestRunPhase1_UnusablePositionFailsClosed covers the zero value and the unexpected kind only, and the new shapes lose answersLookup's out-of-range barrier A table case per new shape, and the sidecar section that explains why that guard is pinned through runPhase1 rather than Resolve
The sidecar asserted the three log sites are "reachable from a virtual read", which is false until part 3 Reachability and the ERROR-volume figures moved to the tense that holds on this branch
The sidecar disclosed stale enumerations as "several doc comments in resolve.go", which a reader cannot check, and missed decision.go All four named by file:symbol in a table, each quoted and measured against its own cap; internal/remote/audit.go gains the pointer that reaches the sidecar from that package
The counter obligation named no tracking item #1038, cited once, in the sidecar paragraph that states the obligation
The hosted _FailsClosed test dropped four assertions its remote twin carries Health-read, NamespaceID, At, and the invalidRuleLog negative restored
excludePosition's eligible-reason guard read as redundant with its caller Comment says what it actually prevents: failing an eligible position marks no data error, so serveNoWinner could answer a false not-found
Two "why"s lost to the cap sweep landed nowhere UpstreamKind regains the no-FK-restricts-kind fact; ErrZeroValueUpstreamSpec regains why it returns where a constructor panics

The one finding left open is the cross-format upstream under Outstanding spec question. It needs a spec ruling, not code, and it is now recorded as the plan's fourth Open Question.

Related to #892 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading