From a4c66a7a07c9a69d8e3ae2bc7adbb03110d3ad90 Mon Sep 17 00:00:00 2001 From: pftg Date: Tue, 6 Oct 2026 10:23:15 +0000 Subject: [PATCH 1/3] refactor: decouple AI from reporters via a Contributions registry Core no longer names the AI module. SnapDiff::Contributions is the single extension point, following Minitest's composite reporter (plugins self-register into a core-owned list) and SimpleCov's formatter pipeline (consumers get a plain payload, never a plugin's class): - annotate(name) -> {source:, text:, data:} feeds the HTML report and the assertion failure message; loading snap_diff/ai self-registers. - one failure-suppression slot replaces AI.gate; AISimple with fail_on: claims it. any_suppressor? keeps the no-gate path from touching compare.difference. User-visible strings ([snap_diff:ai] ..., 'AI triage: ...', HTML badge) are unchanged. --- CHANGELOG.md | 7 ++ docs/ai.md | 42 ++++++++--- lib/snap_diff.rb | 1 + lib/snap_diff/ai.rb | 27 ++++--- lib/snap_diff/contributions.rb | 57 +++++++++++++++ lib/snap_diff/reporters/ai_simple.rb | 20 +++-- lib/snap_diff/reporters/html.rb | 25 +++---- .../reporters/templates/report.html.erb | 23 ++++-- lib/snap_diff/screenshot_assertion.rb | 18 +++-- test/integration/ai_triage_test.rb | 4 +- test/unit/contributions_test.rb | 73 +++++++++++++++++++ test/unit/reporters/ai_simple_test.rb | 20 ++--- 12 files changed, 248 insertions(+), 69 deletions(-) create mode 100644 lib/snap_diff/contributions.rb create mode 100644 test/unit/contributions_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 531e8003..1c36dd68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,13 @@ kept as history. This entry is the one to read if you are coming from **1.15.1** is offline CLIP via the `informers` gem; custom backends (local VLM, decision APIs) register by name via `SnapDiff::AI.register`. Zero new hard dependencies. See `docs/ai.md`. +- **Report contributions registry.** Core reporters and the assertion failure + message no longer reference the AI module at all: `SnapDiff::Contributions` + is the single extension point (Minitest/SimpleCov style — modules + self-register, core renders plain `{source:, text:, data:}` payloads). + Loading `snap_diff/ai` opts into annotations; `fail_on:` claims the one + failure-suppression slot. Any module can contribute annotations the same + way. See `docs/ai.md`. ### Upgrading from 1.15.1: change the version, run your suite diff --git a/docs/ai.md b/docs/ai.md index a468877d..47f41e6e 100644 --- a/docs/ai.md +++ b/docs/ai.md @@ -18,20 +18,39 @@ One line per diff in the test output, and — automatically, via the shared store — a verdict badge plus one-line summary per failure in `snap_diff_report.html`. No files, no duplicate storage. -## How the HTML report gets AI annotations +## How reports get AI annotations -Both reporters read the same shared store: `AISimple` writes each result -into `SnapDiff::AI` (keyed by screenshot name), and the HTML reporter -attaches `SnapDiff::AI[name]` to the failure entry — behind a `defined?` -guard, so `html.rb` never requires the AI module and nothing changes -when AI triage isn't loaded. No wiring between the two reporters: +Core never names the AI module. It exposes one registry, +`SnapDiff::Contributions`, and loading `snap_diff/ai` self-registers +into it — the same shape as Minitest plugins appending to the +`CompositeReporter`, or SimpleCov formatters receiving a plain payload: + +- `AISimple` writes each result into the shared `SnapDiff::AI` store + (keyed by screenshot name). +- `SnapDiff::AI.annotate(name)` adapts a stored result to the generic + `{source:, text:, data:}` contribution shape. +- The HTML reporter and the assertion failure message ask + `Contributions.annotations_for(name)` and render whatever comes back — + empty when AI was never loaded, so the no-AI report is byte-clean. ```ruby require "snap_diff/reporters/html" # already auto-registers -require "snap_diff/reporters/ai_simple" +require "snap_diff/reporters/ai_simple" # self-registers into Contributions SnapDiff::Reporting.register(SnapDiff::Reporters::AISimple.new) ``` +Your own module can contribute the same way — no edits to core: + +```ruby +module TicketLinker + def self.annotate(name) + ticket = JIRA_FOR[name] + ticket && {source: "jira", text: ticket} + end +end +SnapDiff::Contributions.register(TicketLinker) +``` + The report then shows, per failure: an `AI: real bug` / `AI: flaky` badge in the sidebar and top strip, plus `similarity · confidence · summary` when the backend provides them. Under fork-parallel, results merge in the @@ -47,11 +66,12 @@ parent before the report renders. ## Architecture ``` -lib/snap_diff/ai.rb # backend registry, verdict thresholds, shared result store +lib/snap_diff/contributions.rb # core registry: annotations + the one failure gate +lib/snap_diff/ai.rb # backend registry, verdict thresholds, shared store; self-registers lib/snap_diff/ai/backends/clip.rb # built-in offline backend (informers) -lib/snap_diff/reporters/ai_simple.rb # record/finalize/summary; writes the store -lib/snap_diff/reporters/html.rb # annotates failures from the store (defined? guard) -lib/snap_diff/screenshot_assertion.rb# validate: fail-gate + AI line in the failure message +lib/snap_diff/reporters/ai_simple.rb # record/finalize/summary; suppression via Contributions +lib/snap_diff/reporters/html.rb # renders Contributions.annotations_for — no AI reference +lib/snap_diff/screenshot_assertion.rb# validate: Contributions gate + annotation lines ``` A backend is any object with `#call(name:, base:, current:, meta:) -> Hash`. diff --git a/lib/snap_diff.rb b/lib/snap_diff.rb index c0ce8493..386380fe 100644 --- a/lib/snap_diff.rb +++ b/lib/snap_diff.rb @@ -47,6 +47,7 @@ def self.assert_single_gem!(loaded_specs = Gem.loaded_specs) require "capybara/dsl" require "snap_diff/config" require "snap_diff/comparison" +require "snap_diff/contributions" require "snap_diff/legacy_shims" require "snap_diff/version" # SnapDiff.session/.reset/.pending_screenshots_message are part of the diff --git a/lib/snap_diff/ai.rb b/lib/snap_diff/ai.rb index 4eb15698..811e9ea7 100644 --- a/lib/snap_diff/ai.rb +++ b/lib/snap_diff/ai.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "snap_diff/contributions" + # Optional AI triage. A backend is any object responding to # #call(name:, base:, current:, meta:) -> Hash; register one by name or # pass an instance. Builders run lazily, so optional gems load only when @@ -7,8 +9,9 @@ # unless a reporter is configured with fail_on: (see AISimple). # # Results live in a process-wide store so ANY consumer can read them -- -# the AISimple reporter writes, the HTML reporter annotates from it, the -# fail-gate consults it. Keyed by screenshot name; later writes win. +# the AISimple reporter writes, and reports pick them up through the +# SnapDiff::Contributions registry (no consumer names this module). +# Keyed by screenshot name; later writes win. module SnapDiff module AI # The only verdicts a backend may return; anything else maps to @@ -20,15 +23,12 @@ module AI @mutex = Mutex.new class << self - # Optional fail-gate, set by AISimple when configured with fail_on:. - # ScreenshotAssertion#validate consults it on a pixel diff: verdicts - # the gate accepts suppress the failure, the rest still fail. - attr_accessor :gate - - def gated_result(name, difference) = gate&.gated_result(name, difference) - - # No gate -> everything fails, exactly as without AI. - def fails?(verdict) = gate ? gate.fails?(verdict) : true + # Report contribution (SnapDiff::Contributions): whatever the store + # holds for this screenshot, rendered one line plus the raw payload. + def annotate(name) + result = self[name] + result && {source: "ai", text: format(result), data: result} + end def register(name, &build) @mutex.synchronize { @backends[name.to_sym] = build } @@ -113,3 +113,8 @@ def merge_state!(state) end require "snap_diff/ai/backends/clip" + +# Minitest-style self-registration: loading this file opts into +# annotating reports. Core (HTML reporter, assertion message) only ever +# talks to SnapDiff::Contributions and never references this module. +SnapDiff::Contributions.register(SnapDiff::AI) diff --git a/lib/snap_diff/contributions.rb b/lib/snap_diff/contributions.rb new file mode 100644 index 00000000..4e2ff1b5 --- /dev/null +++ b/lib/snap_diff/contributions.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +# Contribution points: how OPTIONAL modules (AI triage today, anything +# else tomorrow) feed the reports and the failure decision without the +# reporters or the assertion knowing those modules exist. +# +# The contract follows the two proven shapes in the ecosystem: +# Minitest's CompositeReporter (plugins append themselves to a +# core-owned list; core calls a uniform interface and never names a +# plugin) and SimpleCov's formatter pipeline (consumers receive a plain +# data payload, never a plugin's class). A contributor is any object +# responding to #annotate(name) -> {source:, text:, data:} or nil; +# reports render whatever comes back. +# +# Failure suppression is deliberately a SINGLE slot (it was +# SnapDiff::AI.gate before): two gates with different accept-lists +# would silently suppress each other's real bugs, so registration +# replaces the previous gate rather than stacking. +module SnapDiff + module Contributions + @providers = [] + @suppression = nil + @mutex = Mutex.new + + class << self + # Register a report contributor. The provider must respond to + # #annotate(name), returning {source:, text:, data: (optional)} + # or nil. Registering the same object twice is a no-op. + def register(provider) + @mutex.synchronize { @providers << provider unless @providers.include?(provider) } + end + + # All contributions for one screenshot, in registration order. + # Empty when nothing is registered -- the no-AI default. + def annotations_for(name) + @mutex.synchronize { @providers.dup }.filter_map { |provider| provider.annotate(name) } + end + + # The one failure gate: #suppress(name, difference) -> + # {source:, text:} (failure waived) or nil (failure stands). + # nil clears the slot (test teardown, reconfiguration). + def register_suppression(provider) + @mutex.synchronize { @suppression = provider } + end + + # Cheap probe so callers can skip building `difference` entirely + # when no gate is registered. + def any_suppressor? + @mutex.synchronize { !@suppression.nil? } + end + + def suppression_for(name, difference) + @mutex.synchronize { @suppression }&.suppress(name, difference) + end + end + end +end diff --git a/lib/snap_diff/reporters/ai_simple.rb b/lib/snap_diff/reporters/ai_simple.rb index a7895305..47ad943f 100644 --- a/lib/snap_diff/reporters/ai_simple.rb +++ b/lib/snap_diff/reporters/ai_simple.rb @@ -33,7 +33,9 @@ def initialize(backend: nil, flaky: nil, intentional: nil, fail_on: nil) # fresh regression. @memo = {} @memo_mutex = Mutex.new - AI.gate = self if @fail_on + # The one failure-gate slot in SnapDiff::Contributions -- core + # consults it without knowing AI exists. + Contributions.register_suppression(self) if @fail_on end def record(assertions) @@ -47,13 +49,17 @@ def record(assertions) end end - # Fail-gate entry point, called by ScreenshotAssertion#validate on a - # pixel diff. Analysis runs there (before the error message is - # built) and is memoized, so #record never re-analyzes. - def gated_result(name, difference) - return unless @fail_on && @backend + # Failure-gate contract (SnapDiff::Contributions), consulted by + # ScreenshotAssertion#validate on a pixel diff, before the error + # message is built. Analysis is memoized, so #record never + # re-analyzes. Returns {source:, text:} to waive the failure, + # nil to let it stand. "unknown" ALWAYS fails -- AI can downgrade + # a diff, never vouch for one it could not classify. + def suppress(name, difference) + return unless @backend - analyze_once(name, difference) + result = analyze_once(name, difference) + {source: "ai", text: AI.format(result)} unless fails?(result[:verdict]) end def fails?(verdict) = verdict == "unknown" || @fail_on.include?(verdict) diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index b0429e81..ba324414 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -97,7 +97,7 @@ def summary end def render - attach_ai_annotations + attach_annotations ERB.new(File.read(self.class.template_path)).result(binding) end @@ -127,19 +127,18 @@ def failure_entry_for(name, compare) }.compact end - # Advisory AI triage annotations, attached at RENDER time: HTML - # records before the AI reporter (auto-registration runs first) and - # fork-parallel merges land after record, so only the final render - # can see every verdict. HTML never requires the AI module -- the - # annotation appears iff the user opted into AI triage. - def attach_ai_annotations - return unless defined?(SnapDiff::AI) - + # Contributed annotations (AI triage, ...), attached at RENDER + # time: HTML records before contributing reporters + # (auto-registration runs first) and fork-parallel merges land + # after record, so only the final render can see everything. + # HTML never names a contributor -- it renders whatever + # SnapDiff::Contributions returns, empty by default. + def attach_annotations failures.each do |entry| - # || would create a nil :ai key on misses; keep the entry clean. - if (annotation = SnapDiff::AI[entry[:name]]) - entry[:ai] ||= annotation - end + # ||= would create a nil :annotations key on misses; keep the + # entry clean. + annotations = SnapDiff::Contributions.annotations_for(entry[:name]) + entry[:annotations] ||= annotations unless annotations.empty? end end diff --git a/lib/snap_diff/reporters/templates/report.html.erb b/lib/snap_diff/reporters/templates/report.html.erb index e4ad5993..f4eab113 100644 --- a/lib/snap_diff/reporters/templates/report.html.erb +++ b/lib/snap_diff/reporters/templates/report.html.erb @@ -274,7 +274,11 @@ '
' + ''; btn.addEventListener('click', function() { selectItem(+this.dataset.idx); }); @@ -311,16 +315,19 @@ } topBadge.className = 'diff-badge ' + (hasDiff ? 'diff-badge-fail' : 'diff-badge-pass'); - /* AI triage strip: shown only when an advisory verdict exists */ + /* Contribution strip (AI triage, ...): shown when a contributor + supplied a verdict payload */ var aiBar = $('ai-bar'); - if (item.ai && item.ai.verdict) { + var note = (item.annotations || []).find(function(a) { return a.data && a.data.verdict; }); + if (note) { + var d = note.data; var aiV = $('ai-verdict'); - aiV.textContent = 'AI: ' + item.ai.verdict.replace('_', ' '); - aiV.className = 'ai-badge' + aiClass(item.ai.verdict); + aiV.textContent = note.source.toUpperCase() + ': ' + d.verdict.replace('_', ' '); + aiV.className = 'ai-badge' + aiClass(d.verdict); var bits = []; - if (item.ai.similarity != null) bits.push('similarity ' + item.ai.similarity); - if (item.ai.confidence != null) bits.push('confidence ' + item.ai.confidence); - if (item.ai.summary) bits.push(item.ai.summary); + if (d.similarity != null) bits.push('similarity ' + d.similarity); + if (d.confidence != null) bits.push('confidence ' + d.confidence); + if (d.summary) bits.push(d.summary); $('ai-text').textContent = bits.join(' · '); aiBar.className = 'visible'; } else { diff --git a/lib/snap_diff/screenshot_assertion.rb b/lib/snap_diff/screenshot_assertion.rb index 1b8cc21c..43f5aa2e 100644 --- a/lib/snap_diff/screenshot_assertion.rb +++ b/lib/snap_diff/screenshot_assertion.rb @@ -64,18 +64,20 @@ def validate return unless compare if compare.different? - # Optional AI gate (SnapDiff::Reporters::AISimple with fail_on:): - # the verdict is computed here, before the message is built, so - # the failure text can quote it and accepted verdicts can skip it. - ai = SnapDiff::AI.gated_result(name, compare.difference) if defined?(SnapDiff::AI) && SnapDiff::AI.gate - if ai && !SnapDiff::AI.fails?(ai[:verdict]) - $stdout.puts "[snap_diff:ai] #{name}: failure suppressed -- #{SnapDiff::AI.format(ai)}" + # Optional contribution gate (e.g. AI triage with fail_on:): a + # registered suppressor runs BEFORE the message is built, so a + # waived diff never fails and a standing failure can quote what + # the contributors know. any_suppressor? keeps the no-gate path + # from even touching compare.difference. + if Contributions.any_suppressor? && (suppressed = Contributions.suppression_for(name, compare.difference)) + $stdout.puts "[snap_diff:#{suppressed[:source]}] #{name}: failure suppressed -- #{suppressed[:text]}" return nil end message = "Screenshot does not match for '#{name}': #{compare.error_message}\n#{caller.join("\n")}" - ai ||= SnapDiff::AI[name] if defined?(SnapDiff::AI) - message += "\n AI triage: #{SnapDiff::AI.format(ai)}" if ai + Contributions.annotations_for(name).each do |note| + message += "\n #{note[:source].upcase} triage: #{note[:text]}" + end message else archive_baseline! diff --git a/test/integration/ai_triage_test.rb b/test/integration/ai_triage_test.rb index 6cd9d0b1..9fcfa042 100644 --- a/test/integration/ai_triage_test.rb +++ b/test/integration/ai_triage_test.rb @@ -19,8 +19,8 @@ class AiTriageTest < ActiveSupport::TestCase # Force with RUN_AI_TESTS=1. Fails OPEN: when git can't tell (shallow # checkout, no origin/master) or we're on master, the tests run. AI_SURFACE = %r{\A(?: - lib/snap_diff/(?:ai\.rb|reporters/ai_simple\.rb) - | test/(?:unit/reporters/ai_simple_test\.rb|integration/ai_triage_test\.rb|fixtures/ai_triage_case\.rb) + lib/snap_diff/(?:ai\.rb|contributions\.rb|reporters/ai_simple\.rb) + | test/(?:unit/(?:reporters/ai_simple_test|contributions_test)\.rb|integration/ai_triage_test\.rb|fixtures/ai_triage_case\.rb) )\z}x def self.ai_surface_changed? diff --git a/test/unit/contributions_test.rb b/test/unit/contributions_test.rb new file mode 100644 index 00000000..b103b6d3 --- /dev/null +++ b/test/unit/contributions_test.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require "test_helper" + +class ContributionsTest < Minitest::Test + Provider = Struct.new(:note) do + def annotate(name) = note + end + + def setup + # Other tests may have loaded AI and filled its store; annotations + # must start from a known-empty state regardless of test order. + SnapDiff::AI.clear_results! if defined?(SnapDiff::AI) + end + + def teardown + SnapDiff::Contributions.register_suppression(nil) + SnapDiff::Contributions.instance_variable_get(:@providers).clear + # ai.rb self-registers on load; other tests rely on that, so put it + # back if the AI module is around. + if defined?(SnapDiff::AI) + SnapDiff::Contributions.register(SnapDiff::AI) + end + end + + def test_annotations_empty_without_providers + assert_empty SnapDiff::Contributions.annotations_for("never-recorded-name") + end + + def test_annotations_skip_nil_and_keep_order + first = Provider.new({source: "first", text: "one"}) + nothing = Provider.new(nil) + second = Provider.new({source: "second", text: "two"}) + [first, nothing, second].each { |p| SnapDiff::Contributions.register(p) } + + assert_equal %w[first second], SnapDiff::Contributions.annotations_for("x").map { |a| a[:source] } + end + + def test_registering_the_same_provider_twice_is_a_no_op + provider = Provider.new({source: "ai", text: "t"}) + 2.times { SnapDiff::Contributions.register(provider) } + + assert_equal 1, SnapDiff::Contributions.annotations_for("x").size + end + + def test_no_suppressor_by_default + refute SnapDiff::Contributions.any_suppressor? + assert_nil SnapDiff::Contributions.suppression_for("x", Object.new) + end + + def test_suppression_single_slot_replaces + waive = ->(_name, _diff) { {source: "ai", text: "FLAKY"} } + waiving = Object.new + waiving.define_singleton_method(:suppress) { |name, diff| waive.call(name, diff) } + standing = Object.new + standing.define_singleton_method(:suppress) { |_name, _diff| nil } + + SnapDiff::Contributions.register_suppression(waiving) + SnapDiff::Contributions.register_suppression(standing) + + # Last registration wins: the earlier gate must not keep waiving. + assert_nil SnapDiff::Contributions.suppression_for("x", Object.new) + end + + def test_suppression_returns_the_waiver + waiving = Object.new + waiving.define_singleton_method(:suppress) { |_name, _diff| {source: "ai", text: "FLAKY (clip)"} } + SnapDiff::Contributions.register_suppression(waiving) + + assert SnapDiff::Contributions.any_suppressor? + assert_equal({source: "ai", text: "FLAKY (clip)"}, SnapDiff::Contributions.suppression_for("x", Object.new)) + end +end diff --git a/test/unit/reporters/ai_simple_test.rb b/test/unit/reporters/ai_simple_test.rb index 21d0a050..f22c7099 100644 --- a/test/unit/reporters/ai_simple_test.rb +++ b/test/unit/reporters/ai_simple_test.rb @@ -30,7 +30,7 @@ def setup end def teardown - SnapDiff::AI.gate = nil + SnapDiff::Contributions.register_suppression(nil) end def gate_assertion(name, different: true) @@ -317,9 +317,11 @@ def test_failures_carry_ai_annotation_after_render reporter.finalize checkout, plain = reporter.failures - assert_equal "real_bug", checkout[:ai][:verdict] - assert_equal "CTA clipped", checkout[:ai][:summary] - refute plain.key?(:ai) + annotation = checkout[:annotations].find { |a| a[:source] == "ai" } + assert_equal "real_bug", annotation[:data][:verdict] + assert_equal "CTA clipped", annotation[:data][:summary] + assert_equal "REAL_BUG (clip, similarity=0.7312) -- CTA clipped", annotation[:text] + refute plain.key?(:annotations) end end @@ -332,7 +334,7 @@ def test_ai_result_recorded_after_html_record_still_renders SnapDiff::AI.record_result(name: "checkout", verdict: "flaky", backend: "clip", similarity: 0.9912) reporter.finalize - assert_equal "flaky", reporter.failures.first[:ai][:verdict] + assert_equal "flaky", reporter.failures.first[:annotations].first[:data][:verdict] end end @@ -351,16 +353,16 @@ def test_rendered_report_includes_ai_bar_markup end def test_rendered_report_without_ai_stays_clean - # AI not enabled (store empty): entries must not gain an :ai key, and - # the serialized DATA must contain no ai annotations at all. + # AI not enabled (store empty): entries must not gain an :annotations + # key, and the serialized DATA must contain no annotations at all. Dir.mktmpdir do |dir| reporter = html_reporter(dir) reporter.record([failed_assertion("checkout"), failed_assertion("plain")]) reporter.finalize - reporter.failures.each { |entry| refute entry.key?(:ai) } + reporter.failures.each { |entry| refute entry.key?(:annotations) } html = File.read(File.join(dir, "report.html")) - refute_includes html, '"ai":' + refute_includes html, '"annotations":' end end end From 1cdfc0ae70232f00d4df4f19ef2204866f7f6d78 Mon Sep 17 00:00:00 2001 From: pftg Date: Tue, 6 Oct 2026 15:10:34 +0000 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20review=20round=201=20=E2=80=94=20nil?= =?UTF-8?q?-analysis=20gate=20crash,=20identity=20dedupe,=20text-only=20an?= =?UTF-8?q?notations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AISimple#suppress: a failed analysis (backend raised) returns nil, the pixel failure stands instead of NoMethodError during validation - Contributions.register dedupes by equal? — two distinct providers that compare == must both contribute - HTML report renders note.text for annotations without a verdict payload (the documented TicketLinker shape showed a bare 'JIRA') - contributions_test snapshots/restores the provider list instead of clearing shared registry state - AI-surface gate: capture2 + status checks — capture2e put git's 'fatal:' text where a merge-base was expected and silently SKIPPED the AI tests on shallow CI checkouts --- lib/snap_diff/contributions.rb | 6 ++- lib/snap_diff/reporters/ai_simple.rb | 3 ++ .../reporters/templates/report.html.erb | 23 ++++++++--- test/integration/ai_triage_test.rb | 11 ++++-- test/unit/contributions_test.rb | 21 ++++++---- test/unit/reporters/ai_simple_test.rb | 38 +++++++++++++++++++ 6 files changed, 83 insertions(+), 19 deletions(-) diff --git a/lib/snap_diff/contributions.rb b/lib/snap_diff/contributions.rb index 4e2ff1b5..99fa12a4 100644 --- a/lib/snap_diff/contributions.rb +++ b/lib/snap_diff/contributions.rb @@ -25,9 +25,11 @@ module Contributions class << self # Register a report contributor. The provider must respond to # #annotate(name), returning {source:, text:, data: (optional)} - # or nil. Registering the same object twice is a no-op. + # or nil. Registering the same object twice is a no-op -- identity, + # not ==: two distinct providers that happen to compare equal + # (e.g. Structs with equal fields) must BOTH contribute. def register(provider) - @mutex.synchronize { @providers << provider unless @providers.include?(provider) } + @mutex.synchronize { @providers << provider unless @providers.any? { |p| p.equal?(provider) } } end # All contributions for one screenshot, in registration order. diff --git a/lib/snap_diff/reporters/ai_simple.rb b/lib/snap_diff/reporters/ai_simple.rb index 47ad943f..54cdd1fa 100644 --- a/lib/snap_diff/reporters/ai_simple.rb +++ b/lib/snap_diff/reporters/ai_simple.rb @@ -58,7 +58,10 @@ def record(assertions) def suppress(name, difference) return unless @backend + # nil analysis (the backend raised) -> the pixel failure stands. result = analyze_once(name, difference) + return unless result + {source: "ai", text: AI.format(result)} unless fails?(result[:verdict]) end diff --git a/lib/snap_diff/reporters/templates/report.html.erb b/lib/snap_diff/reporters/templates/report.html.erb index f4eab113..86c5aa34 100644 --- a/lib/snap_diff/reporters/templates/report.html.erb +++ b/lib/snap_diff/reporters/templates/report.html.erb @@ -276,8 +276,12 @@ '
' + esc(item.name) + '
' + (item.annotations || []).map(function(note) { var v = note.data && note.data.verdict; - return '' + - esc(note.source.toUpperCase() + (v ? ': ' + v.replace('_', ' ') : '')) + ''; + /* verdict annotations badge the verdict; text-only ones + (TicketLinker, ...) must show their text or they render + as a bare source label */ + var label = note.source.toUpperCase() + + (v ? ': ' + v.replace('_', ' ') : (note.text ? ': ' + note.text : '')); + return '' + esc(label) + ''; }).join(' ') + '' + badgeText + '' + ''; @@ -315,8 +319,8 @@ } topBadge.className = 'diff-badge ' + (hasDiff ? 'diff-badge-fail' : 'diff-badge-pass'); - /* Contribution strip (AI triage, ...): shown when a contributor - supplied a verdict payload */ + /* Contribution strip: the verdict layout when a contributor supplied + one (AI triage), source + text for text-only annotations */ var aiBar = $('ai-bar'); var note = (item.annotations || []).find(function(a) { return a.data && a.data.verdict; }); if (note) { @@ -331,7 +335,16 @@ $('ai-text').textContent = bits.join(' · '); aiBar.className = 'visible'; } else { - aiBar.className = ''; + var plain = (item.annotations || [])[0]; + if (plain) { + var plainV = $('ai-verdict'); + plainV.textContent = plain.source.toUpperCase(); + plainV.className = 'ai-badge'; + $('ai-text').textContent = plain.text || ''; + aiBar.className = 'visible'; + } else { + aiBar.className = ''; + } } /* Resolve image sources based on view + annotated toggle */ diff --git a/test/integration/ai_triage_test.rb b/test/integration/ai_triage_test.rb index 9fcfa042..c4b41a09 100644 --- a/test/integration/ai_triage_test.rb +++ b/test/integration/ai_triage_test.rb @@ -25,11 +25,14 @@ class AiTriageTest < ActiveSupport::TestCase def self.ai_surface_changed? return true if ENV["RUN_AI_TESTS"] == "1" - branch, = Open3.capture2e("git", "rev-parse", "--abbrev-ref", "HEAD") + # capture2 (stdout only) + status: with capture2e a missing ref prints + # "fatal: ..." INTO the string and looks like a valid merge-base. + branch, = Open3.capture2("git", "rev-parse", "--abbrev-ref", "HEAD") return true if branch.strip == "master" - merge_base, = Open3.capture2e("git", "merge-base", "HEAD", "origin/master") - return true if merge_base.strip.empty? - changed, = Open3.capture2e("git", "diff", "--name-only", merge_base.strip, "HEAD") + merge_base, status = Open3.capture2("git", "merge-base", "HEAD", "origin/master") + return true unless status.success? && !merge_base.strip.empty? + changed, status = Open3.capture2("git", "diff", "--name-only", merge_base.strip, "HEAD") + return true unless status.success? changed.split("\n").any? { |path| path.match?(AI_SURFACE) } end diff --git a/test/unit/contributions_test.rb b/test/unit/contributions_test.rb index b103b6d3..3335c77b 100644 --- a/test/unit/contributions_test.rb +++ b/test/unit/contributions_test.rb @@ -8,19 +8,17 @@ def annotate(name) = note end def setup - # Other tests may have loaded AI and filled its store; annotations - # must start from a known-empty state regardless of test order. + # Snapshot the shared registry and restore it afterwards -- other + # tests (and ai.rb's load-time self-registration) rely on providers + # registered before this file runs. + @saved_providers = SnapDiff::Contributions.instance_variable_get(:@providers).dup + SnapDiff::Contributions.instance_variable_get(:@providers).clear SnapDiff::AI.clear_results! if defined?(SnapDiff::AI) end def teardown SnapDiff::Contributions.register_suppression(nil) - SnapDiff::Contributions.instance_variable_get(:@providers).clear - # ai.rb self-registers on load; other tests rely on that, so put it - # back if the AI module is around. - if defined?(SnapDiff::AI) - SnapDiff::Contributions.register(SnapDiff::AI) - end + SnapDiff::Contributions.instance_variable_set(:@providers, @saved_providers) end def test_annotations_empty_without_providers @@ -43,6 +41,13 @@ def test_registering_the_same_provider_twice_is_a_no_op assert_equal 1, SnapDiff::Contributions.annotations_for("x").size end + def test_distinct_providers_that_compare_equal_both_contribute + # Structs with equal fields are == but NOT the same provider. + 2.times { SnapDiff::Contributions.register(Provider.new({source: "ai", text: "t"})) } + + assert_equal 2, SnapDiff::Contributions.annotations_for("x").size + end + def test_no_suppressor_by_default refute SnapDiff::Contributions.any_suppressor? assert_nil SnapDiff::Contributions.suppression_for("x", Object.new) diff --git a/test/unit/reporters/ai_simple_test.rb b/test/unit/reporters/ai_simple_test.rb index f22c7099..d62fef90 100644 --- a/test/unit/reporters/ai_simple_test.rb +++ b/test/unit/reporters/ai_simple_test.rb @@ -185,6 +185,19 @@ def test_gate_always_fails_unknown_verdicts assert_includes message, "AI triage: UNKNOWN" end + def test_gate_lets_the_failure_stand_when_analysis_fails + # The backend raising must not turn validation itself into an error: + # the screenshot mismatch is the failure the developer needs. + exploding = ->(name:, base:, current:, meta:) { raise "model server unreachable" } + SnapDiff::Reporters::AISimple.new(backend: exploding, fail_on: %w[real_bug]) + + message = nil + _out, err = capture_io { message = gate_assertion("checkout").validate } + + assert_includes message, "Screenshot does not match for 'checkout'" + assert_includes err, "Backend failed" + end + def test_gate_analysis_is_memoized_for_the_reporter_pass calls = 0 counting_backend = ->(name:, base:, current:, meta:) { @@ -295,6 +308,11 @@ def reporter = HtmlReporterStub.new def setup SnapDiff::AI.clear_results! + @saved_providers = SnapDiff::Contributions.instance_variable_get(:@providers).dup + end + + def teardown + SnapDiff::Contributions.instance_variable_set(:@providers, @saved_providers) end def html_reporter(dir) @@ -365,4 +383,24 @@ def test_rendered_report_without_ai_stays_clean refute_includes html, '"annotations":' end end + + def test_text_only_contribution_renders_its_text + # A contributor without a verdict payload (the documented + # TicketLinker shape) must still show its text, not a bare label. + linker = Object.new + linker.define_singleton_method(:annotate) { |name| {source: "jira", text: "PROJ-123"} } + SnapDiff::Contributions.instance_variable_get(:@providers) << linker + + Dir.mktmpdir do |dir| + reporter = html_reporter(dir) + reporter.record([failed_assertion("checkout")]) + reporter.finalize + + html = File.read(File.join(dir, "report.html")) + assert_includes html, '"source":"jira"' + assert_includes html, '"text":"PROJ-123"' + # and the sidebar badge JS renders the text, not just the source + assert_includes html, "note.text" + end + end end From 5deb73c701156d5a168d38c7211d45abc378aeb7 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:25:46 +0000 Subject: [PATCH 3/3] docs: document AI triage, suppression, reporting, and test behavior --- lib/snap_diff/ai.rb | 1 + lib/snap_diff/contributions.rb | 2 ++ lib/snap_diff/reporters/ai_simple.rb | 4 ++++ lib/snap_diff/reporters/html.rb | 2 ++ lib/snap_diff/screenshot_assertion.rb | 2 ++ test/integration/ai_triage_test.rb | 2 ++ test/unit/contributions_test.rb | 10 ++++++++++ test/unit/reporters/ai_simple_test.rb | 14 ++++++++++++++ 8 files changed, 37 insertions(+) diff --git a/lib/snap_diff/ai.rb b/lib/snap_diff/ai.rb index 811e9ea7..e137b398 100644 --- a/lib/snap_diff/ai.rb +++ b/lib/snap_diff/ai.rb @@ -30,6 +30,7 @@ def annotate(name) result && {source: "ai", text: format(result), data: result} end + # Register a lazy backend factory under a symbolic name, replacing any prior factory. def register(name, &build) @mutex.synchronize { @backends[name.to_sym] = build } end diff --git a/lib/snap_diff/contributions.rb b/lib/snap_diff/contributions.rb index 99fa12a4..cd590a3d 100644 --- a/lib/snap_diff/contributions.rb +++ b/lib/snap_diff/contributions.rb @@ -51,6 +51,8 @@ def any_suppressor? @mutex.synchronize { !@suppression.nil? } end + # Ask the current suppressor to evaluate a screenshot difference. + # Return its {source:, text:} waiver, or nil when no gate waives the failure. def suppression_for(name, difference) @mutex.synchronize { @suppression }&.suppress(name, difference) end diff --git a/lib/snap_diff/reporters/ai_simple.rb b/lib/snap_diff/reporters/ai_simple.rb index 54cdd1fa..25b810f6 100644 --- a/lib/snap_diff/reporters/ai_simple.rb +++ b/lib/snap_diff/reporters/ai_simple.rb @@ -38,6 +38,8 @@ def initialize(backend: nil, flaky: nil, intentional: nil, fail_on: nil) Contributions.register_suppression(self) if @fail_on end + # Analyze differing assertions and store their results, reusing gate-time analysis. + # Do nothing when the backend is unavailable. def record(assertions) return unless @backend @@ -65,6 +67,8 @@ def suppress(name, difference) {source: "ai", text: AI.format(result)} unless fails?(result[:verdict]) end + # Return whether a verdict must fail under the configured fail_on policy. + # Unknown verdicts always fail; requires a reporter configured with fail_on. def fails?(verdict) = verdict == "unknown" || @fail_on.include?(verdict) # Results are already in the shared store -- nothing to write out. diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index ba324414..ca82a2ed 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -96,6 +96,7 @@ def summary "[snap_diff] Report: #{output_path}" if @finalized end + # Attach available contributions and return the rendered HTML report. def render attach_annotations ERB.new(File.read(self.class.template_path)).result(binding) @@ -112,6 +113,7 @@ def self.default_output_path private + # Build a report entry from a comparison, omitting unavailable images and metrics. def failure_entry_for(name, compare) difference = compare.difference { diff --git a/lib/snap_diff/screenshot_assertion.rb b/lib/snap_diff/screenshot_assertion.rb index 43f5aa2e..32d0d304 100644 --- a/lib/snap_diff/screenshot_assertion.rb +++ b/lib/snap_diff/screenshot_assertion.rb @@ -60,6 +60,8 @@ def inspect "#<#{self.class.name} #{name.inspect} #{state} new=#{compare.image_path} base=#{compare.base_image_path}>" end + # Return an annotated failure message for an unsuppressed screenshot mismatch. + # Return nil for absent, matching, or suppressed comparisons; archive matching baselines. def validate return unless compare diff --git a/test/integration/ai_triage_test.rb b/test/integration/ai_triage_test.rb index c4b41a09..72c7e42f 100644 --- a/test/integration/ai_triage_test.rb +++ b/test/integration/ai_triage_test.rb @@ -23,6 +23,8 @@ class AiTriageTest < ActiveSupport::TestCase | test/(?:unit/(?:reporters/ai_simple_test|contributions_test)\.rb|integration/ai_triage_test\.rb|fixtures/ai_triage_case\.rb) )\z}x + # Decide whether to run AI integration tests from the files changed against origin/master. + # Run unconditionally when forced, on master, or when Git cannot determine the changes. def self.ai_surface_changed? return true if ENV["RUN_AI_TESTS"] == "1" # capture2 (stdout only) + status: with capture2e a missing ref prints diff --git a/test/unit/contributions_test.rb b/test/unit/contributions_test.rb index 3335c77b..fd35ba38 100644 --- a/test/unit/contributions_test.rb +++ b/test/unit/contributions_test.rb @@ -4,9 +4,11 @@ class ContributionsTest < Minitest::Test Provider = Struct.new(:note) do + # Return the configured annotation for any screenshot name. def annotate(name) = note end + # Save the existing providers and clear registry annotations for an isolated test. def setup # Snapshot the shared registry and restore it afterwards -- other # tests (and ai.rb's load-time self-registration) rely on providers @@ -16,15 +18,18 @@ def setup SnapDiff::AI.clear_results! if defined?(SnapDiff::AI) end + # Clear failure suppression and restore the providers saved before the test. def teardown SnapDiff::Contributions.register_suppression(nil) SnapDiff::Contributions.instance_variable_set(:@providers, @saved_providers) end + # Verify that an empty registry produces no annotations. def test_annotations_empty_without_providers assert_empty SnapDiff::Contributions.annotations_for("never-recorded-name") end + # Verify that absent annotations are skipped without changing provider order. def test_annotations_skip_nil_and_keep_order first = Provider.new({source: "first", text: "one"}) nothing = Provider.new(nil) @@ -34,6 +39,7 @@ def test_annotations_skip_nil_and_keep_order assert_equal %w[first second], SnapDiff::Contributions.annotations_for("x").map { |a| a[:source] } end + # Verify that registering one provider twice yields only one annotation. def test_registering_the_same_provider_twice_is_a_no_op provider = Provider.new({source: "ai", text: "t"}) 2.times { SnapDiff::Contributions.register(provider) } @@ -41,6 +47,7 @@ def test_registering_the_same_provider_twice_is_a_no_op assert_equal 1, SnapDiff::Contributions.annotations_for("x").size end + # Verify that distinct providers contribute even when their values compare equal. def test_distinct_providers_that_compare_equal_both_contribute # Structs with equal fields are == but NOT the same provider. 2.times { SnapDiff::Contributions.register(Provider.new({source: "ai", text: "t"})) } @@ -48,11 +55,13 @@ def test_distinct_providers_that_compare_equal_both_contribute assert_equal 2, SnapDiff::Contributions.annotations_for("x").size end + # Verify that an unset suppression slot neither advertises a gate nor waives failures. def test_no_suppressor_by_default refute SnapDiff::Contributions.any_suppressor? assert_nil SnapDiff::Contributions.suppression_for("x", Object.new) end + # Verify that the latest suppressor replaces an earlier gate that would waive the failure. def test_suppression_single_slot_replaces waive = ->(_name, _diff) { {source: "ai", text: "FLAKY"} } waiving = Object.new @@ -67,6 +76,7 @@ def test_suppression_single_slot_replaces assert_nil SnapDiff::Contributions.suppression_for("x", Object.new) end + # Verify that a registered suppressor returns its source and explanation unchanged. def test_suppression_returns_the_waiver waiving = Object.new waiving.define_singleton_method(:suppress) { |_name, _diff| {source: "ai", text: "FLAKY (clip)"} } diff --git a/test/unit/reporters/ai_simple_test.rb b/test/unit/reporters/ai_simple_test.rb index d62fef90..2bee8693 100644 --- a/test/unit/reporters/ai_simple_test.rb +++ b/test/unit/reporters/ai_simple_test.rb @@ -25,14 +25,17 @@ def different? = difference.different? def error_message = "diff details" end + # Clear stored AI results before each reporter test. def setup SnapDiff::AI.clear_results! end + # Clear the suppression gate installed by a reporter test. def teardown SnapDiff::Contributions.register_suppression(nil) end + # Build a screenshot assertion with a controllable comparison and a fixed caller trace. def gate_assertion(name, different: true) SnapDiff::ScreenshotAssertion.new(name).tap do |a| a.compare = GateCompare.new(StubDifference.new(different: different)) @@ -177,6 +180,7 @@ def test_gate_still_fails_real_bugs_and_quotes_ai_in_the_message assert_includes message, "AI triage: REAL_BUG (custom, similarity=0.5)" end + # Verify that an unknown verdict preserves the screenshot failure and appears in its message. def test_gate_always_fails_unknown_verdicts SnapDiff::Reporters::AISimple.new(backend: similarity_backend(nil), fail_on: %w[real_bug]) @@ -185,6 +189,7 @@ def test_gate_always_fails_unknown_verdicts assert_includes message, "AI triage: UNKNOWN" end + # Verify that a backend exception logs a warning and leaves the screenshot mismatch standing. def test_gate_lets_the_failure_stand_when_analysis_fails # The backend raising must not turn validation itself into an error: # the screenshot mismatch is the failure the developer needs. @@ -198,6 +203,7 @@ def test_gate_lets_the_failure_stand_when_analysis_fails assert_includes err, "Backend failed" end + # Verify that validation and reporting share a single backend analysis for the same diff. def test_gate_analysis_is_memoized_for_the_reporter_pass calls = 0 counting_backend = ->(name:, base:, current:, meta:) { @@ -306,15 +312,18 @@ def reporter = HtmlReporterStub.new end HtmlAssertion = Struct.new(:name, :compare) + # Clear AI results and save annotation providers before testing HTML rendering. def setup SnapDiff::AI.clear_results! @saved_providers = SnapDiff::Contributions.instance_variable_get(:@providers).dup end + # Restore annotation providers so custom contributors do not leak into later tests. def teardown SnapDiff::Contributions.instance_variable_set(:@providers, @saved_providers) end + # Build an HTML reporter that writes report.html into the supplied directory. def html_reporter(dir) SnapDiff::Reporters::HTML.new(output_path: File.join(dir, "report.html")) end @@ -323,6 +332,7 @@ def failed_assertion(name) HtmlAssertion.new(name, HtmlCompare.new(HtmlDifference.new(ratio: 0.02))) end + # Verify that rendering attaches stored AI data and text only to the matching screenshot. def test_failures_carry_ai_annotation_after_render SnapDiff::AI.record_result( name: "checkout", verdict: "real_bug", backend: "clip", @@ -343,6 +353,7 @@ def test_failures_carry_ai_annotation_after_render end end + # Verify that rendering includes AI results recorded after HTML collected the failure. def test_ai_result_recorded_after_html_record_still_renders # HTML auto-registers before AISimple, so its record runs first; the # annotation must attach at render regardless of reporter order. @@ -356,6 +367,7 @@ def test_ai_result_recorded_after_html_record_still_renders end end + # Verify that the generated report contains the AI strip and stored verdict. def test_rendered_report_includes_ai_bar_markup SnapDiff::AI.record_result(name: "checkout", verdict: "flaky", backend: "clip", similarity: 0.9912) @@ -370,6 +382,7 @@ def test_rendered_report_includes_ai_bar_markup end end + # Verify that an empty AI store adds no annotation keys to failures or serialized report data. def test_rendered_report_without_ai_stays_clean # AI not enabled (store empty): entries must not gain an :annotations # key, and the serialized DATA must contain no annotations at all. @@ -384,6 +397,7 @@ def test_rendered_report_without_ai_stays_clean end end + # Verify that a contribution without a verdict retains its source and text in the report. def test_text_only_contribution_renders_its_text # A contributor without a verdict payload (the documented # TicketLinker shape) must still show its text, not a bare label.