fix: message leak, dead report tests, gem packaging (3.0 readiness backlog) - #234
Merged
Conversation
…cklog) Three independent beta2-review backlog items. 1. Vips::Image leaked into failure messages. ComparisonResult#to_h merged the whole meta hash, including the vips-only :diff_mask image object, so a failing assertion printed "diff_mask":"#<Vips::Image:0x...>". #to_h now excludes it; the object stays reachable via #diff_mask. 2. test/integration/report_screenshot_test.rb skipped all five tests unless RECORD_SCREENSHOTS -- the mode that records baselines rather than verifying them. Baselines existed only for macos/cuprite, so on the only CI platform (linux) there was nothing to compare against. Deleted, along with the orphan baselines. The HTML reporter keeps 18 unit tests, and `rake report:sample` already produces a sample report for eyeballing. 3. The gemspec shipped gems.rb, Rakefile and itself while omitting README.md, leaving a dead ../README.md link in the packaged docs. Replaced the deny-list regex with an allow-list: lib/, docs/, README, LICENSE, CHANGELOG.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
Reviewer's GuideFixes a leak of internal Vips::Image objects into user-facing failure messages, removes non-asserting HTML report screenshot tests and their macOS baselines, and tightens gem packaging to include only runtime code and docs while excluding build/dev artifacts. Sequence diagram for sanitized comparison failure messagessequenceDiagram
participant VipsDriver
participant ComparisonResult
participant DefaultReporter
participant User
VipsDriver->>ComparisonResult: store diff_mask in meta
DefaultReporter->>ComparisonResult: to_h
ComparisonResult-->>DefaultReporter: metrics without diff_mask
DefaultReporter->>User: build_error_message with JSON metrics
User->>ComparisonResult: diff_mask
ComparisonResult-->>User: raw image object
Flow diagram for fail-closed gem packagingflowchart LR
TrackedFiles[git ls-files] --> AllowList{Matches lib/ docs/ or packaged root docs?}
AllowList -->|yes| Gem[Gem contents]
AllowList -->|no| Excluded[Build and development files excluded]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 32 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 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (4)
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. Comment |
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
Second pass, from customer-persona findings. Each verified here before acting; two of the four reported items turned out to be artifacts of the PUBLISHED beta3 rather than of master, and are handled as such. Baselines (the oldest bug in the tracker: #5 and #6 in 2018, #133 in 2024) - README told users to "delete the baseline and re-run" in two places. It cannot work. `Vcs.checkout_vcs` (lib/snap_diff/vcs.rb:24) resolves every baseline with `git show HEAD:<path>`, and `ScreenshotMatcher#check_base_screenshot` calls it before `need_to_compare?` tests `base_path.exist?` -- so a committed baseline is fetched from HEAD no matter what the working tree says, and `rm` changes nothing. - New first-class "Accepting an intentional change" section: the mechanism, the commit that actually accepts it, and the surprising part -- staging is not enough, so no local run goes green until you commit. The FAQ answer now says the same thing instead of the opposite. - Deliberately does NOT document RECORD_SCREENSHOTS. It is printed by our own error message (screenshot_matcher.rb:73) but read nowhere in lib/; a separate lane is implementing it, and it should be documented once it works, not before. Version pinning - `gem "snap_diff-capybara"` unpinned installs 0.0.1 -- a placeholder whose entire payload is one README and zero Ruby files (verified by fetching and unpacking it), so the user gets an immediate LoadError. And unpinned `gem "capybara-screenshot-diff"` resolves to 1.15.1, not to the 2.0 the surrounding prose is selling. Every install instruction now pins, and the README says plainly that the mirror name is not the one to reach for. CHANGELOG, all verified - Failure messages leaked a libvips pointer struct via the comparison metadata; `to_h` excludes `diff_mask` since #234, which landed after the beta3 tag, so 2.0.0 final is the fix. - Known limitation: fork-parallel runs write no HTML report. Workers accumulate assertions per process; the report is written from `Minitest.after_run` in the parent (integrations/minitest.rb:69), which never sees them. Artifacts and pass/fail are unaffected. - A note for anyone sitting on a prerelease: beta3's deprecation channel was incomplete, so its silence is not evidence of being migrated. Constants - `Capybara::Screenshot::Os` -> `SnapDiff::Os` was in no rename table. Gemspec - rubygems_mfa_required. The four URI fields were added in the first commit.
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
* docs: 2.0.0 release readiness Audit of everything a 2.0.0 final would ship, and the fixes that did not need lib/ changes. CHANGELOG - A v2.0.0 entry written for someone upgrading from 1.15.1, not a diff of the betas. What to change (the version), what they will see (exact warning text), the five things that can actually break, and what 2.1 removes. Every claim verified against a real install; the beta sections stay as history. Version consistency - README, docs/UPGRADING.md: no more "beta"/"alpha"/"experiment" framing and no beta pins. Gemfile examples say `~> 2.0`. - Gem name: `capybara-screenshot-diff` is the one we tell people to install; `snap_diff-capybara` is a reserved identical mirror. Stated once in the README with the dual-install consequence, applied everywhere else. - Stale "3.0" references in the Rakefile and docs/architecture.md are now 2.1 (#247 fixed the user docs and missed these). Corrections to claims that were not true - docs/drivers.md promised that everything 2.1 removes "warns once per process naming 2.1". `driver: :auto` is silent whenever ruby-vips is present, and the `driver:` setting itself never warns at all even though 2.1 deletes it (`NoMethodError`). Both are now written down as silent, in drivers.md and UPGRADING.md, since a note is the only notice they can get. - README called ruby-vips "Optional". With neither ruby-vips nor chunky_png installed, comparisons raise `Wrong adapter nil. Available adapters: []`. Says so now. - Setup examples no longer teach `driver: :vips`, a line users have to delete for 2.1. Gem hygiene - gemspec: summary/description that describe what the gem does, the rubygems metadata links (source, changelog, bug tracker, docs), and docs/docker-testing.md dropped from the package (it documents bin/dtest, which is not packaged). Dead bindir/executables removed -- the allow-list never matched exe/. - README's links to CONTRIBUTING.md and docker-testing.md are absolute, so they resolve from inside the gem too. - test/unit/gemspec_packaging_test.rb pins the packaged file list: both Bundler.require entry files present (this broke twice), consumer docs in, contributor docs and build files out, capybara the only runtime dependency. Verified it fails when an entry file is unpackaged. - *.gem is gitignored. Release process - The GitHub Release body linked to blob/main on a repo whose default branch is master -- 404 on every release so far. Links to the tag now. - docs/RELEASE_PREP.md was a stale v1.15.1 checklist. It is now a runbook for how releases actually happen: what the workflow does step by step, the trusted-publisher prerequisite for BOTH gem names, prereleases, post-release verification, and what to do when a run fails halfway. - CONTRIBUTING.md pointed at the wrong version.rb and recommended `rake release`, which publishes only one of the two gem names. Verified with real installs on ruby 4.0.6: 1.15.1 -> this master via path:, a canonical-names setup, `Bundler.require` under each gem name from the built .gem, and the dual-install guard with both gems installed. No lib/ changes. Version not bumped. * docs: SnapDiff::Error is the base class for errors the gem defines, not every error it raises docs/snapdiff.md's object map said "Base class for every error this gem raises". It is not: a missing image backend raises a bare RuntimeError ("Wrong adapter nil. Available adapters: []", reproduced on a bundle with neither ruby-vips nor chunky_png) and StableScreenshoter raises ArgumentError. Verified the four defined errors -- ExpectationNotMet, UnstableImage, WindowSizeMismatchError, DualInstallError -- do all inherit SnapDiff::Error, so the useful half of the promise holds and is now the one being made. * docs: tag protection, not branch protection, is what gates the release tag push Branch protection rules do not govern tag pushes; tag protection rules (or rulesets) do. The runbook prerequisite now names the right control. * docs: the per-screenshot driver: override dies quietly, the config setting dies loudly Both were lumped together as "raises NoMethodError on 2.1". Only the config setting does. Per-screenshot options are a free-form hash, so on 2.1 `screenshot "index", driver: :vips` is inert and nothing tells you the line is dead -- #249's own upgrade note spells out the split. Grep-for-it advice added, since that is the only signal a user gets. * docs: pin versions, and stop telling people to delete baselines Second pass, from customer-persona findings. Each verified here before acting; two of the four reported items turned out to be artifacts of the PUBLISHED beta3 rather than of master, and are handled as such. Baselines (the oldest bug in the tracker: #5 and #6 in 2018, #133 in 2024) - README told users to "delete the baseline and re-run" in two places. It cannot work. `Vcs.checkout_vcs` (lib/snap_diff/vcs.rb:24) resolves every baseline with `git show HEAD:<path>`, and `ScreenshotMatcher#check_base_screenshot` calls it before `need_to_compare?` tests `base_path.exist?` -- so a committed baseline is fetched from HEAD no matter what the working tree says, and `rm` changes nothing. - New first-class "Accepting an intentional change" section: the mechanism, the commit that actually accepts it, and the surprising part -- staging is not enough, so no local run goes green until you commit. The FAQ answer now says the same thing instead of the opposite. - Deliberately does NOT document RECORD_SCREENSHOTS. It is printed by our own error message (screenshot_matcher.rb:73) but read nowhere in lib/; a separate lane is implementing it, and it should be documented once it works, not before. Version pinning - `gem "snap_diff-capybara"` unpinned installs 0.0.1 -- a placeholder whose entire payload is one README and zero Ruby files (verified by fetching and unpacking it), so the user gets an immediate LoadError. And unpinned `gem "capybara-screenshot-diff"` resolves to 1.15.1, not to the 2.0 the surrounding prose is selling. Every install instruction now pins, and the README says plainly that the mirror name is not the one to reach for. CHANGELOG, all verified - Failure messages leaked a libvips pointer struct via the comparison metadata; `to_h` excludes `diff_mask` since #234, which landed after the beta3 tag, so 2.0.0 final is the fix. - Known limitation: fork-parallel runs write no HTML report. Workers accumulate assertions per process; the report is written from `Minitest.after_run` in the parent (integrations/minitest.rb:69), which never sees them. Artifacts and pass/fail are unaffected. - A note for anyone sitting on a prerelease: beta3's deprecation channel was incomplete, so its silence is not evidence of being migrated. Constants - `Capybara::Screenshot::Os` -> `SnapDiff::Os` was in no rename table. Gemspec - rubygems_mfa_required. The four URI fields were added in the first commit. * docs: stop teaching two commands that do not work Two customer personas independently followed the docs and got a green bar on a page they had deliberately broken. `rake test` does not run `test/system/` in a Rails app. The Quick Start told users to run it, so step 1 produced `0 runs` and no baselines -- which reads as a pass. The example is a Rails system test; the command now matches it, with a callout, because "0 runs" is the single easiest way to believe visual testing is working when nothing is running. `RECORD_SCREENSHOTS=1` appeared in three user-facing docs for a feature that has never existed in `lib/` -- it is this repository's own test-suite convention, read by `test/test_helper.rb`. The user-facing copies are replaced with the flow that actually works: run the suite, which rewrites every changed baseline in place, then `git add` and commit. The contributor page keeps it and now says plainly that it is not a library feature. * docs: make the Quick Start something a new user can actually run A reviewer built a stock Rails 8.1 app, ran the documented Quick Start end to end, and none of it worked. Fixes, each verified against a scratch app or against lib/: - `gem "capybara-screenshot-diff", "~> 2.0"` does not resolve. rubygems has 1.15.1 and 2.0.0.alpha1..beta3 and no final 2.x, and Bundler never picks a prerelease from a plain requirement, so `bundle install` fails with `Could not find gem 'capybara-screenshot-diff (~> 2.0)'`. All five install snippets now pin `2.0.0.beta3` and say why. RELEASE_PREP gains the step that swaps them back to `~> 2.0` as part of the 2.0.0 push, so the good pin lands with the release rather than before it. - The Quick Start taught the API 2.1 deletes, silently. `require "capybara_screenshot_diff/minitest"` + `include CapybaraScreenshotDiff::Minitest::Assertions` print nothing: they are eager aliases, so `const_missing` never fires. The Quick Start now starts on canonical `SnapDiff`, and README/CHANGELOG/snapdiff.md say which doors actually warn (config accessors, `include`, `default_options`, `const_missing`) and which cannot. - The CI "Record new baselines" job could not record a new baseline. `check_base_screenshot` runs before `capture_screenshot` and `fail_if_new` is true whenever `ENV["CI"]` is set, so a new screenshot raises before anything is written and the commit step finds nothing. The job now clears `CI` for that step. - `bundle exec rake test` / `rails test` swept out of the three CI workflows, "The Short Version", and the historical upgrade sections: in a Rails app they skip `test/system/` and report `0 runs`. - The delete-the-baselines block in UPGRADING replaced with the commit workflow the README documents. - `docs/drivers.md` told you to delete the `driver:` setting on one screen and to add it on another; same for configuration.md, migration-guide.md. - `rescue SnapDiff::Error` does not catch a failed assertion under the framework integrations -- Minitest converts it to `Minitest::Assertion` and RSpec to `ExpectationNotMetError`. Said so. - "2.0 will not rewrite a baseline you already committed" contradicted the README and reality: a failing run does rewrite the baseline path. Reworded to what is meant (no re-encoding) plus what actually happens. Also, all verified in a scratch app: the `git add test/fixtures/screenshots/` path was never the default (`doc/screenshots` is); the example failure output showed a `max_color_distance` key the vips path never emits; the artifact table listed three of five files; `application_system_test_case.rb` omitted both `require "test_helper"` and `driven_by` (without the latter the same page captures at 2800x1610 instead of 1400x1257); `homepage_test.rb` omitted `require "application_system_test_case"` and raised NameError as printed; and `DEBUG=1` never had anything to do with keeping `.diff.png` files. Docs only. rake test:unit 610/0, standardrb clean. * docs: three residuals from the verifier pass - bug_report template told reporters to run `rake test`, which runs zero system tests in a Rails app -- the same trap this branch exists to remove. - drivers.md said an unknown per-screenshot `driver:` key is "simply inert". It is validated and raises; only the deprecation warning is absent. - drivers.md said `:auto` and `:chunky_png` each warn once per process. `:auto` is silent when ruby-vips resolves; only `:chunky_png` warns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three independent backlog items from the beta2 review. No version/CHANGELOG changes — this line ships nothing until 3.0-readiness completes.
Baseline (
mise x ruby@4.0.6 -- bundle exec):rake test:unit522/0,rake test555/0/6.After:
rake test:unit523 runs, 0 failures, 0 skips ·rake test551 runs, 0 failures, 1 skip ·standardrbclean (149 files, no offenses).1.
Vips::Imageleaked into user-facing failure messagesBroken.
ComparisonResult#to_hmerged the wholemetahash, and the vips driver stores the raw diff mask image atmeta[:diff_mask](lib/snap_diff/drivers/vips_driver.rb:37).Reporters::Default#build_error_messageJSON-dumpsto_h, so a failing assertion printed:#inspectwas already clean (#214);#to_hwas the remaining path.Fix.
#to_hexcludes:diff_mask— it is an image object, not a metric, and the dedicated#diff_maskaccessor still exposes it to programmatic consumers.to_his not documented public API (nothing indocs/references it; its only in-tree caller is the message builder), so no summarisation shim was added.Red → green. Guard test
test/unit/reporters/default_test.rbasserts the metrics line carriesarea_size/region/difference_leveland contains neitherVips::Imagenor0x.Expected "({...,\"diff_mask\":\"#<Vips::Image:0x000000012b152468>\",...})\n" to not include "Vips::Image".— 1 runs, 1 failuresThe assertion is scoped to
message.lines.firstbecause the tmpdir paths on the following lines contain a literal0x.chunky_png cross-check — its metadata is scalars only and never held a mask, so the message was already clean and is unchanged:
2. Five HTML-report tests that never ran anywhere
Decision: delete them (option b). Rationale:
test/integration/report_screenshot_test.rb:13skipped unlessRECORD_SCREENSHOTS— but in this repo that variable means record/refresh baselines, not verify. Everywhere else (browser_screenshot_test.rb,test_helper.rb:104) it is the flag that suppresses comparison. Gating the tests on it meant they only ran in the mode that overwrites the thing they would compare against.macos/cuprite/html_report; the only CI platform is linux, which has nohtml_reportbaselines at all. Wiring the env var into thefull-cijob or the weekly cron would have bought real browser cost for a job that records five PNGs and passes unconditionally.report_both/report_heatmap/report_annotated_both.rake report:sample(scripts/generate_sample_report.rb) already generates a sample HTML report and is wired into CI as thetest-report-uploadjob. The deleted file re-implemented that generation and added browser captures on top.test/unit/reporters/html_reporter_test.rbhas 18 tests over the HTML reporter — the surface refactor: default reporter, Comparison::Images, driver registry get SnapDiff homes (ADR-008 steps 4+5) #227/refactor: session surface and reporter registration get SnapDiff homes (ADR-008 step 6) #228 changed.Deleted the test file and the five orphan macOS baselines.
Outcome check — no test is permanently skipped by an env var nobody sets. The only remaining env-gated skip is
optional_test(test_helper.rb:107), andDISABLE_SKIP_TESTS: 1is set in CI (.github/workflows/test.yml:83). Skip count drops 6 → 1 accordingly.3. Packaging
Broken. The deny-list regex let
gems.rb,Rakefileandcapybara-screenshot-diff.gemspecinto the gem (the gemspec is even named after gem A inside mirror gem B) while excludingREADME.md— leaving[← Back to README](../README.md)dead in 8 packaged doc pages.Fix. Replaced the deny-list with an allow-list over
git ls-files(still tracked-files-only, but fail-closed):Verified by building the gem before and after (
gem build→Gem::Package#spec.files), 97 → 94 files:Unpacked
after.gemtop level is exactlyCHANGELOG.md docs/ lib/ LICENSE.txt README.md;Rakefile,gems.rband*.gemspecare absent.Mirror gemspec (
.github/workflows/release.yml:75-83) re-verified end to end — ran the workflow's ownGem::Specification.load→ rename →to_rubysnippet and thengem buildon the generated file:Successfully built RubyGem / Name: snap_diff-capybara / Version: 2.0.0.beta3, 94 files. The generatedsnap_diff-capybara.gemspecis untracked, so unlike before neither gemspec now ends up inside either gem.Judgment calls worth a look:
CODE_OF_CONDUCT.mdalso drops out (not linked from any packaged page), anddocs/RELEASE_PREP.mdstill ships withdocs/— say the word and I will carve it out.README.md's link toCONTRIBUTING.mdis dead inside the gem; left alone as a GitHub-facing pointer.🤖 Generated with Claude Code
Summary by Sourcery
Clean up 3.0-readiness issues by sanitizing failure output, removing unusable report tests, and tightening gem contents.
Bug Fixes:
Enhancements:
Build:
Tests: