PPLT-5495: Add file diff data to generate graph

[RaghavsBrowserStack]

This PR complies with the new implementation of the smartsnap API contract with backend in which imports and passThroughExports can have a loc array which is an array of tuple of 2, this data is newly added to enriched-stats.json and the generate graph additionally accepts a affected_file_locations which maps files to the index of lines changed.

[RaghavsBrowserStack]

Claude Code PR Review

PR: #2273 • Head: 59b0a66 • Reviewers: stack:code-reviewer

Summary

Adds line-level diff ranges to the SmartSnap graph pipeline: a new getAffectedFileLocations parses git diff --unified=0 hunk headers into per-file [start,end] line ranges keyed by files index, transforms {start,end} loc spans on module imports/exports into [start,end] tuples, threads affectedFileLocations through runGraphGeneration and the client (affected_file_locations), and fixes the graph status guard from 'failure' to 'failed'.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	assertSafeRef(baseRef) fires before any git invocation; git args passed as argv array (no shell injection).
High	Security	No IDOR — resource ownership validated	N/A	No resource access by id.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	HUNK regex handles +c, +c,d, +c,0 forms; /dev/null deletion guard and count===0 skip are correct and tested.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Bails via SmartSnapBailError; no silent catches added.
High	Correctness	No race conditions or concurrency issues	N/A	Synchronous git parsing; no shared mutable state.
Medium	Testing	New code has corresponding tests	Pass	getAffectedFileLocations has 4 dedicated cases; client + runGraphGeneration payload covered.
Medium	Testing	Error paths and edge cases tested	Pass	Unsafe-ref bail, pure-deletion, no-diff, unindexed/deleted files all covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	'failure'→'failed' mock updated in lockstep with source.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Second full git diff against same base (minor duplication with gitDiffNames); not a blocker.
Medium	Performance	Long-running tasks use background jobs	N/A	Unchanged.
Medium	Quality	Follows existing codebase patterns	Pass	camelCase→snake_case client convention matches storybook_paths.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to file-location threading + status rename.
Low	Quality	Meaningful names, no dead code	Pass	Well-commented; nonPassThroughExports skips the loc transform (latent, see findings).
Low	Quality	Comments explain why, not what	Pass	Strong rationale comments throughout.
Low	Quality	No unnecessary dependencies added	Pass	No new deps.

Findings

• File: packages/cli-command/src/smartsnap.js:212

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: imports and passThroughExports run through mapEntry (which applies the loc → tuple transform), but nonPassThroughExports is copied raw. If the bundler ever emits loc on a non-pass-through export, those spans reach the API in {start,end} object form, silently breaking the tuple contract. Currently latent — the contract (and test at smartsnap.test.js:175) shows these entries are bare { type:'module', source } with no loc.

• Suggestion: Either run nonPassThroughExports through mapEntry for uniformity, or add a test asserting loc is intentionally absent there so the divergence is deliberate.

• File: packages/cli-command/src/smartsnap.js:205

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: copy.loc.map(l => [l.start, l.end]) produces [undefined, undefined] for a span missing fields and throws on a null element. Defensive only — current bundler output is well-typed.

• Suggestion: Optional guard: l && l.start != null && l.end != null ? [l.start, l.end] : l, or a comment noting the invariant.

• File: packages/cli-command/src/smartsnap.js:276

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Guard changed from 'failure' to 'failed'. If any backend can still return 'failure' during a rolling deploy, those responses fall through to the timeout/bail path. Assumed to be a coordinated API rename.

• Suggestion: Confirm the API contract value is 'failed'; add a brief comment so the string isn't reverted later.

• File: packages/cli-command/src/smartsnap.js:642-646

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: getAffectedFileLocations shells out to a second full git diff against the same base ref already diffed by gitDiffNames. No correctness issue; minor duplicate work on large repos.

• Suggestion: Optional: parse one --unified=0 diff for both names and ranges if this becomes a hotspot.

• File: packages/cli-command/test/smartsnap.test.js:740

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The applySmartSnap integration test asserts only generate was called, not that affectedFileLocations was forwarded — the sole end-to-end path through the new chain.

• Suggestion: Tighten to toHaveBeenCalledWith(..., jasmine.objectContaining({ affectedFileLocations: jasmine.any(Object) })).

• File: packages/cli-command/src/smartsnap.js:127

• Severity: Low (nitpick)

• Reviewer: stack:code-reviewer

• Issue: toPosix swaps path.sep for / but doesn't collapse ./repeated separators; a Windows src\.\A.js could miss the index lookup. Windows-only edge case.

• Suggestion: Document as a known limitation or normalize via path.posix.normalize.

---

Verdict: PASS

[bhokaremoin]

Remove the claude's unnecessary code comments

[bhokaremoin]

remove not required