chore: upgrade Node 14 -> 20 (toolchain, published packages, executables)

[aryanku-dev]

Ready for review. CI is green (47/47) and master is merged in. The two remaining items are decisions rather than unfinished work — see Open decisions.

Moves all three "Node versions" this repo conflates — the toolchain, the floor the published packages declare, and the runtime pkg compiles into the standalone executables — from 14 to 20.

Node 14 went EOL in April 2023. The pin has already distorted the codebase: image-size was frozen on a vulnerable line to keep Node 14 support (#2301, resolved by #2390), and the whole snyk-backed IntelliStory lockfile path carries istanbul ignore pragmas whose stated reason is "CI runs the suite on Node 14".

Implements the floor recommended in PER-10249, which found Node 20 is the largest single runtime in the customer fleet — 45.8% of builds across 17,563 orgs. The floor lands at 20, not above it, so no customer on a runtime we intend to support is broken.

Why 20 and not an exact patch

CI, .nvmrc and the pkg targets all take the major, not 20.19.5. Node 20 hit EOL on 30 April 2026 and 20.20.2 (2026-03-24) is the final release on the line, so node-version: 20 resolves to a de facto fixed version rather than a moving target — it picks up the four releases missed since 20.19.5 (20.20.0 and 20.20.2 are security releases) without the usual drift risk, because nothing new will ship for 20. This also matches what the repo already expresses: engines says >=20, and scripts/executable.sh pins pkg --targets node20-* as a deliberate range.

.node-version keeps a fully qualified 20.20.2 because nodenv resolves it against a literal directory name and errors on a partial version. That split is the pre-existing convention here — these files were 14 and 14.18.0 respectively.

Why this isn't a version-string bump

1. The test harness could not run on Node 20. scripts/loader.js implemented getFormat/getSource/transformSource — removed in Node 16.12.0 — and needed hooks to run in the main thread so it could read global.__MOCK_IMPORTS__ and the memfs volume at fs.$vol. Node 18.19/20 moved module hooks to a dedicated worker.

2. vercel/pkg cannot build a Node 20 binary. Archived at 5.8.1; fails with No available node version satisfies 'node20'.

3. A latent bug that ships the wrong architecture. pkg ran with no --targets, so output arch follows the host. The only thing keeping the binaries x64 was one architecture: x64 line on an arm64 runner — delete it, or let GitHub change the image, and every x64 customer silently gets an arm64 binary while --version passes on the arm64 runner that built it.

The dangerous part: the coverage gate stops existing

Measured on Node 18.20.8 (same hooks architecture as 20), before this change:

$ yarn workspace @percy/logger test:coverage
Executed 150 of 150 specs SUCCESS
All files | 0 | 0 | 0 | 0
EXIT CODE = 0

The removed hooks are ignored with a warning, not an error, so execution is unaffected — the src is already valid modern ESM and Node runs it natively. But .nycrc sets instrument: false, so nyc relies entirely on babel-plugin-istanbul inside the removed hook. Coverage silently goes to zero, and nyc report --check-coverage passes on an empty coverage map because there are no files to check.

Only mock-heavy suites go red. Fix those, see green everywhere with test:coverage succeeding, and the 100% gate has been switched off for every future PR. A half-finished migration's failure mode is success. Exit code is not evidence here — assert the coverage map has content.

This also means the existing test-node20 job (added by PPLT-5844) has been enforcing nothing since it landed.

Verified on real Node 20.20.2, after merging current master

yarn lint clean. yarn install --frozen-lockfile clean. yarn build succeeds across 18 projects with the new Babel target.

2,798 specs, 100% coverage everywhere it is measured, with populated coverage maps (see the section above for why the map — not the exit code — is the thing to check):

Suite	Result
logger · config · env · cli	150 · 82 · 123 · 27 — all pass, 100%
cli-command	198 / 198, 100% (up from 189 — master added IntelliStory specs)
cli-config · cli-build · cli-upload · cli-snapshot	22 · 84 · 14 · 28 — all pass, 100%
cli-exec · monitoring · webdriver-utils	73 · 40 · 238 — all pass, 100%
client	289 / 289, 100% (up from 288 — master added a request-timeout spec)
sdk-utils	169 / 169
core	1213 / 1213 — green in CI on both ubuntu and Windows

End-to-end, not just unit tests: a real Percy build with the CLI built from this branch — 23/23 snapshots, 46 comparisons, 0 failed.

cli-doctor and the dom karma suite are green in CI; both have local-only failures on a developer machine (PERCY_* env vars leaking into the env-audit assertions, and no local Firefox) which is the same class of false signal retracted below.

Correction to an earlier revision of this description. It claimed the core/sdk-utils/client failures were pre-existing and not regressions, based on a control worktree that reproduced them at master. That control was on a developer laptop, where those suites fail for local environment reasons — it does not transfer to CI, and master's Windows run at f86a18de (the commit this branches from) is green. The claim was wrong and is retracted.

What a clean control does show, run at master with the original loader on Node 20: @percy/sdk-utils cannot even load — ReferenceError: __dirname is not defined in ES module scope, because Node 20 reparses its CJS-typed test files as ESM once the dead Babel transform stops running. This branch takes it from crash-at-load to 169 specs executing.

A fourth bug was only findable on real Node 20: cli-command passed 189/189 on Node 18 but failed 14 on Node 20 with identical code. Node 20 reports packages/sdk-utils/src/index.js as format: module despite its package having no "type": "module" — while babel.config.cjs keys off that same missing field and compiles it to CommonJS. Node then parsed CommonJS output as an ES module and every export vanished. load() now declares the format Babel actually emitted.

CI

47 pass, 0 fail. Test @percy/core passes on both ubuntu (19m49s) and Windows.

One flake was hit and is worth recording, because the retry machinery could not absorb it: a single karma spec (postSnapshot … disables snapshots when a build fails) timed out at 10s in Firefox on Windows only — Chrome passed the same spec on the same runner, and ubuntu passed the identical suite. It went green on re-run with all four retry steps skipped.

The retries were useless the first time for a structural reason: scripts/test.js builds its retry list from the jasmine specDone reporter, but karma runs separately via karma.start() and its failures never enter that array. A karma-only failure therefore writes [] and every retry bails with "No recorded spec failures to retry — preserving the previous failure." The mechanism's own comment says it exists for "Windows CI, where ~1 spec out of 1000+ flakes per run on browser/server timing" — which is exactly the case it cannot handle. Not fixed here (out of scope); worth its own change.

Everything that was red on the first run is now green, and each fix is a separate commit with its cause recorded:

Was failing	Cause	Resolution
client	Node 20's AggregateError carries .code but an empty .message, and client/src/proxy.js + sdk-utils/src/proxy.js classify proxy failures via err.message.includes('ECONNREFUSED')	flattenAggregateError() restores the message Node used to produce. 288/288, 100% coverage
core's /ECONNREFUSED/ spec	same	fixed at source, no assertion weakened
cli-command ×2	intelliStory.js's parser-unavailable bail was only ever covered because Node 14 couldn't install the snyk parser	pragma with the reason stated — forcing it needs a refactor, not a spec. 189/189, 100% coverage
sdk-utils (8)	a regression from this branch. load() returned transformed source alongside format: 'commonjs', which makes Node load a second copy instead of sharing the require cache — so the ESM-importing specs and the CJS-requiring test/helpers.js held two instances and delete utils.percy.enabled mutated the wrong one	declare the format, return no source; let @babel/register transform as on Node 18. 169/169
semgrep/ci (7)	path-join-resolve-traversal on the new harness — the same rule already suppressed for 5 other files	reproduced locally with semgrep 1.173.0 first, then suppressed with rationale
Build & verify executable	pkg-fetch 3.6.x dropped Node 20 entirely (release v3.6 ships only node 22/24/26; v3.5 has 14–24)	pin @yao-pkg/pkg@6.19.0, the last release on the 3.5 line. OK: ./percy is healthy / OK: ./percy is x86-64
14 phantom failures	ubuntu matrix had no fail-fast: false, so one red package cancelled the rest	added; windows.yml already had it

Two of those were mine rather than Node's — the sdk-utils regression and the over-broad first cut of its fix (which broke cli, cli-build, cli-snapshot and cli-upload before being scoped correctly). Both are described in their commits.

Open decisions

• rollup.config.js:56 — the third hardcoded node: '14', compiling the browser bundles. Left untouched on purpose; changing it alters code injected into customer pages.
• Add a permanent CI assertion that the coverage map is non-empty, so the gate can never silently go vacuous again.

Merge order — resolved

An earlier revision of this description said #2382 must merge first, warning that the engines sweep here "rewrites packages/cli-upload/package.json wholesale and would clobber the image-size removal." Both halves of that are now obsolete:

• fix(cli-upload): replace image-size with probe-image-size (PER-10427) #2382 was superseded by fix(cli-upload): replace image-size with probe-image-size (PER-10489) #2390, which merged on 19 Aug and took the same probe-image-size dependency while letting the package do the bounded read. fix(cli-upload): replace image-size with probe-image-size (PER-10427) #2382 is closed.
• The clobber was never real. This branch's only change to packages/cli-upload/package.json is the single engines line — not a wholesale rewrite. master is merged in and the file carries both: engines: >=20 and probe-image-size: ^7.3.0, with image-size gone from yarn.lock entirely.

No ordering constraint remains.

Known breaking changes

• engines: >=20 across all 18 packages — EBADENGINE on older Node, hard fail under engine-strict. Should ship as a major.
• dist/ compiled for Node 20 — syntax errors below it. The bin/run.cjs guard turns that into a readable message for CLI users, but not for SDKs importing @percy/core directly.
• percy-linux.zip now requires glibc ≥2.28 — drops CentOS 7, RHEL 7, Amazon Linux 2, Ubuntu 18.04.
• Startup-failure exit code 0 → 1.

Also worth flagging: Node 20 reached EOL on 30 April 2026. This improves the posture substantially over Node 14 (three years of CVEs, plus OpenSSL 3 instead of an EOL 1.1.1) but will not clear a scanner rule that flags unsupported majors. Treat it as a known-temporary landing spot and schedule 22/24.

🤖 Generated with Claude Code

[aryanku-dev]

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

[aryanku-dev]

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

[aryanku-dev]

[Medium] test-node20 is now a duplicate job

This matrix moving to node: [20] removed the reason test-node20 exists — its comment says it's there because the primary matrix ran Node 14. The primary package list contains all 17 packages including @percy/cli-command, which is the only entry in test-node20's matrix, and both matrices are now [20].

So cli-command's suite runs twice per CI run, same Node, same cache key, same cache paths. Visible in the check list as Test @percy/cli-command alongside Test @percy/cli-command (Node 20).

Suggestion: delete test-node20, or repoint it at Node 22 so it becomes a real forward-looking signal for the next migration. Either way the comment needs rewriting.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Medium] "never shipped" is not true of one of these four files

