Resolve suppress-work identifier lookup through equivalencies (PP-5215)

[dbernstein]

Description

SuppressWorkForLibraryScript (src/palace/manager/scripts/suppress.py) resolved an identifier to a work via Identifier.work, which only follows LicensePools whose own identifier_id matches exactly, and returns the first one it finds. load_works now answers a more precise question — which works does this library carry for this identifier? — in two steps:

1. Exact matches first. Every pool for this exact identifier in one of the library's collections, deduplicated by work. With consistent data that's a single work (see below); every match is nonetheless returned, so inconsistent data meets the ambiguity guard rather than being silently resolved to one of them.
2. Equivalency as a fallback. Failing an exact match, Work.from_identifiers with the strict default policy (equivalent_identifier_levels=1, equivalent_identifier_threshold=0.999) already used in edition.py, customlist.py and work.py, scoped to the library's collections.

Exact matches deliberately aren't merged with equivalency matches: two pools of the same identifier are certainly the same title, whereas an equivalency edge is only ever a vendor's assertion, so widening an exact hit could suppress an unrelated title.

Suppression itself is unchanged and still scoped to exactly the one library passed via -l/--library. Resolving through equivalency only changes which works are found, never which libraries they're suppressed for.

Output now names the work(s) involved, e.g. [SUPPRESSED] ISBN/9781433383670 -- The Great Gatsby (work id: 42), ordered by work id so it's stable between runs.

Why one identifier can legitimately mean more than one work

Different vendor identifiers sharing an ISBN. Standard ingestion links each vendor's own identifier to the book's ISBN at full strength — BibliographicData._handle_identifier_equivalencies (used by Overdrive, Boundless/Axis 360) plus Bibliotheca's importer directly. Each of those vendor identifiers has its own Work, so one ISBN can reach several of them through the equivalents graph. This is routine ingestion, not bad data.

