fix: merge .percy.yml config options with snapshot options for serializeDOM

[rishigupta1599]

Summary

• Config options from .percy.yml were not being passed to PercyDOM.serialize()
• Only per-snapshot options were used for DOM serialization
• Now merges @cli_config['snapshot'] with user options, with per-snapshot options taking priority

Test plan

• Existing tests pass
• Verify .percy.yml config options are applied during DOM serialization

🤖 Generated with Claude Code

[rishigupta1599]

Claude Code Review (automated) — 1 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[rishigupta1599]

Claude Code PR Review

PR: #31 • Head: 493b715 • Reviewers: stack:code-review

Summary

Merges the global .percy.yml snapshot config section with per-call options (intended per-call priority) into merged_options, and passes that merged hash to the DOM-serialization path (responsive_snapshot_capture?, capture_responsive_dom, get_serialized_dom) in Percy.snapshot. Single-file change in lib/percy.rb.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped for this review.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped for this review.
High	Security	Input validation and sanitization	N/A	Security lens skipped for this review.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped for this review.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped for this review.
High	Correctness	Logic is correct, handles edge cases	Fail	lib/percy.rb:84-85 — @cli_config is JSON-parsed (string keys); per-call options use symbol keys. config_options.merge(options) treats 'widths' and :widths as distinct, so the per-call-priority guarantee does not hold and YAML keys are invisible to symbol-key consumers. nil/non-Hash @cli_config['snapshot'] is safely handled via `&.dig ...
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	New code adds no new rescue; wrapping begin/rescue in snapshot unchanged.
High	Correctness	No race conditions or concurrency issues	Pass	No new shared-state mutation; @cli_config read-only here.
Medium	Testing	New code has corresponding tests	Fail	No spec exercises the merge path (config inherited, per-call override, mixed, nil config).
Medium	Testing	Error paths and edge cases tested	Fail	String-vs-symbol priority and degenerate @cli_config states untested.
Medium	Testing	Existing tests still pass (no regressions)	Pass	No existing behavior altered for the POST body; serialization-only change.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	One in-memory hash merge per snapshot; negligible.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to this SDK path.
Medium	Quality	Follows existing codebase patterns	Fail	responsive_snapshot_capture? (:414) and capture_responsive_dom minHeight (:358) still read @cli_config directly; after a correct merge those fallbacks become dead/duplicated, and today they mask the key-collision bug.
Medium	Quality	Changes are focused (single concern)	Pass	Single concern, single file.
Low	Quality	Meaningful names, no dead code	Pass	config_options / merged_options are clear.
Low	Quality	Comments explain why, not what	Pass	Comment states intent (per-call priority); note POST asymmetry below.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: lib/percy.rb:84-85

• Severity: High

• Reviewer: stack:code-review

• Issue: config_options = @cli_config&.dig('snapshot') || {} is a JSON-parsed hash with string keys, while per-call options are conventionally passed with symbol keys. Hash#merge treats 'widths' and :widths as different keys, so config_options.merge(options) does not collapse them: merged_options ends up with both 'widths' => <yml> and :widths => <caller>. Downstream consumers read symbol keys only (e.g. capture_responsive_dom line 346 options[:widths], responsive_snapshot_capture? lines 412-413), so YAML config keys are never seen via the merge, and the documented "per-call priority" is true only by accident of which spelling the consumer reads. This defeats the core purpose of the PR.

• Suggestion: Normalise keys before merging, e.g. config_options = (@cli_config&.dig('snapshot') || {}).transform_keys(&:to_sym) then merged_options = config_options.merge(options). Symbols are preferable since every downstream access uses symbol keys. (If a caller may pass string keys, normalise both sides as resolve_readiness_config already does at lines 132-135.)

• File: lib/percy.rb:358 and lib/percy.rb:414

• Severity: Medium

• Reviewer: stack:code-review

• Issue: minHeight (options[:minHeight] || @cli_config&.dig('snapshot', 'minHeight')) and responsiveSnapshotCapture (options[:...] || @cli_config&.dig('snapshot', 'responsiveSnapshotCapture')) still read @cli_config directly even though these methods now receive merged_options. Today these direct reads are what actually make those two YAML keys work, masking the key-collision bug above for exactly those keys; once normalisation is fixed they become duplicated/dead fallbacks and are inconsistent with the new merge-once approach.

• Suggestion: After fixing key normalisation, drop the @cli_config fallback clauses on lines 358 and 414 so config flows through merged_options in one place; keep the deferUploads guard on line 410.

• File: lib/percy.rb:95

• Severity: Medium

• Reviewer: stack:code-review

• Issue: post_options is built from the raw options, not merged_options, so .percy.yml snapshot keys are merged into the DOM-serialization call but are not forwarded in the percy/snapshot POST body. Per the ticket scope ("merge ... before serializeDOM") this is plausibly intentional (the CLI already holds the config), but the asymmetry is silent.

• Suggestion: Add a one-line comment stating the merge is serialization-only and the CLI sources config independently; or switch to merged_options.reject { ... } if the merged values are also meant to reach the server.

• File: lib/percy.rb (whole change)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No tests cover the new merge behavior: config inherited when per-call key absent, per-call key overriding the YAML key (the priority guarantee — which currently fails for symbol-vs-string), mixed keys, and degenerate @cli_config states (nil, missing snapshot, non-Hash). Existing minHeight/responsiveSnapshotCapture specs call the helpers directly and bypass snapshot's merge.

• Suggestion: Add specs around Percy.snapshot (or extract the merge into a tested helper) asserting symbol-keyed per-call options override string-keyed YAML config.

---

Verdict: FAIL — the string-vs-symbol key collision breaks the per-call-priority merge that is the entire point of the PR; fix key normalisation and add merge-priority tests before merging.

[rishigupta1599]

Claude Code Review (automated) — 1 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] Merge/key-override fix has no test coverage

The core behavior of this fix — symbolizing config keys so per-call options override matching config keys — is not asserted by any spec. Existing serialize specs pass {} and never combine @cli_config['snapshot'] with overlapping per-call options.

Suggestion: Add a unit test around Percy.snapshot that sets @cli_config = {'snapshot' => {'percyCSS' => 'a', 'minHeight' => 700}}, calls with percyCSS: 'b', and asserts the serialized options contain :percyCSS => 'b' (per-call override) and :minHeight => 700 (config inherited), with no duplicate string keys.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #31 • Head: 3a32b11 • Reviewers: stack:code-review

Summary

Merges .percy.yml config (@cli_config['snapshot']) into per-snapshot options before DOM serialization, symbolizing config string keys so per-call symbol options override matching config keys (single logical key per option) instead of producing duplicate string/symbol keys. This is the re-review after the key-normalization fix (commit 3a32b11) that addressed the prior FAIL where per-snapshot options did not override config.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	No security lens this run (skipped per scope).
High	Security	Authentication/authorization checks present	N/A	No security lens this run.
High	Security	Input validation and sanitization	N/A	No security lens this run.
High	Security	No IDOR — resource ownership validated	N/A	No security lens this run.
High	Security	No SQL injection (parameterized queries)	N/A	No security lens this run.
High	Correctness	Logic is correct, handles edge cases	Pass	config_options.transform_keys(&:to_sym).merge(options) (lib/percy.rb:89) correctly symbolizes string config keys and lets per-call symbol options win on collision. @cli_config&.dig('snapshot') || {} guards nil config. Downstream responsive_snapshot_capture?/capture_responsive_dom/get_serialized_dom all read symbol keys, so symbolized config now flows through correctly.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Whole block stays inside the existing begin/rescue (lib/percy.rb:79,114); no new swallowing introduced.
High	Correctness	No race conditions or concurrency issues	Pass	No new shared state; @cli_config read-only here.
Medium	Testing	New code has corresponding tests	Fail	No spec asserts the new merge: that symbolized config options reach PercyDOM.serialize, or that per-call options override config keys. Existing snapshot/serialize specs (spec/lib/percy/percy_spec.rb:592+) pass {} and never set @cli_config['snapshot'] alongside per-call options. Central behavior of the fix is unverified.
Medium	Testing	Error paths and edge cases tested	Fail	No test for the string-vs-symbol collision case the fix targets (e.g. config 'percyCSS' overridden by per-call :percyCSS), nor for nil/empty @cli_config.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive within snapshot; post_options still derives from original options (lib/percy.rb:99), so POST payload behavior is unchanged. Existing serialize specs unaffected.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single transform_keys + merge on a small options hash; negligible.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK snapshot call.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the existing string-normalization pattern in resolve_readiness_config (lib/percy.rb:131-140); comments explain the key-type rationale.
Medium	Quality	Changes are focused (single concern)	Pass	One concern: config/option merge with correct key precedence; single file.
Low	Quality	Meaningful names, no dead code	Pass	config_options/merged_options are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comments (lib/percy.rb:85-88) explain the symbol/string rationale — the non-obvious "why".
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: lib/percy.rb:89
• Severity: Medium
• Reviewer: stack:code-review
• Issue: The core fix — symbolizing config keys so per-call options override config — has no test coverage. No spec sets @cli_config['snapshot'] with options that also appear in the per-call hash and asserts the per-call value reaches PercyDOM.serialize, nor that a config-only option is forwarded.
• Suggestion: Add a unit test around Percy.snapshot (stubbing percy_enabled?, fetch_percy_dom, driver.execute_script, and fetch) that sets @cli_config = {'snapshot' => {'percyCSS' => 'a', 'minHeight' => 700}}, calls with percyCSS: 'b', and asserts the serialized/forwarded options contain :percyCSS => 'b' (override) and :minHeight => 700 (config inherited) with no duplicate string keys.

---

Verdict: PASS — the key-normalization fix is correct (per-call overrides config, single symbol key per option, downstream symbol-key consumers work); the only gap is missing tests for the new merge behavior (Medium, non-blocking).

[rishigupta1599]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[rishigupta1599]

[Medium] Test gap: config-only path and POST body unverified

This test covers "config sets two keys, per-call overrides one" but not the motivating case where options come solely from .percy.yml (per-call options = {}). It also does not assert the POST /percy/snapshot body. The POST is built from raw options (post_options = options.reject {...}), not merged_options, so config-only keys reach PercyDOM.serialize but are not re-sent on POST. That asymmetry matches the existing healthcheck pattern but is currently untested.

Suggestion: Add a test with empty per-call options and a populated config snapshot block, asserting config keys reach serialize; optionally add a have_requested(:post, ...) assertion documenting the POST-body contract for config-derived keys.

Reviewer: stack:code-review

[rishigupta1599]

[Low] Shallow symbolize + now-redundant downstream config fallbacks

transform_keys(&:to_sym) is shallow, so nested config hashes (e.g. a future readiness object) keep string keys. Current consumers re-normalize, so this is safe today — a forward-compat note, not a bug. Separately, responsive_snapshot_capture? and the minHeight read in capture_responsive_dom still keep their own @cli_config&.dig(...) fallbacks, now redundant given this upstream merge (harmless but a maintainability hazard).

Suggestion: Note in the comment that only top-level keys are symbolized; optionally drop the redundant downstream @cli_config fallbacks in a follow-up with a test confirming the merged hash is the single source of truth.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #31 • Head: 93e11f7 • Reviewers: stack:code-review

Summary

Merges .percy.yml snapshot config options into per-snapshot options before DOM serialization (per-call options win), symbolizing string config keys so they override correctly; the head commit adds a test asserting merge precedence (config-only enableJavaScript survives, per-call percyCSS overrides config).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped for this review.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped for this review.
High	Security	Input validation and sanitization	N/A	Security lens skipped for this review.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped for this review.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped for this review.
High	Correctness	Logic is correct, handles edge cases	Pass	Symbolize-then-merge gives correct per-call precedence; &.dig('snapshot') || {} handles nil/missing config.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No new error paths; existing rescue around snapshot body unchanged.
High	Correctness	No race conditions or concurrency issues	Pass	Pure local hash merge; no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Pass	New spec exercises the merge and asserts both config-only survival and per-call override.
Medium	Testing	Error paths and edge cases tested	Fail	No test for config-only options with empty per-call hash, nor an assertion on the POST body. See Findings.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive; downstream consumers already read symbol keys / config fallbacks.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No queries; single shallow hash merge per snapshot.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to this SDK path.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors existing @cli_config&.dig(...) + reject readiness conventions in this file.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config<->option merge before serialization plus its test.
Low	Quality	Meaningful names, no dead code	Pass	config_options/merged_options are clear.
Low	Quality	Comments explain why, not what	Pass	Comment block explains the string-vs-symbol rationale (slightly verbose but accurate).
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: spec/lib/percy/percy_spec.rb:301

• Severity: Medium

• Reviewer: stack:code-review

• Issue: The new test only covers the "config sets two keys, per-call overrides one" path. It does not cover the motivating use case where options come solely from .percy.yml (per-call options = {}), and it does not assert the POST /percy/snapshot body. The POST is built from raw options (post_options = options.reject {...}), not merged_options, so config-only keys reach PercyDOM.serialize but are not re-sent on POST. That asymmetry is consistent with the existing pattern (the CLI already has config via healthcheck), but it is currently unverified.

• Suggestion: Add a test with empty per-call options and a populated config snapshot block, asserting config keys reach serialize. Optionally add a have_requested(:post, ...) assertion documenting the intended POST-body contract for config-derived keys.

• File: lib/percy.rb:89

• Severity: Low

• Reviewer: stack:code-review

• Issue: transform_keys(&:to_sym) is shallow, so nested config hashes (e.g. a future readiness object) keep string keys. Current consumers (resolve_readiness_config) re-normalize, so this is safe today; it is a forward-compatibility note, not a bug. Separately, downstream helpers (responsive_snapshot_capture?, capture_responsive_dom minHeight) retain their own @cli_config&.dig(...) fallbacks, which are now redundant given the upstream merge — harmless but a maintainability hazard.

• Suggestion: Note in the comment that only top-level keys are symbolized. Optionally drop the now-redundant @cli_config fallbacks in the downstream helpers in a follow-up, with a test confirming the merged hash is the single source of truth.

---

Verdict: PASS — Core merge logic is correct and tested; remaining items are a Medium test-coverage gap and Low maintainability/forward-compat notes, none blocking.

[rishigupta1599]

Claude Code PR Review

PR: #31 • Head: 49a19d1 • Reviewers: stack:code-review

Summary

Replaces the previous shallow top-level merge of .percy.yml config (@cli_config.snapshot) with per-snapshot options by a recursive deep merge: both sides are deep_symbolized (every nested Hash key → symbol, Arrays walked, scalars passed through) and then combined with deep_merge_options, where nested Hashes merge recursively while Arrays/scalars from the per-call side replace. This keeps config-only sibling keys (and nested-hash siblings such as discovery.networkIdleTimeout) reaching the DOM serializer instead of being dropped, while per-call leaves still win. Adds two specs (top-level override + nested deep-merge) and makes the spec/spec_helper Capybara driver selectable between Firefox (CI default) and a Chrome binary via CHROME_BIN (e2e container).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Per scope: security lens skipped; no secrets in diff regardless.
High	Security	Authentication/authorization checks present	N/A	Per scope: security skipped; client SDK merge logic, no auth surface.
High	Security	Input validation and sanitization	N/A	Per scope: security skipped.
High	Security	No IDOR — resource ownership validated	N/A	Per scope: security skipped; no resource access.
High	Security	No SQL injection (parameterized queries)	N/A	Per scope: security skipped; no SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Deep merge verified: nested siblings survive, leaves/arrays/scalars replaced by per-call. Hash#merge is non-mutating, so @cli_config and options are not mutated. deep_symbolize walks Hashes+Arrays, scalars pass through (nil-safe via &.dig/`
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	New helpers add no new error paths; existing rescue StandardError in snapshot unchanged.
High	Correctness	No race conditions or concurrency issues	Pass	Pure functional transforms on local hashes; no shared mutable state introduced.
Medium	Testing	New code has corresponding tests	Pass	Two new specs cover top-level override and nested deep-merge (sibling kept, leaf overridden); assert the merged hash reaching PercyDOM.serialize.
Medium	Testing	Error paths and edge cases tested	Partial	Array-replace and empty/absent-config paths not directly asserted; verified manually here. Test-gap is non-blocking per adjudication.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Downstream consumers read symbol keys (options[:widths], :minHeight, :responsiveSnapshotCapture); deep_symbolize makes config keys reachable — strengthens, not breaks, existing behavior.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	No I/O added; merge is O(n) over a small options hash.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to client SDK option merge.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors existing resolve_readiness_config key-normalisation approach; self. module methods consistent with file.
Medium	Quality	Changes are focused (single concern)	Pass	Merge change is focused; spec_helper/spec Chrome-driver toggle is an adjacent test-infra change but contained and gated on CHROME_BIN.
Low	Quality	Meaningful names, no dead code	Pass	deep_symbolize / deep_merge_options clearly named; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comments explain the string-vs-symbol key rationale and array/scalar replace semantics.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies; pure Ruby stdlib.

