PER-9666: avoid require binding name that breaks the packaged binary

[rishigupta1599]

Problem

The packaged pkg binary crashes on startup (./percy --version):

TypeError: _require is not a function
    at Object.<anonymous> (/snapshot/cli/packages/cli-command/dist/lockfileDiff.js:35:43)
    ...
    at Object.<anonymous> (/snapshot/cli/packages/cli-command/dist/smartsnap.js:26:21)

Root cause

lockfileDiff.js and smartsnap.js (both new this release) bind:

const require = createRequire(import.meta.url);

When these ESM files are transpiled to CommonJS for the binary, two Babel transforms collide:

1. @babel/preset-env renames the local require → _require to avoid shadowing CJS's built-in require.
2. babel-plugin-transform-import-meta expands import.meta.url into a require('url').pathToFileURL(__filename)... call — and that emitted require gets renamed to _require too, because the scope now has a _require binding.

The transpiled line becomes:

var _require = (0, _module.createRequire)(_require('url').pathToFileURL(__filename).toString());

_require('url') runs inside _require's own initializer, while _require is still undefined → TypeError: _require is not a function.

(No other file hits this — core/src/api.js uses createRequire(url) inline and never binds it to a name called require.)

Fix

Rename the binding to cjsRequire in both files (plus usages). The import-meta-emitted require('url') then stays the real CJS require, and the transpiled output is correct:

var cjsRequire = (0, _module.createRequire)(require('url').pathToFileURL(__filename).toString());

Pure rename — no behavior change.

Verification

• Reproduced the exact failing line via the project's BABEL_ENV=dev CJS transpile, then confirmed the fixed output executes past the createRequire line.
• All 143 cli-command unit specs pass.
• ESLint clean on both files.

🤖 Generated with Claude Code

[RaghavsBrowserStack]

Claude Code PR Review

PR: #2306 • Head: 7be2263 • Reviewers: stack:code-reviewer

Summary

Renames the const require = createRequire(import.meta.url) binding to cjsRequire in lockfileDiff.js and smartsnap.js to avoid a Babel name collision (preset-env + transform-import-meta) that crashed the packaged CJS binary with TypeError: _require is not a function. Rename-only; no logic changes.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets introduced.
High	Security	Authentication/authorization checks present	N/A	Not applicable — module-level require rename.
High	Security	Input validation and sanitization	N/A	No new external input handled.
High	Security	No IDOR — resource ownership validated	N/A	No resource access logic.
High	Security	No SQL injection (parameterized queries)	N/A	No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	Confirmed correct in build output (var cjsRequire = ...); no collision.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Existing try/catch in loadSnyk untouched.
High	Correctness	No race conditions or concurrency issues	N/A	No concurrency introduced.
Medium	Testing	New code has corresponding tests	N/A	Bug only manifests in transpiled binary; covered by separate executable CI job.
Medium	Testing	Error paths and edge cases tested	N/A	No behavior change.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Rename-only; ESM source semantics unchanged.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Consistent rename across both files.
Medium	Quality	Changes are focused (single concern)	Pass	Single concern: fix binary crash.
Low	Quality	Meaningful names, no dead code	Pass	cjsRequire is descriptive.
Low	Quality	Comments explain why, not what	Pass	Detailed rationale comments added; one vague cross-reference (see Findings).
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes.

Findings

• File: packages/cli-command/src/lockfileDiff.js:14

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Comment cross-reference See PER smartsnap binary. lacks a ticket/PR ID, so it will lose meaning over time.

• Suggestion: Replace with See percy/cli#2306. or the relevant Jira ticket ID.

• File: packages/core/src/api.js:23 (out of scope — pre-existing on master)

• Severity: Low (informational)

• Reviewer: stack:code-reviewer

• Issue: Uses createRequire inline; a separate self-referential __filename pattern exists in its build output. Not introduced by this PR.

• Suggestion: Follow-up audit of other files combining a require binding with import.meta.url usage; out of scope here.

---

Verdict: PASS

[Shivanshu-07]

Add UTs - Rest LGTM

[RaghavsBrowserStack]

Claude Code PR Review

PR: #2306 • Head: 1bc95ea • Reviewers: stack:code-reviewer

Summary

Fixes a pkg-packaged binary startup crash: ESM files binding createRequire(import.meta.url) to a name called require collide with Babel's preset-env + transform-import-meta (both rename to _require), yielding TypeError: _require is not a function. The fix renames the binding to cjsRequire in lockfileDiff.js and smartsnap.js, adds a static-scan regression test, and suppresses the resulting semgrep path-traversal false-positive.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface in this change.
High	Security	Input validation and sanitization	Pass	Test paths derive from process.cwd() + repo's own packages/; no external input.
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	Rename is the correct mechanical fix; verified against compiled dist/ output.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	loadSnyk() still surfaces the underlying require failure; unchanged.
High	Correctness	No race conditions or concurrency issues	N/A	No concurrency.
Medium	Testing	New code has corresponding tests	Pass	New noRequireBinding.test.js static-scan guard added.
Medium	Testing	Error paths and edge cases tested	Pass	Test guards against an empty walk (files.length > 20).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Pure rename of a local binding; call sites updated in lockstep.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	N/A.
Medium	Performance	Long-running tasks use background jobs	N/A	N/A.
Medium	Quality	Follows existing codebase patterns	Pass	Matches existing createRequire interop idiom.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to the binary-crash fix + guard.
Low	Quality	Meaningful names, no dead code	Pass	cjsRequire is clearer; minor dead-code note on SKIP_DIRS 'test' entry.
Low	Quality	Comments explain why, not what	Fail	Test comment claims regex \s "spans newlines" but the scan is line-by-line — misleading.
Low	Quality	No unnecessary dependencies added	Pass	No new deps; test uses only fs/path.

Findings

• File: packages/cli-command/test/noRequireBinding.test.js:67

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The comment states the regex \s "spans newlines, so a wrapped declaration is caught," but the scan reads the file as split('\n') and tests each line individually. A declaration split across two lines would not be caught. Documentation inconsistency, not a coverage gap today (no split declarations exist), but it creates false confidence.

• Suggestion: Either scan the full file content with a multiline regex, or correct the comment to state the scan is line-by-line and does not catch multi-line declarations.

• File: packages/cli-command/test/noRequireBinding.test.js:85

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: FORBIDDEN has no concept of JS comments, so a commented-out // const require = createRequire(...) line in any src/ file would be flagged as a violation. No such lines exist today, but it is a future-robustness concern.

• Suggestion: Strip // comments before testing — e.g. FORBIDDEN.test(line.replace(/\/\/.*$/, '')) — or note the limitation in the comment.

• File: packages/cli-command/test/noRequireBinding.test.js:42

• Severity: Low (informational)

• Reviewer: stack:code-reviewer

• Issue: SKIP_DIRS includes test, but the walk only enters packages/<pkg>/src, so the test entry is effectively dead. Harmless and slightly misleading.

• Suggestion: No action needed; would only matter if the walk entry-point moves up to packages/<pkg>.

---

Verdict: PASS