Skip to content

Fix static HTML scans mixing inactive media styles - #1079

Open
mager wants to merge 9 commits into
pbakaus:mainfrom
mager:codex/static-media-988
Open

mager wants to merge 9 commits into
pbakaus:mainfrom
mager:codex/static-media-988

Conversation

@mager

@mager mager commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Static HTML scans currently combine declarations from mutually exclusive media queries. In #988, dark-mode text variables and a print-only white background produce a contrast error for a combination that the default browser scan never renders.

This applies Paul's approved policy: one screen environment, light color scheme, and the default 1280 × 800 viewport. Inactive and unsupported conditions are excluded before the cascade, custom-property resolution, keyframes, and CSS pattern checks. This covers nested @media blocks and media attributes on inline/local linked stylesheets.

Supports media types, negation, query lists, grouped conditions, light/dark scheme, orientation, and width/height conditions including ranges and px/em/rem units. Unknown conditions remain unknown under negation. CSS token parsing preserves strings, comments, offsets, and line breaks.

Closes #988.

Review fixes

Both findings are addressed in d520c6d145923125e97eb88cab91e6453b4e5da5:

  • Finite negative media lengths are valid comparisons. Conditions such as (width > -1px) and not (max-width: -1px) now retain active styles and their contrast findings.
  • Each active stylesheet is filtered once before its result enters the cascade and pattern corpora. The cascade no longer filters it again. Active ignored linked sheets still affect the cascade but stay out of the pattern scan. Filtering separate sheets also prevents an unclosed inactive media block in one sheet from hiding the next sheet.

Main 5cb95eea9 was merged without rewriting history before these fixes. Current main 7510e233b is now merged without rewriting history in 8d53475c8; see the current verification below.

Test plan

Current merge verification (8d53475c8, 2026-10-28)

  • Merge main 7510e233b without rewriting history. Resolve the delta-note conflict and five aggregate golden conflicts; production source merges cleanly.
  • The first HTML test run caught main's new contrast-pair deduplication collapsing the four media flag cases into one. Give each flag/pass case a distinct ink so all cases remain independently observable. All 237 HTML tests pass after updating the four fixture JSON/text goldens; their only changes are three flag ink values.
  • Build fresh main and PR release engines. Compare aggregate reports on the identical fixture tree: 1,202 → 1,189 non-advisory findings, with all 201 advisories identical and in order. Only the five known media fixture groups differ. Preserve main's newer fixture and pair-counting results; frozen call vectors stay unchanged.
  • Fresh static and real Chromium scans of both static-media-environment.html and negative-media-dimensions.html each report exactly four low-contrast findings and exit 2.
  • bun run build and the default core suite pass (485 Bun tests + 234 Node tests, 2 skips).
  • Full rebuilt-engine oracle replay: 1,325 pass, 0 fail, 10 accepted deltas, 0 missing goldens. The five aggregate reports were also checked directly against the fresh output, so accepted-delta exemptions do not hide a mismatch.
  • Framework suite: 191 pass. Plugin-loader E2E: 4 pass.
  • CARGO_INCREMENTAL=0 CARGO_PROFILE_TEST_DEBUG=0 RUST_TEST_THREADS=2 bun run test:rust-changed --base 7510e233b: 1,368 pass, 1 fail. The sole failure is the previously documented artifact_cleanup_failure_blocks_the_gate (stale evidence: regions/retired.png); its source and test are unchanged from current main. The original run is not counted as green.
  • Full local default suite is not green: core and oracle pass, then Live reports 255 pass, 5 fail, 2 skip. All five failures are in the unchanged live-agent-target.test.mjs lease/timing group: holder becomes busy, denied-claimant pending verdict, disconnected-holder lease, all-idle-pages decline, and last-silent-overlay disconnect. No assertion or timeout was relaxed. A fresh current-main-runtime default run also reports 255 pass, 5 fail, 2 skip in this group; four failure names match. The fifth differs (denied-claimant verdict here, generate-already-answered on main), so this is evidence of the existing timing instability, not an exact match of every failure. The denied-claimant test passes in isolation on both fresh main and PR binaries (1/1 each); the failed full run remains recorded.
  • GitHub CI passes Linux Rust, Node 22/24, oracle, Windows launcher, CLI shim, and Greptile on this merge commit.
  • Windows Rust CI must complete before merge. Earlier timings below retain their original commit labels.