packages/config/test/helpers.js is published:

files:   ["dist", "types/index.d.ts", "test/helpers.js"]
exports: { "./test/helpers": "./test/helpers.js" }

It ships to npm and SDK test suites import it, so a file on the public surface now carries a suppression justified by "dev-only, never shipped". The three scripts/* entries are correctly described.

The inaccuracy predates the trim — the original long comment grouped it the same way — but the trim made it the entire stated rationale.

Suggestion: split the block; give config/test/helpers.js its own line noting it is published for SDK test mocking, but its path joins are test-fixture-derived rather than attacker-influenced.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Medium] The new arch assertion never sees the Linux binary

This script renames run-linux* → percy, then both branches below (signed and unsigned) run mv percy-osx percy — overwriting the Linux artifact with the macOS one before any verification runs.

So verify-executable.sh ./percy validates the macOS binary in both executable-check.yml and the release job, and OK: $BIN is x86-64 never refers to Linux. percy-linux.zip ships with no smoke test and no arch check; on the PR gate (no Apple secrets) the Linux binary isn't even zipped, just silently discarded after being clobbered.

The mv sequencing predates this PR, but this PR adds the arch assertion and the explicit multi-target build, so the net it appears to cast is narrower than it reads.

Suggestion: rename to non-colliding filenames (percy-linux, percy-osx, percy.exe) and verify all three. file can assert arch on all three from the macOS runner without executing them, even though --version can only run natively.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Medium] engines >=20 has no runtime guard for direct SDK consumers

Carried forward from the previous review, still open. bin/run.cjs guards CLI users correctly — it's placed before import('../dist/index.js') and is syntactically safe on old Node. But SDKs importing @percy/core or @percy/sdk-utils directly never pass through it, and engines only warns at install time unless the consumer sets engine-strict.

Those consumers get a raw SyntaxError from the Node-20-targeted dist instead of the message this PR adds everywhere else.

Suggestion: add a minimal guard at @percy/core's entry point, or record the gap in "Known breaking changes" as accepted. A decision, not a defect.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Low] "failed to start" also reports mid-run failures

This .catch wraps the whole chain including await percy(process.argv.slice(2)) — the entire command run, not just startup. A failure deep in a snapshot, build or upload flow still prints Percy failed to start: …, which misleads anyone trying to tell a mid-run error from an import problem.

The comment two lines up also still cites "Node <15 reported this as a warning" — the same stale version rationale that ce32dd4/70625e9 cleaned out of the shell scripts, missed because those commits only touched .sh files.

Suggestion: scope the catch to import() / checkForUpdate() and let percy() own its own errors, or broaden the wording to Percy CLI exited with an error: …. Drop the Node 15 reference while there.

Reviewer: stack-code-reviewer

[aryanku-dev]

[Low] This lookup closure is duplicated in core/src/network.js

The identical (hostname, opts, cb) => dns.lookup(hostname, { ...opts, verbatim: false }, cb) exists in both files. Both are correct today — the spread preserves Happy Eyeballs' opts.all/opts.family, and both respect a caller-supplied lookup — but two copies of a subtle resolution-order override will drift.

Suggestion: extract one shared helper. That would also give this behaviour (currently untested in both places) a single place to test.

Reviewer: stack-code-reviewer

[aryanku-dev]

Claude Code PR Review

PR: #2386 • Head: 70625e9 • Reviewers: stack-code-reviewer

Summary

Moves the three "Node versions" this repo conflates — CI/dev toolchain, the engines floor the published packages declare, and the runtime pkg compiles into the standalone executables — from Node 14 to Node 20.

Re-review at a new head. Since the last review of 9ea27c3, the branch gained a merge of current master (9ba9824), the CI pin change to bare major 20 (5a29b7d), and two comment-only commits (ce32dd4, 70625e9). Scope is FULL rather than delta.

The three risks flagged for scrutiny came back clean, and I verified each rather than taking it on trust:

• No semantic conflict from the master merge. Master's new req.on('timeout') handler destroys the request with a plain Error carrying code: 'ETIMEDOUT'; flattenAggregateError's !Array.isArray(error?.errors) guard passes it through untouched. The two changes coexist in request() without interacting.
• The intelliStory.js pragmas are still correctly placed. Master's additions are in graph generation and trace writing, not the getAffectedPackages catch block, and the pragma rationale (loadSnyk() caches _snykModule at module scope, diffLockfileDeps is statically imported) still holds against the current file.
• The comment-trimming commits leaked no behavioural change. No # nosemgrep or shellcheck directive was dropped with the prose, and all .semgrepignore entries survive. One trimmed comment is now factually wrong, though — see the second finding.
• The dns change is genuinely process-local. Both call sites pass a per-request closure as the lookup option rather than calling dns.setDefaultResultOrder(), and both preserve a caller-supplied lookup.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced. @yao-pkg/pkg@6.19.0 stays pinned exactly, so the registry cannot decide what compiles customer binaries.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	The .semgrepignore suppressions cover path joins whose roots are static literals; no request input reaches them. The rationale text is wrong for one entry (finding 2), but the suppression itself is sound.
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	No SQL in this repo.
High	Correctness	Logic is correct, handles edge cases	Pass	Merge interactions verified clean against the blobs at head. The 9c97034 duplicate-module fix is correctly scoped — load() strips source only when BABEL_REG matches and a package.json positively declares commonjs — so it protects every CJS-typed workspace package, not just @percy/sdk-utils.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	bin/run.cjs gains a .catch that turns a startup crash into exit 1. Its message is over-broad (finding 5) but nothing is swallowed.
High	Correctness	No race conditions or concurrency issues	Pass	The main-thread/worker split in the new harness is the only real concurrency surface; mock interception and the memfs volume behave as before across it.
Medium	Testing	New code has corresponding tests	Fail	Unchanged from the previous review: the two verbatim: false DNS changes (core/src/network.js:637, client/src/utils.js:191) still ship with no coverage, while flattenAggregateError has 11 cases.
Medium	Testing	Error paths and edge cases tested	Pass	The AggregateError paths that motivated the change are covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	47/47 checks green at this head, including Test @percy/core on both ubuntu (19m49s) and Windows.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	—
Medium	Performance	Long-running tasks use background jobs	Fail	@percy/cli-command's suite now runs twice per CI run on identical Node and identical cache keys (finding 1) — wasted runner minutes on every push.
Medium	Quality	Follows existing codebase patterns	Pass	fail-fast: false matches what windows.yml already did; the .nvmrc/.node-version split restores the repo's pre-existing convention.
Medium	Quality	Changes are focused (single concern)	Pass	Wide by necessity — one Node floor cannot move in isolation — and rollup.config.js's node: '14' is deliberately left alone because it compiles code injected into customer pages.
Low	Quality	Meaningful names, no dead code	Pass	The unreachable PE32\+ branch in the arch grep is dead but harmless; see finding 3 for the substantive issue in that file.
Low	Quality	Comments explain why, not what	Fail	Comment volume is much improved (63 added # lines cut to 12), but two stale/incorrect rationales remain: findings 2 and 5.
Low	Quality	No unnecessary dependencies added	Pass	@yao-pkg/pkg replaces the archived vercel/pkg; nothing added to shipped dependencies.

Findings

• File: .github/workflows/test.yml:64
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: The test-node20 job exists because the primary matrix used to run Node 14 — its own comment says so. This PR changed that matrix to node: [20], and the primary matrix's package list contains all 17 packages including @percy/cli-command, which is the only entry in test-node20's matrix. Both matrices are now node: [20], so @percy/cli-command's suite runs twice per CI run on the same Node version, under the same cache key and cache paths. The job's stated premise is now false, and it burns runner minutes on every push. (This is also visible in the check list, where Test @percy/cli-command appears alongside Test @percy/cli-command (Node 20).)
• Suggestion: Delete test-node20 — its coverage is subsumed by the primary matrix — or repoint it at Node 22 so it becomes a real forward-looking signal for the Node 22 migration, and rewrite the comment either way.

---

• File: .semgrepignore:65
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: The block is headed "ESM test harness (dev-only, never shipped)" and lists four files. That is true of scripts/loader.js, loader-alias.js and register-hooks.mjs, but not of packages/config/test/helpers.js, which is published: packages/config/package.json has files: ["dist", "types/index.d.ts", "test/helpers.js"] and exports: { "./test/helpers": "./test/helpers.js" }. It ships to npm and SDK test suites import it. So a file on the public surface carries a suppression justified by "never shipped". The inaccuracy predates the trim (the original long comment grouped it the same way), but the trim carried it forward and made it the entire stated rationale.
• Suggestion: Split the block — keep the three scripts under "dev-only, never shipped", and give config/test/helpers.js its own line noting that it is published for SDK test mocking, but its path joins are test-fixture-derived and not attacker-influenced.

---

• File: scripts/executable.sh:52
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: The new arch assertion never sees the Linux binary. executable.sh renames run-linux* → percy, run-macos* → percy-osx, run-win*.exe → percy.exe, and then both branches (signed and unsigned) run mv percy-osx percy, overwriting the Linux artifact with the macOS one before any verification. So verify-executable.sh ./percy in executable-check.yml and in the release job both validate the macOS binary, and OK: $BIN is x86-64 never refers to Linux. percy-linux.zip therefore ships without ever being smoke-tested or arch-checked. On the PR gate (no Apple secrets) the Linux binary isn't even zipped — it's silently discarded after being clobbered. The mv sequencing predates this PR, but this PR is what introduces the arch assertion and the explicit multi-target build, so the safety net it adds is narrower than it reads.
• Suggestion: Verify all three artifacts, or rename to non-colliding filenames (percy-linux, percy-osx, percy.exe) and verify each. file can assert arch on all three from the macOS runner without executing them, even though the --version smoke test can only run natively.

---

• File: packages/core/package.json:15
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: Carried forward, still open. The bin/run.cjs guard is correctly placed before import('../dist/index.js') and is syntactically old-Node-safe, so it does protect CLI users. But SDKs that import @percy/core or @percy/sdk-utils directly never pass through run.cjs, and engines: ">=20" only produces an install-time warning unless the consumer sets engine-strict. Those consumers get a raw SyntaxError from the Node-20-targeted dist instead of the message this PR took care to add everywhere else.
• Suggestion: Either add a minimal guard at @percy/core's entry point, or state the gap explicitly in "Known breaking changes" as accepted. This is a decision, not a defect.

---

• File: packages/cli/bin/run.cjs:19
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The .catch wraps the whole chain including await percy(process.argv.slice(2)), which is the entire command run — not just startup. Any failure deep in a snapshot, build or upload flow is reported as Percy failed to start: …, which will mislead anyone distinguishing a mid-run failure from an import/startup problem. The comment above it also still cites "Node <15 reported this as a warning" — the same class of stale version rationale that ce32dd4 and 70625e9 cleaned out of the shell scripts, missed because those commits only touched .sh files.
• Suggestion: Scope the catch to the startup portion (import() / checkForUpdate()) and let percy() own its own error messaging, or broaden the wording to Percy CLI exited with an error: …. Drop the Node 15 reference while there.

---

• File: packages/client/src/utils.js:191
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: (hostname, opts, cb) => dns.lookup(hostname, { ...opts, verbatim: false }, cb) is duplicated verbatim between client/src/utils.js and core/src/network.js. Both are correct today — the spread preserves Happy Eyeballs' opts.all/opts.family, and both respect a caller-supplied lookup — but two copies of a subtle resolution-order override will drift.
• Suggestion: Extract one shared helper. Doing so would also give the untested behaviour (finding under Testing above) a single place to test.

Raised by other reviewers (not independently confirmed)

• scripts/register-hooks.mjs — materialised mock modules are written to a real tmp path that never matches BABEL_REG, so with Babel's only: [BABEL_REG] their source now reaches Node's loader untransformed, where previously transformSource ran unconditionally. Harmless if mocks stay simple re-export shims, but worth a pass over existing mockFileModule usages for any syntax Node cannot parse natively. Non-gating; I did not verify this one.

---

Verdict: PASS — no High or Critical findings. Four Mediums: one duplicated CI job, one incorrect suppression rationale, one verification gap on the shipped Linux binary, and one open decision on SDK guards.

[aryanku-dev]

Claude Code PR Review

PR: #2386 • Head: 3dee1ed • Reviewers: stack-code-reviewer

Continues the previous review — changes since 70625e9f (delta, 5 files / 133 lines).

Summary

Raises Percy CLI's minimum Node from 14 to 20. This delta is a cleanup pass: it removes two dead Node 14 fallback branches from PercyServer#close(), deletes the now-redundant test-node20 CI job, corrects an inaccurate .semgrepignore heading, retitles run.cjs's catch message, and reverts three comment-only files to keep the PR's footprint down.

Four of the six previously-open findings are resolved. One new Medium was found and has already been fixed on this head — details below.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None in the delta.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	.semgrepignore keeps all four original entries; the split only corrects which rationale applies to which file.
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Pass	The server.js guard removal is a no-op on Node >=20 — both closeIdleConnections and closeAllConnections exist from 18.2, and nothing in the repo subclasses or stubs them away. The if/else pairs were removed whole, so no uncoverable branch is left behind.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	run.cjs's .catch still exits 1; only the wording changed, and no test asserts on the old string.
High	Correctness	No race conditions or concurrency issues	Pass	#sockets was deliberately kept (still used by the drainMs: 0 path), as was the istanbul ignore next on the 5s force-close timer.
Medium	Testing	New code has corresponding tests	Pass	No new code paths in this delta; it is a net removal.
Medium	Testing	Error paths and edge cases tested	Fail	The release-only Windows leg (executable.yml:107) has no PR-time coverage at all — see the finding below. Pre-existing, but this delta is what surfaced it.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Deleting test-node20 loses no coverage: the primary test matrix still lists all 17 packages including @percy/cli-command, no job references test-node20 via needs:, and the YAML parses with build/test/regression intact.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	—
Medium	Performance	Long-running tasks use background jobs	Pass	Fixed by this delta: @percy/cli-command no longer runs twice per push.
Medium	Quality	Follows existing codebase patterns	Pass	—
Medium	Quality	Changes are focused (single concern)	Pass	Improved by the revert — three comment-only files were pulled back out, taking the footprint from 51 files to 48.
Low	Quality	Meaningful names, no dead code	Pass	Net −134/+15 across the delta; the dead Node 14 branches are gone.
Low	Quality	Comments explain why, not what	Pass	Four stale Node 14 rationales corrected or removed.
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: scripts/verify-executable.sh:42
• Severity: Medium
• Reviewer: stack-code-reviewer
• Status: Fixed on this head (3dee1ede)
• Issue: 69f5e11b removed the PE32\+ alternative from the arch assertion, justified by the claim that the script is only ever handed ./percy — the macOS binary, after executable.sh's mv percy-osx percy. That claim was wrong. executable.yml:107 runs verify-executable.sh ./percy.exe in the Windows signing job, against a genuine 64-bit PE. Whether the remaining x86[-_]64 alternative still matches depends on what that runner's libmagic prints; most builds emit PE32+ executable (console) x86-64, for MS Windows, which would match, but nothing in CI verifies it. The job fires only on release: published, so a wrong guess surfaces as a failed release, not a failed PR check.
• Suggestion: Reverted in 3dee1ede. The removal bought nothing — it deleted an alternative believed dead — so the asymmetry favours restoring it. Worth confirming separately what file percy.exe actually prints on windows-2022, since that would let the check be tightened deliberately rather than by assumption.

Carried forward, still open

• scripts/executable.sh:52 (Medium) — mv percy-osx percy in both branches overwrites the Linux binary before verification, so percy-linux.zip ships without a smoke test or arch check. Untouched by this delta (executable.sh has a zero-line diff across 70625e9f..3dee1ede). The finding above compounds it: of three shipped artifacts, one is verified at PR time and one only at release time.
• packages/core/package.json:15 (Medium) — engines: >=20 has no runtime guard for SDKs importing @percy/core directly. A decision, not a defect.
• packages/client/src/utils.js:191 (Low) — the verbatim: false lookup closure is still duplicated in core/src/network.js.

Resolved by this delta

• .github/workflows/test.yml:64 Medium — test-node20 redundant, cli-command runs twice on Node 20 Resolved — job deleted in 69f5e11b; verified no needs: references remain and the primary matrix still covers @percy/cli-command.
• .semgrepignore:65 Medium — comment calls config/test/helpers.js never-shipped, but it is published Resolved — block split in f1e9ae5a; all four entries retained, and that file now sits under an accurate "published but test-only" heading.
• packages/cli/bin/run.cjs:19 Low — "Percy failed to start" also reports mid-run failures Resolved — now "Percy CLI exited with an error"; no test referenced the old string.
• packages/core/src/server.js — istanbul ignore next resting on a false "CI matrix includes Node 14" premise Resolved — the guards, the empty else and the pragma are all gone; verified nothing stubs the two http.Server methods.

---

Verdict: PASS — the one new Medium is already fixed on this head. Three findings carry forward, none of them regressions and none blocking.