Retire every index built before the classifier changed - #414
Merged
Conversation
#335 added the `isCommitLoreKey` gate and changed no column, so `SCHEMA_VERSION` stayed at 2 and every index an earlier build had written was accepted as current. The only other rebuild trigger is `lastIndexedSha !== head`, which cannot see a classifier change, so the commits were never re-read: ordinary conventional-commit trailers kept being served as active records, under exactly the rule #335 was closed to enforce. Upgrading was silent, and `doctor` compares the cache against HEAD rather than against the classifier, so the one check a user would run reported the stale index `ok`. The mechanism that fixes this already existed. A schema-version mismatch deletes the file and rebuilds (ADR-0003); what was missing is that the version tracked the table's shape and not what its rows mean. The comment said "whenever the table shape changes", which is the reading that produced the bug, so it now says what the version is actually for. Bumping to 3 changes no column and retires every pre-#335 index, which is the only way those rows get re-read. The test reconstructs what the older build left behind, since no released binary is available to a unit test: the rejected trailer stored as a row, its commit path row beside it, the index marked current with HEAD and stamped v2. It asserts the record is not served -- and, separately, that the index was *rebuilt* rather than bypassed. That second assertion is the `doctor` half of the issue: an answer that quietly fell back to a git scan would leave the bad file on disk for `doctor` to keep describing. Limit: an index is derived state with no migration path, so a meaning change can only be handled by discarding the file Ruled-out: shipping a release note instead of a version bump | nothing surfaces the staleness for a user to act on -- `doctor` reported the stale cache as healthy, which is the whole reason the bug is silent Ruled-out: re-applying the classifier when rows are read | the gate belongs at the boundary where raw git output becomes a candidate record, and a second copy at read time is the two-implementations-one-hole shape this codebase already warns about Warn: bump SCHEMA_VERSION when a stored row stops meaning what it meant, not only when a column moves Blast: module Undo: easy Certainty: firm Verified: the test fails before the bump with the stale record served and passes after; it also asserts fromIndex is true and the reopened index reports the current version with zero trailers, so the file was rebuilt rather than bypassed; full suite 84 files and 2111 tests green, spec/verify.sh 26 fixtures OK Unverified: the issue reports this reproduces from v0.5.0 and not v0.5.1; that boundary is not re-checked here, because the fix retires every index below the current version regardless of which build wrote it Provenance: authored Record-Id: r-schema406
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (89)
Ruled out (205)
Warnings (48)
Truncated: 22 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
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.
Closes #406. #407 was closed as a duplicate and its reproduction is covered here.
What was wrong
#335 added the
isCommitLoreKeygate and changed no column, soSCHEMA_VERSIONstayed at2and every index written by an earlier build was accepted as current. The only other rebuild trigger islastIndexedSha !== head, which cannot see a classifier change — so the commits were never re-read.Ordinary conventional-commit trailers (
release:,fix:,sha256:) kept being served as active records, under exactly the rule #335 was closed to enforce. Per the plugin's own instructions aclaimis "a report to weigh", so an agent weighed commit noise as recorded decisions, indefinitely, until someone happened to runindex --rebuild.Upgrading was silent, and
doctorcompares the cache against HEAD and never against the classifier — so the one check a user would run saidok.The fix
The mechanism already existed: a schema-version mismatch deletes the file and rebuilds (ADR-0003). What was missing is that the version tracked the table's shape and not what its rows mean.
The comment said "Bumped whenever the table shape changes" — that reading is what produced the bug, so it now says what the version is actually for, with #406 named in it. Bumping to
3changes no column and retires every pre-#335 index, which is the only way those rows get re-read.The test
No released binary is available to a unit test, so it reconstructs what v0.5.0 left behind: the rejected trailer stored as a row, its
commit_pathsrow beside it, the index marked current with HEAD and stampedschema_version = 2.Getting this genuinely red took two passes worth recording. The first version omitted the
commit_pathsrow, so the path-scoped fetch found nothing and the test passed for the wrong reason — it would have gone green against the unfixed code. With the path row present it fails as the issue describes:The
doctorhalf, asserted rather than assumedA version bump makes
doctorcorrect only if the read actually rebuilds the index rather than bypassing it. An answer that quietly fell back to a git scan would leave the bad file on disk fordoctorto keep describing, and the issue's second finding would survive the fix. So that is asserted directly:All three hold.
doctor'sokis now true rather than uninformed.Verification
test/index-classifier-staleness.test.ts— 3 cases, red before the fix, including a control that the current classifier really does reject the fixture trailer.spec/verify.sh: 26 fixtures + README example sync + vocab table OK.test/dogfood.test.tsre-run after committing: 9 passed.npm run typecheck,npm run buildwithdist/committed.Note
The issue reports this reproduces from v0.5.0 and not v0.5.1. That boundary is not re-checked here, and does not need to be: the fix retires every index below the current version regardless of which build wrote it. Stated in the record as
Unverified:rather than left implied.