plan(signal-store): name the right function, and note the guard it appeased#229
Merged
Conversation
…peased The method that zero-fills the six LAFA scalars is _lafa_default_fields() at line 342, not _reranker_default_fields() at line 34. The latter is a different method, and it is the one that behaves correctly: it writes NaN for a missing KNN measurement and 0 only for a true absence, and says so. The right convention already lives twenty lines above the violation. The zero-fill docstring also states its purpose: to keep _assert_canonical_columns and the contracts producer-coverage guard from failing the dump. The guard meant to catch a missing producer was satisfied by a constant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #228, correcting two things I got wrong there. Both were caught by the subagent writing ADR-D45 and then verified against the code.
1. Wrong function name
#228attributed the zero-fill to_leaf_record_builder._reranker_default_fields(). That method exists (line 34) but does something else. The six LAFA scalars are filled by_lafa_default_fields()(line 342).2. The correction makes the argument stronger, not weaker
_reranker_default_fields()is precisely the method that gets it right:So the codebase already draws the distinction that D45 needs, twenty lines above the method that violates it. Fixing D45 means applying the file's own convention to the LAFA families, not importing a new one.
And
_lafa_default_fields()states its own purpose: the six are emitted unconditionally so that_assert_canonical_columnsand the contracts producer-coverage guard never fail the dump. The guard whose job is to catch a missing producer was appeased with a constant. That is the seam, in one sentence.Also noted
The D45 ADR was not missing, contrary to task #35 as I wrote it:
docs/source/adr/D45-jsonb-blob-feature-governance.rsthas existed since 2026-06-22, carrying a mis-stated value-skew framing. It is corrected in place by PROTEA #708, not duplicated.