Conversation
…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
|
There was a problem hiding this comment.
🟡 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
groupshere, but visual-contrast findings are appended afterward byvisual_contrast_result_findingand by the URL engine'srun_visual_contrast_fallback. Those laterlow-contrastfindings have noignoredBy, so a matching component still reports them even whenignoreSelectorscovers 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_keybypage_passbefore this call, andscoped_html_pattern_findingsreturns no element attribution. Thereforestamp_selector_ignoreschecksbody.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 insidescoped_html_pattern_findingsbefore 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_targetdoes not enforce the documented URL rule. Its scoped-glob matcher checks every slash suffix, sofiles: ["src/**"]can matchhttps://host/src/pageand apply a file-scoped waiver to a URL scan. URL targets should receive only entries withoutfiles, 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_optionscomment. For example,https://example.com/src/demo/page.htmlproduces a suffixsrc/demo/page.html, so an entry scoped tosrc/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:
readDetectionConfigomitsignoreSelectors, the project-config list omits it, andcheckConfigstill 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.
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
|
Second round of bot findings, addressed in b50098a:
One finding left standing on purpose: Greptile's "visual waivers target wrong instance". The waiver resolves the candidate with |
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.
|
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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ 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.
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.
| 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; } | ||
| } | ||
| } |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
|
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. |
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.
|
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. |
|
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. |

The problem, from a real review
On impeccable-site PR #34 the author wanted
undersized-ui-textoff for one component:.ks-tag, a 10px mono index label. The only per-rule opt-out that exists isdata-impeccable-ignoreon an element, so the change carried eleven of them, one per instance of the same component (25 opt-out attributes in the PR overall):The alternatives were worse:
ignoreRules undersized-ui-textturns the rule off for the whole project,ignoreFilesturns off every rule for the file, andignoreValuesonly 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:
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 withfilesglobs likeignoreValues; an entry withoutfilescovers every target.--no-configdisables them.impeccable ignores remove-selectorandclearremove them, andlistshows 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.ignoredByin the serialized group findings,Finding.ignoredByin the extras). The config layer (filterDetectionFindings) drops those and tallies them by{rule, selector, count}, and the CLI prints onedimline per entry on stderr in every mode, so--jsonstdout stays the findings array a consumer parses: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
writeDetectionConfigdoes not add an emptyignoreSelectorsto an existing config.BrowserFinding.ignoredByandBrowserConfig.ignoreSelectorsare skipped in serialization when unset, so existing bundle output is byte-identical.Where it applies
BrowserConfig.ignoreSelectors, also readable fromwindow.__IMPECCABLE_CONFIG__.ignoreSelectorsDetectHtmlOptions.ignore_selectors(element rules and selector-backed html-patterns)detect_html_source_jsonignoreSelectorsoption, host-narrowedimpeccable detectdoctorvalidates theruleof each entry alongsideignoreRules(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.rstests: stamping over the fake DOM (three instances plus a descendant, other rules and other elements clean), serialization carryingignoredByonly when set,BrowserConfigparsing and omitting the key.crates/detect/src/config.rstests: 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:SelectorIgnoreunit tests and theignoredBystamp round-trip.detect-selector-ignoreswithdetect-selector-ignore-{text,json,quiet,no-config}andignores-selector-{list,add,remove,missing-args,star-refused}plusignores-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.jsonis 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, theignoresactions, thelistrow),docs/ENGINE.md(the wasm option),README.md, andskill/reference/hooks.md, where the suppression ladder now names the component ignore between the file-scoped value ignore andignore-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 repeateddata-impeccable-ignoreon many component instances. Manage entries withimpeccable ignores add-selector/remove-selector(optionalfilesscoping andreason), surfaced inignores listand 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--jsonruns). The in-page bundle and offscreen scan path filterignoredByfor 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_selectorsis 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.