Conversation
github-actions
Bot
added
the
bindery-notified
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
gchahcg
added a commit
to gchahcg/bindery
that referenced
this pull request
Sep 15, 2026
internal/models/filter_observation.go was entirely 0%-covered despite its functions being exercised indirectly through internal/metadata/filterengine's tests — go test's default per-package coverage mode doesn't attribute cross-package execution to the file where the code lives, so it needed its own direct test file regardless of the indirect exercise. Adds one. Closes the remaining flagged gaps directly: Registry.Decide (untested one-line wrapper), the nil-Book guard on LanguageSignal/ProviderNoiseSignal, AnyEditionHasISBN's ISBN10-only branch (only ISBN13 was ever exercised before), and strongestObservation's strictly-larger-magnitude branch plus recordExcluded's panic-on-unmapped-signal path (the guard TestEverySignalHasACounter exists to make unreachable in practice, but the panic itself was never actually triggered by a test). Left one flagged gap alone: internal/api/authors.go's UNIQUE/FOREIGN KEY constraint race-recovery branches (~line 2547-2573) predate this PR (confirmed via git log against main) — Codecov's diff view only flags them because this PR's counter refactor touched the surrounding lines, not because they're new logic. Simulating that exact race deterministically would need either a real concurrent goroutine racing the insert (flaky) or an injectable repo interface neither authors.go nor its tests have today — not worth adding for a genuinely pre-existing, untested-before-this-PR path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
10 tasks
gchahcg
marked this pull request as ready for review
September 15, 2026 11:44
gchahcg
pushed a commit
to gchahcg/bindery
that referenced
this pull request
Sep 16, 2026
Add unit tests for the uncovered branches Codecov flagged on vavallee#2626: - openlibrary.GetAuthorWorksSnapshot: primary-cover ImageURL, empty-title skip, and search-enrichment ratings merge (client.go:417/455/470). - api.fetchAuthorBooks: calibre relink failure (1830), works-fetch error (1861), MinPages edition-lookup failure (2253), and series-mode monitored-series load + monitor-on-discovery (2055-2072, 2356-2362). - api.fetchAuthorBooksAsync: nil-author guard (699). The Audible supplement branch (1888-1891) is left untested: it issues a live call to api.audible.com and the aggregator's audible client cannot be injected from the api test package. Coverage: GetAuthorWorksSnapshot 93.1% -> 100%, fetchAuthorBooks 77.0% -> 85.9%, fetchAuthorBooksAsync 88.9% -> 100%.
gchahcg
pushed a commit
to gchahcg/bindery
that referenced
this pull request
Sep 16, 2026
Add unit tests for the uncovered branches Codecov flagged on vavallee#2626: - openlibrary.GetAuthorWorksSnapshot: primary-cover ImageURL, empty-title skip, and search-enrichment ratings merge (client.go:417/455/470). - api.fetchAuthorBooks: calibre relink failure (1830), works-fetch error (1861), MinPages edition-lookup failure (2253), and series-mode monitored-series load + monitor-on-discovery (2055-2072, 2356-2362). - api.fetchAuthorBooksAsync: nil-author guard (699). The Audible supplement branch (1888-1891) is left untested: it issues a live call to api.audible.com and the aggregator's audible client cannot be injected from the api test package. Coverage: GetAuthorWorksSnapshot 93.1% -> 100%, fetchAuthorBooks 77.0% -> 85.9%, fetchAuthorBooksAsync 88.9% -> 100%.
…ee#2235) New package internal/metadata/filterengine: Signal/Observe/Candidate/Context/ Result types, BandFor (KEEP/REVIEW/EXCLUDE banding, half-open on the exclude side so keep==exclude deterministically collapses REVIEW to empty), Registry with duplicate-ID detection, and the 7 v1 signals porting authors.go's existing boolean filters (media type, junk title, language, part-book, missing date, missing ISBN, min pages) — each wrapping its existing predicate rather than reimplementing it. isPartBookTitle/anyEditionHasISBN/passesMinPagesFilter's actual logic moves into filterengine as exported functions (IsPartBookTitle/AnyEditionHasISBN/ PassesMinPagesFilter); authors.go's identically-named functions become thin wrappers in a later commit so catalogue_reconciliation.go's existing call sites are unaffected. Also adds models.FilterObservation (Signal/Weight/Confidence/Reason, Contribution() and Direction() derived rather than stored) and Book.Observations, both transient like Book.ProviderISBNs. TestRegistryIsVetoOnlyAtV1 pins the parity invariant that lets a future migration's keep_threshold=exclude_threshold=0 default reproduce today's boolean-chain behavior exactly: every DefaultSignals() entry must stay veto-weight until that default is deliberately revisited. Cluster/ClusterKey/BuildClusters are also included as explicit groundwork for the dataset's highest-value finding (cluster-level max(edition_count) discriminates real catalogue from noise; per-record does not — see cluster.go for the measurement) — no v1 signal consumes it yet; it's the prerequisite for the first graded signal, not speculative infrastructure. 97% coverage, 0 lint issues on the new package. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
…avallee#2235) Migration 086 adds both columns, REAL NOT NULL DEFAULT 0 — the value that reproduces internal/metadata/filterengine's pre-vavallee#2235 boolean-chain behavior exactly for every existing profile (see filterengine's package doc and TestRegistryIsVetoOnlyAtV1). internal/api/metadata_profiles.go rejects exclude_threshold > keep_threshold outright (an inverted band is meaningless), and at v1 rejects exclude != keep entirely: no v1 signal is graded enough to populate a REVIEW band meaningfully, and there's no UI surface for one. Relaxing to `exclude <= keep` is the one-line change that enables graded filtering later. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
…llee#2235) doc.EditionCount was decoded from the /search.json response in both searchAuthorWorks and GetAuthorWorksSnapshot's merge step, but never actually assigned to the resulting models.Book: - searchAuthorWorks's own Book literal never referenced doc.EditionCount. - GetAuthorWorksSnapshot's primary+enrichment merge selectively copies specific fields from the enrichment (search) result onto the primary (works-endpoint) result when a work appears in both — the common case for an established author — and EditionCount wasn't one of the copied fields, so fixing searchAuthorWorks alone wasn't enough for works present in both sources. Two behavior changes as a result, both expected and correct, not accidental: - internal/metadata/aggregator_canonical.go's four EditionCount-based canonical-search ranking branches, previously always-true dead code since EditionCount was always 0, now actually discriminate. - internal/metadata/filterengine's Cluster.MaxEditionCount (the prerequisite for vavallee#2235's highest-value future signal, per that package's cluster.go doc) now has real data to aggregate instead of always reading 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
vavallee#2235) Replaces the inline boolean checks (strict media type, junk title, language, part-book, missing date, missing ISBN, min pages) in AuthorHandler.fetchAuthorBooks with filterengine.Decide calls against the corresponding ported signal, one per existing call site — preserving every existing gating condition (singleWork exemptions, existing==nil gating on the edition-dependent filters, the editionsByForeignID comma-ok check) and every existing Skipped* counter/sample exactly as before. isPartBookTitle/anyEditionHasISBN/passesMinPagesFilter now delegate to their relocated filterengine equivalents rather than duplicating the logic, so catalogue_reconciliation.go's pre-existing call sites automatically share the same implementation the new signals wrap. TestAuthorSyncSummaryReconciles (a pre-existing test exercising every filter against a catalogue built to trip each one, asserting exact expected counts) passed unmodified against this change — strong evidence the wiring didn't shift behavior. Added two more parity tests: - TestAuthorSyncParity_ShippedDefaultReproducesBooleanChain: an independent smaller-fixture confirmation at the shipped 0/0 default. - TestAuthorSyncParity_NonZeroEqualThresholdsDiverge: documents a REAL bug caught while building this test suite. keep_threshold == exclude_threshold alone does not reproduce the boolean chain — only exactly 0/0 does, because every v1 signal is a veto with Context.Prior hardcoded to 0, so a clean candidate always scores exactly 0. A nonzero equal pair (e.g. both 50) excludes every candidate, filtered or not, since 0 < 50. validateScoreThresholds is tightened accordingly: it now rejects any nonzero value, not merely an unequal pair — the original (equal-only) version of that check would have accepted the broken 50/50 profile this test constructs directly through the repo (bypassing the now-corrected API validation) to demonstrate the failure mode the tightened check exists to prevent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
…them (vavallee#2235) shouldFilterOLNoise's two call sites (GetAuthorWorksSnapshot's primary loop and its enrichment-only tail) used to drop a matched work outright, before it ever reached AuthorHandler.fetchAuthorBooks — and therefore before it ever entered AuthorSyncSummary.Total's accounting at all. That's exactly the failure mode vavallee#2235 exists to fix: a real drop with no visibility, no counter, no way to know it happened short of reading provider logs. olNoiseMatchReason (new) returns the matched pattern instead of a bare bool; both call sites now append a models.FilterObservation (Signal: models.SignalProviderOpenLibraryNoise) to the surviving book instead of skipping it, checked against the primary entry's untruncated Subjects list where available (the enrichment-tail call site was already checking the 10-item-truncated Genres before this change, so no fidelity change there). internal/metadata/filterengine gets a new ProviderNoiseSignal that replays that observation at the registry's configured (veto) weight — the seam that lets a provider flag something without needing profile/threshold context of its own. Wired into authors.go's junk-title check site (both route to the same SkippedJunk counter; there was never a separate counter for the provider-side check). Behavior change, expected and documented, not a bug: an OpenLibrary-primary author's AuthorSyncSummary.Total can now be visibly higher than before this PR, for exactly the works that used to be silently eaten pre-Total. Existing TestGetAuthorWorks_HTTP_NoiseFilter{,SearchEnrichment} rewritten to assert the new flag-not-drop behavior (all works present, noise-matched ones carrying the observation) rather than the old drop-and-count behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
…d truth (vavallee#2235) testdata/works_sample.csv: an 80-row stratified sample of gchahcg/fiction-author-dataset's data/works.csv (commit 1b1751b, CC-BY-4.0), attributed in testdata/README.md, which also documents what this dataset can and can't validate here — it carries title/work_kind ground truth but no language/subjects/edition_count/ISBN/page-count/release-date, so it can only exercise JunkTitleSignal and PartBookSignal, not the other four. TestWorksSample_CoreTitlesNeverFalsePositive pins 0 false positives from either title-only signal against real books — measured against the FULL 1,422-row dataset this holds at 804/804 (100%). TestWorksSample_PartBookSignalOnlyCatchesTitleShapedBundles documents (doesn't demand 100% on) PartBookSignal's real recall on compilation/posthumous-compilation rows — 53/284 (18.7%) on the full dataset — as the concrete evidence for why the cluster-level edition-count signal cluster.go's doc describes is worth building next, not a regression to fix now. TestLanguageSignal_EmptyLanguageIsNotForeign is the named case from vavallee#2235 itself ("Summer Stars: Second Nature / One Summer", dropped for language:""), as a synthetic fixture since it isn't in the ground-truth dataset (it came from a live sync run, not curated data). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
- changelog.d/2235-scored-filter-engine.md: ### Added for the new engine, ### Changed for the AuthorSyncSummary.Total semantics shift (step 8). - docs/ARCHITECTURE.md: one row for internal/metadata/filterengine in the internal-packages table. - docs/User-Guide-Wiki.md: "minimum popularity" was already stale before this PR — MinPopularity has been unenforced since an earlier refactor (web/src/pages/settings/MetadataTab.tsx says so explicitly) — replaced with the profile fields that are actually enforced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
… stages (vavallee#2235) Moving OpenLibrary's companion-material check from "drop inside the provider client" to "flag and let filterengine decide" is what makes those works countable in AuthorSyncSummary.Total. It also, unintentionally, put them into every stage that runs over the RAW provider slice BEFORE the create loop — stages that pre-vavallee#2235 never saw them, because the client had already removed them. Three sites, all restoring the pre-vavallee#2235 result exactly: - isAuthorWorkMonitorCandidate: latestBookMonitorKeys awards the author's MonitorLatestCount slots from the raw slice, by release date, before any filtering. Companion material is usually published well after the work it accompanies, so a flagged study guide won the auction, was then excluded by ProviderNoiseSignal in the loop, and the real book it displaced was created UNMONITORED — no auto-search, no grab, and no counter saying why. This was reproduced end to end, not theorised; the regression test is the reproducer. - applyAuthorMajorityLanguageFallback: flagged works no longer vote in the author's majority-language inference. A run of English-language companion material could otherwise carry both the minimum-sample gate and the dominance ratio for an author whose real catalogue is another language, changing which real works get a language assigned and therefore which ones the language filter drops. - reconciliationRejectReason: the destructive catalogue-reconciliation path reads the same snapshot. With the client no longer dropping these works, reconciliation had silently started treating a study guide as evidence that the work it is a guide TO is still in the upstream catalogue. Routed to the catalogue_filter bucket, matching how fetchAuthorBooks attributes the same signal. models.FindObservation/HasObservation is the one lookup all four consumers (these three plus ProviderNoiseSignal, which needs the matched Reason) share, so the provider-claim probe exists once rather than four times. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
…allee#2235) Five small, safe fixes, no scope expansion: 1. Consolidate the "title is empty or matches the author's own name" check, which JunkTitleSignal reimplemented inline while isAuthorWorkMonitorCandidate and reconciliationRejectReason each had their own independent copy — three copies of the same logic despite this PR's own wrap-don't-reimplement rule. New filterengine.IsJunkTitle is the one shared implementation; all three call sites now use it. 2. TestRegistryIsVetoOnlyAtV1 only checked each signal's Weight() accessor, not the Weight/Confidence on the observation it actually emits — a signal could report a veto-looking Weight() while Observe emits something else entirely, decoupled from the accessor. Added vetoFixtures(), a per-signal Candidate/Context guaranteed to trigger it, and the test now runs every DefaultSignals() member through fires() (the same assertion the per-signal tests already use) — a signal with no fixture entry fails explicitly rather than silently skipping the check. 3. Direct assertion on SkippedMediaType in TestFetchAuthorBooks_StrictMediaType — it previously only checked which books got created, not that the drop was specifically counted as a media-type skip. 4. Renamed TestAuthorSyncParity_ShippedDefaultReproducesBooleanChain to TestAuthorSyncParity_DefaultVetoWeightsMatchExpectedCounts and corrected its doc comment: it's a 3-work hardcoded-expectation fixture, not the golden comparison against the pre-vavallee#2235 chain (that's TestAuthorSyncSummaryReconciles, which passed unmodified). 5. Two stale "exclude != keep" error messages in metadata_profiles_test.go updated to state the actual tightened rule (both must be exactly 0, not merely equal to each other). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
…all-site (vavallee#2235) fetchAuthorBooks called filterengine.Decide separately at 7 call sites across 8 signals, each banding and `continue`-ing immediately on EXCLUDE. A candidate tripping more than one filter was therefore only ever attributed to whichever call site the loop reached first — the package doc's claim that "a record tripping more than one filter now carries every reason it failed" was true of a single Decide call's signal list, never true across the free/junk/language/structural checks as a whole. Worse, every call site hand-built its own filterengine.New*Signal() list, so TestRegistryIsVetoOnlyAtV1 (which checks filterengine.DefaultSignals()) was guarding a construction production never actually called. Fixes both: - internal/api/authors_filterengine.go's filterEngineSignals() sources the free/structure/edition signal lists fetchAuthorBooks evaluates directly from filterengine.DefaultSignals(), split by ID into three buckets (TestFilterEngineSignals_CoversEveryDefaultSignalExactlyOnce pins the split is exhaustive). A signal the registry ships is now a signal production evaluates, full stop — proven concretely, not asserted in prose: temporarily injecting a fake graded signal into DefaultSignals() and re-running TestRegistryIsVetoOnlyAtV1, TestEverySignalHasACounter, and TestFilterEngineSignals_CoversEveryDefaultSignalExactlyOnce showed all three catch it (captured in this session, reverted before commit). - authors.go's two evaluation passes (free signals over every candidate; structural + edition-gated signals over candidates with no existing library row) now call filterengine.Decide exactly once per candidate per pass, accumulating every applicable signal's observations into one Result before checking Band — never banding after a single signal fires. The structural-pass call recombines the free signals too (cheap, pure, already proven non-excluding for anything reaching that loop), so the Result handed to banding is a literal one-candidate, all-applicable- signals ledger, not an assumption that an earlier pass contributed nothing. - authorSyncCounters + signalCounter (authors_filterengine.go) replace the dozen separate Skipped* local variables with one struct and one ID-keyed attribution map, so an EXCLUDE-banded candidate's Skipped* counter and sample come from strongestObservation(result.Observations) — the ledger's largest-magnitude observation, ties broken by registry order (matching the pre-vavallee#2235 boolean chain's check order exactly) — instead of whichever Decide call used to fire and continue first. TestEverySignalHasACounter pins every DefaultSignals() entry has a signalCounter entry. - TestEvaluateCandidate_AccumulatesEveryFiringSignal is the white-box proof multiple observations really do accumulate: a candidate tripping both JunkTitleSignal and LanguageSignal in one Decide call is asserted to carry both observations, not just the first — a black-box counter check alone can't distinguish this from the old first-checked-wins behavior, since equal-magnitude vetoes with registry-order tie-break produce the same counter either way. TestAuthorSyncParity_MultiSignalCandidateCountsOnceAttributedToStrongest is the same proof at the full sync-loop level: two multi-trip fixtures (junk+language in the free pass, part-book+missing-date in the structural pass) each land under exactly one counter, and AuthorSyncSummary.Unaccounted() stays zero. Also: - models.AuthorSyncSkippedBook gains Reason (json:"reason,omitempty", purely additive), populated from the same strongestObservation so a skip sample names the specific evidence, not just the title/language. - internal/metadata/filterengine/cluster.go's ClusterKey now calls the newly-exported metadata.StripLeadingArticle instead of maintaining an independent copy of the same regex — filterengine/predicates.go already imports internal/metadata (metadata.IsBundleTitle), so the import- direction argument for a second copy no longer held. - models.Book.EditionCount gets the godoc it was missing: populated by openlibrary's client, read by aggregator_canonical.go's ranking and filterengine/cluster.go's MaxEditionCount (unused by any v1 signal). - docs/third-party-data.md gets the CC-BY-4.0 attribution entry for testdata/works_sample.csv (fiction-author-dataset), alongside the existing per-file notice in testdata/README.md. Verification: go build ./..., go vet ./..., gofmt -l . (clean after formatting book.go), golangci-lint run ./... (0 issues), govulncheck ./... (0 vulnerabilities affecting this code), go test ./cmd/... ./internal/... (every package green, internal/api 142s). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
…itle matching vavallee#2235 changed OpenLibrary's client from dropping companion material (study guides, film tie-ins, etc.) to flagging it with models.SignalProviderOpenLibraryNoise, so fetchAuthorBooks's ProviderNoiseSignal can count and report it instead of it vanishing invisibly. internal/abs/import_upserts.go's lookupUpstreamBook never routes through filterengine, though — it title-matches raw GetAuthorWorks results directly — so a flagged companion work could now win an ABS import's title match, or turn a previously-unambiguous match ambiguous, a regression an independent review pass caught before this branch was opened as a PR. Also fixes a related, currently-unreachable BuildClusters bug the same review pass found: a cluster's first-seen member having an empty ForeignID let a later, lower-EditionCount member silently re-seed CanonicalID outside the normal comparison (MaxEditionCount itself was never affected). Not reachable via OpenLibrary today, but BuildClusters is exported and provider-agnostic, and is exactly the groundwork the in-flight Phase 2 cluster-signal work builds on. And updates GetAuthorWorks's doc comment, which still claimed noise was filtered at that layer after the flag-not-drop change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
internal/models/filter_observation.go was entirely 0%-covered despite its functions being exercised indirectly through internal/metadata/filterengine's tests — go test's default per-package coverage mode doesn't attribute cross-package execution to the file where the code lives, so it needed its own direct test file regardless of the indirect exercise. Adds one. Closes the remaining flagged gaps directly: Registry.Decide (untested one-line wrapper), the nil-Book guard on LanguageSignal/ProviderNoiseSignal, AnyEditionHasISBN's ISBN10-only branch (only ISBN13 was ever exercised before), and strongestObservation's strictly-larger-magnitude branch plus recordExcluded's panic-on-unmapped-signal path (the guard TestEverySignalHasACounter exists to make unreachable in practice, but the panic itself was never actually triggered by a test). Left one flagged gap alone: internal/api/authors.go's UNIQUE/FOREIGN KEY constraint race-recovery branches (~line 2547-2573) predate this PR (confirmed via git log against main) — Codecov's diff view only flags them because this PR's counter refactor touched the surrounding lines, not because they're new logic. Simulating that exact race deterministically would need either a real concurrent goroutine racing the insert (flaky) or an injectable repo interface neither authors.go nor its tests have today — not worth adding for a genuinely pre-existing, untested-before-this-PR path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky Signed-off-by: gchahcg <7381837@proton.me>
Add unit tests for the uncovered branches Codecov flagged on vavallee#2626: - openlibrary.GetAuthorWorksSnapshot: primary-cover ImageURL, empty-title skip, and search-enrichment ratings merge (client.go:417/455/470). - api.fetchAuthorBooks: calibre relink failure (1830), works-fetch error (1861), MinPages edition-lookup failure (2253), and series-mode monitored-series load + monitor-on-discovery (2055-2072, 2356-2362). - api.fetchAuthorBooksAsync: nil-author guard (699). The Audible supplement branch (1888-1891) is left untested: it issues a live call to api.audible.com and the aggregator's audible client cannot be injected from the api test package. Coverage: GetAuthorWorksSnapshot 93.1% -> 100%, fetchAuthorBooks 77.0% -> 85.9%, fetchAuthorBooksAsync 88.9% -> 100%.
gchahcg
force-pushed
the
feat/2235-scored-filter-engine
branch
from
September 16, 2026 11:42
a115e97 to
4a13b05
Compare
6 tasks
Owner
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
2 participants
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.
Summary
Replaces the author catalogue sync's sequential boolean noise-filtering (strict media type, junk title, provider-flagged noise, language, part-book, missing date, missing ISBN, min pages) with
internal/metadata/filterengine: signals emit weighted observations, summed into a score and banded into KEEP/REVIEW/EXCLUDE. At v1 every signal ships at veto weight (-1000) andmetadata_profiles.keep_threshold/exclude_thresholddefault to 0/0 for every profile — the exact value that reproduces the prior boolean-chain's keep/exclude decision, proven with a golden parity test (TestRegistryIsVetoOnlyAtV1,TestAuthorSyncParity_EqualThresholdsReproduceBooleanChain). This is a pure refactor: no existing profile's sync behavior changes.This is the foundation for #2235's actual ask (additive scored signals instead of first-drop-wins boolean filtering). The first real graded signal (a cluster-level edition-count corroboration signal, validated offline against a 1,422-row hand-verified ground-truth dataset) is a focused follow-up PR on top of this one, opened separately so the risk-free refactor and the real behavior change get reviewed independently.
Closes #2235.
Implementation notes
internal/metadata/filterengineis a new package:Signal/Candidate/Context/Decide/BandForcore, plus one file per v1 signal (media type, junk title, provider noise, language, part book, missing date, missing ISBN, min pages). Registry order matches the pre-Additive scored signals instead of first-drop-wins boolean filtering #2235 boolean chain's check order exactly, sostrongestObservation's magnitude tie-break reproduces first-drop-wins by construction, not by accident.internal/api/authors.go'sfetchAuthorBooksnow does exactly twoDecide()calls per candidate (free-signals pass, then free+structural+edition combined pass) instead of seven fragmented per-call-site checks — the actual Additive scored signals instead of first-drop-wins boolean filtering #2235 gap: a candidate tripping more than one filter used to only ever record the first one a given call site happened to check.filterengine.BuildClusters/Clustership in this PR (no v1 signal reads aClusteryet) as the explicit prerequisite the first graded signal needs — seecluster.go's doc.models.SignalProviderOpenLibraryNoiseinstead of silently dropping it before the sync ever saw it. An independent review pass caught thatinternal/abs/import_upserts.go's upstream title-match lookup doesn't route throughfilterengineand would otherwise start matching against flagged noise — fixed by skipping flagged works there directly (see thefix(abs)commit).migration 086addskeep_threshold/exclude_thresholdtometadata_profiles;validateScoreThresholdscurrently rejects any non-zero value — relaxing that is exactly what the follow-up PR does, once a real graded signal exists to justify it.Why minimal / scope decisions
keep_threshold/exclude_thresholdare DB/API-only in this PR, not surfaced in Settings. The follow-up PR adds a preset-based UI control (never raw threshold numbers).AuthorSyncSummary.Totalis measurably higher for some OpenLibrary-primary authors now that companion material survives as a flagged, counted candidate instead of vanishing before accounting — see the changelog fragment.Follow-ups (not in this PR)
catalogue_reconciliation.goand a couple ofauthors.gohelpers hand-check specificfilterenginesignal IDs instead of routing throughDecide— harmless while every signal is veto-only, worth revisiting once graded signals are real.Checklist
changelog.d/2235-scored-filter-engine.md,docs/ARCHITECTURE.md,docs/User-Guide-Wiki.md)Test plan
go test ./cmd/... ./internal/...go vet ./...golangci-lint run --timeout=5m(0 issues)govulncheck ./...(0 reachable vulnerabilities)internal/abs's title matching, unrelated tofilterengine) and a currently-unreachableBuildClustersedge case.🤖 Generated with Claude Code
https://claude.ai/code/session_01J3A3uW6ZH6m56q1SLVg7Ky