fix(deps): clear audit advisories by dropping rimraf/minimatch and bumping fast-xml-parser

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[rishigupta1599]

Problem

yarn audit --groups dependencies on the published production closure reported 5 advisories (4 high, 1 moderate). All reached us transitively:

Advisory	Path
brace-expansion (ReDoS)	minimatch ← glob ← rimraf@3 (@percy/core)
brace-expansion (ReDoS)	minimatch@9 (@percy/cli-doctor)
inflight (memory leak)	glob ← rimraf@3
fast-xml-parser	direct dependency of @percy/core

Why not the override the ticket recommends

PER-10247 recommends pinning brace-expansion@1 → 1.1.12 / @2 → 2.0.2 via resolutions. That does not clear the audit — only 5.0.8 satisfies the advisory, and 5.0.8 requires Node 20+. Requiring a newer Node is exactly what broke the reporting customer, so re-pinning trades one break for another. This PR removes the dependency chains instead.

Changes

• @percy/core: drop rimraf@3.Browser#close cleans the temp profile with fs.promises.rm({ recursive: true, force: true, maxRetries: 3, retryDelay: 100 }). The retry options preserve rimraf's busy-retry behaviour — on Windows the profile dir stays locked briefly after the browser exits, and a plain rm would throw EBUSY.
• scripts/test.js: same swap for the coverage-dir cleanup (the only other rimraf consumer).
• @percy/cli-doctor: drop minimatch@9. The PAC check only needed shExpMatch semantics (* and ? — no **, braces, or character classes), so it is a small self-contained matcher now. 6 new specs cover the semantics, including the cases where shExpMatch and minimatch differ.
• @percy/core: fast-xml-parser^4.4.1 → ^4.5.7.

On fast-xml-parser: why 4.5.7 and not 5.x

My first push bumped this to ^5.7.0 and broke yarn install on every CI job, all of which pin Node 14:

error xml-naming@0.3.0: The engine "node" is incompatible with this module.
Expected version ">=16.0.0". Got "14.21.3"

^5.7.0 floats to 5.10.1, which depends on xml-naming (engines >=16). Pinning fxp alone does not help — 5.7.x depends on fast-xml-builder ^1.1.7, and fast-xml-builder@1.2.0+ pulls xml-naming in as well. Holding Node 14 on 5.x would require a resolutions pin on a dependency-of-a-dependency — the same class of fragile pin this PR set out to avoid.

So this takes the top of the 4.x line (4.5.7, npm legacy tag), which clears every fast-xml-parser advisory that has a 4.x fix: GHSA-m7jm (CRITICAL, patched 4.5.4), GHSA-8gc5 (HIGH, 4.5.5), GHSA-jp2q (MODERATE, 4.5.5), GHSA-fj3w (LOW, 4.5.4).

One advisory remains and is deliberately accepted:GHSA-gh4j-gqv2-49f6 (MODERATE, vulnerable range < 5.7.0) — XMLBuilder comment/CDATA injection via unescaped delimiters. It is not reachable from our code: maestro-hierarchy.js is the only consumer, it imports XMLParser only (never XMLBuilder), and parses with processEntities: false.

So the honest score is 5 advisories → 1, not zero.

Decision: hold at 4.5.7 until the Node floor moves to ≥16. Reaching literal zero is not viable today:

• Every published package declares "engines": { "node": ">=14" }. fxp ≥5.7.0 transitively requires Node ≥16, so shipping it would violate our own published contract.
• A root resolutions pin on fast-xml-builder (to keep xml-naming out) does not help consumers — resolutions/overrides are not published. A customer running npm install @percy/cli resolves from our declared ranges, pulls fast-xml-builder@1.3.0 → xml-naming (>=16), and hits the same engine failure CI hit. That would clean our audit while degrading their install — the exact failure mode PER-10247 was filed about.

The remaining advisory is unreachable in our code, so this is a documented accept, to be revisited when the Node floor is raised.

Verification

• Production closure computed from yarn.lock (127 external packages, prod deps only, seeded from each workspace package's dependencies): rimraf, minimatch, glob, inflight, brace-expansion all absent; no Node-16-only package present; fast-xml-parser resolves to 4.5.7 with strnum@1.1.2 as its only dependency (strnum has no advisories).
• @percy/cli-doctor: 514/514 specs pass (508 existing + 6 new). Independent confirmation that minimatch@9 is gone: master's pac.js can no longer even load in this tree.
• Repo-wide lint clean.
• fs.promises.rm behaviour checked directly: recursive delete, missing-path tolerance under force, and that maxRetries/retryDelay are accepted on our Node floor.
• @percy/core suite: gating on CI. Local runs here are contaminated by leftover real temp dirs (percy-self-hosted-real-root, percy-bs-tmp-real-root) from an earlier crashed run: core mocks fs with memfs, so the fs.rmSync(recursive, force) cleanup in api.test.js cannot clear real leftovers, and the whole maestro self-hosted group then 404s. Those paths do not exist on a fresh CI machine, where force: true no-ops. Please gate on the CI result rather than my local run.

Ref: PER-10247

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] No shExpMatch coverage for a match target containing /

