Skip to content

Add detector.ignoreSelectors: one component-level opt-out instead of an attribute per instance - #810

Open
pbakaus wants to merge 5 commits into
mainfrom
feat/rule-optout-by-selector
Open

pbakaus wants to merge 5 commits into
mainfrom
feat/rule-optout-by-selector

Conversation

@pbakaus

@pbakaus pbakaus commented Sep 11, 2026 •

Copy link
Copy Markdown
Owner

The problem, from a real review

On impeccable-site PR #34 the author wanted undersized-ui-text off for one component: .ks-tag, a 10px mono index label. The only per-rule opt-out that exists is data-impeccable-ignore on an element, so the change carried eleven of them, one per instance of the same component (25 opt-out attributes in the PR overall):

<span class="ks-tag" data-impeccable-ignore="undersized-ui-text">01 &middot; Explore directions</span>
<span class="ks-tag" data-impeccable-ignore="undersized-ui-text">02 &middot; See one built</span>
...nine more

The alternatives were worse: ignoreRules undersized-ui-text turns the rule off for the whole project, ignoreFiles turns off every rule for the file, and ignoreValues only covers the six rules that report a value. So the author did the only narrow thing available, and the reviewer got markup noise and no number: nothing in the scan says how much was waived.

After

One entry, in the config the project already has:

impeccable ignores add-selector undersized-ui-text ".ks-tag" \
  --reason "10px mono index labels, decorative counters beside the heading"
"detector": {
  "ignoreSelectors": [
    { "rule": "undersized-ui-text", "selector": ".ks-tag", "createdAt": "...", "reason": "..." }
  ]
}

An entry waives its rule for every element the selector matches and for that element's subtree, which is exactly what the attribute grants the element that carries it (element.closest(selector) !== null). Rule * covers every rule, as in the attribute. Entries can be scoped with files globs like ignoreValues; an entry without files covers every target. --no-config disables them. impeccable ignores remove-selector and clear remove them, and list shows them.

The per-instance attribute is untouched and still works; it stays the right tool for a one-off (a single demo block), and the docs now say so.

The finding output change

The waiver is a count, not silence. The engines stamp rather than drop: a waived finding comes back carrying ignoredBy: "<selector>" (BrowserFinding.ignoredBy in the serialized group findings, Finding.ignoredBy in the extras). The config layer (filterDetectionFindings) drops those and tallies them by {rule, selector, count}, and the CLI prints one dim line per entry on stderr in every mode, so --json stdout stays the findings array a consumer parses:

1 anti-pattern found.

3 undersized-ui-text hits ignored by detector.ignoreSelectors on .ks-tag.

That shape is what lets a downstream reviewer (Pristine) print "N hits ignored by the author's config on .ks-tag" instead of reporting nothing: it reads the stamped findings straight from the wasm engine, or the tally line from the CLI.

Nothing changes for a project that does not use the key. No entry means nothing stamped, no line printed, and writeDetectionConfig does not add an empty ignoreSelectors to an existing config. BrowserFinding.ignoredBy and BrowserConfig.ignoreSelectors are skipped in serialization when unset, so existing bundle output is byte-identical.

Where it applies

Surface How
Browser / URL engine BrowserConfig.ignoreSelectors, also readable from window.__IMPECCABLE_CONFIG__.ignoreSelectors
Static HTML engine DetectHtmlOptions.ignore_selectors (element rules and selector-backed html-patterns)
wasm detect_html_source_json ignoreSelectors option, host-narrowed
impeccable detect reads the config, narrows per target, prints the tally
Design hook reads the config, so an opted-out component stops re-firing on every edit
Text/regex engine not applicable, there is no DOM to match against

doctor validates the rule of each entry alongside ignoreRules (detector-ignore-rules-unknown), so a typo'd rule id in a component ignore is reported rather than silently waiving nothing.

Coverage

  • crates/html/tests/selector_ignores.rs: every instance of the component waived from one entry, the component's subtree waived with it, a label outside the component untouched, an entry for another rule waiving nothing, *, and the per-instance attribute still working.
  • crates/core/src/browser/driver.rs tests: stamping over the fake DOM (three instances plus a descendant, other rules and other elements clean), serialization carrying ignoredBy only when set, BrowserConfig parsing and omitting the key.
  • crates/detect/src/config.rs tests: normalization (rule lowercased, selector case preserved, half-entries dropped), merge-by-key, per-target narrowing including a URL target, and the drop-and-tally.
  • crates/foundation: SelectorIgnore unit tests and the ignoredBy stamp round-trip.
  • Oracle: new workspace detect-selector-ignores with detect-selector-ignore-{text,json,quiet,no-config} and ignores-selector-{list,add,remove,missing-args,star-refused} plus ignores-help.

Validation: cargo test --workspace, cargo build --release -p impeccable, IMPECCABLE_BIN=... bun run test (844 oracle cases pass), bun run build, cargo xtask bundle (the tracked in-page bundle is refreshed; antipatterns.json is unchanged since no rule was added).

Eight context/doctor goldens were re-recorded and reviewed by hand: they differ only in the recognized-detector-keys sentence, which now lists ignoreSelectors. Generated provider output is deliberately not staged.

Docs

docs/CLI-CONTRACT.md (config shape, normalization and merge, the post-filter, the tally line, the ignores actions, the list row), docs/ENGINE.md (the wasm option), README.md, and skill/reference/hooks.md, where the suppression ladder now names the component ignore between the file-scoped value ignore and ignore-file, so an agent triaging findings reaches it before reaching for attributes.

This was prepared by an AI agent working under instructions from @pbakaus.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LQBUunp8QttxZqihybNmtL


Note

Medium Risk
Touches the shared detection pipeline (browser core, HTML engine, CLI filtering, hook scans) and changes finding visibility semantics, though behavior is additive and heavily tested.

Overview
Adds detector.ignoreSelectors, a config-driven way to waive one detector rule for every element matched by a CSS selector (and that subtree), replacing repeated data-impeccable-ignore on many component instances. Manage entries with impeccable ignores add-selector / remove-selector (optional files scoping and reason), surfaced in ignores list and README.

Waivers are stamped, not silent: engines tag findings with ignoredBy, then the CLI/config layer removes them from the report and prints a stderr tally per rule+selector (including in --json runs). The in-page bundle and offscreen scan path filter ignoredBy for overlays via __reportableGroups.

Coverage spans browser/WASM collect, static HTML, URL/CDP visual contrast, the design hook (scoped to the file being edited), and doctor validation of rule ids in selector ignores alongside ignoreRules. ScanOptions.ignore_selectors is filled per target; URL scans only apply unscoped entries so path globs cannot accidentally match live URLs.

Reviewed by Cursor Bugbot for commit 5ddf1ad. Bugbot is set up for automated code reviews on this repo. Configure here.

…an attribute per instance

An author who wants a rule off for one component has had two choices: put
`data-impeccable-ignore` on every instance, or silence the rule (or the
file) for the whole project. On impeccable-site #34 that meant eleven
attributes on eleven copies of the same 10px label for `undersized-ui-text`,
25 opt-out attributes in all. The count is the problem: the markup carries
noise, and the reviewer never sees how much was waived.

`detector.ignoreSelectors` is the declared twin of that attribute. One entry,
`{ rule, selector }`, waives the rule for every element the selector matches
and for that element's subtree, the same waiver the attribute grants the
element carrying it:

    impeccable ignores add-selector undersized-ui-text ".ks-tag" \
      --reason "10px mono index labels, confirmed"

The waiver is never silent. The engines stamp a waived finding with
`ignoredBy: "<selector>"` instead of dropping it; the config layer drops and
counts, and every scan prints one line per entry on stderr, in `--json` runs
too, so stdout stays the findings array:

    3 undersized-ui-text hits ignored by detector.ignoreSelectors on .ks-tag.

Where it applies: the browser engine (`BrowserConfig.ignoreSelectors`, also
readable from `window.__IMPECCABLE_CONFIG__`), the static HTML engine
(`DetectHtmlOptions.ignore_selectors`, and the `ignoreSelectors` option of
the wasm `detect_html_source_json` export), the detect CLI, and the design
hook. The text engine has no DOM and ignores the key. Entries can be scoped
with `files` globs like `ignoreValues`; `--no-config` disables them; `doctor`
validates their rule ids alongside `ignoreRules`. Nothing changes for a
project without the key: the engines stamp nothing, the CLI prints nothing,
the config writer does not add an empty `ignoreSelectors`, and the
per-instance attribute keeps working exactly as before.

Coverage: `crates/html/tests/selector_ignores.rs` (component, subtree,
wrong-rule, `*`, attribute parity), driver tests over the fake DOM,
`crates/detect` config tests (normalize, merge, per-target narrowing, the
tally), and oracle cases `detect-selector-ignore-*` / `ignores-selector-*`
over a new workspace. The eight re-recorded context/doctor goldens differ
only in the recognized-detector-keys sentence, which now lists the new key.

Assisted-by: Claude Code
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQBUunp8QttxZqihybNmtL
Copilot AI lite review requested due to automatic review settings September 11, 2026 19:24

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread crates/hook/src/hook_lib.rs Outdated
Comment thread crates/foundation/src/browser/mod.rs
Comment thread crates/browser/src/lib.rs Outdated
@greptile-apps

greptile-apps Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR is not yet safe to merge because rendered line-length checks can produce false findings when hidden descendant text is present.

Fix All in CodexFindings

  1. P2 Suppression credited to wrong selector ▶

Summary

This PR adds component-level detector waivers through detector.ignoreSelectors, propagates waiver stamps through browser, static HTML, WASM, CLI, hook, and live paths, and reports suppression tallies rather than silently discarding findings. Changes merged since the previous review also refine several rendered-design rules and design-system exemptions.

  • Adds selector-based, optionally file-scoped configuration and ignores CLI management.
  • Preserves ignoredBy metadata through detection engines and filters it at reporting boundaries.
  • Handles visual-contrast and page-pattern waiver attribution across browser and static paths.
  • Introduces rendered text-line geometry for line-length, padding, and edge checks.
  • Refines AI-palette and declared design-system component handling.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
    C[detector.ignoreSelectors config] --> N[Normalize and scope per target]
    N --> E[Browser / HTML / WASM engine]
    E --> M[Match finding element or ancestor]
    M --> S[Stamp finding with ignoredBy]
    S --> F[Report-layer filtering]
    F --> R[Reportable findings]
    F --> T[Per-rule and selector suppression tally on stderr]
Loading

Reviews (5) · Last reviewed commit: "Merge main and honor selector waivers in..."

Comment thread crates/browser/src/lib.rs Outdated
Comment thread crates/detect/src/config.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved selector propagation, malformed-entry handling, visual and pattern finding coverage, URL scoping, and hook-path issues block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds detector.ignoreSelectors for component-level detector waivers with scoped matching, stamped findings, CLI management, and suppression counts.

Changes:

  • Adds normalization, file scoping, CRUD commands, and doctor validation.
  • Wires selector ignores through HTML, browser, URL, WASM, and hook paths.
  • Adds documentation, regression tests, oracle fixtures, and updated goldens.
