feat(comicvine): port Grimmory full scoring + special-issue + filter-fallback (bookshelf-23na) #690

Merged

zombor merged 1 commit from bd-bookshelf-23na into main 2026-06-22 16:33:37 +00:00

Conversation 2 Commits 1 Files changed 3 +561 -30

zombor commented 2026-06-22 16:09:05 +00:00

Owner

Copy link

Summary

Completes the multi-step ComicVine lookup to fully mirror Grimmory's ComicvineBookParser.java algorithm. The core volume→issue structured path was already shipped in bookshelf-wzef (PR #414); this PR adds the remaining gaps identified in the bead spec:

• Special-issue pattern — handle "X-Men Annual 5", "Spawn Special 1", "Batman One-Shot 3" (no #). The keyword is folded into the series name so volume search targets the correct annual/special volume.
• Digital/bracketed suffix stripping — "Batman #1 [Digital Edition]" is pre-cleaned before regex matching so year and series are correctly extracted.
• /api/volumes/?filter=name:<series> fallback — when /search/?resources=volume returns 0 results, a second /volumes/?filter= request is tried before giving up, mirroring Grimmory's two-step volume discovery.
• Full Grimmory calculateVolumeScore heuristics — scoring now includes: +50 exact name match, +25 name-contains, +20 count_of_issues ≥ requested issue, +10 major publisher, +5/+5/+5 recency bonuses for start_year ≥ 2000/2010/2020 (in addition to the existing +100 year-match and position bonus). Publisher field added to volumeFieldList.

Test plan

• make test passes (comicvine 100% coverage gate)
• make coverage passes with check-coverage: OK
• New specs: special-issue parsing (annual/special/one-shot), digital suffix stripping, full ScoreVolumes heuristics table, filter-fallback happy path, filter-fallback HTTP 5xx error, filter-fallback malformed JSON error, API key forwarding on /volumes/ request
• No changes to templates or static assets (no UI screenshot required)

Closes bead bookshelf-23na on merge.

## Summary Completes the multi-step ComicVine lookup to fully mirror Grimmory's `ComicvineBookParser.java` algorithm. The core volume→issue structured path was already shipped in bookshelf-wzef (PR #414); this PR adds the remaining gaps identified in the bead spec: - **Special-issue pattern** — handle `"X-Men Annual 5"`, `"Spawn Special 1"`, `"Batman One-Shot 3"` (no `#`). The keyword is folded into the series name so volume search targets the correct annual/special volume. - **Digital/bracketed suffix stripping** — `"Batman #1 [Digital Edition]"` is pre-cleaned before regex matching so year and series are correctly extracted. - **`/api/volumes/?filter=name:<series>` fallback** — when `/search/?resources=volume` returns 0 results, a second `/volumes/?filter=` request is tried before giving up, mirroring Grimmory's two-step volume discovery. - **Full Grimmory `calculateVolumeScore` heuristics** — scoring now includes: +50 exact name match, +25 name-contains, +20 count_of_issues ≥ requested issue, +10 major publisher, +5/+5/+5 recency bonuses for start_year ≥ 2000/2010/2020 (in addition to the existing +100 year-match and position bonus). Publisher field added to `volumeFieldList`. ## Test plan - [ ] `make test` passes (comicvine 100% coverage gate) - [ ] `make coverage` passes with `check-coverage: OK` - [ ] New specs: special-issue parsing (annual/special/one-shot), digital suffix stripping, full `ScoreVolumes` heuristics table, filter-fallback happy path, filter-fallback HTTP 5xx error, filter-fallback malformed JSON error, API key forwarding on `/volumes/` request - [ ] No changes to templates or static assets (no UI screenshot required) Closes bead bookshelf-23na on merge.

zombor added 1 commit 2026-06-22 16:09:05 +00:00

feat(metadata/comicvine): port Grimmory's full volume-scoring + special-issue + filter-fallback (bookshelf-23na) ... 0225e89cdf

Completes the multi-step ComicVine lookup to fully mirror Grimmory's
ComicvineBookParser algorithm. The core volume→issue structured path
was already shipped in bookshelf-wzef; this PR adds the remaining gaps:

1. SPECIAL_ISSUE_PATTERN — handle "X-Men Annual 5", "Spawn Special 1",
   "Batman One-Shot 3" (no #). The special keyword is folded into the
   series name so volume search targets the correct annual/special volume.

2. Digital/bracketed suffix stripping — "Batman #1 [Digital Edition]"
   is now pre-cleaned before regex matching so the year and series are
   correctly extracted.

3. /api/volumes/?filter=name:<series>&limit=20 fallback — when
   /api/search/?resources=volume returns 0 results, a second request
   to the /volumes/ filter endpoint is tried before giving up, mirroring
   Grimmory's two-step volume discovery.

4. Full Grimmory calculateVolumeScore heuristics — scoring now includes:
   +50 exact name match, +25 name contains series, +20 count_of_issues
   >= requested issue number, +10 major publisher, +5/+5/+5 recency
   bonuses for start_year >= 2000/2010/2020 (in addition to the
   existing +100 year match and position bonus).

publisher field added to volumeFieldList so the major-publisher bonus
can be computed without an extra API call.

Tests: 100% coverage on internal/metadata/comicvine. New specs cover:
special-issue parsing, digital-suffix stripping, full scoreVolumes
heuristics table, filter-fallback happy path, filter-fallback error paths
(HTTP 5xx and malformed JSON), API key forwarding on /volumes/ request.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

zombor commented 2026-06-22 16:17:37 +00:00

Author

Owner

Copy link

Security Review — PR #690 (bd-bookshelf-23na)

Scope: ComicVine multi-step volume lookup + caching — internal/metadata/comicvine/provider.go, structured_test.go, export_test.go.

---

1. ReDoS — new title-parsing regexes

All three new regexes are safe.

• titleRE — pre-existing, unchanged. Anchored ^...$ with lazy (.*?) and literal # delimiter. RE2 linear.
• specialIssueRE — new. Anchored ^...$, lazy (.*?), fixed keyword alternation (annual|special|one-?shot), simple digit groups [0-9]+(?:\.[0-9]+)?. No nested quantifiers, no overlapping alternations. RE2 linear.
• stripSuffixRE — new. [\[(][^)\]]*(?:keyword)[^)\]]*[\])]. The [^)\]]* class explicitly excludes the terminators ) and ], so there is no path for the engine to re-enter the bracket after the keyword. RE2 linear. Verified empirically: a 10,002-char crafted non-matching string completes in <1ms.

