fix(metadata): honour field locks on every Hardcover write path (#2767)

[vavallee]

Summary

Closes #2767.

models.Book states the contract: every refresh, enrichment and merge path must check IsFieldLocked before overwriting a lockable field (internal/models/book.go:67-69). Nothing under internal/bookhydrate or internal/metadata did, until #2760 added the first check for Language. This generalises that fix to the rest of the two packages.

What was actually broken

I re-verified each claim in the issue against origin/main at 8e023eb1. Two of the four hold as filed, one is off by a few lines, and two are not reachable the way the issue describes.

Claimed	Verified
aggregator_enrichment.go:36-38, EnrichAudiobook fills Description when empty	Yes, lines 37-38. The live bug.
aggregator_enrichment.go:288, enrichBook overwrites a longer Description	Real write, but at line 293, not 288. See reachability below.
aggregator_enrichment.go:317, enrichBook replaces Genres for Hardcover	Yes, line 317 exactly. See reachability below.
bookhydrate/hardcover.go:237 writes ImageURL	The write is at lines 238-241, not 237. Not a lockable field; see the decision below.

EnrichAudiobook is the one that reaches a stored book. HydrateHardcoverEditions calls it at internal/bookhydrate/hardcover.go:143 on the book it loaded, and the Books.Update at hardcover.go:157 persists whatever it wrote. Two other call sites do the same: internal/api/books.go:174 and internal/abs/importer.go:822. A user who clears a description and locks it got it refilled from the Audnex summary on the next hydrate. That is #2757 one field over, on the same call path.

enrichBook and applyEnrichmentSnapshot never see a stored book today. All four call sites hand them a provider transport object built by a provider client, which carries no LockedFields: aggregator.go:814 (GetBook), aggregator.go:1004 (cacheISBNBook), aggregator_author_works.go:777 (enrichMissingAuthorWorkCovers), aggregator_enrichment.go:79 (the ASIN canonicalizer). I traced where those enriched objects land: the author refresh merge at internal/api/authors.go:2410 already checks the genre lock, the ABS path goes through applyBookFields which checks all five, and rebind (books.go:1149) and metadata re-map (books.go:1277) clear LockedFields on purpose because they mean "take the new record wholesale". So there is no live leak through these two functions. I guarded them anyway, because the contract is unconditional and a future caller passing a stored book is exactly the mistake the contract exists to prevent, but I am not claiming a user-visible fix there. The tests pin the behaviour, not a reproduction.

One consequence worth flagging

enrichBook seeds a process wide cache from the post merge book, keyed on (provider, foreignID), which every book for that work shares across users. Once a locked field keeps the user's own value, caching it would serve that user's hand written description to the next library that enriches the same work. So a book with a description or genre lock no longer seeds the cache at all. It costs one extra enricher round trip in a case that barely happens; every unlocked book caches exactly as before. Pinned by TestEnrichBookDoesNotCacheFromALockedBook.

Also checked, no change needed

• mergeAuthorWorkMetadata (aggregator_author_works.go:831): the audit's judgement is correct. dst is an element of the []models.Book returned by GetAuthorWorks, built by the provider clients, so LockedFields is always empty there. Its output reaches stored books through internal/api/authors.go, which does its own lock check on genres. Left alone: a guard there would be reading a field that is structurally always empty.
• internal/hardcoverlistsyncer (out of scope, confirmed rather than assumed): enrichAudiobook is called from exactly one place, syncer.go:609, immediately after s.books.Create on a book the syncer just built. No lock can exist on a row that did not exist a line earlier.

Decisions

1. ImageURL stays out of LockableBookFields and stays unguarded. There is no cover field in the edit dialog, and a lock is only ever created by editing a field (internal/api/books.go:466-497), so nothing can lock a cover today. Adding it would be a user visible capability change: the edit UI needs a control, docs/Metadata-Editing-Wiki.md lists the five editable fields and would need a sixth row, and the PUT /api/v1/book/{id} contract would widen. That is a feature, not this bug fix. Both ImageURL writes stay fill-empty and now carry a comment saying why, so the next reader does not have to re-derive it.

2. Book.CanWrite(field) is the single mechanism. One name to grep for instead of scattered !book.IsFieldLocked(...). It is used at every one of the seven sites in these two packages, including converting the Language guard #2760 added, so there is no second idiom left here. I deliberately did not churn internal/abs, internal/calibre, internal/importer or internal/api, which keep their existing IsFieldLocked calls: that is unrelated blast radius for a bug fix.

3. Fill and overwrite are separate tests. CanWrite answers ownership only. The emptiness check stays where it was at each site, spelled out next to it. The three shapes now read:

• fill when empty: book.Description == "" && b.Summary != "" && book.CanWrite(...)
• overwrite when longer: len(e.Description) > len(book.Description) && book.CanWrite(...)
• replace outright: enricher.Name() == "hardcover" && len(e.Genres) > 0 && book.CanWrite(...)

A lock blocks all three. Collapsing them into one "is it empty" question is precisely what produced #2757, so the helper refuses to answer it.

Fail before evidence

Every new test was run against origin/main behaviour first (source reverted, CanWrite stubbed in so the tests compile).

$ go test ./internal/bookhydrate/ -run TestHydrateHardcoverEditionsRespectsDescriptionLock
--- FAIL: TestHydrateHardcoverEditionsRespectsDescriptionLock (0.33s)
    --- FAIL: .../locked_empty_survives (0.09s)
        hardcover_locked_description_test.go:92: description after hydration = "Audnex summary", want ""
        hardcover_locked_description_test.go:99: persisted description = "Audnex summary", want ""
FAIL

That is the headline: a description locked while empty, a Hardcover hydration whose Audnex enrichment supplies a summary, and the summary lands in the database. The other three sub-cases pass on main, which is the point: the emptiness gate hid everything except the deliberate clear.

$ go test ./internal/metadata/ -run 'TestEnrich...|TestApplyEnrichmentSnapshotHonoursLocks'
--- FAIL: TestEnrichAudiobookHonoursDescriptionLock/locked_empty_survives
    description = "Audnex summary", want ""
--- FAIL: TestEnrichBookHonoursDescriptionLock/locked_survives
    description = "A considerably longer description supplied by the enricher.", want "mine"
--- FAIL: TestEnrichBookHonoursGenreLock/locked_survives
    genres = [Fantasy], want [Unsorted]
--- FAIL: TestApplyEnrichmentSnapshotHonoursLocks/locked_survives
    description = "A considerably longer cached description.", want "mine"
    genres = [Fantasy], want [Unsorted]
FAIL

The headline test is deliberately wired to a real metadata.Aggregator with a stubbed Audnex client rather than a fake enricher. A fake that consulted the lock itself would have been testing the fake.

Mutation check

Each guard broken in turn (&& book.CanWrite(...) to && true || book.CanWrite(...)), test run, guard restored. All seven are pinned by a named test:

Guard	Test that fails when broken
aggregator_enrichment.go:42 EnrichAudiobook description	TestEnrichAudiobookHonoursDescriptionLock, and TestHydrateHardcoverEditionsRespectsDescriptionLock
:301 enrichBook description	TestEnrichBookHonoursDescriptionLock
:329 enrichBook genres	TestEnrichBookHonoursGenreLock
:351 cache seed	TestEnrichBookDoesNotCacheFromALockedBook
:372 snapshot description	TestApplyEnrichmentSnapshotHonoursLocks
:387 snapshot genres	TestApplyEnrichmentSnapshotHonoursLocks
bookhydrate/hardcover.go:232 language	TestHydrateHardcoverEditionsRespectsLanguageLock (#2760's test, still pinned after the conversion to CanWrite)

Nothing unpinned.

Checklist

• Commits signed off with git commit -s
• Tests added or updated
• docs/DEPLOYMENT.md updated if env vars, config, or upgrade path changed (no env, config or upgrade change)
• Added a changelog fragment under changelog.d/
• Wiki pages updated if user-facing behaviour changed (docs/Metadata-Editing-Wiki.md already promises locks survive every automatic refresh. This makes that true rather than changing it, so there is nothing to correct.)

Test plan

• go build ./... && go vet ./...
• go test ./cmd/... ./internal/..., fully green
• GOOS=windows go build ./... and GOOS=darwin go build ./...
• Web untouched, so no frontend matrix

🤖 Generated with Claude Code

https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Clean fix. The headline bug is real and the test at hardcover_locked_description_test.go:31 wires to the actual call path (aggregator → hydrate → Books.Update) rather than a fake, which is the right choice.

One non-blocking observation on the cache gate at aggregator_enrichment.go:351:

if cacheKey != "" && book.CanWrite(models.BookFieldDescription) && book.CanWrite(models.BookFieldGenres) {

Locking either field suppresses the whole snapshot, so a book with only genres locked also loses the imageURL/averageRating/ratingsCount cache entries. That means the next library enriching the same work pays one extra provider round-trip for those fields even though they are unlocked and their post-merge values in the book are purely provider-owned. The PR body already explains the tradeoff ("barely happens"); confirming the implication is understood and the reasoning holds — it does, since caching partial snapshots or zeroing locked fields before storing would be substantially more complex for a rare case.

Everything else looks right:

• CanWrite nil-safety (models/book.go:165) is correct — returning true is the safe choice.
• The decision to leave ImageURL unguarded is sound; the comment at bookhydrate/hardcover.go:232 documents it for the next reader.
• Mutation table in the PR body accounts for all seven guards.
• Scope is narrow: internal/abs, internal/calibre and internal/api are deliberately untouched.

— 🤖 Bindery triage bot (automated). Reply to correct me; a human will see it.