Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
0e90375 to
7c59f0a
Compare
…egexp' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
RaghavsBrowserStack
changed the title
Claude Code PR ReviewPR: #2207 • Head: f7fdfd5 • Reviewers: stack:code-reviewer (ran but returned no structured findings — analysis completed by the orchestrator) SummaryAdds a SmartSnap feature to the Percy CLI: it streams a Storybook build's module-graph stats ( Review Table
FindingsF1
F2
F3
F4
F5
F6
Strengths worth calling out: the security work is solid — schema-driven Verdict: FAIL |
|
F1: const stripNull = s => (... ? s.replaceAll(NULL_CHAR, '') : s) uses String.prototype.replaceAll, added in Node 15. Fixed |
Claude Code PR Review (re-review)PR: #2207 • Head: e8ee178 • Reviewers: stack:code-reviewer + orchestrator verification
SummaryAdds the SmartSnap feature to the Percy CLI: streams a Storybook build's module-graph stats ( Scope correction (resolves prior F5)The earlier review computed the diff against a stale local Review Table
FindingsResolved since prior review
Accepted by author (non-blocking)
Open (Medium, non-blocking per author's process)
Verdict: PASS |
| // pick a canonical source, so bail. | ||
| const LOCKFILE_NAMES = ['package-lock.json', 'yarn.lock', 'pnpm-lock.yaml']; | ||
| // n is from the hardcoded LOCKFILE_NAMES allowlist (reviewed, approved by security) | ||
| const presentLockfiles = LOCKFILE_NAMES.filter(n => fs.existsSync(path.join(absManifestDir, n))); // nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal |
| // pick a canonical source, so bail. | ||
| const LOCKFILE_NAMES = ['package-lock.json', 'yarn.lock', 'pnpm-lock.yaml']; | ||
| // n is from the hardcoded LOCKFILE_NAMES allowlist (reviewed, approved by security) | ||
| const presentLockfiles = LOCKFILE_NAMES.filter(n => fs.existsSync(path.join(absManifestDir, n))); // nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal |
| throw new SmartSnapBailError(`SmartSnap: invalid statsFile "${statsName}" — must be a .json filename; running full snapshot set`); | ||
| } | ||
| // statsName is path.basename'd and regex-validated above; buildDir is operator-supplied config (reviewed, approved by security) | ||
| const resolvedStatsPath = path.join(path.resolve(buildDir), statsName); // nosemgrep |
| throw new SmartSnapBailError(`SmartSnap: invalid statsFile "${statsName}" — must be a .json filename; running full snapshot set`); | ||
| } | ||
| // statsName is path.basename'd and regex-validated above; buildDir is operator-supplied config (reviewed, approved by security) | ||
| const resolvedStatsPath = path.join(path.resolve(buildDir), statsName); // nosemgrep |
| } | ||
| const manifestDir = uniqueDirs[0]; // repo-relative; '.' for root | ||
| // manifestDir is derived from git-tracked paths under projectRoot (reviewed, approved by security) | ||
| const absManifestDir = path.resolve(projectRoot, manifestDir); // nosemgrep |
| } | ||
| const manifestDir = uniqueDirs[0]; // repo-relative; '.' for root | ||
| // manifestDir is derived from git-tracked paths under projectRoot (reviewed, approved by security) | ||
| const absManifestDir = path.resolve(projectRoot, manifestDir); // nosemgrep |
| } | ||
|
|
||
| // lockfileName is one of the hardcoded LOCKFILE_NAMES allowlist (reviewed, approved by security) | ||
| const newLockfile = fs.readFileSync(path.join(absManifestDir, lockfileName), 'utf8'); // nosemgrep |
| if (oldLockfile === newLockfile) return []; | ||
|
|
||
| // joined with the literal 'package.json' under the resolved absManifestDir (reviewed, approved by security) | ||
| const packageJson = fs.readFileSync(path.join(absManifestDir, 'package.json'), 'utf8'); // nosemgrep |
| // path.resolve treats it as the target directly; otherwise it's joined | ||
| // against `invocationDir`. Then re-base against the git project root. | ||
| // rel comes from build stats file paths, re-based against projectRoot on the next line (reviewed, approved by security) | ||
| const abs = path.resolve(invocationDir, rel); // nosemgrep |
| // applies, without exporting the module-private helpers it delegates to. | ||
| function embeddedJson(html, name) { | ||
| // test-only helper; name is a hardcoded literal ('vertices'/'edges') from the test, not external input (reviewed, approved by security) | ||
| let match = html.match(new RegExp(`const ${name} = (.*);`)); // nosemgrep |
… can't cancel the rest" This reverts commit be3bf0c.
Claude Code PR ReviewPR: #2207 • Head: 9629722 • Reviewers: stack:code-reviewer SummaryExports SmartSnap (Storybook smart-snapshot selection) as a first-class Review Table
Findings
Verdict: PASS |
The PR #2207 revert also reverted the Semgrep image pin back to returntocorp/semgrep, which reintroduces the bug where --sarif counts nosemgrep-suppressed findings as blocking. Keep the 1.164.0 pin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Revert "PPLT-5495: Add file diff data to generate graph (#2273)" This reverts commit 2392017. * Revert "Export applySmartSnap functionality from CLI (#2207)" This reverts commit d3f7f71. * Keep Semgrep.yml pinned to semgrep/semgrep:1.164.0 The PR #2207 revert also reverted the Semgrep image pin back to returntocorp/semgrep, which reintroduces the bug where --sarif counts nosemgrep-suppressed findings as blocking. Keep the 1.164.0 pin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
Summary
Exports the
applySmartSnappipeline andSmartSnapBailErrorfrom@percy/cli-command, plus the SmartSnap client endpoints on@percy/client. Together these let@percy/cli-storybookfilter snapshots down to those actually affected by a diff.What's included
@percy/client:getSmartsnapSnapshotNameToCommit(),generateSmartsnapGraph(), andsmartsnap_graphsupport on the existinggetStatus()poll helper.@percy/cli-command:applySmartSnap(percy, snapshots, smartSnapConfig, buildDir)— orchestrates the pipeline: git diff vs. base build commit →.storybook/lockfile bail checks → stats parse → graph job → snapshot filter. Honorsbaseline,untraced,bailOnChanges,statsFile, andtraceconfig.SmartSnapBailError— recoverable failures throw this so callers fall back to a full snapshot set instead of crashing.lockfileDiff.js— top-level dep diff viasnyk-nodejs-lockfile-parser(optionalDep, Node ≥18).graphTrace.js+graphTraceTemplate.html— renders an interactive Drawflow graph whentrace: true.Notes
snyk-nodejs-lockfile-parseris an optionalDependency because it requires Node ≥18 while the CLI supports ≥14. Missing parser → bail to full set, not a crash.Test plan
yarn testinpackages/clientandpackages/cli-commandpasses.@percy/cli-storybookconsumingapplySmartSnapend-to-end.