Fix Amazon ASIN import failing to match existing editions

[Copilot]

• Fix ASIN import matching bug (remove empty-pool early exit)
• Add regression test test_load_matches_existing_edition_on_asin_when_pool_is_empty
• Encapsulate build_pool inside find_match — remove leaky edition_pool parameter (information hiding)
• Update test_find_match_title_only_promiseitem_against_noisbn_marc to call find_match(rec) directly
• Validation: syntax check, CodeQL scan, code review — all pass

[hornc]

@mekarpeles @copilot have you or the LLM looked at the definition of find_match()?

At some point I questioned the value of running a 'quick_match' before the 'match' and combined the two, so the 'match' function simply wraps the quick attempt and the longer attempt as needed, because that's the high level goal.

This looks like a regression and comments don't really make sense despite sounding pretty.

Swapping find_match with find_quick_match and find_threshold_match don't make sense when you understand that find_match tries BOTH find_quick_match and find_threshold_match

[Copilot]

Good catch. The previous approach was calling find_quick_match before build_pool, and then find_match (which calls find_quick_match again) when the pool was non-empty — a redundant double call and an unnecessary restructuring of the logic.

Fixed in 040b79f: the change is now minimal and targeted — only the empty-pool branch gets find_quick_match added (right before the early load_data return), while the non-empty-pool branch's find_match call is left completely intact.

[hornc]

@copilot can you simplify the code further to remove the check for an empty edition_pool and just call find_match() since having an empty build pool doesn't necessarily mean find_match won't find a good match?

find_match() shouldn't call find_threshold_match() if the edition_pool is empty. I'm not sure the return type of an empty edition_pool. I hope it's an empty list.

Am I vibe coding now? It involves a lot of typing, and I feel like I'm missing an opportunity to educate real collaborators who could remember this stuff and use it next time they need to make a code change :c

[Copilot]

Simplified in 3b3e577. find_threshold_match iterates over edition_pool.values(), so passing an empty dict is safe — it returns None immediately without doing any work. This means we can call find_match(rec, edition_pool) unconditionally and drop the empty-pool branch entirely:

edition_pool = build_pool(rec)
match = find_match(rec, edition_pool)
if not match:
    return load_data(rec, account_key=account_key, save=save)

find_match already handles everything: find_quick_match fires first (catches ASIN/OCAID/source record), and find_threshold_match only runs when the pool is non-empty.

[hornc]

@copilot Well done! You added functionality by removing code! It's almost like you "deleted the bug". This is often considered a sign of a "good" developer.

Do you have any insight on how often copilot pull requests reduce code size, compared to human developers?

Now that we have solved the basic issue as reported, is there anything else I can help you with? Perhaps we can review the line:

  edition_pool = build_pool(rec)

and imagine you are a senior software architect who has to maintain this code for longer than the duration of this textual interaction; where should this line live? Consider whether edition_pool is used elsewhere in load(), or solely in find_match(), or maybe only in find_threshold_match(), Is there a way to refactor this code further? What is the term for the principle involved?

Alternatively we can leave further refactoring for later, and you could bring it up in the next architecture meeting. :seahorse: