Don't let unrecognized BISAC codes classify fiction as nonfiction (PP-4849)

[dbernstein]

Description

Two changes, which must ship together.

1. An unrecognized BISAC code no longer votes nonfiction.

BISACClassifier.is_fiction() and .audience() now skip the BISAC rulesets when the subject identifier did not resolve to a canonical BISAC heading, deferring instead to the KeywordBasedClassifier call already sitting at the end of both methods. That call was unreachable until now, because the catch-all rules always matched first.

2. A recalculation can no longer erase a known fiction status.

Work.assign_genres() gains the guard its audience handling already has: when the classifier reaches no fiction determination, the work keeps the status it had rather than having NULL written over it.

3. A migration repairs the stored side.

Resets checked=false for every subject stored as nonfiction that BISACClassifier no longer scores that way, so classify_unchecked_subjects re-scores them and recalculates the works they are attached to. The migration asks the classifier directly rather than pattern-matching the identifier, so the migration's notion of "not a real BISAC code" cannot drift from the runtime's.

Motivation and Context

Everything on the Palace Marketplace / Feedbooks category scheme is stored with type='BISAC', including codes that are not BISAC at all. In production (illinois), 3,172 of 7,726 BISAC subject rows (41%) are not BISAC codes — mostly Feedbooks' literature-by-language taxonomy:

identifier	name	titles
INFEN000	English literature	3,749
INFENUSA	American and Canadian literature	3,105
INFENGBR	British literature	593
FSHUM000000N	Human science	565
FBSACT000000	News and investigations	258

Such a code cannot be resolved, so classification fell back to the distributor's name and hit the catch-all closing BISACClassifier.FICTION — "not filed under a Fiction heading, therefore nonfiction". That inference is sound for a real BISAC name and unsound for anything else, so each of these cast a nonfiction vote on every work it touched, outvoting the genuine FBFIC* fiction codes on the same book. The worst of it is that these are literature categories: the codes most likely to appear on a novel were the ones voting against it.

The AUDIENCE ruleset has the same catch-all, inferring Adult. That is exactly the bug fixed for FBJUV* codes in PP-4128 — fixed there by making those particular codes resolve, which left the underlying behaviour intact.

Because the keyword classifier recognises "literature", the INF* family does not merely abstain — it flips to voting fiction:

identifier	name	before	after
INFEN000	English literature	False	True
INFENUSA	American and Canadian literature	False	True
INFENGBR	British literature	False	True
FSHUM000000N	Human science	False	None (genre Social Sciences retained)
FBSACT000000	News and investigations	False	None
SOCO32000	(no name)	False	None
FBFIC014000	Historical	True	unchanged

The trade-off is visible: FBSACT000000 loses a nonfiction vote it was arguably entitled to. But the code cannot be resolved, so abstaining is the honest answer — and it is 258 titles against 7,447.

Fix 2 matters more once Fix 1 lands, since abstaining becomes common and a work whose only BISAC evidence is unresolvable codes would otherwise have its existing status nulled.

Migration scope and blast radius

Measured against production: 2,418 subject rows, 4,877 works recalculated — roughly 2% of the nightly volume behind the reindex surge in PP-4472, so no scheduled window is needed.

Treat that as a floor. It was measured with a pattern-based predicate; the classifier-based one now in the migration additionally catches shape-valid-but-non-existent codes and real FIC* codes stored as nonfiction. It can only add rows, and every row it adds is one holding a value the classifier disagrees with. The migration logs each subject it resets plus a total.

Scope is deliberately narrow:

• Canonical BISAC codes are untouched. 3,803 of them legitimately hold fiction=false (real HIS*/BUS* codes), and there is no evidence the FBFIC* rows are stale.
• Non-canonical codes already at true (693) or NULL (61) are untouched. They are not implicated, and re-scoring them through a different classifier risks regressions while roughly doubling the reindex.

Subjects are global and are only re-examined when checked=false, so a subject scored under the old rules keeps its value indefinitely. The FBJUV* resets in 45f74fdcec18 and 05a95c828149 did not cover these codes.

Important

The migration must ship in the same release as the classifier fix. Run on its own, the nightly task would re-score these subjects with the old rules and re-stamp checked=true, paying for a full reindex that changes nothing.

Known remainder

Of 1,871 works that carry an FBFIC* code but are not marked fiction, this reset reaches 1,356. The other 515 carry no non-canonical nonfiction voter, so their nonfiction votes come from real BISAC codes or they have no votes at all — some of those are likely correct (a book carrying both FIC* and HIS* codes). Widening the predicate to force them would mean resetting correct subject rows for an unknown benefit. Better to re-measure after this lands.

Out of scope

• genre and target_age have the same structural issue, but changing genre assignment moves books between lanes and deserves its own change.
• More fundamentally, http://www.feedbooks.com/categories maps wholesale to BISAC in Subject.by_uri, which is how non-subject codes reach the BISAC classifier in the first place. Typing them as tag on import would address the root cause, and is a larger change.

