fix(comicvine): no-year path checks volumes individually, removing nameMatch gate (bookshelf-c51j) #980

Merged

zombor merged 1 commit from bd-bookshelf-c51j into main 2026-07-06 16:44:45 +00:00

Conversation 2 Commits 1 Files changed 2 +128 -154

zombor commented 2026-07-06 16:24:20 +00:00

Owner

Copy link

Summary

• Root cause: searchStructuredNoYear used a batch OR-filter gated by nameMatch=true, excluding volumes like "Spectacular Spider-Man" (nameMatch=false, Jaccard=0.50 with query) that passed the relevance floor and had the requested issue. A broader (no-year) query thus returned zero results when the year-known path found candidates.
• Fix: replaced the batch+nameMatch-gate approach with individual fetchIssuesByFilter calls per scored volume (mirroring the year path), capped at maxNoYearVolumesToCheck=10. No nameMatch gate is applied — any volume that passes the relevance floor (Jaccard >= 0.30 OR nameMatch=true) is checked individually.
• Removed: fetchIssuesBatchByFilter (now dead code).

Test plan

• Added regression test: "Spectacular Spider-Man" (nameMatch=false, Jaccard=0.50) is found when querying "Amazing Spider-Man #5" without a year — previously returned ErrNoMatch, now returns the issue.
• Updated "all top-N volumes checked individually" test: now asserts 5 individual calls (not 1 batch).
• Updated "relevance-floor-drop" test: clarified the filtering happens at the relevance floor (Jaccard<0.30), not the old nameMatch gate.
• Replaced "MaxProviderResults batch cap" test with "maxNoYearVolumesToCheck individual cap" test: 13 volumes, cap=10 → exactly 10 calls, 10 results.
• make test passes, make coverage green (100%).

Closes bead bookshelf-c51j on merge.

## Summary - **Root cause:** `searchStructuredNoYear` used a batch OR-filter gated by `nameMatch=true`, excluding volumes like "Spectacular Spider-Man" (nameMatch=false, Jaccard=0.50 with query) that passed the relevance floor and had the requested issue. A broader (no-year) query thus returned zero results when the year-known path found candidates. - **Fix:** replaced the batch+nameMatch-gate approach with individual `fetchIssuesByFilter` calls per scored volume (mirroring the year path), capped at `maxNoYearVolumesToCheck=10`. No nameMatch gate is applied — any volume that passes the relevance floor (Jaccard >= 0.30 OR nameMatch=true) is checked individually. - **Removed:** `fetchIssuesBatchByFilter` (now dead code). ## Test plan - [x] Added regression test: "Spectacular Spider-Man" (nameMatch=false, Jaccard=0.50) is found when querying "Amazing Spider-Man #5" without a year — previously returned ErrNoMatch, now returns the issue. - [x] Updated "all top-N volumes checked individually" test: now asserts 5 individual calls (not 1 batch). - [x] Updated "relevance-floor-drop" test: clarified the filtering happens at the relevance floor (Jaccard<0.30), not the old nameMatch gate. - [x] Replaced "MaxProviderResults batch cap" test with "maxNoYearVolumesToCheck individual cap" test: 13 volumes, cap=10 → exactly 10 calls, 10 results. - [x] `make test` passes, `make coverage` green (100%). Closes bead bookshelf-c51j on merge.

zombor added 1 commit 2026-07-06 16:24:22 +00:00

fix(comicvine): no-year path checks volumes individually, removing nameMatch gate (bookshelf-c51j) ... bc2646acc6

The old searchStructuredNoYear used a batch OR-filter gated by nameMatch=true,
causing volumes like "Spectacular Spider-Man" (nameMatch=false, Jaccard=0.50) to
be excluded even though they passed the relevance floor and had the requested
issue. A broader (no-year) query thus returned fewer results than the narrower
year-known query.

Fix: check top maxNoYearVolumesToCheck=10 scored volumes individually via
fetchIssuesByFilter (mirroring the year path), with no nameMatch gate. This
guarantees the no-year result set is a superset of the same query with a year.

Remove fetchIssuesBatchByFilter (now dead code).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor force-pushed bd-bookshelf-c51j from bc2646acc6 to e038b379f0 2026-07-06 16:31:03 +00:00 Compare

zombor commented 2026-07-06 16:39:05 +00:00

Author

Owner

Copy link

Security Review — bookshelf-c51j (ComicVine no-year search algorithm)

Four targeted checks performed against the diff (searchStructuredNoYear refactor + structured_test.go):

1. SSRF / unvalidated URL fetch
baseURL is injected at construction time from server config, never from user input. Volume IDs passed to fetchIssuesByFilter are int64 values sourced from the ComicVine API response and formatted with %d — no user-controlled URL fragment is introduced. URL construction uses url.Values.Set() (properly encoded). No new SSRF surface.

