fix: circular require warning, partial-migration constant crash, silent deprecation strategy

[pftg]

Three defects surfaced by running the gem against a real consumer project — jetthoughts.github.io, upgraded in Docker against its committed visual baselines. All three were invisible to our own suite; each now has a subprocess guard, mutation-checked.

Rebased onto 64db3ea. Test numbers on that base: rake test:unit 570, 0 failures; rake test 598, 0 failures, 1 skip; rake test:canonical (the 3.0 gate) 467, 0 failures, 1 skip. standardrb clean; reverse gate + alias-only gate green. No version/CHANGELOG changes.

Probe placement follows the #236 split. Canonical claims (the circular-require check over CANONICAL_ENTRY_POINTS) live in test/unit/support_load_probe_test.rb, with verbose_load promoted to def self.verbose_load alongside the existing self.probe. Everything asserting a v1 name — LEGACY_CONSTANTS, the legacy-constant resolution test, the vips-leaf test, the NameError-message test and the all-legacy-entries circular test — lives in test/legacy/legacy_entry_point_probe_test.rb, which reuses both class methods. CANONICAL_ENTRY_POINTS keeps its name (#238's deletion test depends on it). Net effect: nothing in rake test:canonical asserts a name 3.0 deletes.

---

Defect 1 — circular require warning (regression we introduced)

Reproduction. $VERBOSE on — which Rake::TestTask sets by default, i.e. every standard Rails/Minitest setup, and how the real project runs:

$ bundle exec ruby -w -e 'require "snap_diff"'
warning: loading in progress, circular require considered harmful - lib/snap_diff/drivers.rb
	from -e:1:in '<main>'
	... 18 frames ...

drivers.rb:65 required snap_diff/utils; utils.rb:3 required snap_diff/drivers back. Fired on every entry point, canonical and legacy, and survived into the 3.0 simulation. v1.12.0 emits none. Reproduced on Linux in their container — not a macOS artifact.

Fix. autoload :Utils, "snap_diff/utils" on SnapDiff, replacing the bottom-of-file require. Autoload never fires at load time, so the cycle is gone; and unlike simply deleting the require, it does not narrow the surface — require "snap_diff/drivers" still leaves SnapDiff::Utils resolvable, which drivers_test.rb already asserted and which my first attempt broke.

Guards. support_load_probe_test.rb → "no canonical entry point emits a circular require warning under -w"; legacy_entry_point_probe_test.rb → the same over the v1 entries. A fresh ruby -w process per entry point, since Ruby only warns once per cycle per process.

Mutation. Restored the bottom-of-file require → guards fail naming all 16 entry points:

require "snap_diff" -> warning: loading in progress, circular require considered harmful - lib/snap_diff/drivers.rb
require "snap_diff/dsl" -> ...
require "capybara_screenshot_diff/minitest" -> ...
(16 of 16)

After: all 16 entry points, grep -c "circular require" → 0.

---

Defect 2 — Capybara::Screenshot::Os hard-crashes after a canonical require

Reproduction. UPGRADING.md tells adopters to migrate the require line first and rename constants afterwards. That half-migrated state was broken, and asymmetrically so — config setters kept working, so the mixed state looked supported right until Os died:

canonical require, then Capybara::Screenshot::Os  -> uninitialized constant (NameError)
legacy require,    then the same                  -> "linux"  (works)
canonical require, then a legacy CONFIG setter    -> works

The real project has one such reference. It aborted their entire suite before a single test ran.

Full audit — every legacy constant under a canonical-only require

38 legacy constants a consumer might touch, probed against each canonical entry point. Measured against origin/master (64db3ea):

Entry point	broken
snap_diff	16
snap_diff/dsl	13
snap_diff/integrations/minitest	13
snap_diff/integrations/rspec	13
snap_diff/static	13
snap_diff-capybara	4

(snap_diff/integrations/cucumber is not measurable this way — a bare require dies without cucumber's World.)

Three distinct causes, not two:

Class A — the const_missing shim resolved its mapping, then died on the target. The v1 entry points required the whole gem, so v1 code could name these with nothing else required; the canonical entries are lean, so the shim leaked a bare uninitialized constant SnapDiff::Utils — an internal name the reader cannot act on.

Diff::StableScreenshoter, Diff::AreaCalculator, Diff::Utils, Diff::ScreenshotMatcher, Drivers::BaseDriver, CapybaraScreenshotDiff::AttemptsReporter, Reporters::HTML.

Class B — eager aliases assigned in lib/capybara* forwarder files that only LEGACY entry points load. A canonical require never loaded the forwarder, so the name simply did not exist: Capybara::Screenshot::Os (diff/os.rb), Diff::Reporters::Default (diff/reporters/default.rb), the four CapybaraScreenshotDiff error classes (capybara_screenshot_diff.rb).

Class C — never mapped at all. Not in any const_missing table and not eagerly aliased anywhere a canonical require reaches: CapybaraScreenshotDiff::DSL, CapybaraScreenshotDiff::Minitest::Assertions, Drivers::ChunkyPNGDriver, Drivers::VipsDriver. (The two leaves could never reach const_missing at all — SnapDiff::Drivers is a real module, so a missing leaf on it is just a NameError.)

Not defective (checked, symmetric): BrowserHelpers, Screenshoter, Vcs, ImagePreprocessor, AnnotationService, Drivers, ImageCompare, Difference, Comparison, VERSION, LOADED_DRIVERS, AVAILABLE_DRIVERS, RED_RGBA, ORANGE_RGBA, SnapManager, Snap, ScreenshotNamer, BacktraceFilter, ErrorWithFilteredBacktrace, ScreenshotAssertion, AssertionRegistry, top-level Region.

Fix — the class of bug, not the instance

• Class A: LegacyShims.resolve now loads the replacement before naming it, deriving the file path from the gem's own convention (SnapDiff::AreaCalculator → snap_diff/area_calculator), with a one-entry override table for the odd one out. If it still cannot resolve, the error speaks the user's vocabulary:

  `Capybara::Screenshot::Diff::Utils` maps to `SnapDiff::Utils`, which this process
  cannot load. See docs/UPGRADING.md for the v1 -> SnapDiff name map.
  

  (It deliberately does not advise "reference SnapDiff::Utils directly" — we just failed to load it, so that name does not exist either.)

• Class B: the eager aliases moved into snap_diff/legacy_shims.rb — the one file every entry point loads. That file is already in the reverse gate's DELETED_IN_3_0 set, so this adds no core→legacy edge.

• Class C: DSL and Minitest::Assertions are now mapped lazily. They cannot be eager in legacy_shims (snap_diff/dsl requires snap_diff back; the minitest integration pulls in the minitest gem, which no canonical entry should force), and they stay eager under the legacy entries. The driver leaves became autoload on SnapDiff::Drivers — which fixes the canonical SnapDiff::Drivers::VipsDriver under a bare require too.

The forwarder files now only require; duplicating the assignment would emit already initialized constant (Ruby warns even for identical values).

Guard. legacy_entry_point_probe_test.rb → "every legacy constant resolves under a canonical-only require": 38 constants × all 16 entry points, in subprocesses. Plus the vips-leaf test and "an unloadable legacy constant points at the upgrade guide". Same-object identity is not re-asserted here — namespace_forwarding_test.rb already pins it.

Mutation. Dropped the Os alias and the lazy target load → guard fails on all 16 entry points.

After: 0 broken across all seven canonical entries and all nine legacy ones.

$ ruby -e 'require "snap_diff"; puts Capybara::Screenshot::Os.name'
macos
$ Capybara::Screenshot::Os.equal?(SnapDiff::Os)                                        # => true
$ CapybaraScreenshotDiff::DSL.equal?(SnapDiff::DSL)                                    # => true
$ CapybaraScreenshotDiff::Minitest::Assertions.equal?(SnapDiff::Minitest::Assertions)  # => true

Follow-up: the autoload broke v1 feature detection on a vips-less box

Caught in review. ruby-vips is not a runtime dependency, and the documented v1 pattern is Diff.driver = :vips if defined?(...Drivers::VipsDriver). An unconditional autoload makes that truthy everywhere.

Red (vips hidden from the load path):

AVAILABLE_DRIVERS: [:chunky_png]
const_defined?(:VipsDriver): true
defined?(...Drivers::VipsDriver): "constant"
const_get: RuntimeError: Required ruby-vips gem is missing. Add `gem "ruby-vips"` to Gemfile

v1.12.0 loaded vips_driver.rb only from find_driver_class_for, so defined? was nil there. Fix: declare each leaf only when its gem is present, which is why the two autoload lines sit below AVAILABLE_DRIVERS.

Green:

AVAILABLE_DRIVERS: [:chunky_png]
const_defined?(:VipsDriver): false
defined?(...Drivers::VipsDriver): nil
const_get: NameError: uninitialized constant SnapDiff::Drivers::VipsDriver

Guard: drivers_test.rb → "an unavailable driver leaf stays undefined rather than autoloading into a crash" (subprocess, require "vips" forced to LoadError; also asserts the chunky_png leaf still autoloads). Mutation: dropping the if AVAILABLE_DRIVERS.include?(:vips) gate → VipsDriver is const_defined? without ruby-vips.

---

Defect 3 — the deprecation strategy was silent for real usage

Reproduction. Exercising all 14 legacy APIs the real project's setup file touches (6 Capybara::Screenshot.* setters, 5 Capybara::Screenshot::Diff.* setters, Os.name, DSL, Minitest::Assertions) under -w produced zero warnings.

Structural cause: Deprecation.warn was reachable only via const_missing, so it could never fire for an eagerly-aliased constant, and the generated config accessors were plain delegators with no deprecation path at all. Meanwhile docs/snapdiff.md claimed legacy names work "with a one-time deprecation warning per constant". Net effect: a silent 2.x, then a bare LoadError/NameError at 3.0 with no pointer to SnapDiff.

(a) The docs now tell the truth

docs/UPGRADING.md gained a rewritten Deprecation Warnings section split into three parts — the once-per-process migration notice, the per-constant warnings (each shimmed constant listed by name), and Silent by design, which enumerates every silent name and why. It now also records that DSL / Minitest::Assertions warn only under a canonical require (under the v1 entries — what an unmigrated app actually uses — they are eager and silent), that touching a driver leaf through the old path still warns once for ...::Drivers, and that leaf defined? stays nil without the gem. docs/snapdiff.md and README.md no longer claim every legacy name warns.

(b) One migration notice per process

[snap_diff deprecation] This process uses the v1 `Capybara::Screenshot*` / `CapybaraScreenshotDiff*` API. It still works in 2.x and is REMOVED in 3.0 -- see docs/UPGRADING.md for the SnapDiff replacements. Silence with `SnapDiff.silence_deprecations = true` or SNAP_DIFF_SILENCE_DEPRECATIONS=1. (shown once per process)

Fires from every door that can be hooked, then never again:

Door	Hook
legacy config accessor (read and write)	generated delegators in install_config_accessors
lazily shimmed legacy constant	Deprecation.warn, ahead of the per-constant line
include Capybara::Screenshot[::Diff]	install_include_notice → included hook
Diff.default_options	its own line — hand-written forwarder, not a generated delegator

Reuses the existing switch (SnapDiff.silence_deprecations / SNAP_DIFF_SILENCE_DEPRECATIONS). Lock-free fast path after the first call.

Scoping the gem's own suite. test_helper.rb already raises on any [snap_diff deprecation] output, and the suite configures through Capybara::Screenshot.root= by design — so it calls Deprecation.suppress_migration_notice!, which silences only the notice and deliberately survives reset!. The per-constant raise-on-deprecation guard is untouched and still fires.

Guards (test/legacy/snap_diff_deprecation_test.rb, all subprocess — "once per process" is the contract): one test per door, each asserting exactly one notice on its own; one asserting exactly once across all doors × 3 iterations, containing docs/UPGRADING.md, REMOVED in 3.0, SNAP_DIFF_SILENCE_DEPRECATIONS; one asserting it does not fire for a purely canonical setup; two for the silencing switches.

Mutations — each firing point and each property broken independently:

Mutation	Result
dropped Deprecation.notice from the config delegators	2 failures — config delegator (read), config delegator (write)
dropped it from the included hook	1 failure — legacy include
dropped it from default_options	1 failure — hand-written derived reader
dropped the silence_deprecations? check	2 failures — both silencing tests
called notice at legacy_shims load (mis-scoped)	1 failure — canonical-only setup no longer silent

The first mutation is worth calling out: my initial combined guard passed under it, because the const_missing door was still firing and masking the config-delegator regression. Splitting one test per door is what made it fail.

---

🤖 Generated with Claude Code

[sourcery-ai]

Sorry @pftg, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[sourcery-ai]

Reviewer's Guide

Fixes three production-surfaced issues: removes a circular require between core files, makes all legacy constants resolve correctly under canonical-only requires (including eager aliases), and implements a visible-but-controllable deprecation/migration notice strategy, with updated documentation and subprocess-based guards.

Sequence diagram for canonical and legacy compatibility loading

sequenceDiagram
    participant App
    participant EntryPoint
    participant LegacyShims
    participant SnapDiff
    participant Utils

    App->>EntryPoint: require entry point
    EntryPoint->>LegacyShims: require snap_diff/legacy_shims
    LegacyShims->>SnapDiff: define eager aliases
    App->>LegacyShims: reference legacy constant
    LegacyShims->>LegacyShims: LegacyShims.resolve(old_name, target)
    LegacyShims->>LegacyShims: require_unit(target)
    LegacyShims->>SnapDiff: Object.const_get(target)
    SnapDiff-->>App: same canonical object
    App->>SnapDiff: Drivers.for(driver_options)
    SnapDiff->>Utils: require snap_diff/utils
    SnapDiff->>Utils: find_driver_class_for(driver_option)
    Utils-->>SnapDiff: driver class

Loading

Sequence diagram for the once-per-process legacy migration notice

sequenceDiagram
    participant App
    participant LegacyConfig
    participant LegacyShim
    participant Deprecation
    participant SnapDiff

    App->>LegacyConfig: read or write legacy setting
    LegacyConfig->>Deprecation: notice
    Deprecation->>SnapDiff: silence_deprecations?
    Deprecation-->>App: one migration notice

    App->>LegacyShim: resolve legacy constant
    LegacyShim->>Deprecation: warn(subject, replacement)
    Deprecation->>Deprecation: notice
    Deprecation-->>App: per-constant warning once

    App->>LegacyConfig: read another legacy setting
    LegacyConfig->>Deprecation: notice
    Deprecation-->>App: no additional migration notice

Loading

File-Level Changes

Change	Details	Files
Break circular require between SnapDiff drivers and utils to eliminate -w warnings across all entry points.	Move require "snap_diff/utils" from top-level of SnapDiff::Drivers body to inside Drivers.for so utils is only loaded when needed. Add autoloads for ChunkyPNGDriver and VipsDriver to avoid eager loading heavy driver implementations while keeping leaf constants resolvable. Introduce subprocess-based test helper verbose_load and a guard that exercises all entry points under ruby -w to ensure no circular require warnings are emitted.	lib/snap_diff/drivers.rb test/unit/support_load_probe_test.rb
Harden legacy constant shims so every v1 constant resolves under canonical entry points and failure messages point to SnapDiff and the upgrade guide.	Expand snap_diff/legacy_shims.rb to eagerly require core units (config, deprecation, drivers, errors, os, reporters/default, version) and to define eager same-object aliases for Os, Reporters::Default, error classes, VERSION, Comparison, LOADED_DRIVERS, AVAILABLE_DRIVERS. Move legacy alias assignments for Os, Reporters::Default, and CapybaraScreenshotDiff error classes out of lib/capybara* forwarder files into snap_diff/legacy_shims.rb so they are available under canonical-only requires. Implement LegacyShims.resolve and require_unit to load target SnapDiff constants before returning them, with a NameError message that cites the legacy name, the SnapDiff replacement, and docs/UPGRADING.md when loading fails. Install config accessors on legacy modules that delegate to SnapDiff.config while also triggering the migration notice, and add an include hook for Capybara::Screenshot and ::Diff to trigger the notice when modules are included. Add LegacyShims.install mappings for DSL and Minitest::Assertions, plus a predefined CapybaraScreenshotDiff::Minitest module to host those shims. Add subprocess-based load probes that, for each entry point, assert all legacy constants resolve under canonical require and that unloadable legacy constants report their SnapDiff target in error messages.	lib/snap_diff/legacy_shims.rb lib/capybara/screenshot/diff/os.rb lib/capybara/screenshot/diff/reporters/default.rb lib/capybara_screenshot_diff.rb test/unit/support_load_probe_test.rb
Replace silent deprecation behavior with a structured migration notice and per-constant warnings, document the behavior, and guard it via subprocess tests.	Extend SnapDiff::Deprecation with MIGRATION_NOTICE, notice, suppress_migration_notice!, and enhanced warn/reset! semantics, including once-per-process tracking via @notified/@notice_suppressed. Wire the migration notice into all hookable v1 surfaces: legacy config accessors (read/write), const_missing shims, and include Capybara::Screenshot/::Diff via install_include_notice. Add SnapDiff::Deprecation.suppress_migration_notice! call in test_helper to keep the gem’s own suite from emitting the migration notice while still failing on per-constant deprecations. Create subprocess-based deprecation probes in snap_diff_deprecation_test.rb that verify: one notice per door, exactly one notice per process across multiple uses, no notice for canonical-only usage, and proper silencing via SnapDiff.silence_deprecations and SNAP_DIFF_SILENCE_DEPRECATIONS. Rewrite README.md, docs/UPGRADING.md, and docs/snapdiff.md deprecation sections to describe the migration notice, list which legacy constants emit per-constant warnings, and enumerate silent-by-design names and reasons (eager constants, DSL, settings access, requires).	lib/snap_diff/deprecation.rb lib/snap_diff/legacy_shims.rb test/test_helper.rb test/unit/snap_diff_deprecation_test.rb README.md docs/UPGRADING.md docs/snapdiff.md

Possibly linked issues

• #ADR-004: The PR directly implements namespace migration compatibility, ensuring legacy aliases work under canonical requires with deprecation guidance.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 43 minutes

Limit details: You’ve used the included review currently available.

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?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6eb4e490-41ab-438d-bd0f-96b64b483e4c

📥 Commits

Reviewing files that changed from the base of the PR and between 64db3ea and 7c4da0e.

📒 Files selected for processing (14)

• README.md
• docs/UPGRADING.md
• docs/snapdiff.md
• lib/capybara/screenshot/diff/os.rb
• lib/capybara/screenshot/diff/reporters/default.rb
• lib/capybara_screenshot_diff.rb
• lib/snap_diff/deprecation.rb
• lib/snap_diff/drivers.rb
• lib/snap_diff/legacy_shims.rb
• test/legacy/legacy_entry_point_probe_test.rb
• test/legacy/snap_diff_deprecation_test.rb
• test/test_helper.rb
• test/unit/drivers_test.rb
• test/unit/support_load_probe_test.rb

---

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.}

[github-actions]

Screenshot diffs detected

Artifact	Link
HTML report (inline)	N/A
Full report with images	N/A
All artifacts	Browse all