docs(plans): reverse two Step 2 verdict arms and record the guard
What
Amends docs/plans/2026-09-04-virtual-collection-mode.md. A review of Step 2's implementation branch found two arms of its no-contribution table wrong. This corrects them, corrects the Step 2 test prose that still described the superseded rulings, and records two obligations in Step 4 that until now lived nowhere.
The Status table is untouched. !2483 (merged) owns it.
Merge order
| # | MR | What |
|---|---|---|
| 1 | an S13 amendment, not yet open | settles the mixed absence-plus-PositionDataError shape in the owning spec, per the section below |
| 2 | this MR | amends the Step 2 failure table, the Step 2 test prose, the Step 4 item list, and the amendment ## Dependencies books |
| 3 | !2541 (merged) | Step 2's implementation, which already carries the code for both corrected rows |
This MR has to merge before !2541 (merged). !2541 (merged)'s branch already implements the corrected rulings, so merging it against the unamended plan leaves the tree contradicting its own plan. ## Dependencies states the order in the plan itself, so it survives the branches.
The two reversals
Each is quoted in ## Dependencies against the ruling it overturns.
A PositionDataError position withholds the not-found. Superseded: "PositionDataError, PositionSlow and PositionPanicked do not withhold the not-found and PositionOmitted does not either." That holds for the other three. It is wrong for PositionDataError, which answered with a document and so holds the path. internal/remote/errors.go states the rule on the sentinel itself: ErrFileNotFoundOnUpstreams "asserts an absence, so resolution returns it only when every position that could establish one did."
The cost of the old ruling is why it is corrected in the plan rather than left to a follow-up. On [hosted (Lookup miss), npmjs (over-cap packument)] it answers 404, npm caches that as a hard E404 it does not retry, and a format cap does not change between reads, so the wrong 404 repeats. Reverting the service does not clear the client-side caches.
The fail-closed scan answers ErrUnsettledPositionOutcome, not remote.ErrUpstreamUnavailable. Both of its conditions, an unfilled record and an outcome outside the ten defined, name a defect in this service rather than an upstream that was unavailable. internal/virtual already exports three sentinels for that class (ErrZeroValueUpstreamSpec, ErrResolverNotConfigured, ErrZeroValueCoordinate), which internal/format/npm's virtualServerError answers with a 500. The old row filed a wiring bug in the upstream-outage bucket at WARN and sent a Retry-After for a failure no retry clears.
An S13 amendment is owed
Raised in review and settled by the S13 co-DRI. An earlier revision of this MR said none was owed; that was wrong for one shape, and ## Dependencies now books it as a merge gate.
Which reading row four relies on. "Data-error position" in the withholding rule means the eligibility pass's exclusion. S13-virtual-remote-foundation.md:1190 scopes the veto to a position "excluded for a data error (a corrupt target_field, an unrecognized rule_type, or an out-of-range kind)", and both consumer specs spell the same scope out as "one position ineligible for a data error" (S30-maven-virtual.md:553, S31-npm-virtual.md:1418-1419). That is PositionIneligible, so it does not reach PositionDataError, which answered with a document.
The conflict that leaves. S13's post-basis Collection mode settles the no-contribution path itself: an absence established at any consulted position returns FileNotFoundOnUpstreams; its withholding arm is the eligibility-pass one above, so it does not lift that for a PositionDataError; and the paragraph closes by claiming every arm in it settles a collection that produced no document at all. Row four reverses that for a mixed absence-plus-PositionDataError set. The amendment is owed in the owning spec, and this MR records it as owed rather than authoring it.
Two defects in the old entry, both gone with it. Its stated reason cited S13-virtual-remote-foundation.md:342, a sentence in the vetoed read paragraph that the plan's own row four disclaims by name, and :342 was a head line where the plan's preamble asks for a basis one: at 07f4e8521 that line sits inside the worked example's diagram. Its claim that S30-maven-virtual.md and S31-npm-virtual.md "state no criterion for a record set with no remaining contributor" was false. Both state that set's arms (S30-maven-virtual.md:389, S31-npm-virtual.md:665-673), and each routes its withholding arm to the one S13 rule by anchor (S30-maven-virtual.md:647, S31-npm-virtual.md:672), which is why the amendment belongs in S13 and the two inherit it.
Step 2's test prose still described the superseded rulings
Found reviewing !2541 (merged) against this branch. The amendment reached the failure
table, the precedence paragraph and ## Dependencies, and stopped there, so
the Tests paragraph below the table still described the arms this MR
reverses. Merging the pair as it stood would have left the plan contradicting
its own table and the test file on !2541 (merged)'s branch, which is the thing the merge
order exists to prevent. Five passages:
- The second named case listed
PositionDataErroramong the contributor-side failures an absence stands beside. Row four withholds the not-found for it, andTestCollectVerdict_AnAbsenceStandsBesideAContributorSideFailureexcludes it by name. Its own pair is described instead, with the set carrying both withholding values. - The eighth case and the all-
PositionUnknowncase both said the set answers unavailable. Row two answersErrUnsettledPositionOutcome, and the test asserts it is neither the not-found nor the outage. - The contribution-first note said such an implementation answers unavailable for the wrong reason. It reaches the scan and answers the wiring sentinel, so only a set carrying a contribution separates the two orderings.
- "Failures are the
remotepackage's sentinels" is no longer true of all six arms: five are, and the sixth is this package's own. - The named-case count was seven, and is eleven at !2541 (merged)'s head.
Step 4 gains the record-validity guard and the npm taxonomy registration
The obligation to reject a PositionRecord whose Outcome is PositionUnknown or out of range was recorded nowhere. !2541 (merged) carries Step 2's own fail-closed scan over the same two conditions, but neither that scan nor this plan asked Step 4's mapping to reject them at the boundary that admits them, so Step 4 could have landed without it. It is now an item in Step 4's own list, with what closing it does not buy: CollectVerdict is exported over a PositionRecord that has no constructor, so any caller can declare one and Step 2's scan stays. The mechanism is an error rather than a panic: the state-to-record mapping signs ([]PositionRecord, error) and returns an error wrapping ErrUnsettledPositionOutcome naming the position, matching the shape Step 2's fail-closed arm already uses, and Collect propagates it. That is what gives the item's demanded test something to assert.
The second item is new here. internal/format/npm's virtualServerError names
the three existing internal/virtual wiring sentinels and not
ErrUnsettledPositionOutcome, so the sentinel reaches its 500 through the
fall-through and logs as "a reason the taxonomy does not name" rather than the
named-infrastructure record its siblings get. Set membership is the whole fix,
and no virtualReadAnswerFor arm is added: that function reports false for
every value that answers 500 (virtual_read_errors.go:131), and the three
siblings have no arm there either. The status is already right; only the
classification is not. Nothing can reach the sentinel until Collect is wired,
which is why the registration lands on Step 4 rather than Step 2, and why
Step 2's doc comment states the relationship in the future tense instead of
claiming a slice already answers it that way.
The item covers virtualServerError, five sites in
virtual_read_errors_test.go, and eight count sites in
virtual_read_errors.md: every count either file carries that the
wiring-sentinel group sits inside. The plan enumerates both lists per site with
the arithmetic each one forces. Step 4's Files block declares all three npm
files, and its Shared seams records that S31's Step 13 part 2 edits the same
virtual_read_errors.go for a different concern and the same
virtual_read_errors_test.go among its paired test files, with the merge order
the plan already gates: this step lands first, and S31's Step 13 part 2 rebases onto
it.
Other sites the two facts live at
Corrected in the same pass, because one fact repeated in several places is how a closed finding reopens:
- The precedence paragraph and the success table's last row both named
remote.ErrUpstreamUnavailablefor the fail-closed scan. - The
rapidproperty paragraph named it as the arm the nil-error condition exists to avoid contradicting. - Step 2's Files block said "five failure arms"; there are six, and it now lists
ErrUnsettledPositionOutcomealongside the two types the step creates. - Step 4's item list said "Five items land here"; with the record-validity guard and the npm taxonomy registration it reads "Eight items". The settle wait was uncounted under the old lead-in too, so the count is derived from the eight bullets the list carries rather than by adding two to the old figure.
- Step 2's body carried the same
S13:342citation as the old## Dependenciesentry. It now points at the Collection mode section and names the reversal row four makes. ## Open Questionssaid two S13 items bear on a step of this plan. With the S13 amendment booked it is three, and the entry says the amendment is owed rather than open, since no MR is filed for it.- The withholding rule reads as the pre-reversal binary in four more passages, each corrected here:
## Approach's verdict-as-a-pure-function section, Step 5's record-versus-verdict rationale, the Testing Strategy ledger, and Step 3's pointer at what was a five-row table and is now six. - Step 2's named case for a set carrying both withholding values. Rows three and four return the same
(CollectUnknown, remote.ErrUpstreamUnavailable)pair, but each arm wraps it in its own message (errEligibilityUndeterminedanderrUnservedPresence), so the case pins both that the set answers the withholding sentinel rather than the not-found and which of the two arms answers it. An earlier revision of this MR said the arms were indistinguishable on this set; that was wrong, and !2541 (merged)'sTestCollectVerdict_AnIneligibilityOutranksAnUnservedPresencealready asserts the arm by message. ## Naming ConventionsregisteredErrPositionAbsentbut notErrUnsettledPositionOutcome; it now carries both.
Review round 2
Eight findings on this branch, each corrected in its own commit:
- The Step 4 Files bullet ordered a
virtualReadAnswerForarm the function must not have. Membership invirtualServerErroris what names the record. - Step 2's test prose denied a distinction !2541 (merged) pins: the two withholding arms share the sentinel but not the message.
- The
virtual_read_errors.mdsite list named three of the eight counts the registration moves. - The npm-taxonomy overlap claimed a free merge order where the plan records a hard gate on S31's Step 13 part 2.
- The record-validity guard demanded a test with no observable, the mapping having neither an error return nor a panic path. It now returns an error.
- Step 5's closing absolute about the record being the only surface contradicted the carve-out above it.
## Open Questionssaid the second S13 amendment "is open" where no MR is filed for it.- The
virtual_read_errors_test.gosite list left out that file's four count comments, two of which also stop being able to nameresolve.goas the sentinels' only home.
Related to #1245 (closed)