2. Injection into the filter string
The filter is built as fmt.Sprintf("volume:%d,issue_number:%s", volumeID, issueNumber). The volumeID is int64 (%d — injection-proof). The issueNumber (norm) is the output of metadata.NormalizeIssueNumber, which was the pre-existing code path — unchanged by this PR. That normalization and the fetchIssuesByFilter signature both predate this diff; the PR only changes the call site from a batch loop to an individual loop. No new injection surface.

3. Secrets / PII in logs
The log.Info call in fetchIssuesByFilter logs volume_id (int64) and issue_number (normalized string). The API key is added to url.Values but never logged. Clean.

4. Amplification / rate-limit bypass
The old code made 1 batch call covering up to 50 volumes. The new code makes up to 10 individual calls (cap is the compile-time constant maxNoYearVolumesToCheck = 10, reduced from 50). Each call goes through the same limiter and hourly *rate.Limiter passed to doGetWithRetry — so all 10 calls are individually rate-limited. The fan-out is hard-capped and rate-limited; no unbounded amplification.

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-c51j (ComicVine no-year search algorithm) Four targeted checks performed against the diff (`searchStructuredNoYear` refactor + `structured_test.go`): **1. SSRF / unvalidated URL fetch** `baseURL` is injected at construction time from server config, never from user input. Volume IDs passed to `fetchIssuesByFilter` are `int64` values sourced from the ComicVine API response and formatted with `%d` — no user-controlled URL fragment is introduced. URL construction uses `url.Values.Set()` (properly encoded). No new SSRF surface. **2. Injection into the filter string** The filter is built as `fmt.Sprintf("volume:%d,issue_number:%s", volumeID, issueNumber)`. The `volumeID` is `int64` (`%d` — injection-proof). The `issueNumber` (`norm`) is the output of `metadata.NormalizeIssueNumber`, which was the pre-existing code path — unchanged by this PR. That normalization and the `fetchIssuesByFilter` signature both predate this diff; the PR only changes the call site from a batch loop to an individual loop. No new injection surface. **3. Secrets / PII in logs** The `log.Info` call in `fetchIssuesByFilter` logs `volume_id` (int64) and `issue_number` (normalized string). The API key is added to `url.Values` but never logged. Clean. **4. Amplification / rate-limit bypass** The old code made 1 batch call covering up to 50 volumes. The new code makes up to 10 individual calls (cap is the compile-time constant `maxNoYearVolumesToCheck = 10`, reduced from 50). Each call goes through the same `limiter` and `hourly *rate.Limiter` passed to `doGetWithRetry` — so all 10 calls are individually rate-limited. The fan-out is hard-capped and rate-limited; no unbounded amplification. No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-07-06 16:40:55 +00:00

Author

Owner

Copy link

CODE REVIEW — bookshelf-c51j (PR #980)

Phase 0: DEMO

No interactive DEMO block — this is a logic fix verified by unit tests. CI is green per the implementer. Proceeding.

Phase 1: Spec Compliance

Spec required:

• Replace searchStructuredNoYear batch+nameMatch-gate with per-volume fetchIssuesByFilter calls (mirroring year path) ✓
• maxNoYearVolumesToCheck=10 ✓
• Remove dead fetchIssuesBatchByFilter ✓
• Regression test for nameMatch=false volume case ✓
• 100% coverage, black-box tests ✓

Phase 2: Code Quality

1. CORRECTNESS — verified ✓
Old code gated batchIDs on sv.nameMatch, so a volume whose Jaccard ≥ 0.30 but nameMatch=false was silently excluded. The fix iterates scored (already relevance-floored inside scoreVolumes) unconditionally up to the cap, mirroring the year path exactly. The no-year result set is now structurally guaranteed to be ≥ year result set for the same query.

2. REGRESSION TEST — genuine ✓
Vol 500 ("Amazing Spider-Man", nameMatch=true) has no issue #5; Vol 501 ("Spectacular Spider-Man", nameMatch=false, Jaccard=0.50) has issue #5. Old code: batchIDs=[500] only → batch returns empty → ErrNoMatch. New code: queries 500 (empty) then 501 (hit) → 1 result. Test asserts HaveLen(1) which would fail on old code and pass on new. The 5 updated existing tests are correctly rewritten for the new mechanism — none are loosened.

3. DEAD CODE — fully gone ✓
git grep fetchIssuesBatchByFilter origin/bd-bookshelf-c51j returns empty. No stale references.

4. MERGE ARTIFACT (hd0y.1 associated_images) — intact ✓
associated_images present in field_list at 3 call sites in search.go (lines 416, 689, 735). Rebase did not drop them.

5. PACKAGE/COVERAGE — clean ✓
package comicvine_test at top of structured_test.go. git diff origin/main -- scripts/check-coverage.sh is empty — no new exclusions.

---

[MINOR] internal/metadata/comicvine/scoring.go (sort comparator, end of scoreVolumes) — sort is effectively a total order only by coincidence
The insertion sort uses strict > (stable: ties preserve input order). Ties are prevented in practice because volumeSearchLimit=25 exactly matches the rank-bonus range (ranks 0–24 → bonuses 25–1, all unique). If volumeSearchLimit were bumped above 25 without adding an explicit tiebreaker (e.g., , || (score equal && vol.ID < other.vol.ID)), volumes at ranks ≥25 would all get rank-bonus=0 and could tie on total score, making the top-10 cut order-dependent on input. Currently sound; worth hardening at a future refactor.

[MINOR] internal/metadata/comicvine/search.go:150 — MaxProviderResults soft-cap removed from the no-year accumulation loop
Old code broke when len(results) >= metadata.MaxProviderResults (12). New code only breaks at i >= maxNoYearVolumesToCheck (10). With fetchIssuesByFilter capped at limit=5 per call, the theoretical max output is 50 results (10 volumes × 5 issues) vs the old 12. In practice issue-number queries return 0–1 result per volume, so real-world output stays ≤10. No user-visible impact today, but the cap was silently dropped.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## CODE REVIEW — bookshelf-c51j (PR #980) ### Phase 0: DEMO No interactive DEMO block — this is a logic fix verified by unit tests. CI is green per the implementer. Proceeding. ### Phase 1: Spec Compliance Spec required: - Replace `searchStructuredNoYear` batch+nameMatch-gate with per-volume `fetchIssuesByFilter` calls (mirroring year path) ✓ - `maxNoYearVolumesToCheck=10` ✓ - Remove dead `fetchIssuesBatchByFilter` ✓ - Regression test for nameMatch=false volume case ✓ - 100% coverage, black-box tests ✓ ### Phase 2: Code Quality **1. CORRECTNESS — verified ✓** Old code gated `batchIDs` on `sv.nameMatch`, so a volume whose Jaccard ≥ 0.30 but nameMatch=false was silently excluded. The fix iterates `scored` (already relevance-floored inside `scoreVolumes`) unconditionally up to the cap, mirroring the year path exactly. The no-year result set is now structurally guaranteed to be ≥ year result set for the same query. **2. REGRESSION TEST — genuine ✓** Vol 500 ("Amazing Spider-Man", nameMatch=true) has no issue #5; Vol 501 ("Spectacular Spider-Man", nameMatch=false, Jaccard=0.50) has issue #5. Old code: `batchIDs=[500]` only → batch returns empty → ErrNoMatch. New code: queries 500 (empty) then 501 (hit) → 1 result. Test asserts `HaveLen(1)` which would fail on old code and pass on new. The 5 updated existing tests are correctly rewritten for the new mechanism — none are loosened. **3. DEAD CODE — fully gone ✓** `git grep fetchIssuesBatchByFilter origin/bd-bookshelf-c51j` returns empty. No stale references. **4. MERGE ARTIFACT (hd0y.1 `associated_images`) — intact ✓** `associated_images` present in `field_list` at 3 call sites in `search.go` (lines 416, 689, 735). Rebase did not drop them. **5. PACKAGE/COVERAGE — clean ✓** `package comicvine_test` at top of `structured_test.go`. `git diff origin/main -- scripts/check-coverage.sh` is empty — no new exclusions. --- [MINOR] `internal/metadata/comicvine/scoring.go` (sort comparator, end of `scoreVolumes`) — sort is effectively a total order only by coincidence The insertion sort uses strict `>` (stable: ties preserve input order). Ties are prevented in practice because `volumeSearchLimit=25` exactly matches the rank-bonus range (ranks 0–24 → bonuses 25–1, all unique). If `volumeSearchLimit` were bumped above 25 without adding an explicit tiebreaker (e.g., `, || (score equal && vol.ID < other.vol.ID)`), volumes at ranks ≥25 would all get rank-bonus=0 and could tie on total score, making the top-10 cut order-dependent on input. Currently sound; worth hardening at a future refactor. [MINOR] `internal/metadata/comicvine/search.go:150` — `MaxProviderResults` soft-cap removed from the no-year accumulation loop Old code broke when `len(results) >= metadata.MaxProviderResults` (12). New code only breaks at `i >= maxNoYearVolumesToCheck` (10). With `fetchIssuesByFilter` capped at `limit=5` per call, the theoretical max output is 50 results (10 volumes × 5 issues) vs the old 12. In practice issue-number queries return 0–1 result per volume, so real-world output stays ≤10. No user-visible impact today, but the cap was silently dropped. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor merged commit 553cd137f4 into main 2026-07-06 16:44:45 +00:00

zombor referenced this pull request from a commit 2026-07-06 16:44:47 +00:00

Merge pull request 'fix(comicvine): no-year path checks volumes individually, removing nameMatch gate (bookshelf-c51j)' (#980) from bd-bookshelf-c51j into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!980

No description provided.