Note IdentifierData.weight defaults to 1.0 unconditionally, so "strength 1" means "the vendor's feed said so", not a computed confidence. A shared ISBN can therefore also mean bad data — publisher ISBN reuse or misattribution, or an ebook/audiobook split (Palace's own merge logic excludes cross-medium matches) — where the candidate works really are unrelated.

The same identifier in two collections is not one of these cases: LicensePool.calculate_work() requires that "All LicensePools with a given Identifier must share a work" and points every pool in licensed_through at it, so a consortium's OverDrive collection and a library's own Advantage collection share one Work for a given OverDrive id, and suppressing it covers both. The exact-match path still returns every work it finds rather than the first, but purely as a guard against inconsistent data — the state calculate_work warns about as "more than one Work between them" and repairs.

Why the script holds off, and how to override it

Because the script can't tell a title legitimately reachable under several vendor identifiers from two unrelated books joined by a bad equivalency edge, an identifier resolving to more than one work reports a new AMBIGUOUS result and suppresses nothing, listing every candidate with its work id so a human can look before acting:

[AMBIGUOUS] ISBN/111 -- Moby Dick (work id: 42); Moby Dick (work id: 87)

A suppression script is exactly the kind of catalog-wide, hard-to-audit action where guessing wrong — silently pulling a book the library meant to keep — is worse than asking someone to confirm.

For the common legitimate case, where an operator already knows why a title resolves to more than one work and wants it gone from all of them, --suppress-ambiguous suppresses every candidate instead of refusing. Its output names only the works that actually changed, so a partially-already-suppressed run doesn't read as though everything was just suppressed.

Both resolution paths feed this guard. An earlier revision only guarded the equivalency path, so an exact match resolving to several works would have suppressed whichever was found first and printed [SUPPRESSED] while the others kept circulating.

Other behavior changes worth noting

• NOT_IN_LIBRARY — an identifier that resolves to a work the library doesn't carry is now reported separately from one that matches nothing at all. Scoping resolution to the library means an identifier that previously "worked" can now legitimately find nothing, and an operator scanning a batch should be able to tell a title in someone else's collection from a typo.
• The library's collection ids are resolved once per run rather than once per identifier (Library.associated_collections issues a fresh SELECT on each access).
• A pool whose Work hasn't been calculated yet can't be suppressed — suppression is a Work/Library relation, so there's nothing to attach it to. Such a pool therefore doesn't count as an exact match and resolution falls through to equivalency, rather than refusing and leaving a title the library demonstrably carries unsuppressed.
• Collections are scoped by association, not by whether they're currently active: suppressed_for is a durable flag and shouldn't depend on where today falls in a collection's subscription window.

Motivation and Context

A librarian ran the script with an ISBN they could see in the catalog and got NOT_FOUND, even though the title was circulating. That ISBN's identifiers row had no LicensePool of its own — the actual pool is keyed on a vendor identifier, with the ISBN linked only through an equivalents row. Identifier.work doesn't traverse equivalencies, so the script couldn't find a work that plainly exists and is licensed. This is the usual shape for identifiers library staff have on hand, so it affected ISBN-based suppression broadly, not just this one title.

How Has This Been Tested?

tests/manager/scripts/test_suppress.py — 64 tests passing, covering:

• Resolving through equivalency when the identifier has no pool of its own.
• Suppression via an equivalency-resolved identifier still touching only the specified library.
• A direct match winning over equivalency; a direct match outside the library falling through to equivalency instead of being suppressed on that library's behalf.
• Only high-confidence equivalencies resolving: strength=1 finds the work, 0.85 doesn't. 0.85 isn't an arbitrary low number — it's the strength the importer assigns when it links two identifiers purely because their editions share a permanent work id, so those edges are widespread in production data.
• Two pools of one identifier pointing at different works — the inconsistent state calculate_work warns about and repairs — reaching AMBIGUOUS, and --suppress-ambiguous covering both.
• A work whose pool for the identifier is outside the library but which has another pool inside it still resolving — this is what lets the exact-match path be scoped without losing coverage, and it holds because an identifier is always a member of its own equivalent set (fn_recursive_equivalents seeds the CTE with it at strength 1, independent of the threshold).
• A title carried by another library resolving cleanly for each library rather than looking ambiguous to both.
• NOT_IN_LIBRARY vs NOT_FOUND; a pool whose work hasn't been calculated, both on its own (nothing to suppress) and alongside an equivalent work (falls through and suppresses that one); a library with no collections.
• Idempotent re-runs (a fully-suppressed candidate set reports ALREADY_SUPPRESSED, not AMBIGUOUS, with or without the flag), dry runs, and _print_results output for every status.

The multi-work and library-scoping tests were each checked against the pre-fix code to confirm they fail without the change.

Setup that recurred across tests is parametrized, or pulled into a helper where the cases share setup but assert different things. The regression tests that build deliberately unusual topologies are left standalone, since each documents a distinct shape.

mypy — clean. pre-commit — clean.

Checklist

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

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 2m 33s —— View job

---

Code review

• Gather context (diff, CLAUDE.md, related model code)
• Review src/palace/manager/scripts/suppress.py
• Review tests/manager/scripts/test_suppress.py
• Verify findings
• Post review

Summary

I found no correctness bugs in the pushed code. I checked both resolution paths against LicensePool.calculate_work and the default policy in Work.from_identifiers (levels=1, threshold 0.999). I also traced each deliberately unusual test topology through load_works, and every one lands on the outcome it asserts. That includes the workless-pool fall-through, the case where the identifier's own pool is outside the library but another pool of the work is inside, and the direct match outside the library. The claim that the importer sets strength 0.85 on permanent-work-id links holds (data_layer/bibliographic.py:248). @tdilauro's broader point is still open: an exact match silently wins over equivalents, and the output doesn't say which kind of match was used. The provenance output described in the latest reply ("matched via equivalency" and the summary count) isn't in this head (04e7802), so this review doesn't cover it. The one finding below is operator-facing wording left over from the rationale that 04e7802 corrected.

Details

Minor: src/palace/manager/scripts/suppress.py:86-93

The --suppress-ambiguous help text still gives "the same title licensed through more than one of the library's collections" as its example. That wording came in with d7e850b to describe the consortium + Advantage case, which 04e7802 established can't produce two works (calculate_work puts every pool of an identifier on one Work). This is the text an operator reads when deciding whether to pass the flag, and it presents ambiguity as always meaning the same book. In fact the guard exists because the candidates may be unrelated titles joined by a bad equivalency edge. The test_suppress_work_suppress_ambiguous_suppresses_all_candidates docstring (tests/manager/scripts/test_suppress.py:793-796) has the same stale example, "the same ISBN licensed through more than one collection", even though its fixture actually builds the equivalency case. Suggest describing the cases that really happen, e.g.:

help="If an identifier resolves to more than one distinct work "
"(e.g. an ISBN linked to several vendor identifiers, each with its "
"own work), suppress all of them instead of skipping it. The "
"candidates are not guaranteed to be the same title; check with "
"--dry-run first.",

circulation/src/palace/manager/scripts/suppress.py

Lines 86 to 93 in 04e7802

	parser.add_argument(
	"--suppress-ambiguous",
	help="If an identifier resolves to more than one distinct work "
	"(e.g. the same title licensed through more than one of the "
	"library's collections), suppress it in all of them instead of "
	"skipping it.",
	action="store_true",
	)

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Adds identifier resolution through equivalencies to suppress script.

The PR appears safe to merge based on the current findings.

Summary

The PR updates the suppression script to resolve library-carried works through exact identifier matches or, when none exist, high-confidence equivalencies.

• Ambiguous matches are reported without suppression unless the operator opts in to suppressing every candidate.
• Results distinguish identifiers absent from the library from identifiers with no matching work, and tests cover the new resolution and reporting paths.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Identifier and library] --> B{Exact work in library collections?}
  B -- Yes --> C[Use exact works]
  B -- No --> D[Find equivalent works in library collections]
  C --> E{Number of works}
  D --> E
  E -- None --> F[Not found or not in library]
  E -- One --> G[Suppress or report already suppressed]
  E -- Multiple --> H{Suppress ambiguous enabled?}
  H -- No --> I[Report ambiguous]
  H -- Yes --> J[Suppress remaining candidates]