Boolean-feature review fix (595346e00)

Bare (orientation) and (prefers-color-scheme) now evaluate as true in the supported screen environment, as required by Media Queries boolean context. Previously, those valid conditions were treated as unknown and their active styles were removed.

  • The new integration regression failed before the fix: an active low-contrast finding disappeared under (orientation).
  • All 234 HTML tests pass. The new test covers both boolean forms, their combination with screen, negated controls, and an unsupported feature under both positive and negative forms.
  • Fresh release engine build passes. Default-suite core and full oracle replay pass with that binary; no golden changes were needed for this review fix.
  • Full local default suite is not green: Live reports 252 pass, 5 fail, 2 skip, all five failures in the unchanged agent-target lease/timing tests.
  • bun run test:rust-changed completed with 1,320 pass and 4 fail: three browser shadow-tree tests lost their browser connection, and the previously documented artifact_cleanup_failure_blocks_the_gate failed. These are outside the two files changed in this review fix. All six shadow-tree tests pass on a serial rerun. The initial run is not counted as green.
  • Fresh GitHub CI at 595346e00 passes all selected checks, including Rust on Linux/Windows, Node 22/24, oracle, Windows launcher, and CLI shim. Greptile review passes. Earlier browser checks and timing measurements below retain their original commit labels.

Earlier merge verification (06948f3ee, 2026-10-24)

Verification uses fresh release builds from main 936867543 and the merged PR, not the downloaded engine.

  • Resolve all five aggregate golden conflicts and preserve both sets of accepted-delta notes. No runtime source changes were needed for conflict resolution.
  • Compare fresh main and merged reports finding by finding and line by line: 1,156 → 1,161 non-advisory findings, with the same 190 advisories in the same order. Differences are limited to this PR's two fixtures, its two existing removals, and the newly merged inactive-at-rule fixture. Exit codes remain 2; frozen function vectors are unchanged.
  • Update the new inactive-at-rule fixture's JSON/text goldens and comment: inactive magenta candidates under min-width: 4000px are excluded, and active cyan candidates are reported. Its existing Rust oracle regression failed before the golden correction. Seven direct oracle checks (the five aggregate reports plus this fixture's two reports) now match exactly, without accepted-delta exemptions.
  • cargo build --release -p impeccable and bun run build pass.
  • Fresh static and real Chromium scans of both static-media-environment.html and negative-media-dimensions.html each report exactly four low-contrast findings and exit 2.
  • All 233 HTML tests pass. Full cargo test --workspace --no-fail-fast: 2,368 pass, 2 fail, 9 ignored. Failures: the previously reproduced artifact_cleanup_failure_blocks_the_gate (stale evidence: regions/retired.png) and disabled_project_or_environment_blocks_user_hooks (local config re-enables hooks). The hook test passes on an isolated rerun. Both crates and their tests are byte-identical to current main. This is not a green full local workspace run.
  • Default suite core (470 Bun + 196 Node tests, one skip), full oracle replay, and detector (28 tests) pass. Framework (191) and plugin-loader (4) suites pass separately.
  • Full default suite attempted: Live has 251 pass, 6 fail, 2 skip. A fresh current-main build reproduces all six failures in tests/live-agent-target.test.mjs (33 pass, 7 fail in that group). No Live tests, assertions, or timeouts were changed. The full local default run is not green.
  • Fresh CI on the merge commit must complete before merge.

Earlier review-fix verification (d520c6d14)