No test in the suite calls shExpMatch (directly or via runPacScript) with a target containing / — verified, zero such call sites. Every case uses hostname-like targets.

That gap lands on the one input class whose result actually changed in this PR: minimatch treated / as a path-segment boundary that * would not cross, the new matcher lets * cross it (which is what the PAC spec requires). It is also the only example in the canonical Netscape PAC documentation. So the behaviour change is untested in both directions.

Suggestion: add the spec example, plus one end-to-end case through runPacScript with a full-URL pattern:

it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Behaviour change vs minimatch worth a CHANGELOG note

This is not a like-for-like replacement. Verified against the installed minimatch:

• minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) → false (minimatch will not let * cross /); the new shExpMatch → true, which is what the PAC spec specifies.
• minimatch('', '*') → false; the new implementation → true.

Both are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call using a path pattern — but they are functional changes riding in under a dependency-hygiene commit, and they can flip a PAC script between DIRECT and PROXY.

Suggestion: no code change; mention in the CHANGELOG and PR description that removing minimatch also fixes full-URL and empty-string matching.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR:#2356 • Head:b7f4131 • Reviewers: stack-code-reviewer

Summary

Removes rimraf@3 from @percy/core and minimatch@9 from @percy/cli-doctor to clear npm-audit advisories reaching us through brace-expansion, replacing them with fs.promises.rm and a hand-written PAC shExpMatch; bumps fast-xml-parser^4.4.1 → ^4.5.7.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	shExpMatch keeps the type guards and the 256-char cap; treating brace/bracket/extglob syntax as literal removes an unbounded-expansion vector on attacker-influenced PAC content.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	shExpMatch fuzz-checked against a reference */? matcher over 200k random pairs with zero mismatches; loop terminates (s strictly increases on backtrack); no recursion, so no stack-overflow risk.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	fs.promises.rm(...).catch(...) preserves the prior reject-and-log contract; force: true mirrors rimraf v3's tolerance of a missing path. See Low finding on scripts/test.js.
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup still sequenced after process-exit and ws-close.
Medium	Testing	New code has corresponding tests	Pass	6 new specs covering wildcards, empty inputs, case sensitivity, literal metacharacters, and a timing-bounded pathological case.
Medium	Testing	Error paths and edge cases tested	Fail	The one canonical PAC spec example for shExpMatch — a match target containing / — has no coverage, and it is exactly where behaviour changed. See Medium finding.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI 46/46 green, including Test @percy/core on both Linux and windows-latest.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data access.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Matches surrounding style in both packages.
Medium	Quality	Changes are focused (single concern)	Pass	Dependency removal plus the minimum code to replace each dependency.
Low	Quality	Meaningful names, no dead code	Pass	No leftover references beyond one explanatory comment.
Low	Quality	Comments explain why, not what	Pass	The shExpMatch docstring states the security rationale specifically and correctly.
Low	Quality	No unnecessary dependencies added	Pass	Net removal of two published dependencies; nothing added.

Findings

• File:packages/core/package.json (the engines field)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue:@percy/core declares "engines": { "node": ">=14" }, but fs.promises.rm — and its recursive/maxRetries/retryDelay options — landed in Node v14.14.0. On Node 14.0.0–14.13.x, fs.promises.rm is undefined, so the call throws TypeError inside the .then() callback of Browser#close. The throw happens while evaluating the call expression, so the adjacent .catch never applies and this._closed rejects, surfacing at browser close rather than being logged and ignored.
• Suggestion: Set "engines": { "node": ">=14.14.0" } in packages/core/package.json. One line, and correct independently of this PR.
• Note on severity: the reviewer rated this High and blocking; I downgraded it to Medium after verifying that packages/core/src/cache/byte-lru.js:244 already calls fs.rmSync — also added in 14.14.0 — and that this line is present on master (1c83908c), not introduced here. So the declared floor was already inaccurate before this PR; what changes is exposure, from an opt-in disk-spill path to the default browser-close path. Combined with 14.0–14.13.x being superseded patch releases of an EOL major (CI runs 14.21.3), that reads as a real defect worth fixing but not a merge blocker. A maintainer who holds the engines contract strictly could fairly call it blocking — flagging the judgement rather than burying it.