File summaries
File Description
tests/oracle/workspaces/detect-selector-ignores/src/page.html Selector-ignore HTML fixture.
tests/oracle/workspaces/detect-selector-ignores/package.json Oracle workspace configuration.
tests/oracle/workspaces/detect-selector-ignores/.impeccable/config.json Fixture detector configuration.
tests/oracle/golden/ignores-selector-star-refused.json Wildcard selector rejection output.
tests/oracle/golden/ignores-selector-remove.json Selector removal output.
tests/oracle/golden/ignores-selector-missing-args.json Missing-argument output.
tests/oracle/golden/ignores-selector-list.json Selector list output.
tests/oracle/golden/ignores-selector-add.json Selector addition output.
tests/oracle/golden/ignores-help.json Ignore-command help output.
tests/oracle/golden/doctor-legacy-text.json Legacy doctor text output.
tests/oracle/golden/doctor-legacy-json.json Legacy doctor JSON output.
tests/oracle/golden/doctor-legacy-fix.json Doctor fix output.
tests/oracle/golden/doctor-legacy-fix-twice.json Repeated doctor fix output.
tests/oracle/golden/doctor-legacy-fix-no-overwrite.json Non-overwriting doctor fix output.
tests/oracle/golden/doctor-legacy-fix-json.json JSON doctor fix output.
tests/oracle/golden/detect-selector-ignore-text.json Text detection output.
tests/oracle/golden/detect-selector-ignore-quiet.json Quiet detection output.
tests/oracle/golden/detect-selector-ignore-no-config.json No-config detection output.
tests/oracle/golden/detect-selector-ignore-json.json JSON detection output.
tests/oracle/golden/detect-help.json Detection help output.
tests/oracle/golden/context-staleness-throttle.json Staleness throttle output.
tests/oracle/golden/context-staleness-cache-env.json Staleness cache environment output.
tests/oracle/golden/context-legacy.json Legacy context output.
tests/oracle/cases/detect.mjs Registers selector-ignore oracle cases.
skill/reference/hooks.md Updates suppression guidance.
README.md Documents selector-ignore configuration.
docs/ENGINE.md Documents the WASM option.
docs/CLI-CONTRACT.md Documents configuration and CLI behavior.
crates/wasm/src/exports_detect.rs Parses selector-ignore options.
crates/html/tests/selector_ignores.rs Tests static HTML selector behavior.
crates/html/src/static_engine.rs Forwards selector options.
crates/html/src/engine.rs Applies selector waivers to HTML findings.
crates/hook/src/hook_lib.rs Integrates selector ignores with the design hook.
crates/foundation/src/selector_ignores.rs Defines selector-ignore matching.
crates/foundation/src/lib.rs Registers selector-ignore support.
crates/foundation/src/findings.rs Adds ignored-finding metadata.
crates/foundation/src/browser/mod.rs Adds browser config and finding fields.
crates/detect/src/ignores.rs Implements selector-ignore CLI actions.
crates/detect/src/engines.rs Builds per-target scan options.
crates/detect/src/config.rs Normalizes, scopes, and filters entries.
crates/detect/src/cli.rs Applies ignores and reports tallies.
crates/core/src/lib.rs Re-exports selector-ignore types.
crates/core/src/browser/element_checks.rs Updates browser finding construction.
crates/core/src/browser/driver.rs Stamps and serializes browser findings.
crates/context/src/staleness.rs Recognizes the new detector key.
crates/context/src/staleness_deep.rs Validates selector rule IDs.
crates/browser/src/snapshot_engine.rs Builds browser scan configuration.
crates/browser/src/lib.rs Converts URL scan findings and options.
Review details

Suppressed comments (5)

crates/core/src/browser/driver.rs:1511

  • Selector stamping happens only for the base groups here, but visual-contrast findings are appended afterward by visual_contrast_result_finding and by the URL engine's run_visual_contrast_fallback. Those later low-contrast findings have no ignoredBy, so a matching component still reports them even when ignoreSelectors covers the rule. Apply the same selector waiver to visual additions before serialization/filtering.
    stamp_selector_ignores(dom, &mut groups, &config.ignore_selectors);

crates/core/src/browser/driver.rs:1511

  • Selector-backed HTML-pattern findings are added to body_key by page_pass before this call, and scoped_html_pattern_findings returns no element attribution. Therefore stamp_selector_ignores checks body.closest(selector) and an entry such as { rule: "side-tab", selector: ".ks-tag" } never stamps a pattern hit on .ks-tag; only element-group findings work. Preserve the matched element(s) for pattern findings or apply the selector waiver inside scoped_html_pattern_findings before collapsing the result into the body group.
    stamp_selector_ignores(dom, &mut groups, &config.ignore_selectors);

