Review follow-ups: PR #531 — feat(import): TypeSafe Jev re-ranks ambiguous import rows (flag-gated, off by default) #778

New Issue

2026-09-22T20:26:17Z

gambit-admin commented

2026-09-22 20:26:17 +00:00

Non-blocking findings from the auto code review of PR #531 (feat(import): TypeSafe Jev re-ranks ambiguous import rows (flag-gated, off by default)) — captured for batch cleanup rather than blocking the PR.
Commit 104dd93 · https://github.com/Gambit-Inc/gambit/pull/531

SHOULD-FIX:

backend/app/services/import_match.py:722 (and the promotion at :731-732) — For a row whose status is already "confirm" (e.g. cert+price, graded-price-confirm, widened-set, price-confirm), the low-confidence guard (conf < rank_floor and order[0].justtcg_id not in ladder_ids) only blocks picks that fall outside the ladder's own pool. A low-confidence Jev pick that happens to land inside the ladder pool is not blocked at all: match.candidates = order[:6] and match.chosen = order[0] execute unconditionally once match.status == "confirm" is already true, regardless of conf. This lets an unmeasured, low-confidence Jev guess (e.g. conf=0.1) silently displace the price-ranked pre-selection — the thing the module's own docstring calls "the load-bearing rule" (price ranks; it never decides) — as the candidate shown first to the human reviewer. No test exercises this path (JevRanks/JevAutoIsGated only cover the manual→confirm promotion case and the outside-pool low-confidence case); a status=="confirm" row at low confidence with an in-pool pick is untested.
backend/app/services/import_match.py:789-799 — the (game, title) widen cache is populated (an actual search_local DB call inside a SAVEPOINT) whenever a title is seen for the first time in the chunk, even for a cert-numbered row whose own wide result is unconditionally discarded two lines later (if rec.get("cert_number"): wide = []). If a batch happens to have several distinct titles whose first occurrence in rows is a cert row, this burns real time against the shared widen_until/deadline budget for a result nobody uses, which can push the time.monotonic() >= widen_until check to break before later, genuinely eligible non-cert rows are even considered — a silent reduction in Jev coverage for that chunk with no signal surfaced.

NITS:

backend/app/config.py:474-475 — import_jev_timeout_seconds (default 5) is unchecked against import_jev_budget_seconds (default 15). If someone ever configures timeout_seconds >= budget_seconds, widen_until = deadline - timeout_seconds in _jev_rerank (import_match.py:781) comes out ≤ "now", so the very first iteration's time.monotonic() >= widen_until is already true and the loop breaks before asking anything — Jev silently never fires for that run, with no warning that the config is self-defeating.

Filed from a Claude Code session

Non-blocking findings from the auto code review of **PR #531** (feat(import): TypeSafe Jev re-ranks ambiguous import rows (flag-gated, off by default)) — captured for batch cleanup rather than blocking the PR. Commit `104dd93` · https://github.com/Gambit-Inc/gambit/pull/531 SHOULD-FIX: - `backend/app/services/import_match.py:722` (and the promotion at `:731-732`) — For a row whose status is already `"confirm"` (e.g. `cert+price`, `graded-price-confirm`, `widened-set`, `price-confirm`), the low-confidence guard (`conf < rank_floor and order[0].justtcg_id not in ladder_ids`) only blocks picks that fall **outside** the ladder's own pool. A low-confidence Jev pick that happens to land **inside** the ladder pool is not blocked at all: `match.candidates = order[:6]` and `match.chosen = order[0]` execute unconditionally once `match.status == "confirm"` is already true, regardless of `conf`. This lets an unmeasured, low-confidence Jev guess (e.g. conf=0.1) silently displace the price-ranked pre-selection — the thing the module's own docstring calls "the load-bearing rule" (price ranks; it never decides) — as the candidate shown first to the human reviewer. No test exercises this path (`JevRanks`/`JevAutoIsGated` only cover the manual→confirm promotion case and the outside-pool low-confidence case); a `status=="confirm"` row at low confidence with an in-pool pick is untested. - `backend/app/services/import_match.py:789-799` — the `(game, title)` widen cache is populated (an actual `search_local` DB call inside a SAVEPOINT) whenever a title is seen for the first time in the chunk, even for a cert-numbered row whose own `wide` result is unconditionally discarded two lines later (`if rec.get("cert_number"): wide = []`). If a batch happens to have several distinct titles whose *first* occurrence in `rows` is a cert row, this burns real time against the shared `widen_until`/`deadline` budget for a result nobody uses, which can push the `time.monotonic() >= widen_until` check to `break` before later, genuinely eligible non-cert rows are even considered — a silent reduction in Jev coverage for that chunk with no signal surfaced. NITS: - `backend/app/config.py:474-475` — `import_jev_timeout_seconds` (default 5) is unchecked against `import_jev_budget_seconds` (default 15). If someone ever configures `timeout_seconds >= budget_seconds`, `widen_until = deadline - timeout_seconds` in `_jev_rerank` (import_match.py:781) comes out ≤ "now", so the very first iteration's `time.monotonic() >= widen_until` is already true and the loop breaks before asking anything — Jev silently never fires for that run, with no warning that the config is self-defeating. --- Filed from a Claude Code session

gambit-admin added the enhancement label 2026-09-22 20:26:17 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gambit/gambit#778