ADR-004: Incremental migration toward SnapDiff v2 architecture

[pftg]

Status 2026-08-24 — superseded by ADR-010, kept as the vision record

This issue's Phase 1–3 checklist has been overtaken by events. The consolidation
happened, but through ADR-008 → ADR-010 rather than these PR steps. Read
.ai/adr-010-everything-in-2.0.md first; this issue is now the historical
record of where the idea started.

What actually shipped

The SnapDiff namespace exists, the config surface was inverted to a single
object, the test suite was split so rake test:canonical proves the
post-deletion state, and the comparison pipeline lost its driver indirection on
the 2.1 branch. The measured spike (.ai/endstate-spike-rewrite.md) found a
from-scratch rewrite only 23% smaller with 70% file reuse — "a rewrite that keeps
70% is consolidation wearing a rewrite costume" — so the vision was delivered by
consolidation, not by the rewrite this issue implies.

The release plan that replaced it

• 2.0 — the rename, plus every feature: honest reporting, the accept
  workflow, capture-before-raise (The new-screenshot error prints a path that will never exist in CI #260), explicit-config-over-env,
  zero-setup fork-parallel reports (Fork-parallel reports should need zero developer setup #258). Everything destined for removal
  warns, naming 2.1. 2.0 deletes nothing.
• 2.1 — every deletion: legacy namespaces, the 27 v1 config accessors,
  chunky_png, the driver abstraction, shift_distance_limit. ruby-vips
  becomes a hard runtime dependency.

What the original adopter question here was gating

This issue was the adopter-feedback gate on shipping 2.0. That feedback now
exists and was decisive — two customer personas ran the real product and both
got a green bar on a page they had deliberately broken, inside ten minutes.
See .ai/experience-map.md for both journeys and .ai/adr-009-* for the full
research record.

Remaining, live

• Ship 2.0.0 final — also the fix for gem "snap_diff-capybara" resolving to
  the empty 0.0.1 name reservation
• HUMAN: branch/tag protection (the 2.1 PR changes CI check names, 2
  dimensions → 1)
• Concurrency follow-ups tracked in Fiber and thread safety: remaining decisions and hardening after the v2 core work #217, fork-parallel reports in Fork-parallel reports should need zero developer setup #258

---

Original vision and phased plan (2026, superseded)

Vision

Consolidate the comparison pipeline from 25+ files / ~2,000 lines to 16 files / ~1,400 lines with 3 core objects: Comparison, Driver, Reporter.

Full ADR: .ai/adr-004-snap-diff-v2-vision.md
Migration plan: .ai/adr-004-migration-plan.md

Phase 1: Make Things More Alike (internal, no API changes)

• PR 1: Add vocabulary aliases (baseline_path, current_path, Driver#load, Driver#mask)
• PR 2: Merge DifferenceFinder into ImageCompare (delete 1 file)
• PR 3: Unify quick_equal? and different? into lazy evaluation
• PR 4: Merge StableScreenshoter into Screenshoter (delete 1 file)
• PR 5: Move skip_area masking from comparison-time to capture-time
• PR 6: Consolidate mattr_accessor into Config object (keep old API as delegates)

Phase 2: Separate Annotation from Reporting

• PR 7: Extract annotation (draw rectangles, heatmaps) into AnnotationService
• PR 8: Give Reporters::Default the record/finalize interface

Phase 3: Namespace Migration (needs major version)

• PR 9: Add SnapDiff namespace aliases (additive, non-breaking)
• PR 10: v2.0 — Make SnapDiff canonical, old namespace becomes shims

Constraints

• Each PR keeps tests green and public API unchanged (until Phase 3)
• No big-bang rewrite — Flocking Rules: make things more alike, then unify
• Do NOT delete Reporters::Default — it generates annotation images the HTML reporter needs
• Do NOT drop features: screenshot_section/group, median_filter, shift_distance_limit, LFS, delayed validation

Expected Outcome

Metric	Current	After Phase 1-2	After Phase 3
Files	25+	~18	~16
Lines	~2,000	~1,600	~1,400
Namespaces	2	2 (with aliases)	1 (SnapDiff)
Classes to understand pipeline	7	4	3
Config points	24 mattr_accessor	8 in Config + delegates	8 in Config

References

• ADR-004 vision: .ai/adr-004-snap-diff-v2-vision.md
• ADR-004 migration plan: .ai/adr-004-migration-plan.md
• ADR-001 (driver protocol): resolved — BaseDriver sufficient
• ADR-002 (HTML reporter): implemented in PR feat: add HTML reporter for visual diff dashboard #164
• ADR-003 (ComparisonResult): superseded by this issue

[pftg]

Status Update (2026-04-12)

Completed prerequisites

Item	PR	Notes
Driver contract tests (shared examples)	#174	Satisfies "PR 0" prerequisite from cross-review
Dead code removal (3 items)	#174	Cleaner starting point
Reporter record/finalize interface	#175	Thread-safe reporters with mutex; both Default and HTML now share the protocol
Reporter diff cleanup in Snap#delete!	#173	Annotation artifacts cleaned up properly
Test simplification (ADR-006)	#174, #177	Fixtures README, global state guard, test doubles cleanup

Validated cross-review findings

Three independent reviewers (code, architecture, DevXP) agreed on the same ordering:

1. PR 2 first — merge DifferenceFinder into ImageCompare (−1 file, ���80 lines)
2. PR 8 done — record/finalize interface shipped in thread safety: reporters and docs #175
3. PR 7 next ��� extract annotation from Reporters::Default
4. PR 9 — add SnapDiff namespace aliases
5. PR 10 — v2.0 make SnapDiff canonical

Rejected from original plan

PR	Reason
PR 1 (aliases)	Rename-before-extract anti-pattern
PR 3 (unify quick_equal?/different?)	Different semantics and performance characteristics
PR 4 (merge screenshoters)	Already resolved; different concepts
PR 5 (skip_area at capture-time)	Breaks baseline masking in real-world apps
PR 6 (Config object)	Touches every file; defer to after structural work
P1.1 (VIPS sequential)	Benchmarked, no measurable benefit

Next action

PR 2: Merge DifferenceFinder into ImageCompare — 7 micro-steps, all tests exist, independently revertable. Detailed plan in .ai/adr-004-migration-plan.md.

[pftg]

PR #208 adversarial review — in progress, interim findings

Reviewing refactor/v2-step3-file-tree-move (PR #208) against the require-cycle guard machinery and forwarder completeness. Findings so far:

Critical — reproducible deadlock (regression): require "capybara_screenshot_diff" on the main thread racing Thread.new { require "snap_diff" } (both first-touch, no prior load) deadlocks Ruby's require lock 10/10 runs (fatal: No live threads left. Deadlock?). Root cause: the Thread.current[...] guard flags are thread-local, but Ruby's per-file require lock is process-global, so each thread proceeds past its own (unset) flag and each ends up waiting on the file the other thread is mid-loading — classic lock-order inversion. Confirmed not present on master (10/10 clean), which uses autoload :SnapDiff, "snap_diff" instead of the eager guarded mutual require this PR introduces. Reverse thread order (capybara_screenshot_diff in the thread, snap_diff on main) does not reproduce it (0/10) — timing-dependent, not universally deterministic, but fully reproducible in the direction shown.

Warning gap: ruby -w -Ilib -r snap_diff/config -e ... (direct entry, not one of the PR's three claimed load orders) prints a "circular require considered harmful" warning even with all guards intact — snap_diff.rb's own require "snap_diff/config" has no reentry guard, so entering via snap_diff/config directly re-enters itself mid-load via the capybara_screenshot_diff.rb → snap_diff.rb → snap_diff/config chain.

Dead guard: the Thread.current[:capybara_screenshot_diff_loading] guard in snap_diff/dsl.rb never actually fires in any tested entry order — dsl.rb isn't reachable from the core mutual-require cycle, so it's cargo-culted from config.rb's (genuinely load-bearing) guard; comment there overstates the cycle.

Pre-existing, not a regression: capybara_screenshot_diff/reporters/html (old or new path) raises NameError/NoMethodError if required standalone before the umbrella file — confirmed present on master too, unrelated to this PR's move.

Still running: forwarder constant-identity loop, screenshot_assertion split verification, full test suite, mutation probe. Will post the final verdict here once complete.

[pftg]

PR #208 adversarial review — final verdict: request-changes

Full results (worktree review/pr208 off refactor/v2-step3-file-tree-move, diff vs master):

Critical

1. Reproducible require-time deadlock (regression): Thread.new { require "snap_diff" } racing require "capybara_screenshot_diff" on the main thread deadlocks Ruby's require lock 10/10 runs (fatal: No live threads left. Deadlock?). The Thread.current[...] guard flags are per-thread but Ruby's per-file require lock is process-global — each thread passes its own unset flag and ends up waiting on the file the other thread holds. Confirmed absent on master (10/10 clean, which uses autoload :SnapDiff, "snap_diff" instead). Reverse thread order doesn't reproduce it (0/10) — direction-dependent, not universal, but fully deterministic in the direction shown.

Warnings / gaps in the "warning-free" claim
2. ruby -w -Ilib -r snap_diff/config -e ... (not one of the PR's 3 claimed load orders) prints a circular-require warning even with all guards intact — snap_diff.rb's own require "snap_diff/config" has no reentry guard.
3. Thread.current[:capybara_screenshot_diff_loading] guard in snap_diff/dsl.rb never fires under any tested entry order (single-thread or concurrent) — dead/cargo-cult code; the comment overstates a cycle that doesn't reach that file.
4. Git history is not preserved for any of the 23 moved files (only the ERB template asset shows as a real rename). All 5 batch commits show create mode for every new .rb file, not rename from/to — git log --follow and git blame on new paths stop at the move commit. This directly contradicts the PR's "git-mv, history preserved" claim; it's structural, not an execution slip — keeping the old path alive as a forwarder (rather than deleting it) removes the delete/create pair git's rename heuristic needs.
5. Mutation probe: replacing CapybaraScreenshotDiff::AttemptsReporter's alias with a wrong constant leaves rake test at 417/0/6 — unchanged. That forwarder's constant identity is untested by the suite.

Confirmed clean

• 384/0 unit, 417/0/6 full, standardrb 61 files/no offenses — all match the PR's claims exactly.
• All 19 old→new constant pairs are equal? (same object), including the screenshot_assertion.rb split — no stranded state; every new-namespace integration file correctly delegates back to the single CapybaraScreenshotDiff singleton.
• All 23 forwarder paths load standalone except capybara_screenshot_diff/reporters/html and capybara_screenshot_diff/cucumber — both fail identically on master too (pre-existing, not a regression from this PR).

Merge verdict: request-changes — item 1 is a real, reproducible deadlock in a plausible concurrent-boot scenario and should block merge; items 2–3 should be fixed or the "warning-free" / guard commentary corrected; item 4 should either get a real fix (two-commit-per-batch pattern: pure git mv then a second commit adding the forwarder) or the PR description's history claim should be withdrawn.

[pftg]

Status update — v2 experiment train through the core simplification (2026-08-22 evening)

The sequence has advanced past the file-tree move. Everything below is merged to master (now 31e02c6, unit suite 455 runs / 0 failures), each PR shipped behavior-preserving with guard tests first, adversarial review with mutation checks, and a green tree at every step:

Steps 3–5 (namespace groundwork)

• refactor: v2 step 3 — move implementation under lib/snap_diff (compat forwarders kept) #208 — file-tree move to lib/snap_diff/ (23 files, mv-then-rewrap commit pairs for rename detection). The require-cycle deadlock found in review (comments above) was fixed via legacy-config leaf extraction — acyclic require graph, no Thread.current guards; documented in docs/thread_safety.md.
• refactor: v2 step 4 — SnapDiff::Driver mixin (method names unchanged) #209 — SnapDiff::Driver mixin (was BaseDriver).
• refactor: v2 step 5 — Comparison and ComparisonResult canonical names #210 — canonical names: ImageCompare → SnapDiff::Comparison, Difference → SnapDiff::ComparisonResult (old constants alias the same objects).

Step 5.5-lite (core findings from the 3-architect KISS/YAGNI/DRY panel, trimmed from the full 8-object redesign)

• test: core guard tests ahead of 5.5-lite fixes (+ SnapManager cleanup fix) #211 — guard-test net ahead of the fixes (+ SnapManager cleanup fix: cleanup! clears registered snapshots; thread-local manager memoization).
• refactor: non-destructive capture/comparison option partitioning #212 — capture/comparison option split is a pure partition (was destructive delete on a shared hash).
• refactor: represent stability failure as data on the capture result #213 — stability failure represented as data on the capture result; the UnstableImage raise is now an explicit boundary decision.
• refactor: explicit baseline archiving + #inspect on assertion/result #214 — hidden baseline FileUtils.mv inside the validate path extracted to explicit, idempotent ScreenshotAssertion#archive_baseline!; #inspect on assertion/result.
• refactor: extract viewport preparation seam (Capture::Viewport) #215 — SnapDiff::Capture::Viewport.prepare!(size, anchor: nil) seam (module function); anchor is the v3 hook for scroll-position preservation after window resize.
• refactor: separate reporter and session lifecycle at module level #216 — reporter lifecycle (process-global, mutexed) separated from session lifecycle (thread-local) into SnapDiff::Reporting; public surface byte-identical (verified by singleton-method/arity dump diff).

Deliberately not built (panel verdict): Orchestrator/Verifier/ReporterHub objects, class renames beyond #210, value types with single consumers.

In progress — step 6 (PR 10 territory): deprecation shim layer — legacy constants keep resolving to the same objects but warn once per constant per process via const_missing + SnapDiff::Deprecation, with a silencing switch. Lands next, then 2.0.0.alpha1 ships as the opt-in experiment prerelease (RubyGems prerelease — never installed by default resolution). Migration guide (UPGRADING.md) and test migration follow, closing with 2.0.0.beta1.

Gate unchanged: final 2.0.0 remains evidence-gated on alpha/beta adopter feedback — the prerelease train exists to collect it.