[RaghavsBrowserStack]

This PR makes 2 very small cosmetic changes and 1 cleanup

1. If a customer doesn't have access to the smartsnap feature, a very pretty error will be used to inform them of the same
2. In each log the buildId we generate randomly in enriched-stats.json will be logged, this way if a person accidentally uses a stale enriched-stats.json we'll able to determine that through the logs
3. The base build endpoint now doesn't need git info so the cli will not send it anymore

[RaghavsBrowserStack]

Claude Code PR Review

PR: #2385 • Head: adbe1c7 • Reviewers: stack:code-reviewer

Summary

Wraps IntelliStory's three API call sites (base-build lookup, graph generation, graph-status poll) so 403 entitlement errors and transient 5xx/network errors degrade to IntelliStoryBailError (full snapshot run) instead of hard-failing percy storybook; also logs the stats-file buildId for support correlation and drops the now-unused git/target/PR/partial query params from getIntelliStorySnapshotNameToCommit.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets introduced; diff touches error handling and query-string construction only.
High	Security	Authentication/authorization checks present	Pass	403 is now surfaced as a clean entitlement bail rather than a crash; no auth logic weakened.
High	Security	Input validation and sanitization	Pass	build_id still goes through URLSearchParams, so it stays encoded. Existing unsafe baseline ref validation untouched.
High	Security	No IDOR — resource ownership validated	Pass	Ownership is enforced server-side; build_id comes from percy.build.id created by this client, not user input.
High	Security	No SQL injection (parameterized queries)	N/A	No database access in this package.
High	Correctness	Logic is correct, handles edge cases	Pass	403 short-circuits the poll, transient errors retry within the existing 12×5s budget; no attempt-budget or wall-clock regression. One narrower gap noted below at Medium.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	All three call sites wrap and rethrow as IntelliStoryBailError; transient poll errors are logged at debug before retry, not silently dropped.
High	Correctness	No race conditions or concurrency issues	Pass	Poll loop is strictly sequential; no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Pass	7 new tests cover 403 and transient paths for base lookup, generate, and poll. Gap: no test for a getStatus failure inside writeIntelliStoryTrace.
Medium	Testing	Error paths and edge cases tested	Pass	Covers 403, 5xx-with-response, and bare network error (no response object).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Reviewer ran both suites: cli-command 198/198; client suite clean apart from a pre-existing proxy.test.js Node-version failure that also fails on master. All 30 CI checks green.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Poll bound unchanged at POLL_ATTEMPTS=12 × POLL_INTERVAL_MS=5000 (~60s worst case).
Medium	Performance	Long-running tasks use background jobs	N/A	Graph generation is already an async server-side job polled by this code.
Medium	Quality	Follows existing codebase patterns	Pass	apiBailError matches the module's established "convert every failure to IntelliStoryBailError" convention.
Medium	Quality	Changes are focused (single concern)	Pass	Error wrapping, one debug log, and the query-param cleanup — all in service of the same IntelliStory hardening ticket.
Low	Quality	Meaningful names, no dead code	Pass	isNotAllowed / apiBailError are clear. Removed query params leave no dead code behind.
Low	Quality	Comments explain why, not what	Pass	Comments on apiBailError, the 403 short-circuit, and the param removal all explain rationale.
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes.

Findings

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

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: pollGraphStatus is shared by runGraphGeneration (pre-finalize, where a bail is meaningful) and writeIntelliStoryTrace (post-finalize, best-effort trace rendering). The new 403 short-circuit throws immediately, and writeIntelliStoryTrace has no try/catch around its pollGraphStatus call — so a 403 there propagates uncaught after the build has already completed, carrying the message "...running full snapshot set" when there is no snapshot set left to run. This is inconsistent with maybeWriteTrace a few lines below, which logs a warn and swallows failures. Not a new regression (pre-PR, any getStatus error propagated here too), but the PR's stated goal of never hard-failing the storybook command is only fully delivered for transient errors, not for 403 in this path. No test covers a getStatus failure inside writeIntelliStoryTrace.

• Suggestion: Wrap the pollGraphStatus call in writeIntelliStoryTrace in try/catch (log at debug/warn and return), or add a throwOn403 flag so the finalize path never throws. Add a test stubbing getStatus to reject with a 403 in that call path.

• File: commit 850804ed (message only)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Commit message and PR title read "Wrap smartsnap API failures in SmartsnapBailError", but the class is IntelliStoryBailError — no symbol named Smartsnap* exists anywhere in the repo. Commit adbe1c72 is also titled "one extra lgo" (typo, non-descriptive).

• Suggestion: Squash and reword to reference IntelliStoryBailError before merge so master history reads correctly on a future bisect.

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

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The new line logs IntelliStory: stats file buildId = ... (the Storybook stats-file id) while other debug lines in the same module use buildId for the Percy build id. A support engineer grepping logs for "buildId" gets two different identifiers under one word. No sensitive data, low volume.

• Suggestion: Optional — phrase the two consistently (e.g. "stats file buildId" vs "Percy build id"). The new line is already qualified, so leaving as-is is defensible.

• File: packages/client/src/client.js:462-470

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Pre-merge coordination note, not a code defect. Removing branch/target_branch/commit_sha/target_commit_sha/pull_request_number/partial unconditionally is a wire-format change — if percy-api's /intelli_story/snapshot-name-to-commit has not already shipped the "resolve everything from build_id" behaviour, this is a hard break rather than a graceful degradation. The param-less GET path exercised in tests is not reachable in production, since applyIntelliStory asserts percy.build?.id before this runs.

• Suggestion: Confirm the backend change is deployed ahead of this CLI release.

---

Verdict: PASS