How Has This Been Tested?

New tests:

• TestBISACClassifier.test_heading_in_identifier_field_is_matched_as_a_name — parametrized over four heading shapes. Some distributors put the BISAC heading in the identifier field (Boundless sends "FICTION / Horror", not "FIC015000"); that is a name, not a code that failed to resolve, and must still be matched against the rulesets.

• TestBISACClassifier.test_unrecognized_code_abstains — parametrized over no name, language name, territory name, FB-prefixed, and a partial BISAC heading.

• TestBISACClassifier.test_unrecognized_code_still_uses_keyword_fallback — abstaining is not the same as ignoring the name; a name that does carry a signal is still honoured.

• TestBISACClassifier.test_recognized_code_unaffected_by_abstention — regression anchor on FBFIC000000/014000/016000/019000, whose partial names ("Historical", "Literary") would each vote nonfiction if the canonical lookup ever missed.

• TestWorkClassifier.test_unrecognized_bisac_codes_do_not_imply_nonfiction — two junk codes produce no vote and no determination; one resolvable code is then decisive.

• TestWork.test_assign_genres_does_not_overwrite_fiction_with_null.

• tests/migration/test_20260902_52d1bbdd4671_... — 12 cases. Five confirm the reset fires (each offender shape from production, including one with a NULL name); seven confirm it does not — canonical codes in all three prefix/suffix forms, a real nonfiction code, non-canonical rows already at true or NULL, and a tag-typed subject with the same identifier.

Full suite locally against Postgres + Valkey: 6,203 passed, 0 failed. 104 errors, all infrastructure gaps from not running two of the tox service containers — OpenSearch (test_search.py, test_delete_work_not_in_search_end2end) and MinIO/S3 (test_marc.py, which errors with ValidationError: 3 validation errors for S3UploaderIntegrationConfiguration). Verified earlier by reverting the source changes and reproducing identical setup errors.

tests/migration: 31 passed, including the built-in up/down checks, so the revision chain is intact. mypy clean across 1,169 files; all pre-commit hooks pass.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

The PR makes unrecognized BISAC identifiers defer to keyword classification, preserves an existing fiction determination when recalculation abstains, and adds a migration to rescore affected subjects.

• Skips BISAC catch-all fiction and audience rules for identifiers absent from the canonical heading map.
• Prevents an indeterminate recalculation from clearing Work.fiction.
• Resets affected subject rows for nightly reclassification, although its shape-based predicate misses well-formed identifiers unknown to the runtime classifier.

Confidence Score: 4/5

The migration should be corrected before merging because it can leave stale nonfiction votes on shape-valid identifiers that the runtime classifier considers unrecognized.

Runtime recognition uses canonical heading-map membership, but the migration uses only identifier syntax, so demonstrated inputs such as FBZZZ000000 are excluded from the required reclassification.

Files Needing Attention: alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py, tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py

Important Files Changed

Filename	Overview
src/palace/manager/core/classifier/bisac.py	Correctly separates runtime recognition from distributor-provided names and sends unknown codes through keyword fallback.
src/palace/manager/sqlalchemy/model/work.py	Adds intentional retention of an existing fiction determination when recalculation produces no evidence.
alembic/versions/20260902_52d1bbdd4671_reset_checked_for_non_bisac_nonfiction_.py	Resets malformed identifiers but misses syntactically valid codes absent from the runtime canonical heading map.
tests/manager/core/classifiers/test_bisac.py	Covers recognized and unrecognized classifier behavior, including a shape-valid unknown code that exposes the migration mismatch.
tests/migration/test_20260902_52d1bbdd4671_reset_checked_for_non_bisac_.py	Covers malformed and canonical identifiers but lacks a shape-valid identifier absent from BISACClassifier.NAMES.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Stored BISAC subject] --> B{Identifier matches migration regex?}
    B -->|No| C[Set checked=false]
    B -->|Yes| D[Remain checked]
    C --> E[Nightly reclassification]
    E --> F{Scrubbed code in NAMES?}
    F -->|Yes| G[BISAC rules]
    F -->|No| H[Keyword fallback]
    D --> I[Shape-valid unknown code keeps stale vote]

Loading

_{Reviews (1): Last reviewed commit: "Reclassify subjects holding a fabricated..." | Re-trigger Greptile}

[greptile-apps]

Migration recognition criteria diverge

If a stored subject has a shape-valid but unknown identifier such as FBZZZ000000, this regex treats it as canonical and leaves it checked, while the runtime classifier treats it as unrecognized. The nightly task therefore never rescores its stale fiction=false value, allowing the incorrect nonfiction vote to remain on attached works.

[dbernstein]

Good catch, and it was a real hole. FBZZZ000000 is shape-valid, so the regex left it checked, while the classifier calls it unrecognized — its fabricated fiction=false would have survived indefinitely.

Rather than tightening the pattern, I removed it. The migration now asks BISACClassifier directly and resets every subject stored as nonfiction that the classifier no longer scores that way, so the two definitions cannot drift apart by construction. Precedent for importing application code into a migration is already here (Identifier, Timestamp, BaseCoverageRecord), and a pure-logic classifier is a safer import than an ORM model.

Added shape_valid_but_nonexistent (FBZZZ000000) to the migration test as a regression guard for exactly this case, plus stale_canonical_fiction_code — a real FIC* code stored as nonfiction, which the old predicate also missed.

[greptile-apps]

Test lacks return annotation

The new test omits the required -> None return annotation, making its signature inconsistent with the repository's type-hinting requirement for new functions.

Suggested change

	def test_unrecognized_bisac_codes_do_not_imply_nonfiction(
	self, work_classifier_fixture: TestWorkClassifierFixture
	):
	def test_unrecognized_bisac_codes_do_not_imply_nonfiction(
	self, work_classifier_fixture: TestWorkClassifierFixture
	) -> None:

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[dbernstein]

Fixed. Added -> None here and to test_assign_genres_does_not_overwrite_fiction_with_null, which had the same omission.

Both were written to match the immediately-surrounding tests in those files, which are unannotated — but CLAUDE.md is explicit about new functions, so the convention wins over local consistency.

[greptile-apps]

Public functions lack docstrings

The new public upgrade, downgrade, and fetch_subject functions omit the required reStructuredText docstrings, leaving their function-level contracts undocumented.

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[dbernstein]

Half taken. I added the docstring to fetch_subject — that one belongs, since its sibling helpers on the fixture are documented.

Declining on upgrade/downgrade, though. Those are alembic framework entry points rather than public API, and the convention here is that the module docstring explains a migration: none of the ~50 revisions in alembic/versions/ documents either function. Adding them to this one would make it the odd one out, and the module docstring already covers what the migration does and why. Worth remembering for future migrations in this repo.

[greptile-apps]

That distinction makes sense, and I agree the module docstring is sufficient for Alembic’s upgrade()/downgrade() entry points while fetch_subject() follows the documented helper convention. I’ll apply that standard to future migrations.

View rule

[codecov]

Codecov Report

❌ Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.61%. Comparing base (a55b97e) to head (6c69e22).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/palace/manager/core/classifier/bisac.py	80.00%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3710   +/-   ##
=======================================
  Coverage   93.61%   93.61%           
=======================================
  Files         514      514           
  Lines       47065    47075   +10     
  Branches     6408     6412    +4     
=======================================
+ Hits        44060    44070   +10     
+ Misses       1944     1941    -3     
- Partials     1061     1064    +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbernstein]

Note on the red codecov/patch check

Patch coverage reads 81.8% — 1 missing line and 3 partials, all in bisac.py. Every one of them is pre-existing unreachable code that this diff re-indents rather than introduces; nothing in the change is actually untested. Project coverage is unchanged at 93.61%, and codecov/patch is not one of main's required checks.

The specific items:

• audience()'s if audience is cls.stop: return None — the AUDIENCE ruleset contains zero m(stop, ...) rules, so the branch cannot be taken. FICTION has three, which is why the equivalent line there is covered.
• The loop-exhaustion branches on for ruleset in cls.FICTION and cls.AUDIENCE — both rulesets end in a catch-all m(..., anything) that always matches, so neither loop can run to completion.

Both predate this PR. The diff only shifts their indentation by one level, which is enough for codecov to attribute them to the patch. target_age() and genre() carry the same dead stop check with no stop rules in their rulesets, and those lines show as uncovered too — they just fall outside this diff.

I have deliberately not papered over it. Adding # pragma: no cover would introduce a convention the repo does not use anywhere today (zero occurrences under src/), and deleting the dead stop handling is a cleanup worth doing across all four methods at once rather than in the single method this PR happens to touch. Happy to do either if preferred.

For reference, mergeStateStatus is BLOCKED on the required approving review plus the deploy-workflow checks (Docker build, Integration test, Migration test), not on codecov.

[jonathangreen]

@dbernstein you might want to reopen this on from as a PR from the org repo instead of your own fork, so we get full docker build and testing on it. That way the CI won't stay red on it.

[dbernstein]

oops - good catch.

[dbernstein]

Closing in favour of #3726, which is the same work branched in-repo rather than from a fork, so the required checks that fork PRs cannot run (Docker build, Integration test, Migration test, Unit tests) are able to report.

Same five commits, rebased onto current main. All review feedback from this PR is carried over and summarised in the new description — thanks @greptile-apps, both P1 and P2 findings are addressed there.

[greptile-apps]

Understood. Closing this PR in favor of #3726 is appropriate, and carrying the fixes and review feedback forward there should allow the previously unavailable required checks to run.