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 PositionDataError among the contributor-side failures an absence stands beside. Row four withholds the not-found for it, and TestCollectVerdict_AnAbsenceStandsBesideAContributorSideFailure excludes it by name. Its own pair is described instead, with the set carrying both withholding values.
  • The eighth case and the all-PositionUnknown case both said the set answers unavailable. Row two answers ErrUnsettledPositionOutcome, 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 remote package'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.ErrUpstreamUnavailable for the fail-closed scan.
  • The rapid property 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 ErrUnsettledPositionOutcome alongside 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:342 citation as the old ## Dependencies entry. It now points at the Collection mode section and names the reversal row four makes.
  • ## Open Questions said 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 (errEligibilityUndetermined and errUnservedPresence), 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)'s TestCollectVerdict_AnIneligibilityOutranksAnUnservedPresence already asserts the arm by message.
  • ## Naming Conventions registered ErrPositionAbsent but not ErrUnsettledPositionOutcome; 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 virtualReadAnswerFor arm the function must not have. Membership in virtualServerError is 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.md site 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 Questions said the second S13 amendment "is open" where no MR is filed for it.
  • The virtual_read_errors_test.go site list left out that file's four count comments, two of which also stop being able to name resolve.go as the sentinels' only home.

Related to #1245 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading