Skip to content

Read stored ImageEvidence first in the lands identifier - #391

Merged
WilfordGrimley merged 2 commits into
masterfrom
worktree-agent-a4707815350f91f3b
Jul 23, 2026
Merged

Read stored ImageEvidence first in the lands identifier#391
WilfordGrimley merged 2 commits into
masterfrom
worktree-agent-a4707815350f91f3b

Conversation

@WilfordGrimley

@WilfordGrimley WilfordGrimley commented Jul 23, 2026

Copy link
Copy Markdown

Description

Issue #359, Phase 1 of the owner-approved post-2026-07-23 plan. Today
local_lands_identify fetches a fresh image and runs fresh OCR for
every candidate card, ignoring the harvest-calculate pipeline's own
ImageEvidence rows — even though the issue's own live sizing found
99.6% of the 1,609 unresolved basic-land cards already carry a
current one. This is a data-source patch only, never a logic change:

  • _current_evidence_for_card — the currency check (content_hash
    matches the card's live content_phash, plus both the
    collector_line_ocr/artist_ocr extractor-version keys present) —
    same shape local_calculate_verdicts's three Stage D calculators
    already use.
  • _ocr_result_from_evidence — step 1's evidence-first replacement
    for run_ocr_for_card: reconstructs an OcrParseResult from
    already-persisted, already-parsed fields and calls the existing,
    unmodified local_ocr.validate_against_candidates — the exact
    technique calculate_join_key_verdict already established.
    evidence.artist_ocr_name directly replaces detect_illus_anchor
    for the artist step.
  • identify_land_printing (steps 2-3, artist-narrow + phash) is
    completely untouched.
  • _process_land_card factors the shared step-1-through-3 outcome
    logic out of run_lands_identify's loop so both the evidence-backed
    and live-fetch-fallback branches call the exact same code — this is
    what guarantees the two data sources produce identical verdicts on
    identical inputs, not just that each works in isolation.
  • A card without current evidence still falls back to the original
    live fetch + OCR + artist-detection path, unchanged, still gated by
    --fetch-budget. An evidence-backed card never counts against that
    budget (new LandsIdentifyResult.evidence_backed counter, separate
    from fetch_attempted).

Also wires the management command into pilot_run_lifecycle's forced
dry-run guard / counters-before-output / ledger-counters rails
(issues #345/#373's convention, already used by
local_calculate_verdicts/consensus_recompute) — local_lands_identify
wasn't on that PR's original five-command list, but a cheap
evidence-backed dry-run now makes the guard's cheap-preview rationale
hold here for the first time, and the "house lifecycle" ask in #359
covers it.

docs/features/catalog-completion-plan.md's Part 4 section updated in
place with the new sizing/behavior.

Checklist

  • I have installed pre-commit and installed the hooks with pre-commit install before creating any commits.
  • I have updated any related tests for code I modified or added new tests where appropriate.
  • I have manually tested my changes as follows:
    • test_local_lands_identify.py (36 tests, up from 19: currency-check unit tests, OCR-from-evidence unit tests, evidence-first orchestrator branching, and an explicit fixture proving the evidence-backed and live-fetch-fallback paths produce byte-identical verdicts on identical inputs).
    • New test_command_local_lands_identify.py (9 tests: dry-run default, stale-image guard, ledger self-recording, broken-pipe-during-summary hardening, forced-dry-run guard blocked/passed/skip-recorded).
    • Full pytest cardpicker suite: 1847 passed, 4 skipped, 130/130 snapshots (a first attempt caught a real bug of my own — the new command test file was missing the standard _preserve_shared_factory_sequences fixture, letting its factory calls permanently shift the process-global sequence counter and break test_views.py's snapshot expectations; fixed in a follow-up commit and reconfirmed clean).
    • ruff/black/isort clean on all changed/added files; mypy shows zero new errors on the two changed core files (confirmed identical 212-error pre-existing baseline on master for the same file set).
  • I have updated any relevant documentation or created new documentation where appropriate (docs/features/catalog-completion-plan.md's Part 4 section, in place).

Part of #359 (Phase 1 only, per this repo's task framing — the read-only
full-pool sizing and the owner poll issue #359 also asks for are separate,
still-open pieces of that issue; not closing it here).

local_lands_identify now consumes a card's current ImageEvidence row
(same currency test as Stage D's calculators) for collector-line/
artist signals when one exists, skipping fetch+OCR entirely; falls
back to the original live-fetch path only when no current evidence
exists. Also wires the command into pilot_run_lifecycle's forced
dry-run guard/counters-before-output rails (issues #345/#373).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd tests

test_command_local_lands_identify.py was missing the standard
_preserve_shared_factory_sequences fixture, letting its
CanonicalArtistFactory calls permanently shift the process-global
sequence counter and break test_views.py's snapshot expectations
in a full-suite run.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@WilfordGrimley
WilfordGrimley merged commit fbb74e4 into master Jul 23, 2026
7 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.

1 participant