feat(virtual): scope rules, split aggregates, fail unexpected kinds

What

Implements item 2 of #609 (closed)'s scope, per the amended S13 (!1502 (merged)):

• Coordinate becomes an opaque struct built by NewCoordinate(defined, addressed). A rule on a defined-but-unaddressed field takes no part in eligibility (a skipped allow does not count toward "allow rules are present"); a rule on a field outside the defined set stays the fail-closed corrupt-rule arm, with precedence over a deny match on the same position; an addressed empty-string value (npm's unscoped scope) is a genuine value. The zero Coordinate defines no fields and fails every rule closed.
• The nothing-eligible aggregate splits. EligibleUpstreams widens from [][]Rule to (Coordinate, []UpstreamSpec) and returns the new remote.ErrAllUpstreamsIneligible when any position's ineligibility was a data error, remote.ErrAllUpstreamsDenied only when every position was a policy denial. The outcome slice rides beside either aggregate so the resolver still emits per-position events.
• The upstream-kind seam. NewUnexpectedKindUpstream is how a loading slice hands an out-of-range repositories.kind to the resolver as a position-level data error instead of dropping the row. The position fails closed (never looked up, probed, fetched, or recursed into), is error-logged, and its exclusion is audited under the new remote.ExclusionUnexpectedKind (unexpected_kind), distinct from the corrupt-rule reason, so the audit stream carries an event for the position and that event is not a denial. Kind() records the state in a field, so it cannot misreport hosted or remote.
• decisionForLookup validates the kind ahead of the fresh-hit success return, so its fail-closed arm covers every path now that Kind() can answer an out-of-range value (second line of defense behind the load-and-filter classification).
• Comment corrections riding along: the UpstreamKind doc retires the "rejected at association time and never reaches resolution" claim (the #609 (closed) follow-up the spec names), exclusion-arm enumerations gain the fourth arm, and the Reason/UpstreamOutcome/EligibleUpstreams docs state the true log-plus-audit pairing. Open !1665 (merged) fixes that same pairing wording on main; these comments are rebuilt here for the new semantics, and a rebase reconciles whichever lands second.

No production caller of the widened API exists outside internal/virtual, so no consumer accepts a value its write path rejects and there is no widened-predicate merge-order gap.

Review follow-ups

Four commits on top of the original pair, each in a surface this MR already introduces:

• A data error vetoes the not-found for the whole list. serveNoWinner no longer answers ErrFileNotFoundOnUpstreams when any position was excluded for a data error. That position was never consulted, so its eligibility was never determined; the positions that did report an absence establish it only for themselves. Resolution falls through to the cache, so a copy from a consulted position is still served and the terminal answer is ErrUpstreamUnavailable, which no client caches as an absence. The split aggregate only covered the nothing-eligible shape of this harm; this is the other one. S13 is amended to match (a design subsection, two Error Cases rows, an acceptance criterion, and a ## Resolutions entry), because its Error Cases table and its aggregates subsection pointed opposite ways on this case.
• Coordinate is guarded at both ends. NewCoordinate rejects an empty addressed set, which used to switch every rule off (a deny that stops denying, an allow that admits everything, ReasonEligible, no log line and no audit event); and Resolve rejects a Coordinate that bypassed the constructor with the new ErrZeroValueCoordinate, the third wiring sentinel beside ErrZeroValueUpstreamSpec and ErrResolverNotConfigured.
• runPhase1 checks each position against what its fan-out is about to do, rather than trusting the marking done in loadAndFilter. On today's wiring the check never fires. What it buys is the failure mode if that marking ever stops: the dereference sits in an errgroup child, and x/sync deliberately does not propagate a child panic to Wait, so one bad row would take the process down on every pod rather than fail the request. Mutation-checked: without the guard the new test crashes on the nil dereference.
• The exclusion audit surface is renamed off "denial", which no longer describes it: UpstreamExclusionAuditSink, RecordUpstreamExclusion, UpstreamExclusionEvent, UpstreamRepositoryID, and PhaseEligibilityFilter (eligibility_filter). The event now carries reason=unexpected_kind, a phase whose kind check consults no rule, and a repository id whose row is precisely not remote. The two data-error ERROR logs carry upstream_repository_id (the row an operator paged on this line has to fix) and upstream_kind, matching the audit event's field name rather than the sibling package's remote_repository_id.

NewCoordinate builds its defined set per call, and that map escapes: go build -gcflags=-m ./internal/virtual/ reports rules.go:133:20: make(map[TargetField]struct {}, len(defined)) escapes to heap. Kept as is. One map per resolution is noise beside the Lookup every resolution already does, and the alternatives (a caller-built set, or a bitmask since the values are 0..2 per format) move a correctness obligation the constructor's doc already spends a paragraph on into a shape each format slice has to get right on its own. Noted here so the next reader does not re-derive it.

Why

A single "nothing was eligible" verdict forces a slice to choose between reporting an absence it never established and reporting an error on a decision the rules did make. npm caches a 404 as a hard E404 it never retries, so a corrupt rule set answered as not-found fails every install permanently. The split is only computable in the foundation, where the per-position reasons exist. The kind check runs at resolution time because no foreign key can restrict kind and the association-time guard does not exist on every surface, so a row written by direct SQL reaches resolution on a client-reachable read path.

There is no plan MR for this work: the operator waived it (as for #549 (closed)), with the merged spec amendment as the plan-equivalent. This is deliberate, not a missed step.

Diff size

1984 insertions / 406 deletions across 21 files, by file group:

Group	Files	Diff
Production	internal/virtual/rules.go, resolve.go, upstream.go, decision.go, internal/remote/errors.go, internal/remote/audit.go, internal/managementapi/audit.go	+671 / -229
Test support	internal/remote/remotetest/fakes.go, logcapture.go	+65 / -13
Tests	11 files	+1213 / -145
Spec	docs/specs/S13-virtual-remote-foundation.md	+35 / -19

Splitting would not help: the Coordinate API change ripples through every existing test in the package, so any cut point leaves an intermediate state that does not compile or a temporary shim with no consumer.

Test plan

The first two commits keep the test-first split: the test commit lands the full suite against a panic skeleton, the feat commit makes it green. The review follow-ups ride in four commits after them, each carrying its own tests. Coverage below maps each spec criterion to its tests; the property test asserts the per-position Reason (not the collapsed boolean).

Three things were mutation-checked rather than reasoned about: the load-bearing evaluation order (moving the malformation check below the unaddressed skip fails exactly the pinning rows), the not-found veto and the constructor's copy guarantee (each fails with its production change reverted), and the Phase 1 guard (without it the new internal test crashes on the nil dereference inside the errgroup child).

Full untagged go test ./... passes, go test -race on the touched packages passes, golangci-lint (repo-pinned 2.12.2, --max-same-issues=0 --max-issues-per-linter=0) reports 0 issues on ./internal/virtual/... and ./internal/remote/..., including the --build-tags=integration run for the touched integration-tagged file.

No e2e scenario is added or affected: the change is foundation-internal, and no HTTP surface on main consumes the widened API yet. The client-visible scenarios land with the virtual slice MRs that map the new aggregate.

Spec coverage

Spec: docs/specs/S13-virtual-remote-foundation.md

Scope: work item #609 (closed) item 2. Rows cover the Upstream-set validation criteria plus the dispatch brief's pins and the related error case. The remaining S13 criteria are owned by previously merged S13/S14/S15 MRs and are unchanged by this work.

Acceptance criteria

#	Criterion	Tests
USV-1	Empty virtual repository returns NoUpstreams (held before this MR)	TestResolve_NoUpstreams, TestEligibleUpstreams/empty upstreams returns ErrNoUpstreams
USV-2	All upstreams denied by well-formed rules returns AllUpstreamsDenied, one audit event per denial (held before this MR)	TestResolve_AllUpstreamsDenied, TestResolve_TwoAggregatesDistinguishable/every position denied by a well-formed rule is AllUpstreamsDenied, TestEligibleUpstreams/every position denied by well-formed rules returns ErrAllUpstreamsDenied
USV-3	A rule on an unaddressed field does not decide: an allow on such a field does not exclude by its miss, a deny on it does not exclude, no filtering by the calling slice	TestResolve_RuleOnUnaddressedField_DoesNotDecide, TestEligible_UnaddressedField, TestEligible_EmptyStringAddressedValue, TestEligible_UnaddressedRulesTakeNoPart_Property
USV-4	A corrupt target_field fails closed: ineligible, error-logged, audited under the corrupt-rule reason and the event is not a denial; unrecognized rule_type behaves the same; AllUpstreamsIneligible when nothing is left eligible	TestEligible_UndefinedTargetField, TestEligible_UnknownRuleType, TestResolve_InvalidRule_ExcludedAndAuditedAsInvalidRule, TestResolve_TwoAggregatesDistinguishable/one corrupt target_field flips the aggregate to AllUpstreamsIneligible, TestEligibleUpstreams/denial beside an invalid rule returns ErrAllUpstreamsIneligible
USV-5	An upstream of an unexpected kind fails closed: ineligible, error-logged, audited under the kind data-error reason (not the corrupt-rule or policy-denial one), not recursed into	TestResolve_UnexpectedKindUpstream_FailsClosed, TestNewUnexpectedKindUpstream, TestEligibleUpstreams/unexpected kind alone returns ErrAllUpstreamsIneligible, TestEligibleUpstreams/unexpected kind beside an eligible position returns no error, TestOutcomeStringers/exclusion unexpected kind
USV-6	The two aggregates are distinguishable: all-denied returns AllUpstreamsDenied, the same list with one corrupted position returns AllUpstreamsIneligible	TestResolve_TwoAggregatesDistinguishable, TestResolve_DenialBesideUnexpectedKind_Ineligible, TestSentinelErrors_IsRoundTrip/all upstreams ineligible
USV-7	A data error vetoes a lower position's absence: never FileNotFoundOnUpstreams, a cached copy served when one exists; a policy denial beside the same absence does not veto it (added to S13 by this MR)	TestResolve_DataErrorBesideEstablishedAbsence_NotNotFound, TestResolve_DataErrorBesideProbeMiss_NotNotFound, TestResolve_DataErrorBesideCachedCopy_ServesCache, TestResolve_EstablishedAbsenceWithoutDataError_StillNotFound, TestResolve_DenialBesideEstablishedAbsence_StillNotFound
P-1	Within one position a data error outranks a deny match (brief pin)	TestEligibleUpstreams/invalid rule wins over a matching deny in the same upstream, TestEligibleUpstreams/corrupt target_field wins over a matching deny in the same upstream, TestEligibleUpstreams/unexpected kind outranks a matching deny in its own position
P-2	Mixed list: the denial beside a data error is still audited as a denial, the aggregate is the ineligible one (brief pin)	TestResolve_DenialBesideUnexpectedKind_Ineligible, TestEligibleUpstreams/denial beside an unexpected kind returns ErrAllUpstreamsIneligible
P-3	Zero-value pins: the zero Coordinate fails rules closed; the zero UpstreamSpec never reads as remote (brief pin)	TestEligible_ZeroCoordinate, TestEligibleUpstreams_ZeroCoordinate, TestUpstreamSpec_ZeroValueKind, TestResolve_ZeroValueUpstreamSpec_Guarded
P-4	NewCoordinate validates at entry: an addressed field outside the defined set is a caller bug, and so is addressing no field at all (the second rejection added on review)	TestNewCoordinate, TestNewCoordinate_CopiesItsInputs
P-6	Wiring guards: a Coordinate that bypassed the constructor is rejected before any phase, and a position that cannot answer a lookup is failed rather than dereferenced (both added on review)	TestResolve_ZeroValueCoordinate_Guarded, TestRunPhase1_UnusablePositionFailsClosed
P-5	The outcome slice is still returned beside either aggregate (brief pin)	TestEligibleUpstreams (assertEligibleUpstreams compares outcomes on every error row)

Error cases

#	Condition	Tests
E-1	Virtual: nothing eligible and at least one position ineligible for a data error returns AllUpstreamsIneligible; each data-error position error-logged and audited under the reason naming its cause; a policy denial at another position still recorded as a denial	TestResolve_TwoAggregatesDistinguishable, TestResolve_DenialBesideUnexpectedKind_Ineligible, TestResolve_UnexpectedKindUpstream_FailsClosed
E-2	Virtual: every upstream filtered out by well-formed rules returns AllUpstreamsDenied (held before this MR)	TestResolve_AllUpstreamsDenied
E-3	Virtual: no winner, some position established an absence, and another was excluded for a data error: serve the cached copy if one exists, otherwise UpstreamUnavailable, never FileNotFoundOnUpstreams (S13 row added by this MR)	TestResolve_DataErrorBesideEstablishedAbsence_NotNotFound, TestResolve_DataErrorBesideProbeMiss_NotNotFound, TestResolve_DataErrorBesideCachedCopy_ServesCache

Security considerations

#	Concern	Tests
S-1	SSRF and outbound request safety (security covers)	Owned by the upstream HTTP client, shipped in prior S13 MRs; this MR changes no outbound surface. Not tested in this MR.
S-2	Fail-closed rule evaluation: no misreading may turn a deny rule into a silent no-op	TestEligible_UndefinedTargetField, TestEligible_UnaddressedField, TestEligible_ZeroCoordinate, TestEligible_UnknownRuleType
S-3	A deny rule is scoped to the requests that address its field, so it is a request filter and not a content block. Named in S13's Security Considerations by this MR, since only the allow half was argued there	TestEligible_UnaddressedField/deny on the unaddressed field does not exclude, TestResolve_RuleOnUnaddressedField_DoesNotDecide
S-4	A coordinate addressing no field would switch every rule off with no log line and no audit event; the constructor rejects it	TestNewCoordinate/empty addressed set is rejected, TestNewCoordinate/addressed set present but empty is rejected

Chosen readings (spec section -> reading pinned by the tests):

• Scoping to the fields a request addresses -> Coordinate is an opaque struct built by NewCoordinate(defined []TargetField, addressed map[TargetField]string) (Coordinate, error); an addressed field outside the defined set is a constructor error. Only "an error is returned" is pinned; no sentinel, since neither spec nor brief names one.
• The two ineligibility aggregates -> the split is computed by EligibleUpstreams, whose input widens from [][]Rule to []UpstreamSpec so the kind seam can surface as a per-position Reason (ReasonUnexpectedKind), matching the brief's "any position's reason is a data error".
• Phases, load-and-filter kind check -> the slice hands the row over as NewUnexpectedKindUpstream(repositoryID, kind, rules); the exclusion audit event's UpstreamRepositoryID carries that repositoryID, since the row id is the operator's handle on the defective row (the spec does not name which id the event carries). The field was RemoteRepositoryID until this MR: the value it now also holds is by definition not a remote repository, and no production sink or dashboard consumes it yet, so the rename is free here and breaking later.
• Upstream-kind eligibility is a resolution-time concern -> UpstreamSpec.Kind() for such a position is pinned only as neither UpstreamHosted nor UpstreamRemote; the exact value is the implementation's choice. The zero-value UpstreamSpec.Kind() is pinned as not UpstreamRemote.
• Allow/deny rules -> a zero Coordinate with no rules at all stays eligible, as a consequence of "an upstream with no rules at all is eligible for every path" (no rule ever consults the coordinate).

Related to #609 (closed)