These results describe the previous revision; the current merge checks are above.

  • Failing-first parser and integration tests reproduced negative-length false negatives and the unclosed-sheet boundary bug before the fixes.
  • cargo test -p impeccable-html: all 232 tests pass, including active/inactive negative ranges, min/max, negation, px/em/rem, unsupported-unit controls, and the existing media/ignored-linked-sheet regression.
  • cargo build --release -p impeccable and bun run build pass.
  • Fresh static and real Chromium scans of both static-media-environment.html and negative-media-dimensions.html each report exactly four low-contrast findings and exit 2.
  • The new negative-media-dimensions.html fixture has four flag cases and five pass controls. Rebuilt static and real Chromium scans both report exactly four low-contrast findings and exit 2. Browser selectors are only #flag-range-width > p, #flag-negated-width > p, #flag-range-height > p, and #flag-chained-width > p.
  • Oracle replay passes. Compared with the pre-review ec80ea6b9 binary on the identical fixture tree, the five aggregate cases gain only the four new fixture findings, in the expected order: non-advisory count 1,028 → 1,032, all 178 advisories unchanged, exit 2. JSON and text differences were reviewed by hand and recorded in tests/oracle/DELTAS.md. Frozen function vectors are unchanged.
  • Default suite core, oracle, and detector stages pass. Framework and plugin-loader suites pass separately (191 and 4 tests).
  • Full Rust workspace attempted with cargo test --workspace --no-fail-fast: 2,257 pass, 1 fail, 9 ignored. The sole failure is impeccable-comp-verbs::build_phase::integrity_tests::artifact_cleanup_failure_blocks_the_gate (stale evidence: regions/retired.png). The same exact failure was reproduced on a fresh build of unchanged main 490141501; that test and its implementation were not changed by this PR. This is not a green full local workspace run.
  • Full default suite attempted with IMPECCABLE_BIN="$PWD/target/release/impeccable" bun run test: Live stage has 230 pass, 6 fail, 2 skip. All six failures are in the unchanged tests/live-agent-target.test.mjs lease/timing group, and all six were also present in the sequential fresh-main 490141501 baseline (33 pass / 7 fail). No assertions or timeouts were relaxed. This is not a green full local default run.
  • GitHub CI on d520c6d14 passed all selected checks, including Linux/Windows Rust, Node 22/24, and oracle. Greptile confirmed both review findings fixed (5/5); both threads are resolved.

Cold edit-hook timing

Measured sequentially after local builds and tests finished, using release rule_cost binaries on macOS/aarch64. Each version uses the same frozen 347-input corpus: anti-pattern fixtures from main 5cb95eea9 plus magerblog src/ at eb21cd8ef586289db11f201423b103883ec0b798. There are 196 static HTML and 151 text inputs, with nine cold processes per file (1,764 HTML and 1,359 text samples per version). The new regression fixture is excluded from this frozen corpus.

Milliseconds per edit (mean / p50 / p95) Main 5cb95eea9 Before review fix ec80ea6b9 Fixed d520c6d14
Static HTML 8.969 / 8.294 / 14.063 9.004 / 8.279 / 14.410 8.985 / 8.284 / 14.436
Text 3.284 / 3.197 / 5.714 3.279 / 3.211 / 5.833 3.303 / 3.246 / 5.760

The fixed static mean is +0.016 ms (+0.18%) versus main and -0.019 ms (-0.21%) versus the pre-fix PR. This whole-tree measurement includes run-to-run noise and does not establish a measurable speedup. The repeated media passes are removed in code; the scan still spends most time elsewhere.

Repeat with cargo build --release -p impeccable-html --example rule_cost, then target/release/examples/rule_cost --runs 9 --out rule-cost.json <same-frozen-fixtures> <same-frozen-project-src> for each version.

Repeat the visible check

cargo build --release -p impeccable
target/release/impeccable detect --no-config --json tests/fixtures/antipatterns/negative-media-dimensions.html
target/release/impeccable detect --no-config --json "file://$PWD/tests/fixtures/antipatterns/negative-media-dimensions.html"

Both scans should report the four active cases and exit 2. Open that HTML file at 1280 × 800/light to inspect the flag/pass columns. The original static-media-environment.html fixture can be checked the same way; it also reports four findings in both paths.