Loading

_{Reviews (12) · Last reviewed commit: "Correct the same-identifier multi-work r..."}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.75%. Comparing base (5dde763) to head (04e7802).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3759   +/-   ##
=======================================
  Coverage   93.74%   93.75%           
=======================================
  Files         510      510           
  Lines       46417    46470   +53     
  Branches     6300     6310   +10     
=======================================
+ Hits        43513    43567   +54     
+ Misses       1876     1875    -1     
  Partials     1028     1028           

☔ 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]

@tdilauro : this one is ready for review. I've run through multiple rounds of review with Claude and I think I've worked through the legitimate objections. I'm going to address the documentation issue above (Minor: src/palace/manager/scripts/suppress.py:197-205) before merging (as well as anything you come back with).

[tdilauro]

@tdilauro : this one is ready for review. I've run through multiple rounds of review with Claude and I think I've worked through the legitimate objections. I'm going to address the documentation issue above (Minor: src/palace/manager/scripts/suppress.py:197-205) before merging (as well as anything you come back with).

I’m not sure which documentation issue you mean. Claude updates its comment every time it runs, so it’s a good idea to quote-reply when referencing anything it says. Otherwise it might not be anywhere obvious later.

[tdilauro]

I’ve added some specific comments below, but I have an overarching concern about kind of a dual standard for the acceptability of euivalents. A book with an equivalent ID will be suppressed if there is not an exact ID match present, but won’t be, if there is. And there’s no hint that this is happening. And it means that there is no determinism from the POV of a work (it depends on what else is there).

In the description, we say that equivalents come from vendors and that we can’t trust them as much as we do matching IDs. But in the absence of an exact ID, we seem to bump up our trust of the equivalents.

It seems like it would be useful to get each of the matching works and keep track of whether it is an exact or equivalent match. Then when we report out on what was suppressed or ambiguous, we’d be able to include that information to help understand what happened and why. It might also be useful to add a —allow-equivalents (or similar) option to have finer-grained control.

[tdilauro]

This makes sense in a way, but we will use that equivalency if we don’t have an exact match. That means that in some cases the equivalent is okay, but in others it’s not?

[dbernstein]

In almost every case cases it will be okay and if it's not it will likely be due to bad vendor data which is relatively rare. I think there is some value to suggest the probability that a title could be accidentally suppressed greater than zero.

[tdilauro]

I think there is some value to suggest the probability that a title could be accidentally suppressed greater than zero.

I don’t understand what this means.

[dbernstein]

Sorry - this is a missing "is" between "suppressed" and "zero". And it was not a very well written sentence even with the grammar corrected.

Anyway I think I misunderstood your point which is that we want consistency, determinism, and transparency. I'm going to work on this a little more - perhaps it should wait until the next release because I don't want to hold things up any longer.

[dbernstein]

@tdilauro: I stepped back, did a little more investigation into the equivalency subsystem to make sure I'm not confusing myself. You're right that there's an inconsistency here: the PR justifies preferring exact matches on the grounds that equivalents are vendor assertions we trust less — and then, when there's no exact match, treats those same assertions as good enough to suppress on. The trust we place on an association ends up depending on what else happens to be in the data rather than on the association itself.

I've implemented the provenance half of your suggestion. Every match now carries whether it was exact or equivalency-derived, and that shows up in the output:

[SUPPRESSED] ISBN/9781433383670 -- The Great Gatsby (work id: 42, matched via equivalency)
[SUPPRESSED] Overdrive ID/abc123 -- Moby Dick (work id: 7, exact match)
[AMBIGUOUS] ISBN/111 -- A (work id: 1, matched via equivalency); B (work id: 2, matched via equivalency)

plus a "Matched via equivalency" count in the run summary, so a long --file run can't bury them in the detail lines. This doesn't change which works get suppressed.

On the determinism point : as far as I can see it only two things actually fix it: never use equivalents unless asked, or always union them and let the ambiguity guard sort out disagreements. We had the second behavior earlier in this PR and pulled it, because an exact match that happened to also be equivalent to something unrelated would then refuse to act.

So the flag question is really about the default. My inclination is to keep equivalents on with an --exact-only escape hatch rather than making them opt-in: staff almost always have ISBNs while pools are keyed on vendor ids, so opt-in means most runs come back NOT_FOUND and we're back to the bug that started this. But I don't feel strongly — and with --dry-run plus the new provenance you can now preview exactly which matches are inferred before committing to anything, which may cover most of what the flag would buy us. What sounds best to you?

[tdilauro]

I'm okay with either default. We must just be sure to document it, either way. My main concern was making sure that we all understood what is re-articulated in the in the first paragraph, and responded appropriately.

[tdilauro]

I don’t see a test verifying that a lower-confidence equivalency wouldn’t be used. It would be good to have something that proves this out.

[dbernstein]

Every equivalency test used strength=1. Added a parametrized pair: 1 resolves, 0.85 doesn't. I picked 0.85 deliberately rather than an arbitrary low number — it's the strength the importer itself assigns when it links identifiers purely because their editions share a permanent work id, so those edges are all over production data. The pair is self-verifying: identical setup, only strength differs, so the passing high-confidence case proves the low-confidence NOT_FOUND comes from the threshold and not a broken fixture.

[tdilauro]

So, if we have a pool for an exact id, but the pool has no work, then we fall through and might suppress a different, equivalence-based book?

[dbernstein]

Yes. You can't suppress a workless pool at all: suppressed_for is a Work↔Library relation, so with no Work there's nothing to attach a suppression to. That means the choice isn't "suppress the right book vs. the equivalent book" — it's "suppress the equivalent book vs. suppress nothing." Refusing would mean telling a librarian NOT FOUND for a title their library demonstrably carries (under the equivalent identifier, with a real Work), purely because some other pool of the same identifier happened to be mid-import. That's the worse outcome.

The "different book" risk is real if low, but it's the generic equivalency risk that the whole fallback carries — not something specific to this path — and it's bounded three ways: the 0.999 threshold, the AMBIGUOUS guard if more than one work matches, and the output naming the title and work id so a wrong hit is visible.

One residual limitation worth knowing, which nothing can fix at suppression time: when that workless pool does get its Work calculated later, the new Work won't be retroactively suppressed. Same partial-coverage hazard as the multi-collection case. I documented the fall-through in the load_works docstring and added a test pinning it, since your question showed the behavior was both unstated and untested.

To address this we could add a new scheduled celery task (in a separate PR) that periodically ensures that if one title is suppressed and an equivalent title is not, the equivalent title should be suppressed.

[dbernstein]

https://ebce-lyrasis.atlassian.net/browse/PP-5261