---

• File:packages/cli-doctor/test/pac.test.js:92
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: No test anywhere in the suite calls shExpMatch (directly or through runPacScript) with a match target containing / — verified: zero such call sites. Every case uses hostname-like targets. That matters twice over: it is the only example given in the canonical Netscape PAC documentation (shExpMatch("http://home.netscape.com/people/ari/index.html", "*/ari/*")), and it is precisely the input class whose result changed in this PR (see the Low finding below). The behaviour change is therefore untested in both directions.
• Suggestion: Add the spec example directly, plus one end-to-end case through runPacScript using a full-URL pattern:

  it('matches wildcards across "/" when matching a full URL (spec: */ari/*)',()=>{expect(shExpMatch('http://home.netscape.com/people/ari/index.html','*/ari/*')).toBe(true);expect(shExpMatch('http://home.netscape.com/people/montulli/index.html','*/ari/*')).toBe(false);});

---

• File:packages/cli-doctor/src/checks/pac.js:335
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The replacement is not behaviour-preserving, in two ways that both alter PAC proxy-selection outcomes for customers. Verified against the installed minimatch: minimatch('http://home.netscape.com/people/ari/index.html', '*/ari/*', { dot: true, nocase: false }) returns false, because minimatch treats / as a path-segment boundary that * will not cross; the new shExpMatch returns true, matching the PAC spec. Likewise minimatch('', '*') returns false while the new implementation returns true. Both changes are corrections — the old code silently mis-evaluated any shExpMatch(url, ...) call with a path pattern — but they are functional changes shipping under a dependency-hygiene commit message, and they can flip a PAC script's verdict between DIRECT and PROXY.
• Suggestion: Call this out in the CHANGELOG and the PR description: the minimatch removal also fixes full-URL and empty-string matching. No code change needed.

---

• File:scripts/test.js (coverage-cleanup loop)
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The previous new Promise(r => rimraf(pattern, r)) used the resolver as the callback, so any error — not just a missing directory — was silently discarded. The new loop awaits fs.promises.rm with no .catch, so a genuine EPERM/EACCES now aborts the coverage run. This is an improvement in failure visibility, noted because it is a behaviour change rather than a like-for-like swap. The brace pattern {.nyc_output,coverage} was also correctly unrolled into an explicit list, since fs.rm does no brace expansion.
• Suggestion: None required — intentional and better. Dev-only script, not published, so the engines concern above does not apply here.

Other observations (non-blocking)

• A reviewer claim I checked and rejected. The reviewer stated that GHSA-mh99-v99m-4gvg is "fixed in 5.0.8, or the 1.x/2.x maintenance backports 1.1.17/2.1.3". Those versions do exist (maintenance-v1, maintenance-v2), but the advisory has a single vulnerable range, <= 5.0.7, with firstPatchedVersion: 5.0.8 — so 1.1.17 and 2.1.3 remain inside it and do not clear this advisory. They fix different brace-expansion advisories (GHSA-3jxr needs ≥1.1.16/≥2.1.2; GHSA-f886 needs ≥1.1.13/≥2.0.3). This supports the PR's approach: removing the chains is what clears GHSA-mh99 on the 1.x/2.x lines, because no backport does.
• Dev-tree findings remain, correctly out of scope.yarn.lock still resolves rimraf@^3.0.0, rimraf@^3.0.2 and brace-expansion@1.1.11/2.0.1 through dev tooling such as nyc. These are not shipped and not exposed to PAC content. A production-closure walk of the lockfile (127 external packages, seeded from each workspace package's dependencies) confirms rimraf, glob, minimatch, inflight and brace-expansion are all absent from what customers install. If the goal is a fully clean whole-repo yarn audit rather than a clean published closure, that is a separate piece of work.
• fast-xml-parser^4.5.7 is a sound choice given engines: >=14: the 5.x line reaches xml-naming (engines: >=16) both directly and through fast-xml-builder@1.2.0+, so it cannot be adopted without raising the Node floor. 4.5.7 clears every fxp advisory that has a 4.x fix. GHSA-gh4j-gqv2-49f6 (< 5.7.0) remains and is unreachable here — maestro-hierarchy.js is the only consumer and imports XMLParser, never XMLBuilder.

---

Verdict: PASS — two Medium findings worth addressing (engines floor, and the untested / case that is exactly where behaviour changed); no Critical or High issues survived verification.

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.