Claude Code PR Review

PR: #2339 • Head: 001c036 • Reviewers: stack:code-reviewer

Summary

Adds IntelliStory — an affected-story graph feature for @percy/storybook: parses Storybook build stats, diffs against a git baseline (incl. lockfile/dependency diffing via optional snyk-nodejs-lockfile-parser), enqueues a server-side affected-story graph job, tags snapshots for selection, and optionally renders a debug trace.html. Plumbs intelliStory/storybookPath through @percy/client and @percy/core, and adds a Node 20 CI leg for the Node-18+-only snyk path.

Review Table

Findings

• File: packages/cli-command/src/graphTrace.js:114-116

• Severity: Medium

• Reviewer: stack:code-reviewer (verified)

• Issue: .replace('__VERTICES_JSON__', safeJson(...)) uses plain-string replacement values. JS honors $$, $&, $`, $', $n special patterns in the replacement string even when the search arg is a literal. Embedded JSON contains arbitrary file paths; a path with $' re-injects the rest of the template, breaking the embedded JS. safeJson's escaping doesn't cover this — the corruption happens in the .replace call itself.

• Suggestion: Pass a function as the replacement: .replace('__VERTICES_JSON__', () => safeJson(...)). Add a graphTrace.test.js case with $&/$'/$$ in a vertex name.

• File: packages/cli-command/src/intelliStory.js:16, 34-41

• Severity: Medium — ✅ RESOLVED in 001c036

• Reviewer: stack:code-reviewer (verified)

• Issue: GLOB_CHARS = /[*?]/ only routed */? patterns through the glob matcher, but patternToRegex enables {extended:true, globstar:true} (brace/bracket support). A configured bailOnChanges: ['src/{a,b}.js'] or bracket pattern silently fell through to literal str === pattern and never matched — silently disabling a documented config option (bailOnChanges/untraced).

• Fix applied: Broadened detection to const GLOB_CHARS = /[*?{}[\]]/;. Added brace- and bracket-glob tests to assertNoBailOnChanges() and enforceUntraced() — verified failing under the old regex and passing under the fix (full 156-spec suite green).

• File: packages/cli-command/src/intelliStory.js:326-336

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: Only SNYK_LOCKFILE_PARSER_UNAVAILABLE is converted to a graceful IntelliStoryBailError; a buildDepTree parse failure (malformed/older-ref lockfile) rethrows as a raw Error. Every other failure path degrades to "running full snapshot set."

• Suggestion: Confirm the @percy/storybook caller treats all applyIntelliStory throws as fall-back-to-full-set. If not, wrap this rethrow in IntelliStoryBailError for consistency.

• File: packages/cli-command/src/intelliStory.js:67-70

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: assertSafeRef regex correctly blocks leading -, but also rejects ordinary safe ref syntax like HEAD~5 / abc123^2.

• Suggestion: Confirm intentional, or widen charset to include ~^ (still safe given the leading-char restriction).

• File: packages/cli-command/src/intelliStory.js:438-440

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The packageAffectedNodes merge into affectedNodes (the actual mechanism folding lockfile-driven dep changes into the graph) is /* istanbul ignore next */ rather than covered by a real test.

• Suggestion: Add an itPosix-style Node≥18 integration test exercising the merge end-to-end.

• File: packages/cli-command/src/intelliStory.js (resolveAndIndex)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Absolute module ids resolving outside projectRoot (symlinked pkg, monorepo sibling) produce ../-prefixed paths added straight into the files index without a guard.

• Suggestion: Bail or drop entries where rel.startsWith('..').

• File: .semgrepignore:118-131 / packages/client/src/client.js:2591-2592 / .github/workflows/test.yml

• Severity: Info

• Reviewer: stack:code-reviewer

• Issue: (a) Whole-file semgrep suppression means future traversal code in the growing intelliStory.js goes unchecked. (b) client.intelliStoryStats is lazily-created, never reset — accumulates across builds if a client is reused. (c) New test-node20 CI job (workflow change) — flagged for visibility; mirror logic looks consistent, no action requested.

• Suggestion: Consider narrowing semgrep scope over time; reset stats in createBuild.

Verdict: PASS

Claude Code PR Review

PR: #2339 • Head: 966c05d • Reviewers: stack:code-reviewer (definition run inline — the colon-prefixed agent name is not registered as a launchable agent type), findings then re-verified against source by the orchestrator

Summary

Adds IntelliStory to @percy/cli-command: for a Storybook build it resolves a baseline (explicit or API-predicted), diffs the working tree against it, ships the enriched module graph to the new /intelli_story endpoints, and tags each snapshot with intelliStory/storybookPath so the API performs affected-story selection server-side — bailing to a full snapshot run via IntelliStoryBailError whenever it can't reason safely about a change. Also folds in the packaged-binary crash fix (createRequire bound as cjsRequire, guarded by a new static noRequireBinding test) and a Node 20 CI leg for the Snyk-backed lockfile-diff path.

The bail-first posture is the right architecture for a feature that can silently drop visual coverage, and it is applied thoroughly — 15+ explicit bail sites covering missing build id, unreadable/invalid stats, git failures, ambiguous or absent lockfiles, manifest changes spanning directories, browser-upgrade builds, and graph-job timeouts. All four Medium findings below are gaps in that otherwise-sound safety net, not architectural problems.

Review Table

Findings

1. API/network failures escape as raw errors instead of IntelliStoryBailError

• File: packages/cli-command/src/intelliStory.js:181, :220, :354
• Severity: Medium
• Reviewer: stack:code-reviewer (refined by orchestrator verification)
• Issue: Every git, fs, config, and lockfile failure in this module is wrapped in IntelliStoryBailError so the caller can degrade to a full snapshot run. The three percy.client calls — getIntelliStorySnapshotNameToCommit (:220), getStatus('intelli_story_graph', …) inside pollGraphStatus (:181), and generateIntelliStoryGraph (:354) — are not. A transient 5xx, timeout, or DNS failure therefore propagates as a plain Error. If @percy/storybook catches IntelliStoryBailError specifically (the type this module exports for exactly that purpose), an API blip hard-fails the whole storybook command instead of falling back to a full run — the opposite of the module's fail-safe posture. The graph-job timeout path is handled correctly (:359); it is only thrown exceptions that leak.
• Suggestion: Wrap each client call and rethrow, e.g.

  let baseLookup;
  try {
    baseLookup = await percy.client.getIntelliStorySnapshotNameToCommit(percy.build?.id);
  } catch (e) {
    throw new IntelliStoryBailError(`IntelliStory: base build lookup failed: ${e.message}; running full snapshot set`);
  }

  Same for generateIntelliStoryGraph, and in pollGraphStatus treat a throw as a non-done status.

2. .storybook config directory is hardcoded — a custom configDir never trips the bail

• File: packages/cli-command/src/intelliStory.js:114, :244
• Severity: Medium
• Reviewer: stack:code-reviewer (verified)
• Issue: Both the graph exclusion (EXCLUDED_DIRS = new Set(['node_modules', '.storybook'])) and the bail check (assertNoDotStorybookChange) match the literal path segment .storybook. Storybook's config directory is configurable (-c <dir> / configDir), and intelliStoryConfig has no field to communicate it. For a project using e.g. config/storybook/, a change to main.js or preview.js — which can affect every story — produces no bail, and filtering proceeds as though nothing global changed. This is the under-snapshot direction: real visual regressions ship unnoticed.
• Suggestion: Thread the resolved config-dir name through intelliStoryConfig from @percy/storybook into both call sites, defaulting to .storybook:

  export function assertNoDotStorybookChange(affectedNodes, configDir = '.storybook') {
    const hit = affectedNodes.find(p => p.split(/[/\\]/).includes(configDir));
    …
  }

3. Bare-extension bailOnChanges globs silently match nothing below the repo root

• File: packages/cli-command/src/intelliStory.js:28 (patternToRegex), consumed at :251
• Severity: Medium
• Reviewer: stack:code-reviewer (severity downgraded from High by the orchestrator — see below)
• Issue: With { extended: true, globstar: true }, glob-to-regexp('*.css') compiles to ^([^/]*)\.css$ — verified locally — so it matches only root-level files, never src/components/Button.css. A user who configures bailOnChanges: ['*.css'] expecting "bail on any stylesheet change" gets no bail, and IntelliStory filters as if nothing unsafe changed. untraced shares the semantics but fails safe (nothing is dropped, so more snapshots are taken).
  Downgraded from the sub-reviewer's High because this is standard glob behavior — minimatch, picomatch, and GitHub Actions paths: filters all treat a bare *.ext as root-only — and the tests assert it deliberately (intelliStory.test.js:266, :286-289), so it is a considered choice rather than a coding error. Still worth flagging: bailOnChanges is the one user-facing safety valve and the failure mode is silent.
• Suggestion: Either document that patterns are path-segment-scoped and **/*.css is required (there is currently no documentation for bailOnChanges/untraced anywhere in the repo), or auto-broaden bare-name patterns before compiling: const p = pattern.includes('/') ? pattern : '**/' + pattern;. If the current semantics stay, rename the test to state the intent ('a bare-extension pattern does not match nested paths') so the behavior reads as deliberate.

4. Stats modules dropped from the graph are logged only at debug, with no threshold

• File: packages/cli-command/src/intelliStory.js:132-133, :161-166
• Severity: Medium
• Reviewer: orchestrator
• Issue: transformModule returns null for any module whose id does not resolve to an absolute path, and readStats counts those into droppedModules and emits a single log.debug. Missing modules mean missing edges in the dependency graph, and a missing edge is exactly what makes the API judge a story unaffected. If a Storybook/webpack version emits a large share of synthetic or virtual module ids, the graph is quietly degraded and the run still proceeds — visible only under --verbose. Every other incomplete-data condition in this file bails.
• Suggestion: Escalate the existing message to log.warn, and bail above a threshold:

  if (droppedModules) {
    const ratio = droppedModules / rawModules.length;
    if (ratio > 0.1) throw new IntelliStoryBailError(`IntelliStory: ${droppedModules}/${rawModules.length} modules had unresolved ids; graph would be incomplete; running full snapshot set`);
    log.warn(`IntelliStory: dropped ${droppedModules} module(s) with unresolved (non-absolute) ids`);
  }

5. mapEntry normalizes source only for type === 'src'

• File: packages/cli-command/src/intelliStory.js:137
• Severity: Low
• Reviewer: stack:code-reviewer
• Issue: resolveAndIndex runs only when copy.type === 'src'. Fixtures only exercise 'src' and 'module' (the latter a no-op, since package specifiers are never absolute). If the external enrichment tool ever emits a third type carrying an absolute path — an asset, CSS, or JSON import — that raw absolute path ships unnormalized in the modules payload: it leaks the developer's home or CI runner path, and it cannot be correlated against the project-relative affectedNodes, which is a false-negative path.
• Suggestion: Drop the type gate — resolveAndIndex already returns non-absolute values untouched, so running it for every string source is safe and type-agnostic.

6. gitDiffNames omits --no-renames while getAffectedFileLocations uses it

• File: packages/cli-command/src/intelliStory.js:75 vs :84
• Severity: Low
• Reviewer: stack:code-reviewer (correction: rename detection is git's default since 2.9, so the inconsistency is present out of the box, not only under custom config)
• Issue: affectedNodes comes from git diff --name-only with rename detection active, which collapses a rename to the new path alone; getAffectedFileLocations runs the same diff with --no-renames, which yields a delete + add pair. The two views of one diff can disagree for renamed files. Low practical impact — the old path no longer exists to be imported — but it makes the two graph inputs non-comparable.
• Suggestion: Add --no-renames to the gitDiffNames invocation for determinism and parity with :84.

7. trace.html loads Drawflow from a public CDN

• File: packages/cli-command/src/graphTraceTemplate.html:8, :12
• Severity: Low
• Reviewer: stack:code-reviewer (verified)
• Issue: The debug trace viewer pulls Drawflow's CSS and JS from cdn.jsdelivr.net at view time. This is opt-in (trace: true) and off the production path, but it is precisely the artifact someone opens after a bail or a mis-selection — often inside a locked-down or air-gapped CI environment where the CDN is unreachable and the trace renders blank.
• Suggestion: Vendor the (small) Drawflow bundle into the package and inline it into the template.

8. Whole-file .semgrepignore suppression for intelliStory.js

• File: .semgrepignore:49-57
• Severity: Low
• Reviewer: stack:code-reviewer
• Issue: The suppression is file-level rather than scoped to the specific path.join/path.resolve sites that trigger the false positive. intelliStory.js does a fair amount of fs/path work, so any genuinely unsafe join added later goes unscanned. The stated rationale (inline // nosemgrep not honored by the CI semgrep version) is legitimate and well documented.
• Suggestion: Track a follow-up to narrow the suppression once the CI semgrep version honors inline annotations.

9. No bail is logged at the throw site

• File: packages/cli-command/src/intelliStory.js — all IntelliStoryBailError throw sites
• Severity: Low
• Reviewer: stack:code-reviewer
• Issue: Every bail constructs a carefully worded message but none logs at warn/info before throwing, so whether a user ever learns that filtering did not apply depends entirely on the catch site in @percy/storybook — outside this repo and, per the PR body, still a follow-up. If that catcher logs at debug or swallows while falling back, the fail-safe is invisible.
• Suggestion: log.warn(message) at the throw site (or inside a small bail(message, log) helper) so visibility does not depend on the external consumer.

Verified non-issues

• stream-json leftover: confirmed absent from packages/cli-command/package.json and unreferenced anywhere in cli-command — correctly removed in 9c525f10.
• Backward compatibility: the new snapshotSchema fields (intelliStory, storybookPath) are optional, and createSnapshot sends 'intelli-story': intelliStory || null / 'storybook-path': storybookPath || null, so non-IntelliStory callers are unchanged — cli-upload/test/upload.test.js pins exactly that.
• getStatus response shape: intelli_story_graph returns a top-level { status, data }, unlike the batched-by-id shape used for snapshot/comparison. pollGraphStatus and client.test.js:890-905 agree on it; an API-contract quirk, not a defect.

Verdict: PASS

Claude Code PR Review

PR: #2339 • Head: 01aa1f6 • Reviewers: stack:code-reviewer (definition run inline — the colon-prefixed agent name is not registered as a launchable agent type), second independent pass; all findings re-verified against source by the orchestrator

Summary

Adds IntelliStory to @percy/cli-command: for a Storybook build it resolves a baseline (explicit or API-predicted), diffs the working tree against it, ships the enriched module graph to the new /intelli_story endpoints, and tags each snapshot with intelliStory/storybookPath so the API performs affected-story selection server-side — bailing to a full snapshot run via IntelliStoryBailError whenever it can't reason safely about a change. Also folds in the packaged-binary crash fix (createRequire bound as cjsRequire, guarded by a static noRequireBinding test) and a Node 20 CI leg for the Snyk-backed lockfile-diff path.

Note on this run. Head moved from 966c05d to 01aa1f6, but both new commits (01aa1f6, f6ef573) are Merge branch 'master' into PPLT-5844 — git diff 966c05d 01aa1f6 over the PR-owned paths is empty, so the reviewable code is byte-identical to the previously reviewed head. The fast-path did not fire because the intermediate merge f6ef573 carries no prior Claude Code Review status, so a full review ran. This second, independent pass surfaced finding 1, which the first pass missed. The code has not regressed — the assessment has sharpened, and the verdict changes accordingly.

Review Table

Findings

1. bailOnChanges/untraced globs are never rebased to the invocation directory — the safety valve is silently inert in a monorepo

• File: packages/cli-command/src/intelliStory.js:250-252, :259-262 (compare :415-426)

• Severity: High

• Reviewer: stack:code-reviewer (second pass); mechanism verified by the orchestrator

• Issue: affectedNodes comes from git diff --name-only, which always emits repo-root-relative paths regardless of cwd. assertNoBailOnChanges and enforceUntraced match user-supplied globs directly against those paths, and glob-to-regexp fully anchors the result (^…$). Meanwhile, twenty lines further down, the same function builds invocationDir = process.cwd() and rebases each story's importPath from the invocation directory to project-relative (:415-426) — so the file demonstrably does expect cwd ≠ projectRoot. User globs get no such treatment.

  Concrete failure: a monorepo user runs cd packages/ui && percy storybook ./storybook-static with bailOnChanges: ['webpack.config.js']. A change to that file lands in affectedNodes as packages/ui/webpack.config.js; the pattern compiles to /^webpack\.config\.js$/ and does not match. No bail fires, no warning is logged, and IntelliStory filters as though nothing risky changed — the under-snapshot direction, where a genuine visual regression ships unreviewed. Monorepo-with-subdirectory is the mainstream Storybook layout, and bailOnChanges exists precisely as the escape hatch for changes the graph can't reason about.

  Not covered by any test: every fixture in intelliStory.test.js constructs a repo with files at its root, so no cwd ≠ projectRoot case is exercised.

• Suggestion: Rebase user patterns the same way story paths already are, before matching:

  const rebase = g => {
    if (path.isAbsolute(g)) return path.relative(projectRoot, g).split(path.sep).join('/');
    const abs = path.resolve(invocationDir, g);
    const rel = path.relative(projectRoot, abs);
    return rel.startsWith('..') ? g : rel.split(path.sep).join('/');
  };

  and pass bailOnChanges.map(rebase) / untraced.map(rebase) into the two functions. Add a test with cwd set to a nested package directory. If root-relative globs are the intended contract instead, that must be documented and a bail should fire when cwd !== projectRoot and patterns are configured — silently doing nothing is the one outcome to avoid.

2. API/network failures escape as raw errors instead of IntelliStoryBailError

• File: packages/cli-command/src/intelliStory.js:181, :220, :354
• Severity: Medium
• Reviewer: stack:code-reviewer — independently reported by both review passes
• Issue: Every git, fs, config, and lockfile failure in this module is wrapped in IntelliStoryBailError so the caller can degrade to a full snapshot run. The three percy.client calls — getIntelliStorySnapshotNameToCommit (:220), getStatus('intelli_story_graph', …) inside pollGraphStatus (:181), and generateIntelliStoryGraph (:354) — are not. A transient 5xx, timeout, or connection reset therefore propagates as a plain Error. If @percy/storybook catches IntelliStoryBailError specifically (the type this module exports for exactly that purpose), an API blip hard-fails the whole storybook command instead of falling back to a full run — the opposite of the module's fail-safe posture. The graph-job timeout path is handled correctly (:359); only thrown exceptions leak. diffLockfileDeps (:316) has the same shape: it remaps only SNYK_LOCKFILE_PARSER_UNAVAILABLE and re-throws anything else raw.
• Suggestion: Wrap each client call and rethrow, e.g.

  let baseLookup;
  try {
    baseLookup = await percy.client.getIntelliStorySnapshotNameToCommit(percy.build?.id);
  } catch (e) {
    throw new IntelliStoryBailError(`IntelliStory: base build lookup failed: ${e.message}; running full snapshot set`);
  }

  Same for generateIntelliStoryGraph; in pollGraphStatus, treat a throw as a non-done status. Add tests where each client method rejects.

3. .storybook config directory is hardcoded — a custom configDir never trips the bail

• File: packages/cli-command/src/intelliStory.js:114, :244
• Severity: Medium
• Reviewer: stack:code-reviewer (first pass); verified
• Issue: Both the graph exclusion (EXCLUDED_DIRS = new Set(['node_modules', '.storybook'])) and the bail check (assertNoDotStorybookChange) match the literal path segment .storybook. Storybook's config directory is configurable (-c <dir> / configDir), and intelliStoryConfig has no field to communicate it. For a project using e.g. config/storybook/, a change to main.js or preview.js — which can affect every story — produces no bail. Same under-snapshot direction as finding 1, and the two compound for monorepo users.
• Suggestion: Thread the resolved config-dir name through intelliStoryConfig from @percy/storybook into both call sites, defaulting to .storybook:

  export function assertNoDotStorybookChange(affectedNodes, configDir = '.storybook') {
    const hit = affectedNodes.find(p => p.split(/[/\\]/).includes(configDir));
    …
  }

4. A malformed glob is silently treated as "matches nothing"

• File: packages/cli-command/src/intelliStory.js:34-43 (matchesPattern)
• Severity: Medium
• Reviewer: stack:code-reviewer (second pass); verified — glob-to-regexp('src/[') throws Unterminated character class, 'src/{a' throws Unterminated group
• Issue: try { … } catch { return false } means a typo'd pattern in a user's config compiles-fails and is then treated as never matching. For untraced that fails safe (the file stays traced). For bailOnChanges it fails open: the guardrail is silently disabled with nothing logged anywhere. The over-length guard at :22 has the same shape and is explicitly tested as non-throwing (intelliStory.test.js:269-271).
• Suggestion: log.warn when a pattern fails to compile — a broken guardrail must be visible:

  } catch (e) {
    log.warn(`IntelliStory: ignoring invalid pattern "${pattern}": ${e.message}`);
    return false;
  }

  Better still, validate the patterns once at config-parse time rather than per-path.

5. Stats modules dropped from the graph are logged only at debug, with no threshold

• File: packages/cli-command/src/intelliStory.js:132-133, :161-166
• Severity: Medium
• Reviewer: orchestrator (first pass)
• Issue: transformModule returns null for any module whose id does not resolve to an absolute path, and readStats counts those into droppedModules and emits a single log.debug. Missing modules mean missing edges in the dependency graph, and a missing edge is exactly what makes the API judge a story unaffected. If a Storybook/webpack version emits a large share of synthetic or virtual module ids, the graph is quietly degraded and the run proceeds — visible only under --verbose. Every other incomplete-data condition in this file bails.
• Suggestion: Escalate to log.warn and bail above a threshold:

  if (droppedModules) {
    const ratio = droppedModules / rawModules.length;
    if (ratio > 0.1) throw new IntelliStoryBailError(`IntelliStory: ${droppedModules}/${rawModules.length} modules had unresolved ids; graph would be incomplete; running full snapshot set`);
    log.warn(`IntelliStory: dropped ${droppedModules} module(s) with unresolved (non-absolute) ids`);
  }

6. Bare-extension globs don't match nested paths

• File: packages/cli-command/src/intelliStory.js:28 (patternToRegex)
• Severity: Medium
• Reviewer: stack:code-reviewer (first pass); verified
• Issue: With { globstar: true }, glob-to-regexp('*.css') compiles to ^([^/]*)\.css$, so bailOnChanges: ['*.css'] never matches src/Button.css. This is standard glob behavior — minimatch, picomatch, and GitHub Actions paths: filters agree — and the tests assert it deliberately (intelliStory.test.js:266, :286-289), so it is a considered choice rather than a coding error. It is listed because it lands on the same safety valve as findings 1 and 4, and there is currently no documentation for bailOnChanges/untraced anywhere in the repo.
• Suggestion: Document that patterns are path-segment-scoped and **/*.css is required. If you prefer forgiving semantics, auto-broaden bare-name patterns: const p = pattern.includes('/') ? pattern : '**/' + pattern; — note this must be decided together with finding 1's rebasing.

7. noRequireBinding guard is narrower than the crash class it protects

• File: packages/cli-command/test/noRequireBinding.test.js:40
• Severity: Low
• Reviewer: stack:code-reviewer (second pass)
• Issue: FORBIDDEN = /\b(?:const|let|var)\s+require\s*=\s*createRequire\b/ only catches a single-line declaration. The same startup crash returns via import { createRequire as require } from 'module', const { default: require } = …, or a line-wrapped const require =\n createRequire(…) — none of which match.
• Suggestion: Flag any binding of the bare identifier require (import specifier, declaration LHS, or parameter), not just the createRequire-RHS form.

8. mapEntry normalizes source only for type === 'src'

• File: packages/cli-command/src/intelliStory.js:137
• Severity: Low
• Reviewer: stack:code-reviewer (first pass)
• Issue: Fixtures only exercise 'src' and 'module' (the latter a no-op, since package specifiers are never absolute). If the enrichment tool ever emits a third type carrying an absolute path, that path ships unnormalized: it leaks a local or CI-runner path and cannot be correlated against project-relative affectedNodes.
• Suggestion: Drop the type gate — resolveAndIndex already returns non-absolute values untouched, so running it for every string source is safe.

9. gitDiffNames omits --no-renames while getAffectedFileLocations uses it

• File: packages/cli-command/src/intelliStory.js:75 vs :84
• Severity: Low
• Reviewer: stack:code-reviewer (first pass)
• Issue: Rename detection is git's default since 2.9, so affectedNodes collapses a rename to the new path alone while getAffectedFileLocations (run with --no-renames) sees a delete + add pair. The two views of one diff disagree for renamed files.
• Suggestion: Add --no-renames to gitDiffNames for parity.

10. path.relative does not case-fold when computing project-relative module ids

• File: packages/cli-command/src/intelliStory.js:117-128 (resolveAndIndex)
• Severity: Low (mechanism demonstrated, real-world trigger unconfirmed)
• Reviewer: stack:code-reviewer (second pass)
• Issue: path.relative('/Users/bob/proj', '/Users/Bob/Proj/src/App.js') returns '../../Bob/Proj/src/App.js' — exact segment comparison, no case folding, on every platform. On case-insensitive-but-preserving filesystems (APFS, NTFS), any casing divergence between the bundler's absolute module ids and git rev-parse --show-toplevel yields ..-prefixed index keys that can never match a real repo-relative path. The reviewer could not force an actual divergence, so this is flagged as plausible rather than confirmed.
• Suggestion: At minimum, warn when the computed relative path escapes projectRoot (starts with ..) rather than silently indexing it; optionally normalize both sides through fs.realpathSync.

11. trace.html loads Drawflow from a public CDN

• File: packages/cli-command/src/graphTraceTemplate.html:8, :12
• Severity: Low
• Reviewer: stack:code-reviewer — reported by both passes
• Issue: The debug trace viewer pulls Drawflow's CSS and JS from cdn.jsdelivr.net at view time. Opt-in (trace: true) and off the production path, but it is precisely the artifact someone opens after a bail or mis-selection — often inside a locked-down or air-gapped CI runner where it renders blank.
• Suggestion: Vendor the small Drawflow bundle and inline it into the template.

12. Minor: bail visibility, semgrep scope, stats-shape logging

• Severity: Low
• Reviewer: stack:code-reviewer (both passes)
• Issue: (a) No bail logs at the throw site, so user visibility depends entirely on the @percy/storybook catch — outside this repo and still a follow-up per the PR body. (b) .semgrepignore:49-57 suppresses intelliStory.js file-wide rather than at the flagged joins, so future unsafe joins in this fs/path-heavy file go unscanned; the rationale is documented and the constraint (inline // nosemgrep unsupported by the CI version) is real. (c) readStats's stats.modules || [] (:161) makes a stats file with no modules key indistinguishable from a legitimately empty one.
• Suggestion: (a) log.warn(message) inside a small bail(message, log) helper. (b) Track narrowing the suppression. (c) log.debug when the key is absent versus empty.

Informational — not counted as defects

• devDependencies excluded from the lockfile gate (lockfileDiff.js:53-63, tested at lockfileDiff.test.js:85): Storybook addons are typically devDependencies and are imported directly by preview.js/story files. A pure addon version bump with no source diff will not surface through this path. It is explicit and tested, so likely intentional (relying on the server-side module graph instead) — worth a sanity check with the team given how conservative this feature aims to be.
• stream-json leftover: confirmed absent from packages/cli-command/package.json and unreferenced — correctly removed in 9c525f10.
• Backward compatibility: the new snapshotSchema fields (intelliStory, storybookPath) are optional, and createSnapshot sends 'intelli-story': intelliStory || null / 'storybook-path': storybookPath || null, so non-IntelliStory callers are unchanged — cli-upload/test/upload.test.js pins exactly that.
• getStatus response shape: intelli_story_graph returns a top-level { status, data }, unlike the batched-by-id shape used for snapshot/comparison. pollGraphStatus and client.test.js:890-905 agree on it; an API-contract quirk, not a defect.

Verdict: FAIL

Claude Code PR Review

PR: #2339 • Head: 91bd770 • Reviewers: stack:code-reviewer (prior passes), fix verification by the orchestrator against source at this head

Summary

Adds IntelliStory to @percy/cli-command: for a Storybook build it resolves a baseline (explicit or API-predicted), diffs the working tree against it, ships the enriched module graph to the new /intelli_story endpoints, and tags each snapshot with intelliStory/storybookPath so the API performs affected-story selection server-side — bailing to a full snapshot run via IntelliStoryBailError whenever it can't reason safely about a change. Also folds in the packaged-binary crash fix (cjsRequire) and a Node 20 CI leg for the Snyk-backed lockfile-diff path.

This head resolves the High finding from the 01aa1f6 review along with three of the five Mediums. Verdict flips to PASS.

Fix verification (vs. 01aa1f6)

Finding 1 is fixed properly, not papered over. resolvePattern (intelliStory.js:88-100) rebases each user pattern from the invocation directory onto the repo root — the same basis normalizeImportPath already used for story paths, so the asymmetry that caused the bug is gone. Beyond that, the fix handles the two readings asymmetrically by direction of harm, which is the right call:

• assertNoBailOnChanges (:394-425) bails when a pattern matches only under the repo-root reading, logging a warning with a concrete rewrite suggestion — a stale root-relative config still protects the user.
• enforceUntraced (:433-462) declines to apply such a pattern and warns, since applying it would drop snapshots.

A <rootDir>/ prefix is an explicit opt-out, patterns resolving outside the repo bail rather than matching nothing, and resolveConfigDirs (:361-390) additionally checks that a configured directory actually exists — catching a wrong-basis configDir at config time instead of silently never matching. Findings 3, 4 and 5 are each fixed with matching care: isInsideDirs distinguishes single-segment (any depth) from multi-segment (prefix) config dirs so config/storybook can't match config/storybook-old; patternError validates globs up front and routes a compile failure to a bail for bailOnChanges and a warn-skip for untraced; and readStats now separates deliberately-excluded modules from unresolved ones, warns on the latter and bails past a 10% ratio of resolvable candidates — so a stats file heavy in node_modules no longer trips the threshold. intelliStory.test.js grows 364 lines with cwd ≠ projectRoot fixtures (projectRoot: '/repo', invocationDir: '/repo/packages/ui') and a resolveConfigDirs() suite — the gap that let finding 1 through is now covered.

Review Table

Findings

2. API/network failures still escape as raw errors instead of IntelliStoryBailError

• File: packages/cli-command/src/intelliStory.js:292, :331, :555
• Severity: Medium
• Reviewer: stack:code-reviewer (reported independently by both prior passes); re-verified unfixed at this head
• Issue: Verified still open: getStatus('intelli_story_graph', …) in pollGraphStatus (:292), getIntelliStorySnapshotNameToCommit (:331), and generateIntelliStoryGraph (:555) are all bare awaits with no surrounding try/catch. Every other failure mode in this file — git, fs, config, lockfile, malformed glob, degraded graph — is deliberately converted to IntelliStoryBailError so the caller can degrade to a full snapshot run. A transient 5xx, timeout, or connection reset from these three still propagates as a plain Error, so if @percy/storybook catches IntelliStoryBailError specifically, an API blip hard-fails the storybook command instead of falling back. The graph-job timeout path is handled correctly (:561); only thrown exceptions leak. Not a merge blocker under the verdict rule, but it is now the last inconsistency in an otherwise uniformly fail-safe module.
• Suggestion:

  let baseLookup;
  try {
    baseLookup = await percy.client.getIntelliStorySnapshotNameToCommit(percy.build?.id);
  } catch (e) {
    throw new IntelliStoryBailError(`IntelliStory: base build lookup failed: ${e.message}; running full snapshot set`);
  }

  Same for generateIntelliStoryGraph; in pollGraphStatus, treat a throw as a non-done status so the existing :561 bail covers it. Add tests where each client method rejects.

6. Bare-extension globs still don't match nested paths, and the contract is undocumented

• File: packages/cli-command/src/intelliStory.js:28
• Severity: Medium
• Reviewer: stack:code-reviewer (first pass)
• Issue: glob-to-regexp('*.css', { globstar: true }) compiles to ^([^/]*)\.css$, so bailOnChanges: ['*.css'] never matches src/Button.css. This is standard glob behavior (minimatch, picomatch, GitHub Actions paths: all agree) and is deliberately asserted by the tests, so it is not a defect. It is still listed because the new rootReadingHit warning covers the basis mistake but not the depth mistake — a user who writes *.css gets no warning at all — and there remains no documentation for bailOnChanges/untraced/configDir anywhere in the repo.
• Suggestion: Document the semantics now that the basis rules are non-trivial (invocation-relative, <rootDir>/ opt-out, path-segment-scoped globs). Optionally extend the existing warning: if a pattern has no / and no **, and some affected node matches **/<pattern>, warn the same way the root-reading case does.

7–11. Carried-over Lows, unchanged at this head

• noRequireBinding.test.js:40 — FORBIDDEN still only matches a single-line const/let/var require = createRequire. The same crash returns via import { createRequire as require } from 'module' or a line-wrapped declaration. Broaden to flag any binding of the bare require identifier.
• intelliStory.js:229 — mapEntry still gates normalization on copy.type === 'src'. Drop the gate; resolveAndIndex already passes non-absolute values through untouched.
• intelliStory.js:138 — gitDiffNames still omits --no-renames while getAffectedFileLocations uses it, so the two views of one diff disagree for renamed files.
• intelliStory.js resolveAndIndex — path.relative does not case-fold, so a casing divergence between bundler module ids and git rev-parse --show-toplevel yields ..-prefixed index keys that never match. Mechanism is real, real-world trigger still unconfirmed. Cheapest guard: warn when the computed relative path starts with ...
• graphTraceTemplate.html:8, :12 — trace.html still loads Drawflow from cdn.jsdelivr.net, so the diagnostic renders blank in an air-gapped or locked-down CI runner. Vendor the bundle.

12. Bail visibility — partially addressed

• Severity: Low
• Issue: This push adds ten log.warn calls, so pattern and configDir problems are now visible. The IntelliStoryBailError throw sites themselves still don't log before throwing, so whether a user learns that filtering did not apply continues to depend on the @percy/storybook catch site — outside this repo and still a follow-up per the PR body.
• Suggestion: A small bail(message, log) helper that warns and throws.

Informational

• devDependencies excluded from the lockfile gate (lockfileDiff.js:53-63, tested): Storybook addons are usually devDependencies imported directly by preview.js/story files, so a pure addon bump with no source diff won't surface here. Explicit and tested, so presumably intentional — worth a sanity check with the team.
• .semgrepignore still suppresses intelliStory.js file-wide rather than at the flagged joins. The file grew ~290 lines this push, which widens the unscanned surface; the documented rationale (inline // nosemgrep unsupported by the CI semgrep version) still holds. Worth narrowing when tooling allows.
• Version strings moved 1.32.6-beta.3 → 1.32.6 via the master release merge; no functional change.

Verdict: PASS