The original #988 reproducer and an explicit screen/dark-versus-print example previously produced zero contrast findings in both static and rendered scans. An unconditional contrast failure remained detectable in both. The original page's separate cream-palette warning remains; this change addresses its false contrast finding.

Scope and limits

This is a single-environment static scan, not coverage of every responsive state. Unsupported features, including prefers-reduced-motion, are excluded. This intentionally removes the static .rec-mark finding from the existing pulsing-dot fixture; the rendered scanner is unchanged. The other existing fixture change removes a gradient contrast finding under (max-width: 1px).

Plain CSS/JS text scans, @supports evaluation, and layer behavior are unchanged. No shared detection rule or browser bundle changes. Generated provider output is intentionally omitted; no version or changelog changes.

AI assistance: implementation, tests, review replies, and this PR description were prepared with OpenAI Codex under Mager's direction, following the maintainer-approved scope above.

Apply the screen/light/1280x800 policy approved in pbakaus#988 before the static cascade and CSS checks. Add regressions and review the intended CLI golden changes without changing frozen vectors.

AI assistance: Prepared with OpenAI Codex under Mager's direction.
@github-actions github-actions Bot added the waiting on contributor Waiting for the PR author to respond or make changes label Oct 19, 2026
@github-actions github-actions Bot added the blocked: merge conflicts PR cannot merge until conflicts are resolved label Oct 20, 2026
mager added 2 commits October 21, 2026 16:40
Retain media filtering in both cascade and pattern CSS corpora. Cover inactive media and ignored linked sheets together.

AI assistance: prepared with OpenAI Codex under Mager direction.
Give each helper call a unique directory so parallel tests cannot remove each other files.

AI assistance: prepared with OpenAI Codex under Mager direction.
@github-actions github-actions Bot removed the blocked: merge conflicts PR cannot merge until conflicts are resolved label Oct 22, 2026
mager added 2 commits October 22, 2026 18:36
Compare the five aggregate cases against fresh main and branch binaries. Retain all upstream findings and the four expected media fixture findings.

AI assistance: prepared and validated with OpenAI Codex under Mager's direction.
Resolve the append-only DELTAS.md conflict without changing the static media implementation or its goldens. Release build and oracle replay pass.

AI assistance: prepared and checked with OpenAI Codex under Mager's direction.
@mager
mager marked this pull request as ready for review October 23, 2026 14:48
@mager
mager requested a review from pbakaus as a code owner October 23, 2026 14:48
@cursor

cursor Bot commented Oct 23, 2026

Copy link
Copy Markdown

This PR is too large for Bugbot to review. It changes 690 lines and 3,992,095 characters. Split the change into smaller pull requests to get a review.

@greptile-apps

greptile-apps Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issue was found.

Summary

Static HTML scans now use one screen environment: light colors at 1280 × 800.

  • Static HTML findings now use styles that match one screen environment.
  • Valid negative media lengths keep their width and height conditions active.

The previous-review merge-base check failed, so this review used the full PR diff. No runtime tests were run during this review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Inline and local linked stylesheets] --> B{Media attribute matches?}
  B -->|No| C[Skip stylesheet]
  B -->|Yes or absent| D[Filter inactive nested media blocks once]
  D --> E[Cascade CSS]
  D --> F{Linked stylesheet ignored?}
  F -->|No| G[Pattern CSS]
  F -->|Yes| H[Exclude from pattern checks]
  E --> I[Resolve custom properties and styles]
  E --> J[Check keyframes]
  G --> K[Check CSS patterns]
Loading

Reviews (5) · Last reviewed commit: "Merge main into static media scan fix" · Reviewed by Greptile

Comment thread crates/html/src/cascade/media.rs Outdated
Comment thread crates/html/src/cascade/build.rs Outdated
@github-actions github-actions Bot added blocked: ci Latest commit has failing required checks blocked: review threads Unresolved review feedback or requested changes remain labels Oct 23, 2026
Preserve valid negative width and height comparisons. Filter each stylesheet once before sharing it with the cascade and pattern scan, with regression coverage for sheet boundaries and browser-matched fixtures.