Findings

No blocking findings.

Non-blocking observations (do not affect verdict):

• Test gap (Medium, non-blocking per adjudication): array-replace semantics and the no-config (@cli_config nil) path aren't directly asserted by the new specs. Verified correct manually (arrays replace, base unmutated). Optional follow-up.
• Pre-existing, not introduced by this diff (non-blocking): the POST body (post_options, line 119) is still built from the raw per-call options, not merged_options. Per adjudication this is a VERIFIED FALSE POSITIVE — the CLI applies .percy.yml config server-side, so config/merged options are intentionally not duplicated in the SDK POST body. The deep merge here only governs what reaches PercyDOM.serialize client-side, which is the correct scope.

Verification performed:

• Deep-merge correctness: nested sibling discovery.networkIdleTimeout preserved, disableCache leaf overridden, top-level percyCSS overridden, arrays replaced — matches both new specs.
• Key symbolization keeps downstream consumers working: responsive_snapshot_capture?, capture_responsive_dom (options[:widths], :minHeight), get_serialized_dom, and resolve_readiness_config (handles both :readiness/'readiness') all read symbol keys correctly.
• to_json on the symbol-keyed merged hash emits correct string JSON keys for PercyDOM.serialize.

---

Verdict: PASS — deep merge and symbolization are correct, non-mutating, and strengthen config reachability; only a non-blocking test gap and a pre-existing (adjudicated false-positive) POST-body note remain.