Go's regexp package is RE2 — guaranteed linear-time — so catastrophic backtracking is architecturally impossible regardless of pattern shape.

No length cap on q.Title before regex. The only guard is q.Title == "". For a personal self-hosted server with RE2 this carries no practical exploit risk, but a MaxTitleLen constant (e.g. 512) would be belt-and-suspenders hygiene.

---

2. URL / Query injection — filter=name:<series>

volumeFilterSearch constructs:

params.Set("filter", "name:"+series)
...
reqURL = baseURL + "/volumes/?" + params.Encode()

url.Values.Encode() percent-encodes the entire value, including &, ?, =, ,, CRLF, and the colon itself. A crafted series name like batman&api_key=evil becomes filter=name%3Abatman%26api_key%3Devil — the injected fragment is data, not syntax. CRLF is similarly encoded to %0D%0A. No injection risk.

This pattern is consistent with the pre-existing fetchIssuesByFilter on main (params.Set("filter", fmt.Sprintf("volume:%d,issue_number:%s", ...)) + params.Encode()).

---

3. API key handling

• API key read from config at search time via injected getAPIKey(ctx) — not hardcoded.
• redactURLErrors() walks the full *url.Error chain and replaces api_key=… with REDACTED before any log or error-string emission. This applies to all new paths (volumeSearch, volumeFilterSearch) which both call doGetWithRetry — the existing redaction covers them automatically.
• API key not present in any log Info/Warn call in the new code.
• The new test ("API key is sent on /volumes/ request") confirms the key is forwarded correctly.

No API key leak risk in this diff.

---

4. SSRF

baseURL defaults to the hardcoded constant "https://comicvine.gamespot.com/api" and is only overridden in tests via httptest.NewServer. No user-supplied input flows into the host or scheme. No SSRF risk.

---

5. Response parsing / cache DoS

• doGetWithRetry uses io.LimitReader(resp.Body, maxBodyBytes) (2 MiB cap) before io.ReadAll. All new code paths (volumeSearch, volumeFilterSearch) share the same doGetWithRetry call — the cap is inherited automatically.
• json.Unmarshal on the capped buffer: 2 MiB of JSON cannot cause unbounded allocation with simple flat structs like volumeSearchResult.
• Volume cache (volumeCache) is an unbounded map[string]volumeCacheEntry. This is pre-existing on main (15 references, TTL-based expiry, no max-entries eviction). This PR does not change the cache implementation. Not flagged as a new finding; tracking as a pre-existing minor.

---

6. Error classification / domain boundary

No go-workflows or wfengine imports appear anywhere in the diff. Domain package returns plain errors (fmt.Errorf). Boundary maintained.

---

Verdict

No new security issues introduced by this diff.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #690 (bd-bookshelf-23na) **Scope:** ComicVine multi-step volume lookup + caching — `internal/metadata/comicvine/provider.go`, `structured_test.go`, `export_test.go`. --- ### 1. ReDoS — new title-parsing regexes **All three new regexes are safe.** - `titleRE` — pre-existing, unchanged. Anchored `^...$` with lazy `(.*?)` and literal `#` delimiter. RE2 linear. - `specialIssueRE` — new. Anchored `^...$`, lazy `(.*?)`, fixed keyword alternation `(annual|special|one-?shot)`, simple digit groups `[0-9]+(?:\.[0-9]+)?`. No nested quantifiers, no overlapping alternations. RE2 linear. - `stripSuffixRE` — new. `[\[(][^)\]]*(?:keyword)[^)\]]*[\])]`. The `[^)\]]*` class explicitly excludes the terminators `)` and `]`, so there is no path for the engine to re-enter the bracket after the keyword. RE2 linear. Verified empirically: a 10,002-char crafted non-matching string completes in <1ms. Go's `regexp` package is RE2 — guaranteed linear-time — so catastrophic backtracking is architecturally impossible regardless of pattern shape. **No length cap on `q.Title` before regex.** The only guard is `q.Title == ""`. For a personal self-hosted server with RE2 this carries no practical exploit risk, but a `MaxTitleLen` constant (e.g. 512) would be belt-and-suspenders hygiene. --- ### 2. URL / Query injection — `filter=name:<series>` `volumeFilterSearch` constructs: ```go params.Set("filter", "name:"+series) ... reqURL = baseURL + "/volumes/?" + params.Encode() ``` `url.Values.Encode()` percent-encodes the entire value, including `&`, `?`, `=`, `,`, CRLF, and the colon itself. A crafted series name like `batman&api_key=evil` becomes `filter=name%3Abatman%26api_key%3Devil` — the injected fragment is data, not syntax. CRLF is similarly encoded to `%0D%0A`. **No injection risk.** This pattern is consistent with the pre-existing `fetchIssuesByFilter` on `main` (`params.Set("filter", fmt.Sprintf("volume:%d,issue_number:%s", ...))` + `params.Encode()`). --- ### 3. API key handling - API key read from config at search time via injected `getAPIKey(ctx)` — not hardcoded. - `redactURLErrors()` walks the full `*url.Error` chain and replaces `api_key=…` with `REDACTED` before any log or error-string emission. This applies to all new paths (`volumeSearch`, `volumeFilterSearch`) which both call `doGetWithRetry` — the existing redaction covers them automatically. - API key not present in any log `Info`/`Warn` call in the new code. - The new test (`"API key is sent on /volumes/ request"`) confirms the key is forwarded correctly. **No API key leak risk in this diff.** --- ### 4. SSRF `baseURL` defaults to the hardcoded constant `"https://comicvine.gamespot.com/api"` and is only overridden in tests via `httptest.NewServer`. No user-supplied input flows into the host or scheme. **No SSRF risk.** --- ### 5. Response parsing / cache DoS - `doGetWithRetry` uses `io.LimitReader(resp.Body, maxBodyBytes)` (2 MiB cap) before `io.ReadAll`. All new code paths (`volumeSearch`, `volumeFilterSearch`) share the same `doGetWithRetry` call — the cap is inherited automatically. - `json.Unmarshal` on the capped buffer: 2 MiB of JSON cannot cause unbounded allocation with simple flat structs like `volumeSearchResult`. - **Volume cache** (`volumeCache`) is an unbounded `map[string]volumeCacheEntry`. This is **pre-existing on `main`** (15 references, TTL-based expiry, no max-entries eviction). This PR does not change the cache implementation. Not flagged as a new finding; tracking as a pre-existing minor. --- ### 6. Error classification / domain boundary No `go-workflows` or `wfengine` imports appear anywhere in the diff. Domain package returns plain errors (`fmt.Errorf`). **Boundary maintained.** --- ## Verdict No new security issues introduced by this diff. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-22 16:17:46 +00:00

Author

Owner

Copy link

CODE REVIEW — bookshelf-23na (PR #690)

Phase 0: DEMO Verification

No DEMO block is present in the PR body or bead comments. This is a library-only change (no CLI surface, no HTTP endpoint) so the test run is the operative verification. CI state: success (SHA 0225e89c). Proceeding under the library-only exception.

---

Phase 1: Spec Compliance

All seven requirements from the bead description are implemented:

• Special-issue pattern (Annual N, Special N, One-Shot N) — specialIssueRE at line 87, keyword folded into series name.
• Digital/bracketed suffix stripping — stripSuffixRE at line 93, applied before both regex paths.
• /api/volumes/?filter=name: fallback — volumeFilterSearch function, called from searchVolumes when primary returns 0 results.
• Full Grimmory calculateVolumeScore heuristics — scoreVolumes with +50/+25/+20/+10/+5×3 bonuses.
• Publisher field added to volumeFieldList — correct.
• Cache concurrency-safe — sync.Mutex in volumeCache.get/set.
• No workflow engine import — confirmed.

---

Phase 2: Code Quality

Regex correctness

titleRE (provider.go:82): Anchored ^…$, lazy (.*?), no nested quantifiers. Correctly handles Batman #18 #2 (lazy → last #), fractional issues (#1.5), letter suffixes (#18AU). No ReDoS risk.

specialIssueRE (provider.go:87): Anchored, fixed keyword alternation, no nested quantifiers. Correctly handles X-Men Annual 5, Batman One-Shot 3, year suffix. No ReDoS risk.

stripSuffixRE (provider.go:93): NOT anchored — ReplaceAll tries every character position. For input with an unmatched [ or ( and no closing bracket, the engine tries O(n) start positions with up to O(n) backtrack each → O(n²). In practice, comic title lengths are bounded (<200 chars) and the input is not adversarial, so the practical risk is negligible. Note for awareness only.

Multi-step search correctness

searchStructured (provider.go:390): cachedVolumeSearch → scoreVolumes → per-volume fetchIssuesByFilter → early-stop on yearMatch. The normalizeIssueNumber(issueNum) call is correctly moved before scoreVolumes so norm is passed to scoring. The fallback to searchFreeText when structured returns empty is correct.

searchVolumes split into volumeSearch + volumeFilterSearch: the fallback path is exercised by tests with atomic.Bool. Cache stores the empty-result case correctly (prevents thundering-herd re-queries for known-barren series).

Sort determinism

scoreVolumes uses insertion sort with strict > comparison — equal scores preserve original array order (stable). With volumeSearchLimit=25 max results from primary and 20 from fallback, all volumes have different position bonuses (25−i), so ties only occur if two volumes are at position ≥ 25 (impossible from primary) or if all other scores are also identical. Effectively deterministic. No flake risk.

Cache

• Keyed by strings.ToLower(strings.TrimSpace(series)) — correct normalization.
• Concurrency-safe via sync.Mutex in get/set.
• No size cap — unbounded growth for large libraries with many series. Pre-existing issue (cache struct was on main already); the new code does not make it worse. Not a blocker.
• Cache-hit test (structured_test.go:493) uses atomic.Int32 to count volume search calls, asserts Equal(int32(1)) after two same-series queries. Correctly proves the 2nd call makes no HTTP volume request. ✓

Tests

• HTTP boundary mocked via httptest.NewServer — correct, no interface mocks.
• ScoreVolumes table covers all seven bonus types individually.
• Filter-fallback tests: happy path, 5xx propagation, malformed JSON, API key forwarding. Complete.
• Special-issue integration test ("X-Men Annual 5") covers the full path to issue result.
• One-Expect-per-It upheld throughout (multi-Expect relaxation in JustBeforeEach results used correctly).

---

Findings

[MINOR] internal/metadata/comicvine/provider.go (scoreVolumes) — Year-match bonus uses exact string equality; bead spec says ±1 tolerance
Bead description (referencing Grimmory): "+100 if start_year matches extracted year (±1)". Implementation uses v.StartYear == year (exact string match). A title with year 2012 vs a volume with start_year=2011 gets 0 instead of +100. Degrades ranking quality for off-by-one years but does not select the wrong series. Follow-up bead if desired.

[MINOR] internal/metadata/comicvine/provider.go:93 (stripSuffixRE) — stripSuffixRE drops the year when it is embedded inside the digital parenthetical
Input "Batman #1 (2016 Digital Edition)" → strip removes the entire (2016 Digital Edition) group → cleaned "Batman #1" → year parsed as "". The inline comment says this gives year="2016" but that is only true when the year appears outside the bracket: "Batman #1 [Digital Edition] (2016)". The behavior is acceptable (falls back to unbosted scoring) but the comment is misleading and no test covers the year-embedded variant.

[MINOR] PR #690 description — No DEMO block
Library-only change with no runnable CLI/HTTP surface; CI green is the operative verification. Process note only.

---

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## CODE REVIEW — bookshelf-23na (PR #690) ### Phase 0: DEMO Verification No DEMO block is present in the PR body or bead comments. This is a library-only change (no CLI surface, no HTTP endpoint) so the test run is the operative verification. CI state: **success** (SHA `0225e89c`). Proceeding under the library-only exception. --- ### Phase 1: Spec Compliance All seven requirements from the bead description are implemented: - Special-issue pattern (`Annual N`, `Special N`, `One-Shot N`) — `specialIssueRE` at line 87, keyword folded into series name. - Digital/bracketed suffix stripping — `stripSuffixRE` at line 93, applied before both regex paths. - `/api/volumes/?filter=name:` fallback — `volumeFilterSearch` function, called from `searchVolumes` when primary returns 0 results. - Full Grimmory `calculateVolumeScore` heuristics — `scoreVolumes` with +50/+25/+20/+10/+5×3 bonuses. - Publisher field added to `volumeFieldList` — correct. - Cache concurrency-safe — `sync.Mutex` in `volumeCache.get`/`set`. - No workflow engine import — confirmed. --- ### Phase 2: Code Quality #### Regex correctness **titleRE** (`provider.go:82`): Anchored `^…$`, lazy `(.*?)`, no nested quantifiers. Correctly handles `Batman #18 #2` (lazy → last `#`), fractional issues (`#1.5`), letter suffixes (`#18AU`). No ReDoS risk. **specialIssueRE** (`provider.go:87`): Anchored, fixed keyword alternation, no nested quantifiers. Correctly handles `X-Men Annual 5`, `Batman One-Shot 3`, year suffix. No ReDoS risk. **stripSuffixRE** (`provider.go:93`): NOT anchored — `ReplaceAll` tries every character position. For input with an unmatched `[` or `(` and no closing bracket, the engine tries O(n) start positions with up to O(n) backtrack each → O(n²). In practice, comic title lengths are bounded (<200 chars) and the input is not adversarial, so the practical risk is negligible. Note for awareness only. #### Multi-step search correctness `searchStructured` (`provider.go:390`): `cachedVolumeSearch` → `scoreVolumes` → per-volume `fetchIssuesByFilter` → early-stop on `yearMatch`. The `normalizeIssueNumber(issueNum)` call is correctly moved before `scoreVolumes` so `norm` is passed to scoring. The fallback to `searchFreeText` when structured returns empty is correct. `searchVolumes` split into `volumeSearch` + `volumeFilterSearch`: the fallback path is exercised by tests with `atomic.Bool`. Cache stores the empty-result case correctly (prevents thundering-herd re-queries for known-barren series). #### Sort determinism `scoreVolumes` uses insertion sort with strict `>` comparison — equal scores preserve original array order (stable). With `volumeSearchLimit=25` max results from primary and 20 from fallback, all volumes have different position bonuses (25−i), so ties only occur if two volumes are at position ≥ 25 (impossible from primary) or if all other scores are also identical. Effectively deterministic. No flake risk. #### Cache - Keyed by `strings.ToLower(strings.TrimSpace(series))` — correct normalization. - Concurrency-safe via `sync.Mutex` in `get`/`set`. - No size cap — unbounded growth for large libraries with many series. Pre-existing issue (cache struct was on `main` already); the new code does not make it worse. Not a blocker. - Cache-hit test (`structured_test.go:493`) uses `atomic.Int32` to count volume search calls, asserts `Equal(int32(1))` after two same-series queries. Correctly proves the 2nd call makes no HTTP volume request. ✓ #### Tests - HTTP boundary mocked via `httptest.NewServer` — correct, no interface mocks. - `ScoreVolumes` table covers all seven bonus types individually. - Filter-fallback tests: happy path, 5xx propagation, malformed JSON, API key forwarding. Complete. - Special-issue integration test (`"X-Men Annual 5"`) covers the full path to issue result. - One-Expect-per-It upheld throughout (multi-Expect relaxation in `JustBeforeEach` results used correctly). --- ### Findings [MINOR] `internal/metadata/comicvine/provider.go` (scoreVolumes) — Year-match bonus uses exact string equality; bead spec says ±1 tolerance Bead description (referencing Grimmory): "+100 if start_year matches extracted year (±1)". Implementation uses `v.StartYear == year` (exact string match). A title with year 2012 vs a volume with `start_year=2011` gets 0 instead of +100. Degrades ranking quality for off-by-one years but does not select the wrong series. Follow-up bead if desired. [MINOR] `internal/metadata/comicvine/provider.go:93` (stripSuffixRE) — stripSuffixRE drops the year when it is embedded inside the digital parenthetical Input `"Batman #1 (2016 Digital Edition)"` → strip removes the entire `(2016 Digital Edition)` group → cleaned `"Batman #1"` → year parsed as `""`. The inline comment says this gives `year="2016"` but that is only true when the year appears *outside* the bracket: `"Batman #1 [Digital Edition] (2016)"`. The behavior is acceptable (falls back to unbosted scoring) but the comment is misleading and no test covers the year-embedded variant. [MINOR] PR #690 description — No DEMO block Library-only change with no runnable CLI/HTTP surface; CI green is the operative verification. Process note only. --- REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor force-pushed bd-bookshelf-23na from 0225e89cdf to aa8ffec09b 2026-06-22 16:23:59 +00:00 Compare

zombor force-pushed bd-bookshelf-23na from aa8ffec09b to 022bfe6561 2026-06-22 16:29:58 +00:00 Compare

zombor merged commit 195c9156ae into main 2026-06-22 16:33:37 +00:00

zombor referenced this pull request from a commit 2026-06-22 16:33:39 +00:00

Merge pull request 'feat(comicvine): port Grimmory full scoring + special-issue + filter-fallback (bookshelf-23na)' (#690) from bd-bookshelf-23na 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!690

No description provided.