Import unification PR 6b: unify and extract the stamping loop (spec decision 11)

[jss367]

What this is

The stamping alignment + extraction PR carved out of PR 6 (#1444): the post-scan stamping loop was the last piece of shared logic hand-mirrored between run_import_job and _run_remote_import_job, held back because it hid genuine behavioral divergences. This PR resolves them one reviewable commit at a time, proves the two loops textually identical, and extracts the result as _stamp_landed_and_validate_catalog.

• Spec: docs/superpowers/specs/2026-08-06-import-path-unification-design.md (decision 11 now RESOLVED; PR-sequence 6b entry updated)
• Plan: docs/superpowers/plans/2026-08-09-import-unify-pr6b-stamping.md

Commits

1. Docs — spec decision-11 resolution + PR-sequence update; "As built" erratum on the PR-6 plan answering CodeRabbit's two comments on Import unification PR 6: extract the shared import phases #1444 (seven commits landed, not six; as-built signatures: _rollback_on_mount_loss takes no rel and reads batch_st.rel; _batch_preflight returns dest_folder and takes missing_root_check; merged code is truth).
2. D4, red-green — remote file_hash backfill (decision 11, THE vetoable commit). When scan leaves a landed file's row hash NULL and the destination re-read agrees with the hash this import recorded, remote now backfills photos.file_hash, matching local. The zero-byte exclusion (the backfill trap): the formula is file_hash=(hash if hash != EMPTY_FILE_SHA256 else None) — backfilling EMPTY_FILE_SHA256 would recreate the exact collision the scanner's zero-byte NULL convention exists to prevent (every empty file an exact duplicate of every other; empty_hash_needs_repair churning repairs). file_hash=None hits update_photo_hash_check's status-only arm, so the row keeps scan's NULL. The scan-hash-agrees path needs no backfill (the row already holds the hash; local doesn't backfill there either). Two guard tests pin file_hash IS NULL after zero-byte accepts on both paths, forever.
3. D2/D3, red-green — normalize zero-byte verified_hash at _LandedFile construction. Both remote construction sites now produce EMPTY_FILE_SHA256 instead of None for zero-byte files; _LandedFile.verified_hash tightened to str. The src_hash computation is deliberately untouched (the intra-batch dedup maps, adopt gate, and _record_checker rely on the None convention). The one behavior flip: remote zero-byte re-import over a pre-existing row now fires the import's defense-in-depth derived-cache diff loop (NULL != EMPTY), aligning with local. This is NOT a user-visible cache change — scanner's own zero-byte content-change arm already invalidated that row during the batch scan; the pin test therefore uses a timing-gated spy that counts only post-scan invalidations (counting scanner's in-scan call would have made the red test green pre-change).
4. D1 + messages — zero-flip unification. Stamps gated on verified_counted_for_copies in both loops (a no-op locally, where it is always True; identical to remote's old params.verify_by_hash gate); imported_photo_ids.add outside the gate on every accept path; _rehash_dest_or_none hoisted to module level; unified five-message reason table via a dest_noun local. Textual-identity proof: both loop regions, comments stripped and the noun literal normalized, diff EMPTY (the full-text diff including comments is also empty modulo the literal). 231 passed at this commit — zero test flips.
5. Move-only extraction. _stamp_landed_and_validate_catalog(state, batch_st, db, params, rel, *, attests_bytes, dest_noun, stop_requested) returns raw_companion_invalidations; both functions call it and feed _invalidate_changed_and_sweep unchanged. Proof: the pre-extraction loop body equals the function body modulo an 8-space dedent and the attests_bytes/stop_requested parameter renames (normalized diff: empty). The docstring carries the three invariants (attests_bytes gate; imported_photo_ids on every accept path outside the gate; backfill excludes EMPTY).

Reason wordings, old → new

The "recorded, not verified" choice is deliberate UI transparency: a no-verify remote run computed the card-side hash but verified nothing destination-side, so the wording must not overclaim.

Local (all unpinned by tests):

• "destination changed between copy verification and catalog scan (hash mismatch)" → "scanned archive row hash does not match the hash this import recorded (archive base is likely stale or misconfigured)"
• "scan wrote no archive row hash and a re-read of the archive file disagrees with the copy-time hash (archive file vanished, changed, or is unreadable)" and "archive file unhashable after copy verification (scan wrote no hash and re-hash disagrees)" → both collapse to "scan wrote no archive row hash and a re-read of the archive file disagrees with the hash this import recorded (archive file is likely stale, unreadable, or misconfigured)"
• "paired companion archive bytes no longer match the copy-time hash" → split into "paired companion archive bytes could not be read" / "paired companion archive bytes do not match the hash this import recorded (archive base is likely stale or misconfigured)"
• "not cataloged after scan" → "not cataloged after scan (no photo row)"

Remote (verify/no-verify tail collapse at three sites):

• "...does not match the source hash (...)" [no-verify] / "...does not match the hash verified on the NAS (...)" [verify] → "...does not match the hash this import recorded (...)" for the direct-row, NULL-re-read, and companion mismatches; the NULL-re-read parenthetical also changes "mount base" → "mount file" per the unified table.

All 4 pinned reason prefixes at all 9 assertion sites are unchanged. One test conjunct was updated as part of the tail collapse: test_remote_import_paired_jpeg_no_verify_fails_on_mount_mismatch now expects "hash this import recorded" instead of "source hash".

Edge note: legacy EMPTY rows

Adopting local's raw-compare shape drops remote's defensive scan_h EMPTY→None normalization: a convention-violating legacy row whose file_hash column holds EMPTY_FILE_SHA256 under a non-empty landed file now reclassifies as failed (hash mismatch) instead of re-read-accepting. Unreachable by any test or by scanner-written rows, and the tightened behavior is fail-safe.

Preflight-mirror check

No duplicate-walk changes in this PR — the spec's preflight-mirror checklist item is n/a.

What this means

The LAST piece of shared logic is unified: both import functions are now transport cores plus calls into shared phase functions. PR 7 is the transport merge.

Test results

• vireo/tests/test_import_job.py: 231 passed, 1 skipped at every production commit boundary (baseline 225 + 4 D4 tests + 2 D2/D3 tests); red-green patterns exactly as planned (1 red for D4, 1 red for D2/D3).
• Parity net (-k "behavior or agree_on_plain or adoption or renamed_twin or zero_byte or dest_read_cancel or mount_detach or pairs"): 29 passed.
• Required suite (workspaces, db, app, photos/edits/jobs/darktable/config APIs): 2077 passed, 14 skipped, 1 failed — the known env-only failure test_api_exiftool_status_reports_missing.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@jss367, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7578175d-1afb-4edb-aff9-e0adc2b54a96

📥 Commits

Reviewing files that changed from the base of the PR and between c21d55f and acdc22b.

📒 Files selected for processing (5)

• docs/superpowers/plans/2026-08-08-import-unify-pr6-phase-extraction.md
• docs/superpowers/plans/2026-08-09-import-unify-pr6b-stamping.md
• docs/superpowers/specs/2026-08-06-import-path-unification-design.md
• vireo/import_job.py
• vireo/tests/test_import_job.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}