fix(sweep): provider-aware EnrichComicDetail — stop Metron id fetched as ComicVine id (bookshelf-vbtl) #866
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-vbtl"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Root Cause
buildBulkLLMSweepDeps.EnrichComicDetailwas provider-blind — it always calledcomicvine.NewFetchIssueDetail(m.ExternalID)regardless of which provider produced the matched candidate.Metron list candidates (
searchResultToListCandidate) setExternalID = the Metron issue id(cv_id is only populated by the full detail endpoint, which the sweep never called for list results). So a Metron match causedComicVine /issue/{metron_id}/— Metron and ComicVine ids are both small ints and collide — fetching a completely unrelated ComicVine issue. Book 3815 (ASM #611) got saved as "The Secret of Skartaris / Warlord #5" withcomicvine_id = 17164(the Metron id).The interactive path (
CandidateDetailHandler) was correct — it switches on provider and callsfetchMetronDetailfor Metron candidates. The bulk sweep lacked the same switch.Fix
ProviderID stringtometadata.Metadata— set by each provider's result mappers:"comicvine"inissueListResultToMetadata,searchResultToMetadata,issueDetailToMetadata"metron"insearchResultToListCandidate,detailToMetadatawfengine.NewEnrichComicDetailFunc(fetchCV, fetchMetron)— routes the detail fetch bym.ProviderID:"metron"→ calls Metron detail endpoint withm.ExternalID(the Metron issue id); returns full metadata withcv_id → ExternalIDsocomicvine_idgets the correct value"comicvine"→ calls ComicVine detail endpoint (as before)munchangedbuildBulkLLMSweepDepsand replace the old inline closure withwfengine.NewEnrichComicDetailFunc(fetchDetail, fetchMetronDetail)ID Flow (corrected)
Metron candidate:
ExternalID = 17164 (Metron issue id)→fetchMetronDetail(17164)→ detail:cv_id = 42→ExternalID = 42→ saved ascomicvine_id = 42(correct)ComicVine candidate:
ExternalID = 42 (CV issue id)→fetchCV(42)→comicvine_id = 42(unchanged)Test Plan
NewEnrichComicDetailFunctests: Metron candidate calls Metron fetcher with the Metron id (not ComicVine); result has cv_id as ExternalID; ComicVine candidate calls CV fetcher; nil fetcher / zero ExternalID / unknown provider return input unchangedmake test+make test-policy-check+make e2e-policy-checkall greenCloses bead bookshelf-vbtl on merge.
Root cause: buildBulkLLMSweepDeps.EnrichComicDetail was provider-blind — it always called comicvine.NewFetchIssueDetail with m.ExternalID regardless of which provider produced the candidate. Metron list candidates set ExternalID = the Metron issue id (cv_id is only available from the detail endpoint). So a Metron match resulted in ComicVine /issue/{metron_id}/ which fetches an unrelated ComicVine issue (Metron id 17164 → ComicVine "Warlord #5"), and that wrong comic was saved. Fix: - Add ProviderID string field to metadata.Metadata — set to "comicvine" in ComicVine's issueListResultToMetadata/searchResultToMetadata/issueDetailToMetadata, and "metron" in Metron's searchResultToListCandidate/detailToMetadata. - Add wfengine.NewEnrichComicDetailFunc(fetchCV, fetchMetron) that routes the detail fetch by m.ProviderID: "metron" → Metron detail (ExternalID becomes cv_id after the call); "comicvine" → ComicVine detail (as before). - Wire a Metron detail fetcher in buildBulkLLMSweepDeps and replace the old inline EnrichComicDetail closure with wfengine.NewEnrichComicDetailFunc. Tests: NewEnrichComicDetailFunc coverage: Metron candidate calls Metron fetcher (not ComicVine), result has cv_id as ExternalID; ComicVine candidate calls CV fetcher; nil fetcher / zero ExternalID / unknown provider all return input unchanged. Provider-level tests assert ProviderID is set correctly by both providers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Workflow Detail page screenshot (wf-detail-older-execution)
Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.
Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)
Code Review — PR #866 (bookshelf-vbtl)
Phase 0: DEMO block
Not applicable — P1 data-corruption fix; no DEMO block required per review dispatch.
Phase 1 + 2: Spec compliance and code quality
Reviewed diff:
git diff $(git merge-base origin/main origin/bd-bookshelf-vbtl)..origin/bd-bookshelf-vbtlROUTING (check 1):
NewEnrichComicDetailFuncininternal/wfengine/bulk_llm_workflow.go:220has an explicitswitch m.ProviderIDwithcase "metron"routing tofetchMetronandcase "comicvine"routing tofetchCV. TheExternalID == 0guard fires before the switch, so zero-ID candidates skip both fetchers. A Metron candidate with a non-zero ExternalID cannot reach the ComicVine endpoint. Verified.CORRECT ID (check 2):
metron/provider.go:574setsm.ExternalID = *d.CVIDinsidedetailToMetadata. The existing test atmetron/provider_test.go:515–516assertsresult.ExternalID == 111222where the stub hascv_id = 111222. The PR addsProviderID == "metron"at line 519.searchResultToListCandidate(line 692) setsm.ExternalID = r.ID(Metron issue id) for list candidates, which is what gets passed intofetchMetron(ctx, m.ExternalID)— correct: Metron detail is fetched by Metron id, and its return value carries the cv_id as ExternalID. Verified.PROVIDERID coverage (check 3): All five mappers set ProviderID:
comicvine/provider.go:1425—issueListResultToMetadata✓comicvine/provider.go:1527—searchResultToMetadata✓comicvine/provider.go:1560—issueDetailToMetadata✓metron/provider.go:571—detailToMetadata✓metron/provider.go:693—searchResultToListCandidate✓Note:
searchResultToMetadatadoes not setm.ExternalID(pre-existing behavior, not a regression), so its ProviderID will never reach the routing switch — theExternalID == 0guard fires first. Safe.WIRING (check 4):
build_extended_deps.go:1481–1490constructsfetchMetronDetailusing Metron config andwaitFor(settings.ProviderMetron)rate limiter. BothfetchDetailandfetchMetronDetailare passed towfengine.NewEnrichComicDetailFunc(fetchDetail, fetchMetronDetail)at line 1545. Verified.REGRESSION TEST (check 5):
bulk_llm_workflow_test.goDescribe("NewEnrichComicDetailFunc")at line 648:{ProviderID: "metron", ExternalID: 17164}(exact bug ID): assertsmetronCalledWith == 17164andcvCalledWith == 0. ✓cvCalledWith == 42andmetronCalledWith == 0. ✓ExternalID == 0: asserts no fetcher called, input returned unchanged. ✓GATE-GAMING (check 7): Test file declares
package wfengine_test(black-box).NewEnrichComicDetailFuncis a legitimate production export consumed bybuild_extended_deps.go:1545— not a test-only shim. No unexported symbols referenced. ✓[MINOR] internal/wfengine/bulk_llm_workflow.go:239,244 — ProviderID routing uses bare string literals repeated across three packages
"comicvine"and"metron"appear incomicvine/provider.go,metron/provider.go, AND the switch here. A one-character typo in any of them silently falls through to the default branch (no enrichment, no error). Tests catch this today, but package-level constants (const ProviderIDComicVine = "comicvine") in themetadatapackage would make a mismatch a compile error. Low risk while the test suite is green; worth a follow-up bead.[MINOR] internal/wfengine/bulk_llm_workflow_test.go:652 —
Orderedon a unit test container causes cascade-skip on spec failureDescribe("NewEnrichComicDetailFunc", Ordered, ...)means if the Metron-routing spec fails, all subsequent specs (ComicVine, nil-fetcher, etc.) are skipped instead of running independently. For unit tests, a non-Ordered outer container is conventional — eachDescribesub-block is already isolated by its ownBeforeEach. Not a correctness issue; nit.REVIEW VERDICT: 0 blocker, 0 major, 2 minor