From 45943c3b1f98c7eccb8bdd37229e2aa4723d0d99 Mon Sep 17 00:00:00 2001 From: Guitaraholic Date: Mon, 24 Aug 2026 10:24:53 +0100 Subject: [PATCH] Fix: assert only the common ancestor of the glob roots as a URL prefix Cursor's review caught the previous commit over-correcting. One tree listed at two depths (prototype/*.html plus prototype/library/**/*.html) derived two roots, and requiring a waiver to match under both stopped a normal project-relative waiver like prototype/index.html from applying anywhere. The rule both reviews were circling is simpler: one live session is served by one server, so a single document root must sit at or above every configured page. The only prefix the resolver can safely assert is the deepest common ancestor of the glob roots. Nested roots collapse to their shared tree, so normal waivers keep applying. Disjoint roots (src/ and public/) share nothing, so no prefix is asserted and only the URL path itself matches, which keeps the earlier fix intact: a src/foo.html waiver still cannot hide a finding on a page served from public/foo.html. This also deletes the match-under-every-root machinery from the previous commit; with a single asserted prefix, plain matching is enough. Also switches the new test file to derive the repo root from import.meta.url rather than process.cwd(), per review. Co-Authored-By: Claude Fable 5 --- skill/scripts/live-browser-ignores.js | 65 +++++++++++++++------------ tests/live-browser-ignores.test.mjs | 56 ++++++++++++++--------- 2 files changed, 70 insertions(+), 51 deletions(-) diff --git a/skill/scripts/live-browser-ignores.js b/skill/scripts/live-browser-ignores.js index 649302fa6..92df1132b 100644 --- a/skill/scripts/live-browser-ignores.js +++ b/skill/scripts/live-browser-ignores.js @@ -90,12 +90,17 @@ // findingMatchesScopedIgnoreFile in cli/lib/impeccable-config.mjs (which // matches globs against every path suffix of the finding's file). // - // With several roots configured, one URL has several possible identities - // and the browser cannot tell which root actually serves it. The rooted - // groups are therefore kept separate: matchesScope treats a root-prefixed - // match as valid only when it holds under every root, so a waiver scoped - // to src/foo.html never hides a finding on a page served from - // public/foo.html. With a single root this reduces to plain matching. + // One live session is served by one server, so a single document root must + // sit at or above every configured page. The only prefix that can safely + // be asserted is therefore the deepest common ancestor of the glob roots. + // Treating each glob's own prefix as an identity goes wrong in both + // directions: disjoint roots (src/ and public/) invent simultaneous + // identities for one URL, so a waiver scoped to src/foo.html hides a + // finding on a page served from public/foo.html; nested roots (prototype/ + // and prototype/library/, from globs at two depths in one tree) are not + // alternatives at all, and demanding a waiver match under both stops + // prototype/index.html from applying anywhere. When the globs share no + // common root, no prefix is asserted and only the URL path itself matches. function pageCandidates(pathname, roots) { let pagePath = String(pathname || ''); try { @@ -108,39 +113,41 @@ // name files. Without this, /news/ never matches prototype/news/index.html. if (pagePath === '' || pagePath.endsWith('/')) pagePath += 'index.html'; - const suffixesOf = (fullPath) => { - const parts = fullPath.split('/').filter(Boolean); - const out = []; - for (let i = 0; i < parts.length; i++) { - out.push(parts.slice(i).join('/')); - } - return out; - }; - - const rooted = []; + const prefixes = []; for (const entry of Array.isArray(roots) ? roots : []) { if (typeof entry !== 'string') continue; - const prefix = entry === '' || entry.endsWith('/') ? entry : entry + '/'; - rooted.push(suffixesOf(prefix + pagePath)); + prefixes.push(entry.split('/').filter(Boolean)); } - return { bare: suffixesOf(pagePath), rooted }; + let common = prefixes.length > 0 ? prefixes[0] : []; + for (const segments of prefixes.slice(1)) { + let i = 0; + while (i < common.length && i < segments.length && common[i] === segments[i]) i += 1; + common = common.slice(0, i); + } + + const candidates = new Set(); + const addSuffixes = (fullPath) => { + const parts = fullPath.split('/').filter(Boolean); + for (let i = 0; i < parts.length; i++) { + candidates.add(parts.slice(i).join('/')); + } + }; + addSuffixes(pagePath); + if (common.length > 0) addSuffixes(common.join('/') + '/' + pagePath); + return [...candidates]; } function matchesScope(globs, candidates) { - const regexes = []; - for (const glob of globs) { + return globs.some((glob) => { + let re; try { - regexes.push(globToRegex(String(glob))); + re = globToRegex(String(glob)); } catch { // Malformed glob: skip it, as matchesAnyGlob does in the CLI. + return false; } - } - if (regexes.length === 0) return false; - const hits = (paths) => paths.some((path) => regexes.some((re) => re.test(path))); - // Matches on the URL path itself hold whichever root serves the page. - if (hits(candidates.bare)) return true; - // Root-prefixed matches only hold if no possible identity disagrees. - return candidates.rooted.length > 0 && candidates.rooted.every(hits); + return candidates.some((candidate) => re.test(candidate)); + }); } /** diff --git a/tests/live-browser-ignores.test.mjs b/tests/live-browser-ignores.test.mjs index 69586d86a..811eed6d7 100644 --- a/tests/live-browser-ignores.test.mjs +++ b/tests/live-browser-ignores.test.mjs @@ -1,10 +1,11 @@ import { describe, it } from 'node:test'; import assert from 'node:assert/strict'; import { readFileSync } from 'node:fs'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import vm from 'node:vm'; -const REPO_ROOT = process.cwd(); +const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), '..'); const SCRIPT = join(REPO_ROOT, 'skill/scripts/live-browser-ignores.js'); // Evaluated in the test realm (not a vm context) so the arrays the resolver @@ -183,35 +184,46 @@ describe('live-browser-ignores resolver', () => { assert.deepEqual(out.disabledRules, ['dark-glow']); }); - it('does not apply a waiver scoped to one root when several roots could serve the URL', () => { - // With src/**/*.html and public/**/*.html both configured, /foo.html - // could be served from either root. A waiver naming only src/foo.html - // must not hide a finding on a page actually served from - // public/foo.html; ambiguity resolves to showing the finding. + it('asserts no prefix when the configured roots share no common ancestor', () => { + // With src/**/*.html and public/**/*.html both configured, no single + // document root maps /foo.html to a unique project file, so no prefix + // is asserted. A waiver naming src/foo.html must not hide a finding on + // a page served from public/foo.html; ambiguity resolves to showing + // the finding. Bare-path spellings still apply whichever root serves it. const ignores = { roots: ['src/', 'public/'], ignoreValues: [ { rule: 'dark-glow', value: '*', files: ['src/foo.html'] }, - { rule: 'gradient-text', value: 'teal', files: ['src/foo.html'] }, - ], - }; - const out = resolve({ ignores, pathname: '/foo.html' }); - assert.deepEqual(out, EMPTY); - }); - - it('applies a scoped waiver under several roots when every identity matches', () => { - const ignores = { - roots: ['src/', 'public/'], - ignoreValues: [ - // Two globs covering both identities. - { rule: 'dark-glow', value: '*', files: ['src/foo.html', 'public/foo.html'] }, - // A bare-path glob holds whichever root serves the page. + { rule: 'clipped-overflow-container', value: '*', files: ['src/foo.html', 'public/foo.html'] }, { rule: 'em-dash-overuse', value: '*', files: ['foo.html'] }, { rule: 'gradient-text', value: '*', files: ['**/foo.html'] }, ], }; const out = resolve({ ignores, pathname: '/foo.html' }); - assert.deepEqual(out.disabledRules.sort(), ['dark-glow', 'em-dash-overuse', 'gradient-text']); + assert.deepEqual(out.disabledRules.sort(), ['em-dash-overuse', 'gradient-text']); + }); + + it('reduces nested roots to their common ancestor so normal waivers keep applying', () => { + // Globs at two depths in one tree (prototype/*.html plus + // prototype/library/**/*.html) derive the prefixes prototype/ and + // prototype/library/. Those are not alternative identities: one server + // serves both, so the document root sits at their common ancestor and + // a project-relative waiver like prototype/index.html must apply. + const ignores = { + roots: ['prototype/', 'prototype/library/'], + ignoreValues: [ + { rule: 'dark-glow', value: '*', files: ['prototype/index.html'] }, + { rule: 'em-dash-overuse', value: '*', files: ['prototype/library/**'] }, + ], + }; + assert.deepEqual( + resolve({ ignores, pathname: '/index.html' }).disabledRules, + ['dark-glow'], + ); + assert.deepEqual( + resolve({ ignores, pathname: '/library/buttons.html' }).disabledRules, + ['em-dash-overuse'], + ); }); it('survives malformed roots and percent-escapes without throwing', () => {