mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
filip131311
c52f2dba9e
Fixes #617. ## Reproduced three ways **The reported false green**, using an image whose 2× downscale is lossless so no resampling noise can mask it: ``` baseline 800x1600 current 400x800 (exact half) → status: unchanged pixel_mismatch: 0% (size difference never mentioned) ``` **Worse on real screenshots** — a 1152×648 capture vs its aspect-preserving half: ``` → status: changed pixel_mismatch: 0.35% changed_areas: 8 Text changes: Disappeared "General" / Appeared "Remotes and Devices" ``` The content is identical; every region and both OCR "text changes" are resampling artifacts. So the tool doesn't merely stay silent about the size difference — it manufactures a plausible-looking regression report out of noise. **The control shows the asymmetry is the bug** — aspect-*breaking* correctly reports `dimension_mismatch: expected=1,152x648 actual=1,152x400`. Sizes are disclosed exactly when the tool refuses to compare, and never when it silently rescales. ## The fix `PngDiffResult` gains `sizeNormalization` (both original sizes plus the size compared at), populated only when a rescale actually happened — so its presence *is* the signal that the comparison carries resampling uncertainty. The summary prints it directly under the status: ``` - status: resized_no_change - size_normalized: baseline=800x1,600 current=400x800 compared_at=400x800 - the inputs share an aspect ratio but not a resolution, so the larger was downscaled before comparing; any pixel or text differences below may include resampling artifacts, and the diff images are at compared_at rather than full size - re-capture the baseline at the same scale as the current image (screenshot with scale: 1.0) to compare without resampling ``` **`resized_no_change` is checked last**, after the difference checks, so a rescaled comparison that *did* find something is still a plain `changed` — a caveat status must never soften a real regression. There's a test pinning that ordering specifically. **The name matters.** `unchanged_after_resize` contains `"unchanged"`, so the one naive gate this protects — `summary.includes("status: unchanged")` — would still read it green. `resized_no_change` contains neither `"unchanged"` nor `"changed"`. ## What I deliberately did not do **Report `dimension_mismatch` whenever sizes differ** — the issue's primary suggestion. The normalization is deliberate: `screenshot-diff/index.ts:292-294` states that same-aspect normalization *"keeps a scaled capture diff-compatible with a baseline saved at any scale"*. The `screenshot` tool defaults to **0.3** for iOS/Android while the diff's live capture is full-res, and on Android the live side falls back to the default scale when full-res capture fails. Hard-failing would break the case this code exists for. **Expose the internal `normalizeSizes` flag.** `screenshot-diff-tool.test.ts:10-23` is titled *"rejects public tuning options so defaults stay internal"* and asserts exactly this kind of param is refused. And the better remedy is the one the summary now gives — re-capture the baseline at the same scale, which the skill already instructs and which removes the resampling rather than reporting it. Worth reconsidering separately if a user actually wants "green means byte-identical". *(An earlier draft proposed exposing it via `z.coerce.boolean()`. Worth recording why that was dropped beyond the posture conflict: in zod, `z.coerce.boolean("false")` is **`true`** — so a user asking for strict comparison would have silently got normalization anyway, re-creating this exact bug through the fix's own escape hatch.)* **Suppress the OCR noise.** Text analysis runs on each *original file at its own resolution* and only rescales the resulting bounds, so a diff-time filter cannot separate "OCR read a 576px image badly" from a real text change. Suppressing it whenever normalization occurred would silence text analysis on every scaled comparison — a new, worse false green of exactly the class this issue is about. Disclosure is the honest treatment; running OCR on the normalized pair is a separate change to `text-diff`'s contract. ## Tests Three new cases plus three strengthened ones: the false green now discloses and no longer reads `unchanged`; a normalized comparison with a real change still reports `changed` *and* still discloses; equal sizes say nothing; and the retina case still compares rather than hard-failing. Mutation-verified both halves — removing the status fails the #617 regression test; moving its check before the difference checks fails the ordering test. Full tool-server suite green — 3089 passed. No existing assertion needed changing, which is also why this survived: nothing pinned it. ## Notes for the reviewer - The flow snapshot path is untouched and **cannot** reach normalization: full-screen baselines key on the capture's own pixel dimensions (`flow-visual.ts:212`), so a differently-sized capture misses the baseline entirely and fails; `cropOn` diffs already pass `normalizeSizes: false`. - `SummaryStatus` is unexported and nothing switches on it — the status reaches the outside only as text inside `summary`, which the MCP layer passes through verbatim. No skill doc, flow assertion or e2e script matches on a status string. - The diff artifacts were already produced at the normalized size; that was simply never stated. The caveat now says so.
22 KiB
22 KiB