fix: do not read a four-digit issue number as a publication year - #541
Conversation
The date check scans a filename for a standalone four-digit number in the 1900-2099 range and treats it as the year the issue was published. "Topolino 1904.cbz" is issue #1904 of a run that has passed allaboutduncan#3,600, so the check compares 1904 against the issue's real date of 1992-05-24, calls an 88-year gap a conflict, and under `enforce` drops a match that was correct. The log then blames a date conflict, which points the investigation the wrong way. Nothing in the string separates the two cases. "Topolino 1904.cbz" and "Batman 001 (2016).cbz" both put four digits between a non-alphanumeric on each side, which is exactly what _YEAR looks for, and the existing guards already handle everything that can be told apart from the filename alone -- "Hal2008" has a letter before, "1920px" a letter after, "v2004" a letter before. The only thing that distinguishes a year from an issue number here is that the caller has already read those digits as the issue number, and the parser was never told. So tell it. issue_year_from_filename and evaluate take an optional issue_number, and a candidate year equal to it is discarded. The three call sites that run the check -- accept_match and the search-metadata path in routes/metadata.py, and _date_conflicted in core/bulk_metadata.py -- all have the number in scope already. Callers that pass nothing get exactly the old behaviour. The candidate is discarded before the "exactly one year" test rather than after, which helps in both directions: "Topolino 1904.cbz" loses its only candidate so the check abstains instead of rejecting, and "Topolino 1904 (1992).cbz" drops from two candidates to one, so a file the check used to skip as ambiguous is now checked against the year it actually states. This can only turn a conflict into no opinion, or no opinion into a real comparison. It cannot produce a new false rejection, because the only year it removes is one the caller has already committed to reading as the issue number. Measured over a 5,369-file library with the check enforcing. Of the files carrying an issue number, 4,406 parse identically, 49 lose a year and 58 gain one the check could not use before. Scored against the matched issue's real date: 36 were being rejected and are now accepted, 10 lose a check whose verdict changed nothing about what was written, and one flips from accepted to rejected -- correctly, catching a file whose issue number is itself mis-parsed. The cost is a publication numbered by its year: a yearbook such as "L'economia di Zio Paperone 1992", issue 1992 published in 1992, loses the check rather than passing it. Nothing is written differently, since a skipped check and a passed check both write the metadata, and the filename cannot tell that case from the broken one. There is a test pinning it so it stays a decision. Closes allaboutduncan#540 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2zch6Fz6z2WxRnVNCa2XZ
|
Reviewed. The diagnosis is right and the approach is the correct one — the caller knowing the issue number really is the only thing that can separate 1. Discarding before the count can create new false rejectionsThe docstring and the PR body both state the invariant:
That doesn't hold when the filename carries a second year that isn't the publication year — a scan or release year, which is endemic in exactly the Italian scene releases this targets. On this branch: Issue #1904 is dated 1992-05, so under The cause is the placement. Removing the issue-number candidate before Suggested change: apply the discard after the count, so it can only ever produce abstention: plausible = {year for year in years if 1900 <= year <= upper_bound}
if len(plausible) != 1:
return None
year = plausible.pop()
return None if _is_the_issue_number(year, issue_number) else yearThat still fixes #540 in full — If you'd rather keep the promotion, then the docstring and PR body need the invariant corrected rather than restated — the next reader will otherwise rely on a guarantee that isn't there. 2.
|
Two blocking issues from review on allaboutduncan#541. 1. Discarding the issue-number candidate before the "exactly one year" count could promote an ambiguous filename to a confident-but-wrong one, instead of only ever turning a conflict into abstention. Italian scene releases routinely carry a second year that is a scan or reprint date, not the publication date: issue_year_from_filename("Topolino 1904 (c2c) (2008).cbz", "1904") used to return 2008 -- issue #1904 is dated 1992-05, so under `enforce` that reported a 16-year conflict and threw away a match main accepted, the same false-rejection shape the PR exists to fix. The discard now happens after the candidate count, so it can only ever turn a single confident-but-wrong candidate into abstention; an ambiguous filename stays ambiguous whether or not one of its years happens to equal the issue number. This gives up the "Topolino 1904 (1992).cbz" -> 1992 improvement described in the PR body -- that file abstains again, same as before this branch -- but allaboutduncan#540 itself (`Topolino 1904.cbz` alone) is unaffected, since a single-candidate filename was never at risk. 2. `_is_the_issue_number` required `str.isdigit()` on the whole issue number, so a decimal or lettered point issue ("1904.1", "1904.MU") was never recognised as the issue number and kept the exact bug this PR fixes -- "." is non-alphanumeric, so `_YEAR` matches "1904" inside "Topolino 1904.1.cbz" same as it matches "Topolino 1904.cbz". It now compares against the issue number's leading integer run instead. That also closes a smaller hole the docstring's "pure and never raises" promised against: `str.isdigit()` is True for digit characters `int()` rejects (superscript '²', circled '①'); a regex match on `\d` doesn't match those characters, so it never reaches `int()` on them. Also addresses both non-blocking notes: - `routes/metadata.py`'s two enforcement sites already passed `issue_number` through, but had no regression guard -- app.py can't be imported in tests, so nothing pinned it structurally. Added `test_both_paths_pass_the_issue_number_to_the_date_check`, alongside the existing provider-argument guard. - `core/bulk_metadata.py:_date_conflicted` took the provider's own `issue.issue_number` rather than the filename-derived text the caller already has in scope. Equivalent today only because both callers select `issue` via `issues_by_norm`, keyed on the filename's normalised number -- an invisible coupling. It now takes `issue_text` explicitly and both call sites pass it. Tests: scan-year abstention (`test_a_second_year_that_is_a_scan_credit_stays_a_conflict_free_abstention`), the corrected two-candidate invariant (`test_a_second_candidate_stays_ambiguous_even_once_the_issue_number_is_known`), point-issue numbers (`test_a_decimal_or_lettered_point_issue_is_still_the_issue_number`), non-ASCII digits never raising (`test_a_non_ascii_digit_never_raises`), the routes/metadata.py wiring guard, and bulk_metadata now exercising `issue_text` against a deliberately-different `issue.issue_number` (`test_uses_the_filename_issue_number_not_the_providers`). Full suite: 4,622 passed (excluding 11 pre-existing failures in tests/unit/test_monitor.py unrelated to this change -- a macOS pytest tmp-dir path containing a space that a permission check rejects, reproduced identically on 317873a before this commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8MJHCSmP9Sx7GARqxLShc
|
Thanks for the thorough review — pushed 1af4801 addressing both blockers and both notes. 1. Discarding before the countFixed as suggested: the discard now happens strictly after the "exactly one candidate" count. plausible = {year for year in years if 1900 <= year <= upper_bound}
if len(plausible) != 1:
return None
year = plausible.pop()
return None if _is_the_issue_number(year, issue_number) else yearConfirmed against your repro: issue_year_from_filename("Topolino 1904 (c2c) (2008).cbz") # -> None
issue_year_from_filename("Topolino 1904 (c2c) (2008).cbz", "1904") # -> None (was 2008)I went with correcting the invariant rather than keeping the promotion, per your suggestion — it's the smaller, provable claim, and #540 itself only ever needed the single-candidate case. The "ambiguous → checked" improvement for 2.
|
main landed the same issue-number exemption as part of the INDUCKS provider (allaboutduncan#539), so core/metadata_dates.py conflicted in five hunks with two independent implementations of one idea. Both discard a sole candidate year that equals the issue number, and both do it after the "exactly one candidate" count -- the invariant this branch's review turned on -- so the conflict is over form, not behaviour. Resolved to this branch's version throughout, which is a superset: - _is_the_issue_number compares the issue number's leading integer run rather than requiring str.isdigit() on the whole string, so a decimal or lettered point issue ("1904.1", "1904.MU") is recognised too. main's inline int(str(issue_number).strip()) raises on those and swallows it, leaving the exact bug this branch exists to fix -- "." is non-alphanumeric, so _YEAR matches "1904" inside "Topolino 1904.1.cbz" the same as in "Topolino 1904.cbz". - The docstrings record why the discard has to happen after the count and what the fix deliberately gives up (a yearbook numbered by its year loses the check). main's own addition to the file, "inducks" joining _ISSUE_YEAR_PROVIDERS, is untouched. routes/metadata.py, core/bulk_metadata.py and the three test files merged without conflict; main's INDUCKS call sites already pass issue_number to issue_year_from_filename, and the wiring guard added here covers the two enforcement sites unchanged. Tests: 4,726 passed, 7 skipped. The 10 failures in tests/routes/test_folder_access.py and test_library_access.py reproduce identically on a clean origin/main worktree at a8fedb9 and are unrelated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q1t9fNhnZEymgFbZ24MkzF
📝 Description
Closes #540
The date check reads a standalone four-digit number in the 1900–2099 range as a publication year.
Topolino 1904.cbzis issue #1904 of a run that has passed #3,600, so the check compares 1904against the issue's real date of 1992-05-24, calls the 88-year gap a conflict, and under
enforcedrops a match that was correct. The log then blames a date conflict, which points the
investigation the wrong way.
Nothing in the string separates the two cases.
Topolino 1904.cbzandBatman 001 (2016).cbzboth put four digits between a non-alphanumeric on each side, which is exactly what
_YEARlooksfor. The existing guards already handle everything that can be told apart from the filename
alone —
Hal2008has a letter before,1920pxa letter after,v2004a letter before. The onething distinguishing a year from an issue number here is that the caller has already read those
digits as the issue number, and the parser was never told.
So tell it.
issue_year_from_filenameandevaluatetake an optionalissue_number, and acandidate year equal to it is discarded. The three call sites that run the check —
accept_matchand the search-metadata path inroutes/metadata.py, and_date_conflictedincore/bulk_metadata.py— all have the number in scope already. A caller that passes nothing getsexactly the old behaviour, which is what keeps this safe to land.
The candidate is discarded before the "exactly one year" test rather than after, and thatplacement earns its keep in both directions:
Corrected: the candidate is discarded after the "exactly one year" test, not before it —
so it can only ever turn a single confident-but-wrong candidate into abstention, never manufacture
a new conflict out of an ambiguous filename:
Topolino 1904.cbznames one candidate, 1904, which is the issue number, so the check abstainsinstead of rejecting. This is the whole fix for Date check reads a four-digit issue number as a year, rejecting correct matches #540.
No longer true, andTopolino 1904 (1992).cbzgoes from two candidates to one, so a file the check used to skip asambiguous is now checked against the year it actually states.
deliberately given up: a filename naming two years stays ambiguous and the check still
abstains, whether or not one of the two happens to equal the issue number — see the review
discussion on
Topolino 1904 (c2c) (2008).cbzfor why promoting it would create a falserejection instead.
The change can only turn a conflict into no opinion, or no opinion into a real comparison. It
cannot produce a new false rejection, because the only year it removes is one the caller has
already committed to reading as the issue number, and it is only ever removed once that year is
already the sole candidate.
What it costs. A publication numbered by its year — a yearbook such as L'economia di Zio
Paperone 1992, issue 1992 published in 1992 — loses the check rather than passing it. Nothing is
written differently for those files, since a skipped check and a passed check both write the
metadata, and the filename cannot tell that case from the broken one. There is a test pinning it,
so it stays a recorded decision rather than a later surprise.
This is not specific to any provider:
core/metadata_dates.pyis shared, so GCD, ComicVine andMetron are affected equally. It stays invisible in a library of American comics and is unavoidable
in one holding Topolino, Diabolik, Tex or Dylan Dog.
🛠️ Changes Made
docker build -t dev .)Four files plus tests: the parser and
evaluateincore/metadata_dates.py, the two call sites inroutes/metadata.py(now guarded by a wiring regression test), one incore/bulk_metadata.py(now passing the filename-derived issue number instead of the provider's own copy of it).
🧪 Testing Performed
devcontainerBefore and after, in two containers with identical inputs: a local ComicVine SQLite database
holding volume "Topolino" with an issue numbered
1904dated1992-05-24, one file/data/Topolino/Topolino 1904.cbz, acvinfonaming the volume, anddate_check_modeset toenforce.On
mainat 7e0f867 the correct match is thrown away:{"processed": 0, "errors": 1, "details": [ {"file": "Topolino 1904.cbz", "status": "error", "reason": "date conflict", "date_conflict": true}]}On this branch, same database, same file, same enforce setting:
{"processed": 1, "errors": 0, "details": [ {"file": "Topolino 1904.cbz", "status": "success", "source": "ComicVine (Local DB)"}]}Every pre-existing test calls
issue_year_from_filename(name)with no number, so it takes thedefault and hits the old path unchanged. Backwards compatibility is demonstrated by the untouched
suite rather than asserted.
Beyond that, the old and new parsers were run side by side over a real 5,369-file library, and
every file whose parse changed was scored against the matched issue's actual publication date:
That single flip is correct, and worth spelling out because it looks like a regression:
Topolino 0330 (Mondadori 1962-03-25) [c2c Hal 2008 & Bibbo64].cbr.extract_issue_numberreturns
2008rather than330here, because it strips parentheses but not square brackets, sothe file matches issue #2008 — published 1994 — when it is issue #330 from 1962. Previously the
filename named two years, the check abstained, and the wrong metadata was written silently. Now
2008 is discarded as the issue number, 1962 remains, and the mismatch is caught.
That bracket-parsing detail is a separate bug and this PR does not touch it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01C2zch6Fz6z2WxRnVNCa2XZ
https://claude.ai/code/session_01Q8MJHCSmP9Sx7GARqxLShc