AI assistance: implemented and tested with Codex.
@github-actions github-actions Bot added blocked: merge conflicts PR cannot merge until conflicts are resolved and removed blocked: ci Latest commit has failing required checks blocked: review threads Unresolved review feedback or requested changes remain labels Oct 24, 2026
Preserve current main findings and both review fixes. Review refreshed aggregate and inactive-media fixture goldens against fresh main and PR builds.

AI assistance: conflict resolution and verification performed with Codex under Mager's direction.
@mager

mager commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Merged current main (936867543) in 06948f3ee without rewriting history. GitHub now reports no merge conflicts; fresh CI and Greptile review are running.

Both review fixes are preserved. The conflicts were in aggregate oracle results and their notes. I compared fresh main and PR builds before updating them: only the intended media-filter findings change, with all 190 advisories unchanged. The newly merged inactive-at-rule fixture now expects active cyan candidates instead of magenta candidates behind min-width: 4000px.

The test plan is updated. All 233 HTML tests pass, the seven changed oracle reports match exactly, and static/Chromium checks of both PR fixtures each produce four contrast findings. Full-run limits are recorded too: two local Rust failures (the hook test passes alone) and six Live failures, all six reproduced on fresh main. No assertions or timeouts were weakened.

AI assistance: Codex performed the merge, verification, and this update under Mager's direction.

Comment thread crates/html/src/cascade/media.rs Outdated
@github-actions github-actions Bot added blocked: review threads Unresolved review feedback or requested changes remain and removed blocked: merge conflicts PR cannot merge until conflicts are resolved labels Oct 25, 2026
AI assistance: Codex reproduced the review finding and added the fix and regression tests under Mager’s direction.
@github-actions github-actions Bot added blocked: merge conflicts PR cannot merge until conflicts are resolved and removed blocked: review threads Unresolved review feedback or requested changes remain blocked: merge conflicts PR cannot merge until conflicts are resolved waiting on contributor Waiting for the PR author to respond or make changes labels Oct 27, 2026
@github-actions github-actions Bot added the needs maintainer review Ready for a maintainer to review or decide label Oct 28, 2026
Preserve independent media fixtures after contrast pair deduplication. Review refreshed aggregate oracle reports against fresh main and merged engines; retain upstream accepted deltas.

AI assistance: prepared and tested with OpenAI Codex under Mager's direction.
@mager

mager commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Merged current main (7510e233b) in 8d53475c8, without rewriting history. The merge conflicts are cleared.

Main's new contrast-pair deduplication exposed a fixture problem: four media cases shared one ink and collapsed into one finding. Each flag/pass case now has its own ink, so the tests still verify four independent active cases and five inactive controls. No production source change was needed for this merge.

Verification and the PR test plan are updated:

  • 237 HTML tests pass. Fresh static and real Chromium scans both report exactly four findings for each media fixture.
  • Fresh main/PR aggregate reports were compared on the same fixtures. All 201 advisories and all findings outside the five expected media fixture groups are unchanged. Full oracle replay passes (1,325 pass, 0 fail).
  • Core, detector, framework (191), and plugin-loader (4) suites pass.
  • Local full runs still have the documented macOS failures: affected Rust is 1,368 pass / 1 fail (artifact_cleanup_failure_blocks_the_gate); Live is 255 pass / 5 fail / 2 skip. Current main also fails five Live lease tests, with four matching names; the remaining denied-claimant case passes alone on both binaries. No assertions or timeouts were relaxed.

Linux Rust, Node 22/24, oracle, launcher, CLI shim, and Greptile CI pass. Windows Rust is still running its test step. The PR remains ready for review, with that CI result still to check before merge.

AI assistance: prepared, tested, and posted with OpenAI Codex under Mager's direction.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs maintainer review Ready for a maintainer to review or decide policy: approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Static scan mixes values from mutually exclusive @media blocks, producing false low-contrast findings

2 participants