crates/detect/src/cli.rs:304

  • Passing the full URL to selector_ignores_for_target does not enforce the documented URL rule. Its scoped-glob matcher checks every slash suffix, so files: ["src/**"] can match https://host/src/page and apply a file-scoped waiver to a URL scan. URL targets should receive only entries without files, as stated in the config contract.
            ignore_selectors: selector_ignores_for_target(&self.config, url),

crates/detect/src/config.rs:827

  • This applies file-scoped entries to URL targets whose path happens to match the glob, contrary to the documented contract and url_scan_options comment. For example, https://example.com/src/demo/page.html produces a suffix src/demo/page.html, so an entry scoped to src/demo/** is incorrectly sent to the URL engine; URL scans should receive only unscoped entries.
        .filter(|e| match &e.files {
            Some(files) if !files.is_empty() => path_matches_scoped_globs(target, files),

docs/CLI-CONTRACT.md:284

  • The new config shape is added here, but the same CLI contract still documents the old detector keys/defaults below: readDetectionConfig omits ignoreSelectors, the project-config list omits it, and checkConfig still says it is an unknown detector key. That contradicts the implementation and can tell users that a valid selector-ignore key is invalid. Update those contract sections together.
                  "ignoreSelectors": [{ "rule": "undersized-ui-text", "selector": ".ks-tag", "files": ["src/a.astro"], "createdAt": "ISO", "reason": "..." }],
                  "designSystem": { "enabled": true }, "advisoryRules": "include"|"exclude" },
  • Files reviewed: 48/49 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/foundation/src/browser/mod.rs
Comment thread crates/foundation/src/browser/mod.rs
Comment thread crates/browser/src/lib.rs Outdated
Comment thread crates/hook/src/hook_lib.rs
Four gaps the review bots found in the first commit:

- **The hook dropped the waivers when the design system was off.** Both
  `design_system_options` paths returned `HookScanOptions::default()` when
  `designSystem.enabled` is false, which left `ignore_selectors` empty, so an
  opted-out component kept re-firing on every edit in exactly the projects
  that have no DESIGN.md. Component ignores are not design-system state; they
  travel either way now.
- **The in-page and extension scans never saw the key.** `BrowserConfig`
  reads `ignoreSelectors`, but the two JS adapters that build that config
  (`browser-bundle/50-scan.js` `collectConfigJson`, `60-offscreen.js`
  `configJson`) listed their keys explicitly and dropped it, so the documented
  `window.__IMPECCABLE_CONFIG__.ignoreSelectors` path did nothing. Both
  forward it now, and the bundle is regenerated.
- **Visual-contrast findings skipped the stamp.** The URL engine's visual pass
  produces its findings outside `collect_browser_findings`, so a
  `low-contrast` hit on an opted-out component was reported rather than
  waived. Each candidate carries its own selector, so the pass now resolves
  that element against the same post-reveal snapshot and stamps what the
  config waives. Page-level results (`content-hidden-at-rest`, `script-error`)
  stay unstamped: they name no element.
- **One bad entry could discard the whole page config.** `ignoreSelectors`
  used strict deserialization, so a hand-edited `{}` or `null` in the array
  failed the parse of `BrowserConfig`, which the wasm entry points answer with
  `unwrap_or_default()` — losing the design system and every other setting.
  It now filters bad entries the way `disabledValues` does.

Also: a `files` glob no longer applies to a URL scan. Globs name repo paths,
and `index.html` reaching `https://example.com/index.html` would scope an
ignore to a page the entry never named. URL scans take the unscoped entries
only, which is what the docs already promised.

Assisted-by: Claude Code
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQBUunp8QttxZqihybNmtL
Comment thread crates/browser/src/lib.rs Outdated

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread browser-bundle/50-scan.js
Comment thread crates/browser/src/lib.rs Outdated
@pbakaus

pbakaus commented Sep 11, 2026

Copy link
Copy Markdown
Owner Author

Second round of bot findings, addressed in b50098a:

  • Hook dropped the waivers when the design system was off (Bugbot, Copilot): fixed, component ignores now travel through both design_system_options early returns.
  • In-page and extension scans never saw the key (Bugbot, Copilot): fixed, collectConfigJson and configJson in browser-bundle/ forward ignoreSelectors, bundle regenerated.
  • Visual-contrast findings skipped the stamp (Bugbot, Copilot): fixed, the URL engine's visual pass now resolves each candidate against the post-reveal snapshot and stamps what the config waives.
  • One bad entry could discard the whole page config (Copilot): fixed, ignoreSelectors gets the tolerant deserializer disabledValues already had.
  • File scopes leaking into URLs (Greptile): fixed, a URL scan takes the unscoped entries only, which is what the docs promised.

One finding left standing on purpose: Greptile's "visual waivers target wrong instance". The waiver resolves the candidate with query_one(selector), which is the same resolution the visual pass itself uses — screenshot_contrast resolves its candidate in the page with document.querySelector(selector), and the fallback's existing dedup (existing_low_contrast, browser_resolved) keys on that selector too. The analyses carry a selector and no element handle, so matching the exact instance is not available without changing that protocol, and for a component-level ignore the failure mode is benign: two elements sharing a generated selector are the same component, which is the thing being waived.

@github-actions github-actions Bot added blocked: review threads Unresolved review feedback or requested changes remain waiting on contributor Waiting for the PR author to respond or make changes labels Sep 12, 2026
AI-assisted implementation by Codex at maintainer pbakaus request. Merge current main without dropping upstream oracle updates. Resolve component waivers while the actual visual candidate is in hand; carry stamps through sampling and screenshot fallback. Forward project selectors through the live prelude and per-page scan config, and keep waived findings out of direct/snapshot/offscreen UI groups. Add Rust and Node regressions. Rust workspace, rebuilt bundle/engine, full Bun/Node/oracle/plugin suite and real Chrome direct/snapshot checks passed. Deterministic full live E2E sweep is still running. Generated provider harness output intentionally omitted.
@pbakaus

pbakaus commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

AI-assisted repair requested by pbakaus: pushed 0579367, merging current main and completing selector waivers end-to-end through live project collection, per-page scoping, scan messages, visual candidate collection, and browser UI filtering. Visual waivers now use the actual candidate element before serialization, avoiding ambiguous display-selector lookups without redesigning the element identity protocol. Added Rust and JS regressions, including duplicate generated selectors with only one waived instance. Validation passed: cargo test --workspace; fresh cargo xtask bundle; fresh release engine build; bun run build; full rebuilt-engine bun run test (including oracle and real plugin loader); real Chrome direct/snapshot and duplicate-selector checks; full live E2E (38 passed, 1 skipped, 0 failures). Tracked detector bundle regenerated; feature changes omit generated provider harness churn. All inline bot findings have evidence-backed replies; CI and quiet-window review remain in progress. Nothing merged.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0579367. Configure here.

Comment thread crates/hook/src/hook_lib.rs
Comment thread crates/core/src/browser/driver.rs
Comment thread crates/core/src/browser/visual.rs
AI-assisted repairs requested by pbakaus. Resolve proposed HTML scopes against the original file; stamp browser CSS-pattern waivers across all active matches; combine attribute/config coverage in static HTML; reserve the visual budget for unwaived candidates while retaining a bounded waived sample; omit waived page-banner findings. Added failing-before Rust regressions and verified real Chrome direct/snapshot behavior. Workspace tests, bundle build, final release build, source distribution build, and rebuilt-engine full Bun/Node suite pass. Final live E2E sweep is running; the prior sweep passed 38 tests with one skip.
Comment on lines +796 to +801
for el in active {
match waiving_selector(ignores, &f.id, |sel| matches!(dom.closest(el, sel), Ok(Some(_)))) {
Some(sel) => { pattern_waived = pattern_waived.or(Some(sel.to_string())); }
None => { pattern_waived = None; break; }
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Suppression credited to wrong selector

When different ignoreSelectors entries jointly cover the elements represented by one selector-backed HTML-pattern finding, this code keeps only the first covering selector in ignoredBy. The static HTML path does the same, and the tally attributes the whole finding to that one selector. As a result, the per-entry suppression report credits one selector while another contributing entry appears unused. Preserve all contributing selectors, or attribute the aggregate finding to one selector only when that selector covers every active match.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex Fix in Claude Code

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-assisted follow-up for pbakaus: confirmed at 2011e5c in both the browser pattern loop and static HTML path. The waiver correctly requires coverage of all active matches, but ignoredBy retains only the first contributing selector, so the per-selector tally is incomplete. I have asked pbakaus to choose the reporting contract: preserve joint coverage and credit every contributor (recommended; requires additional multi-selector metadata), or require one selector to cover all matches (changes suppression behavior). Leaving this thread open pending that decision; no schema or suppression-policy change has been made.

@pbakaus

pbakaus commented Sep 15, 2026

Copy link
Copy Markdown
Owner Author

AI-assisted verification for pbakaus at 2011e5c: all GitHub CI checks and both review checks succeeded. Local cargo test --workspace, regenerated bundle, final release engine, source distribution build, rebuilt-engine Bun/Node suite, and real Chrome selector/pattern/banner checks pass. The final full live sweep had 37 passes, 1 skip, and one pricing-card worker-fetch/cycling timeout; the isolated pricing-card rerun passed both tests without code changes (the earlier full sweep also passed). Review is intentionally not declared clean: new finding discussion_r4011203000 asks how one shared CSS warning should credit multiple contributing selector waivers. This requires a reporting-contract decision, requested from pbakaus; the thread remains open and the quiet window stopped on that finding. Previously fixed Copilot threads have evidence-backed replies and remain available for reviewer verification. No merge performed.

@github-actions github-actions Bot added needs maintainer review Ready for a maintainer to review or decide and removed blocked: review threads Unresolved review feedback or requested changes remain waiting on contributor Waiting for the PR author to respond or make changes labels Sep 20, 2026
Keep selector-waived colors out of the page-wide two-hue palette gate introduced on main. A focused regression fails without the fix; Rust workspace, rebuilt-engine Bun/Node, distribution and direct/snapshot Chrome checks pass. Rebuild the detector bundle. Leave aggregate multi-selector attribution unchanged pending pbakaus's reporting-contract decision.

AI-assisted by Codex under pbakaus's instructions.
@pbakaus

pbakaus commented Sep 21, 2026

Copy link
Copy Markdown
Owner Author

AI-assisted update for pbakaus: 5ddf1ad reconciles current main and fixes the new page-wide palette interaction: selector-waived colors cannot make unrelated unwaived ink become a palette finding. The regression fails before the fix; full Rust workspace, fresh bundle/release build, rebuilt-engine Bun/Node suite, source-first distribution build, and real Chrome direct/snapshot checks pass. Full live E2E: 38 passed, 1 documented pre-existing skip. Fresh CI and review checks are green. Four previously fixed Copilot threads have been revalidated and resolved. The aggregate multi-selector attribution thread remains open pending the requested reporting-contract decision; no schema or suppression-policy change was assumed. No PR merged; generated provider harness output was omitted.

@github-actions github-actions Bot added blocked: merge conflicts PR cannot merge until conflicts are resolved waiting on contributor Waiting for the PR author to respond or make changes and removed needs maintainer review Ready for a maintainer to review or decide labels Sep 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for the PR. Impeccable is moving quickly, and this PR is currently waiting on contributor action.

It has been waiting for contributor action for 7 days. Please address the outstanding review feedback, draft state, merge conflict, policy requirement, or explicit maintainer wait request. PRs that are still waiting on contributor action after 14 days are closed automatically.

If nothing changes, this PR may be closed on or after 2026-10-07. Happy to reopen when it is ready to continue.

@github-actions github-actions Bot added the stale Inactive PR that may be closed soon label Sep 30, 2026

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

blocked: merge conflicts PR cannot merge until conflicts are resolved stale Inactive PR that may be closed soon waiting on contributor Waiting for the PR author to respond or make changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants