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 both, with per-snapshot options taking priority

Changes

• Merge data['config']['snapshot'] with kwargs before calling get_serialized_dom() and capture_responsive_dom()

Test plan

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

🤖 Generated with Claude Code

[github-actions]

This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This PR was closed because it has been stalled for 28 days with no activity.

[rishigupta1599]

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

[rishigupta1599]

[Medium] No test for the config/kwargs merge

The new merge (config snapshot options + per-call kwargs, kwargs winning) has no test coverage — not the happy path, the precedence rule, nor config-only forwarding. The PR's own test-plan checkboxes are unchecked.

Suggestion: Add a percy_snapshot test mocking _is_percy_enabled to return {"config": {"snapshot": {"enableJavaScript": True, "percyCSS": ".x{}"}}, ...}, pass an overriding kwarg, and assert the args in page.evaluate("PercyDOM.serialize(...)") reflect the merge with the kwarg winning.

Reviewer: stack:code-review

[rishigupta1599]

[Low] data["config"] assumed non-null

data["config"].get("snapshot", {}) will raise AttributeError if the healthcheck body has an explicit "config": null (then data["config"] is None). Not a regression — pre-existing code assumes config is dict-like — but this line widens the surface and is inconsistent with the data.get("config") form used at line 509.

Suggestion: Use (data.get("config") or {}).get("snapshot", {}) for null safety and consistency.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #141 • Head: 7cf477e • Reviewers: stack:code-review

Summary

Merges the global .percy.yml config under config['snapshot'] with per-call kwargs (per-call options take priority) into merged_kwargs, then forwards that merged set to is_responsive_snapshot_capture, capture_responsive_dom, and get_serialized_dom so .percy.yml snapshot options reach PercyDOM.serialize(). Single-file change in percy/screenshot.py.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens disabled per skill; no secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Security lens disabled; not applicable to this change.
High	Security	Input validation and sanitization	N/A	Security lens disabled.
High	Security	No IDOR — resource ownership validated	N/A	Security lens disabled.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens disabled; no SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	{**config_options, **kwargs} gives per-call kwargs priority as intended. Inline merge, no JS-util dependency. Edge case: data["config"] could be None if healthcheck returns "config": null (see Findings, Low).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Whole body already wrapped in try/except; no new error paths introduced.
High	Correctness	No race conditions or concurrency issues	Pass	Pure dict merge before serialize; no shared mutable state or concurrency added.
Medium	Testing	New code has corresponding tests	Fail	No test added covering the config→kwargs merge or precedence; test-plan checkboxes left unchecked. See Findings.
Medium	Testing	Error paths and edge cases tested	Fail	No test for config['snapshot'] populated, precedence, or null-config path.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Existing tests mock config: {}, so config_options is empty and merged_kwargs == kwargs; behavior unchanged for them.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	One dict merge; no added I/O.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK snapshot call.
Medium	Quality	Follows existing codebase patterns	Pass	Uses existing kwargs-forwarding pattern; is_responsive_snapshot_capture already tolerates both camelCase and snake_case keys.
Medium	Quality	Changes are focused (single concern)	Pass	Single concern, single file, minimal diff.
Low	Quality	Meaningful names, no dead code	Pass	config_options / merged_kwargs are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comment states intent (precedence) clearly.
Low	Quality	No unnecessary dependencies added	Pass	No new imports or dependencies.

Findings

• File: percy/screenshot.py:499

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No test covers the new merge behavior — neither the happy path (a populated config['snapshot'] reaching PercyDOM.serialize), the precedence rule (per-call kwarg overriding a same-named config key), nor that config-only options are forwarded. The PR's own test-plan checkboxes are unchecked.

• Suggestion: Add a percy_snapshot test that mocks _is_percy_enabled to return {"config": {"snapshot": {"enableJavaScript": True, "percyCSS": ".x{}"}}, ...}, call with an overriding kwarg, and assert the serialized args (page.evaluate("PercyDOM.serialize(...)")) reflect the merged set with kwargs winning.

• File: percy/screenshot.py:498

• Severity: Low

• Issue: data["config"].get("snapshot", {}) assumes data["config"] is a dict. _is_percy_enabled defaults it to {}, but a healthcheck body with an explicit "config": null would make data["config"] be None, raising AttributeError. This mirrors a pre-existing assumption (is_responsive_snapshot_capture also does config["snapshot"].get(...)), so it is not a regression, but the new line widens the surface.

• Suggestion: Use (data.get("config") or {}).get("snapshot", {}) to be null-safe and stay consistent with the data.get("config") form used a few lines below at line 509.

• File: percy/screenshot.py:513

• Severity: Low

• Issue: The POST body still derives post_kwargs from the original kwargs, not merged_kwargs. This appears intentional (the CLI already has .percy.yml config, so snapshot options from config should not be re-sent), and it correctly preserves prior POST behavior. Flagged only for visibility; no change required.

• Suggestion: None — confirm this is intended; a one-line comment noting "POST uses per-call kwargs only; CLI sources config itself" would prevent a future regression toward merged_kwargs.

---

Verdict: PASS

[rishigupta1599]

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

[rishigupta1599]

Claude Code PR Review

PR: #141 • Head: 4bf72e0 • Reviewers: stack:code-review

Summary

Merges .percy.yml config.snapshot options into the per-call snapshot kwargs (per-call wins) so config-defined options like enableJavaScript/percyCSS flow into PercyDOM.serialize and the responsive-capture path; adds a unit test covering merge precedence.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped per scope.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped per scope.
High	Security	Input validation and sanitization	N/A	Security lens skipped per scope.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped per scope.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped per scope.
High	Correctness	Logic is correct, handles edge cases	Fail	post_kwargs (screenshot.py:513) still built from raw kwargs, not merged_kwargs — config-only snapshot options reach PercyDOM.serialize but are NOT forwarded to the CLI POST body. config.snapshot of null would also TypeError on {**None}.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Outer try/except unchanged; behavior preserved.
High	Correctness	No race conditions or concurrency issues	Pass	No concurrency introduced.
Medium	Testing	New code has corresponding tests	Pass	New test covers serialize-path precedence (config-only key + per-call override).
Medium	Testing	Error paths and edge cases tested	Fail	No test asserts the merged config keys reach the POST body (would have caught the :513 bug); null snapshot section untested.
Medium	Testing	Existing tests still pass (no regressions)	Fail	test_percy_snapshot_responsive_capture (test_screenshot.py:767) asserts capture_responsive_dom called with only config=...; merge now also passes responsiveSnapshotCapture=True, so the existing assertion will fail. Not updated in this PR.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Two dict spreads 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	Pass	Dict-spread merge is idiomatic; comment explains precedence.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config/kwargs merge plus its test.
Low	Quality	Meaningful names, no dead code	Pass	merged_kwargs / config_options are clear.
Low	Quality	Comments explain why, not what	Pass	Comment notes precedence rule.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: percy/screenshot.py:513

• Severity: High

• Reviewer: stack:code-review

• Issue: post_kwargs = {k: v for k, v in kwargs.items() if k != "readiness"} still uses the original kwargs, not merged_kwargs. Config-only snapshot options (e.g. percyCSS/enableJavaScript set only in .percy.yml) are serialized into the DOM but are NOT sent to the CLI in the POST body, so the CLI receives an incomplete snapshot-options object. This line was not updated alongside the rest of the change.

• Suggestion: Build it from the merged map: post_kwargs = {k: v for k, v in merged_kwargs.items() if k != "readiness"}.

• File: percy/screenshot.py:498

• Severity: Medium

• Reviewer: stack:code-review

• Issue: data["config"].get("snapshot", {}) returns None (not {}) when the CLI healthcheck sends snapshot: null, making {**config_options, **kwargs} raise TypeError. The file already uses the defensive (config or {}).get('snapshot') or {} pattern elsewhere (_resolve_readiness_config at :164, minHeight at :355).

• Suggestion: config_options = data["config"].get("snapshot") or {}.

• File: tests/test_screenshot.py:767

• Severity: Medium

• Reviewer: stack:code-review

• Issue: Pre-existing test_percy_snapshot_responsive_capture mocks config {"snapshot": {"responsiveSnapshotCapture": True}} and asserts capture_responsive_dom is called with only config=.... After this PR merged_kwargs also contains responsiveSnapshotCapture=True, so the call signature gains that kwarg and the existing assert_called_once_with will fail. The PR does not update this test.

• Suggestion: Update the assertion to include responsiveSnapshotCapture=True (or exclude config-only keys from the merge before forwarding).

• File: tests/test_screenshot.py (new test, ~line 588)

• Severity: Low

• Reviewer: stack:code-review

• Issue: The new test verifies PercyDOM.serialize args but never inspects mock_post.call_args for the POST body, so it does not lock in the SDK→CLI contract and would not catch the :513 bug.

• Suggestion: Add posted = mock_post.call_args.kwargs["json"] and assert posted.get("enableJavaScript") is True and posted.get("percyCSS") == "FROM_CALL" (note: this currently fails until :513 is fixed).

• File: percy/screenshot.py:499

• Severity: Low

• Reviewer: stack:code-review

• Issue: The merge is fully permissive — any key the CLI places in config.snapshot (e.g. responsiveSnapshotCapture, minHeight, readiness) is spread into merged_kwargs and forwarded to PercyDOM.serialize. Some are already consumed by name upstream (is_responsive_snapshot_capture reads config["snapshot"]["responsiveSnapshotCapture"]), risking double-application / pollution of serialize args.

• Suggestion: Strip known non-serialize keys (readiness, responsiveSnapshotCapture, minHeight) from config_options before merging, or use an explicit allowlist.

---

Verdict: FAIL — config-derived options silently dropped from the CLI POST body, plus an unupdated existing test that this change breaks.

[rishigupta1599]

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

[rishigupta1599]

[Low] Null-config guard branch is untested

The ... or {} guard handles config.snapshot being present-but-None, which is the stated motivation of this revision, but no test exercises that branch — only the populated-config path is covered.

Suggestion: Optionally add a case where config = {"snapshot": None} and assert percy_snapshot serializes/posts without raising. Non-blocking; the guard is a well-understood one-line idiom.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #141 • Head: 468c5db • Reviewers: stack:code-review

Summary

Merges .percy.yml config.snapshot options with per-call percy_snapshot kwargs (per-call wins) so config-only keys (e.g. enableJavaScript, responsiveSnapshotCapture) flow through to client-side PercyDOM.serialize and the responsive-capture decision. Adds a null-config guard (... or {}) for when config.snapshot is present-but-null, and updates the responsive test plus adds a precedence test for the merge.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Per scope: Security skipped. No secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Per scope: Security skipped. No auth surface touched.
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.
High	Security	No SQL injection (parameterized queries)	N/A	Per scope: Security skipped. No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	config_options = data["config"].get("snapshot") or {} correctly guards the present-but-null case; data["config"] is always set (_is_percy_enabled defaults config to {} at line 58). Dict-spread merge {**config_options, **kwargs} gives per-call precedence as intended.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Logic sits inside the existing try/except in percy_snapshot; no new swallow points introduced.
High	Correctness	No race conditions or concurrency issues	Pass	Pure local dict construction; no shared/mutable state added.
Medium	Testing	New code has corresponding tests	Pass	New test_percy_snapshot_merges_config_with_per_call_options asserts config-only enableJavaScript flows through and per-call percyCSS overrides config.
Medium	Testing	Error paths and edge cases tested	Partial	Precedence + config-only flow-through covered; the null-snapshot (or {}) branch is not directly asserted by a test. Low risk — guard is trivial. See Findings.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Responsive test updated to expect responsiveSnapshotCapture=True now propagated via merged kwargs — assertion matches new behavior.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	No queries; single small dict merge.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to SDK snapshot path.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors existing **kwargs forwarding and data.get("config") usage; comment explains intent.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config<->snapshot merge + its tests.
Low	Quality	Meaningful names, no dead code	Pass	config_options / merged_kwargs are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comment states precedence rule (snapshot options take priority).
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

No Critical/High findings.

• File: percy/screenshot.py:498
• Severity: Low
• Reviewer: stack:code-review
• Issue: The null-config guard branch (config.snapshot present but None → or {}) is the stated motivation of this revision but has no direct unit test; only the populated-config path is exercised.
• Suggestion: Optionally add a case where config = {"snapshot": None} and assert percy_snapshot still serializes/posts without raising. Non-blocking — the guard is a one-line, well-understood idiom.

Note on prior-review false positive (verified non-issue, not counted): the POST body intentionally uses kwargs (not merged_kwargs) because the Percy CLI applies config.snapshot server-side to every posted snapshot (packages/core/src/snapshot.js getSnapshotOptions = PercyConfig.merge([defaults, config.snapshot, options])). The SDK merge is correctly scoped to client-side PercyDOM.serialize only. N/A.

---

Verdict: PASS — correct, focused merge with per-call precedence; null-config guard is sound and tests cover the meaningful paths.

[rishigupta1599]

Claude Code PR Review

PR: #141 • Head: 712f97f • Reviewers: stack:code-review

Summary

Merges .percy.yml config snapshot options into per-call kwargs (per-call wins) before DOM serialization in percy_snapshot(), so config-only keys such as enableJavaScript flow through to PercyDOM.serialize(). The POST body intentionally keeps original kwargs (config is merged server-side by the CLI). Adds a merge-precedence test and updates the responsive-capture test.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	No security surface; no secrets/credentials touched.
High	Security	Authentication/authorization checks present	N/A	No auth surface in this change.
High	Security	Input validation and sanitization	N/A	Inputs are SDK kwargs/CLI healthcheck config; no external/untrusted input.
High	Security	No IDOR — resource ownership validated	N/A	No resource-ownership concern.
High	Security	No SQL injection (parameterized queries)	N/A	No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	{**config_options, **kwargs} correctly lets per-call kwargs win; data["config"].get("snapshot") or {} guards a null snapshot section. POST body correctly uses original kwargs (CLI merges config server-side — N/A false positive, not flagged).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Existing try/except behavior unchanged; no new swallowing introduced.
High	Correctness	No race conditions or concurrency issues	Pass	Synchronous, single-pass logic.
Medium	Testing	New code has corresponding tests	Pass	New test_percy_snapshot_merges_config_with_per_call_options covers config-only passthrough and per-call override.
Medium	Testing	Error paths and edge cases tested	Pass	Merge precedence + responsive promotion covered; minor gap noted (POST-body cleanliness assertion) in Findings.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Responsive test updated to assert responsiveSnapshotCapture=True is now promoted into the call — consistent with new behavior.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching introduced; one dict merge.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to SDK snapshot path.
Medium	Quality	Follows existing codebase patterns	Pass	or {} guard mirrors _resolve_readiness_config; merge style is idiomatic.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config<->kwargs merge plus tests.
Low	Quality	Meaningful names, no dead code	Pass	config_options / merged_kwargs are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comment states intent (priority of snapshot options).
Low	Quality	No unnecessary dependencies added	Pass	Dependency-free inline change.

Findings

• File: percy/screenshot.py:282 (cross-origin iframe path in get_serialized_dom)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: process_frame(page, frame, kwargs, ...) forwards the raw kwargs (now merged_kwargs) and serializes them via PercyDOM.serialize({**options, 'enableJavaScript': True}) without stripping readiness, unlike the main path which strips it into serialize_kwargs. A .percy.yml snapshot.readiness value is now promoted into merged_kwargs and reaches every cross-origin iframe serialize call. Pre-existing iframe-path inconsistency, made newly reachable by this PR. Behavioral impact is limited (only cross-origin iframes + a readiness config; PercyDOM.serialize likely ignores the unknown key).

• Suggestion: Pass serialize_kwargs to process_frame instead of kwargs so readiness is stripped on the iframe path too.

• File: tests/test_screenshot.py:540-588

• Severity: Low

• Reviewer: stack:code-review

• Issue: The merge test verifies PercyDOM.serialize() receives merged kwargs but does not assert the POST body stays clean (no config-only key like enableJavaScript leaks into mock_post json). This is the main regression surface if a future change swaps post_kwargs for merged_kwargs.

• Suggestion: Add posted = mock_post.call_args.kwargs["json"]; self.assertNotIn("enableJavaScript", posted); self.assertEqual(posted.get("percyCSS"), "FROM_CALL").

• File: percy/screenshot.py:498

• Severity: Low

• Reviewer: stack:code-review

• Issue: data["config"] is indexed directly (not .get). Pre-existing pattern, not introduced here, but this PR adds another read; a healthcheck body without a config key would KeyError.

• Suggestion: Optional: data.get("config", {}).get("snapshot") or {} for defensiveness. Non-blocking.

---

Verdict: PASS — core merge logic is correct and well-tested; findings are Medium/Low refinements, none blocking.

[rishigupta1599]

Claude Code PR Review

PR: #141 • Head: a7f512f • Reviewers: stack:code-review

Summary

Replaces the config↔per-call snapshot-options merge with a recursive _deep_merge helper so nested .percy.yml config blocks (e.g. discovery) merge key-by-key with per-snapshot kwargs instead of being clobbered wholesale; per-call leaf values still win, lists/scalars replace. Adds two unit tests for _deep_merge and two integration-style tests asserting config-only keys flow through to PercyDOM.serialize while per-call keys override.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped per scope; no secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped; no auth surface touched.
High	Security	Input validation and sanitization	N/A	Security lens skipped; internal dict merge only.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped; not applicable.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped; no DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	_deep_merge recurses only when both sides are dicts; per-call leaves win; lists/scalars replace. Verified against both unit tests and the serialize-path wiring.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	data["config"].get("snapshot") or {} guards None; no new try/except introduced; consistent with existing is_responsive_snapshot_capture usage.
High	Correctness	No race conditions or concurrency issues	Pass	Pure function over local dicts; no shared/mutable state. dict(base) avoids mutating the input config.
Medium	Testing	New code has corresponding tests	Pass	Two _deep_merge unit tests + two percy_snapshot integration tests covering flat override, nested deep-merge, and scalar/list replace.
Medium	Testing	Error paths and edge cases tested	Pass	Nested-dict, scalar-replaces-dict, and list-replace cases all asserted. Empty-config path covered by or {} (untested but trivial).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Responsive test updated to expect new responsiveSnapshotCapture=True kwarg now flowing from merged config — consistent with the merge change.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching; in-memory merge of small option dicts.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the adjacent _resolve_readiness_config shallow-merge helper; same module-private naming and docstring style.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to merge behavior + tests; no unrelated edits.
Low	Quality	Meaningful names, no dead code	Pass	_deep_merge, merged_kwargs, config_options are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Docstring explains merge semantics (leaves win, lists/scalars replace); inline comment explains precedence.
Low	Quality	No unnecessary dependencies added	Pass	No new imports or dependencies.

Findings

No blocking findings.

Note (non-blocking, adjudicated false positive): post_kwargs (line 528) is still derived from raw kwargs rather than merged_kwargs, so config-derived options are not added to the POST body. Per adjudication this is VERIFIED FALSE POSITIVE — the CLI merges .percy.yml config server-side via the healthcheck, so the SDK intentionally posts only per-call options. merged_kwargs is correctly used for the DOM serialization path where config-only keys (e.g. enableJavaScript) must take effect client-side. No regression.

---

Verdict: PASS — deep-merge is correct, well-scoped, and well-tested.