feat(qa): footer-collision detector for Rules overflow (#412 follow-up) - #441
Conversation
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[NanoClaw] Review PR #441
Detector design is sound — scanning the 82%-92% buffer zone above the absolute-positioned footer cleanly separates overflow signal from legitimate footer content, which is the right fix for the BottomSaturation false-negative gap identified in #440/#438.
BUG: Summary flagged count never includes footer-collision flags
Around line 392 in the full grid summary:
r.FooterCollision == $"FLAG"The FooterCollision field holds values like "FLAG(3.2%)", never the literal string "FLAG". This exact-match comparison will never evaluate to true, so the summary flagged/N count silently undercounts — footer collisions are invisible in the summary even when they fire. Fix with StartsWith("FLAG") (and drop the $ prefix on the string — no interpolation holes, so it is misleading).
Minor observations
- The empty
catchinDetectFooterCollisionsilently swallows errors as(false, 0f, 0)— consistent with the existing detectors, but a corrupt image would report as "no collision" rather than "could not scan." Worth noting for future hardening. IsCoverImage()is not in the diff (pre-existing). The entire cover-exclusion strategy depends on its accuracy — a false negative (cover reported as non-cover) reproduces the exact +22000px artifact the PR warns about. Worth a quick audit of that method.- The dedicated test
VisualQa_FooterCollision_Rules_NoBodyFooterOverlapis informational-only (no Assert fail). Intentional given merge-after-#438 timing, but add a TODO to transition it to a hard-fail once the fix lands — otherwise regression protection is purely manual.
Security: Clean — no credentials, no eval, no user input handling.
The summary-count bug is the only blocker — it would make the full grid report silently wrong.
…ow-up) Recalibrate the Rules detector to catch body text overflowing under the absolute-positioned footer (colorPalette, pageNumber, :before labels). - Add DetectFooterCollision: scans buffer zone (82%-92% height) for non-white pixels = text spillover into footer area - Rules threshold: 0.03 (3%) — even tiny overflow is flagged - Cover images (variante 1) excluded (footer display:none = false artifact) - New dedicated test VisualQa_FooterCollision_Rules_NoBodyFooterOverlap - Integrated into FullGrid report with FooterCollision column + pivot table - CardSetThresholds extended with FooterCollision field - All existing tests unchanged (148 pass, 0 fail) The old DetectBottomSaturation measures the bottom 10% which includes legitimate footer content and can't distinguish body overflow. The new detector scans the gap zone between body end and footer start. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3dba707 to
17e8a35
Compare
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[NanoClaw]
Overall this is well-structured and follows the established detector pattern cleanly. A few observations:
Logic & threshold calibration
- The 3% threshold for Rules is reasonable given the PR description's evidence (real overflow produces 4-18% density in the buffer zone, while clean cards should be near-zero). The 10% default for other card sets is a safe upper bound. The calibration is well-documented with specific card examples (FR #9/#15, RU #9/#12, etc.).
- The buffer zone (82%-92%) is a sensible choice — it sits above where the footer lives (~bottom 10%) but below where body content should stop on a well-laid-out card. The 82% start line leaves room for cards that legitimately use most of their vertical space.
One concern: buffer zone hardcoded to 82-92%
The FooterCollisionBufferStart (0.82) and FooterCollisionBufferEnd (0.92) are class-level constants, not per-CardSet. The PR description explains that Rules cards use position:absolute; bottom:1.5em for the footer. If other card sets ever use a different footer positioning scheme (e.g., taller footers, relative positioning), these constants would need adjustment. This isn't a problem now — only Rules uses FooterCollision — but it's worth noting for future reference if this detector gets extended to other sets.
Edge cases handled well
- Cover image exclusion via
IsCoverImageis correct and reuses the existing heuristic. Thedisplay:nonefooter on covers would produce massive false positives (the PR mentions +22000px artifacts), so skipping them is essential. - The
(totalPixels > 0 ? ... : 0f)guard inDetectFooterCollisionis correct for very small images where the buffer zone might collapse to zero rows. - The
try/catchwith silent failure(false, 0f, 0)matches the pattern used byDetectBottomSaturation— consistent with the rest of the harness.
Code quality
- Follows the existing detector pattern precisely: same tuple return style, same threshold accessor pattern, same test structure.
- The
CardCheckResultextensions (IsCover,FooterCollision,FooterCollisionValue) are clean additions. - The pivot table in the full grid report is a nice touch — it makes it easy to scan which specific cards are problematic across languages.
One minor formatting note
In the full grid's summary flagged-count filter:
r.FooterCollision.StartsWith("FLAG")This works because the three possible values are "PASS", "FLAG(...)", and "COVER". A !r.FooterCollision.Equals("PASS") && !r.FooterCollision.Equals("COVER") would be more defensive, but StartsWith("FLAG") is readable and correct for the current values.
No security concerns — this is a test-only visual QA harness that operates on local image files.
Solid addition. The detector fills a real gap where BottomSaturation couldn't distinguish body overflow from legitimate footer content.
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[Hermes] Review footer-collision detector (#412 follow-up)
Checklist: ✅ Security clean ✅ Cross-repo: none ✅ Code: clean
Analysis: New footer-collision detector measures non-white pixel density in buffer zone (82%-92% height) between card body and absolute-positioned footer. Rules threshold = 0.03 (3%). Cover images excluded. Thresholds model cleanly extended. Well-documented.
APPROVE: Clean detector extension, well-calibrated thresholds.
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[Hermes] Review COMMENT LGTM
Checklist:
- ✅ Security scan: CLEAN
- ✅ No cross-repo impact
- ✅ Notebooks: N/A (C# test project)
- ✅ Fix/incident: Addresses specific detection gap (#412 follow-up, #29 overflow)
Assessment:
Well-designed footer-collision detector:
- Buffer zone (82%-92% height) correctly targets the gap between body end and absolute footer — avoids false positives from legitimate footer content that BottomSaturation couldn't distinguish.
- Rules threshold 0.03 (3%) appropriately sensitive for text overflow detection.
- Cover image exclusion (
IsCoverImage→ display:none footer) prevents the +22000px artifact false positive. - Integration into FullGrid with FooterCollision column + Rules pivot table is thorough.
- Informational mode (no hard-fail) is the right call for a new detector — avoids CI breakage while collecting data.
DetectFooterCollision()implementation is clean: pixel-by-pixel scan with SixLabors.ImageSharp, proper error handling (try/catch → false).
One note: Merge timing noted as HELD until #438 merges. Verify that dependency is tracked.
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[Hermes] Review (17e8a35) — new PR, not previously reviewed.
Checklist:
- ✅ Security scan: CLEAN
- ✅ Cross-repo impact: None (VisualTests only)
- N/A Notebooks
- ✅ Fix/incident: Addresses #412 follow-up (#29 overflow recalibration)
Analysis:
New DetectFooterCollision detector targets the specific overflow pattern BottomSaturation missed:
- Scans buffer zone (82%-92% of card height) between body and absolute-positioned footer
- Rules threshold = 0.03 (3%) — appropriately sensitive for text spillover
- Cover images correctly excluded (display:none → false +22000px artifacts)
- Informational reporting (no hard-fail) — appropriate for a detector
- Follows existing detector pattern — consistent codebase style
Minor note: IsCoverImage() referenced but not in diff — assuming existing from prior work.
Verdict: LGTM — clean detector following established patterns. Proper threshold calibration and false-positive exclusion.
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[Hermes] Review cycle 07:00Z — Stall 4d, new commit detected
Checklist:
- ✅ Security scan: no secrets in diff
- ✅ Cross-repo impact: none (VisualTests only)
- N/A Notebooks
- ✅ Fix/incident: #29 overflow recalibration — addresses missed footer collisions
Notes:
- New
DetectFooterCollisiondetector: measures non-white pixel density in buffer zone (82%-92% height) between card body and absolute-positioned footer - Rules threshold = 0.03 (3%) — appropriately sensitive for text spillover
- Cover images (variante 1) correctly excluded (
IsCoverImagecheck) - Test method:
VisualQa_FooterCollision_Rules_NoBodyFooterOverlap— scans all Rules card images per language - Architecture is consistent with existing detectors (BottomSaturation, BlankRatio)
Concerns:
- Stall 4d — author action required for merge
- FooterCollision only calibrated for Rules (0.03f). Virtues/Fallacies/Scenarii use default (0.10f) — confirm these don't have the same overflow pattern
- No update to CI pipeline integration — ensure new detector is included in CI runs
Decision: COMMENT (LGTM) — well-designed detector addressing a real false-negative gap. Clean implementation.
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[Hermes] APPROVE — Footer-collision detector for Rules overflow (#29).
Checklist:
- Security scan: clean.
- Cross-repo impact: none.
- Quality:
- New detector follows established pattern (BlankRatio, BottomSat → FooterCollision).
- Buffer zone (82%-92%) well-calibrated for absolute-positioned footer.
- Rules threshold 0.03 (3%) is appropriately strict for text spillover detection.
- Cover image exclusion (display:none footer → false positive prevention) is smart.
- Per-CardSet thresholds + defaults follow existing architecture.
- 148 new lines of well-documented C# test code.
LGTM. Good extension of the Visual QA harness.
clusterManager-Myia
left a comment
There was a problem hiding this comment.
Hermes Review — #441 (SHA 17e8a35, delta from NC 3dba70)
Scope: 1 commit. Adds footer-collision detector to VisualQaHarness — detects card body text overflowing into the absolute-positioned footer zone.
✅ Checklist
- Security: CLEAN — no secrets
- Cross-repo impact: None
- Notebooks: N/A (C# test code)
- Fix/incident: Follow-up to #412/#29 overflow recalibration
Findings
- Detector logic: Scans buffer zone (82%-92% of card height) for non-white pixels. Threshold: Rules=3% (strict), default=10%. Sound approach.
- Cover exclusion: Correctly skips cover images (display:none footer) to avoid false +22000px artifacts. Smart.
- Integration: Properly integrated into FullGrid report with new FooterCollision column. CardCheckResult extended.
- Code quality: Well-documented (XML doc + inline comments). Threshold system consistent with existing BottomSaturation pattern.
⚠️ Minor
- Test is informational (doesn't hard-fail on FLAG) — intentional per existing pattern, but worth noting that high-density flags won't block the build.
Verdict: COMMENT — LGTM
Clean, well-documented addition to the QA harness. No blockers.
Summary
Recalibrate the Rules QA detector to catch body text overflowing under the absolute-positioned footer (the specific overflow that the existing BottomSaturation detector missed).
Problem
The existing
DetectBottomSaturationmeasures non-white pixel density in the bottom 10% of the card. For Rules cards, the threshold was 12%. However:position:absolute; bottom:1.5emSolution
New
DetectFooterCollisiondetector:display:none, producing false +22000px artifactsChanges
VisualQaHarness.cs: +158/-10 linesCardSetThresholdsextended withFooterCollisionfieldDetectFooterCollision()method with buffer zone scanningGetFooterCollisionThreshold()accessorVisualQa_FooterCollision_Rules_NoBodyFooterOverlapVisualQa_FullGrid_AllCards_AllDetectorswith FooterCollision column + Rules pivot tableCardCheckResultextended withFooterCollision,FooterCollisionValue,IsCoverfieldsTest plan
Merge timing
HELD — merge after #438 (Rules fix) is merged and release régén is validated. This detector validates that the fix works; merging before the fix would just produce expected flags.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com