Skip to content

fix(hltb): keep a pinned HowLongToBeat match through a rescan - #4262

Merged
gantoine merged 4 commits into
rommapp:masterfrom
sdornan:claude/beautiful-colden-2f5983
Aug 22, 2026
Merged

fix(hltb): keep a pinned HowLongToBeat match through a rescan#4262
gantoine merged 4 commits into
rommapp:masterfrom
sdornan:claude/beautiful-colden-2f5983

Conversation

@sdornan

@sdornan sdornan commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Description

A scan could silently discard a HowLongToBeat match the user had pinned by hand.

fetch_hltb_rom in backend/handler/scan_handler.py called meta_hltb_handler.get_rom(fs_name, platform_slug) unconditionally, so an UPDATE scan re-ran the fuzzy filename search even when rom.hltb_id was already set. Any other game scoring above the similarity floor then replaced the pinned ID.

HLTBHandler.get_rom_by_id has existed since #2926 was fixed, but only the Edit ROM path ever called it; the scan path never did. This restores the ID as the source of truth on a rescan: an ID already on the ROM is refreshed by ID, never traded for a filename guess.

What changed

  • New module-level resolve_hltb_rom(rom, fs_name, platform_slug, scan_type), placed beside resolve_launchbox_rom. It calls get_rom_by_id on an UPDATE scan when hltb_id is set, and on an UNMATCHED scan when the ID is set but hltb_metadata is empty; otherwise it falls through to the filename lookup.
  • fetch_hltb_rom delegates to it. The outer gate is untouched, so which scans consult HLTB at all is unchanged, only the choice of lookup inside it.
  • New backend/tests/handler/test_resolve_hltb_rom.py, five cases mirroring the existing LaunchBox suite.

Why extract instead of inlining

The sibling providers (Flashpoint, MobyGames, ScreenScraper, SteamGridDB) all express this same gate inline, and matching them would have been a smaller diff. I extracted instead because the closure captures rom, rom_attrs, platform, and scan_type from scan_rom's frame, so there is no way to test it without standing up the whole scan pipeline. This bug has now surfaced twice (here, and for the new Steam provider in #4241), which seemed worth a regression test. resolve_launchbox_rom is the existing precedent for this shape.

I verified the tests actually pin the bug: reverting the function body to the old single get_rom call fails three of the five.

Not in scope

Flashpoint, MobyGames, ScreenScraper, and SteamGridDB still have this logic inline and untested. This change does not make them worse, but giving them the same guard would be a separate pass.

Verification

  • uv run pytest tests/handler -q — 1247 passed, 1 failed. The single failure is tests/handler/auth/test_auth.py::test_hybrid_auth_backend_with_refresh_token, a redis.exceptions.ConnectionError from having no local Redis; it is unrelated to this change.
  • trunk fmt and trunk check --filter=ruff,black,isort,mypy,bandit on both files — clean.

AI assistance disclosure

This PR was written by Claude Code (Opus 5), including the code, the tests, and this description. A human reviewed the change before it was opened.

Checklist

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

fetch_hltb_rom called the fuzzy filename search unconditionally, so an
UPDATE scan re-guessed the match even when rom.hltb_id was already set. Any
other game scoring above the similarity floor replaced an ID the user had
pinned by hand in Edit ROM.

get_rom_by_id has existed since rommapp#2926 was fixed, but only the Edit ROM path
called it. Extract resolve_hltb_rom as a module-level function, following
resolve_launchbox_rom, so the ID-first branch is covered by tests rather
than trapped in a closure over scan_rom's frame.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 21, 2026 23:06
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extracts HLTB match resolution and uses stored IDs for UPDATE scans and selected UNMATCHED scans so pinned matches survive rescanning.

  • Adds a dedicated resolve_hltb_rom helper beside the existing LaunchBox resolver.
  • Adds focused tests for filename, UPDATE, and UNMATCHED resolution behavior.
  • COMPLETE scans remain outside the stored-ID branch and can still discard pinned matches.

Confidence Score: 4/5

The COMPLETE rescan path can still discard a manually pinned HLTB match and should be fixed before merging.

COMPLETE scans reach the new resolver but bypass its stored-ID lookup, while COMPLETE initialization omits the existing ID and persists either the fuzzy replacement or no match.

Files Needing Attention: backend/handler/scan_handler.py, backend/tests/handler/test_resolve_hltb_rom.py

Important Files Changed

Filename Overview
backend/handler/scan_handler.py Adds ID-aware HLTB resolution, but COMPLETE scans still fall through to fuzzy filename matching and can replace or clear a pinned ID.
backend/tests/handler/test_resolve_hltb_rom.py Covers UPDATE and UNMATCHED behavior but omits the reachable COMPLETE-with-stored-ID case that exposes the remaining defect.

Fix all with Greploop Fix All in Claude Code

Prompt To Fix All With AI
### Issue 1
backend/handler/scan_handler.py:344-347
**Complete scans bypass pinned IDs**

When a COMPLETE scan processes a ROM with a stored `hltb_id`, this condition skips `get_rom_by_id` and falls through to fuzzy filename matching, causing the manually pinned match to be replaced by another result or cleared when no filename match is found.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix(hltb): keep a pinned HowLongToBeat m..." | Re-trigger Greptile

Comment thread backend/handler/scan_handler.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes the HLTB scan path so that an UPDATE (and certain UNMATCHED) scan refreshes an existing rom.hltb_id via get_rom_by_id instead of re-running the fuzzy filename search, preventing a user-pinned HLTB match from being silently replaced during rescans.

Changes:

  • Added resolve_hltb_rom(...) helper to centralize the “prefer ID refresh vs filename lookup” decision for HLTB during scans.
  • Updated fetch_hltb_rom to delegate to resolve_hltb_rom (without changing the outer gating for when HLTB is consulted at all).
  • Added a new unit test module covering key resolver behaviors across scan types.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
backend/handler/scan_handler.py Adds resolve_hltb_rom and routes scan-time HLTB resolution through it to preserve pinned IDs on update-style rescans.
backend/tests/handler/test_resolve_hltb_rom.py Adds focused unit tests for the HLTB resolver decision logic (ID refresh vs filename lookup).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread backend/handler/scan_handler.py Outdated
sdornan and others added 3 commits August 21, 2026 18:26
The docstring read as though a stored ID always wins, but the ID branch is
scoped to UPDATE and to UNMATCHED with no metadata. Say so, and cover the
COMPLETE fall-through with a test so the exclusion reads as deliberate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The helper had a single caller, so the stored-ID branch now sits in
fetch_hltb_rom like the Flashpoint and MobyGames fetchers. Its tests move
with it and drive the same six cases through scan_rom.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gantoine
gantoine merged commit 57badfd into rommapp:master Aug 22, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants