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

[rishigupta1599]

Summary

• Config options from .percy.yml (like widths, minHeight, enableJavaScript, etc.) 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) — 1 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[rishigupta1599]

Claude Code PR Review

PR: #223 • Head: b9758c6 • Reviewers: stack:code-review

Summary

Inline-merges global .percy.yml config['snapshot'] options with per-call kwargs (per-call priority) into merged_kwargs, which is then passed to is_responsive_snapshot_capture, capture_responsive_dom, and get_serialized_dom so config-level DOM serialization options are honored before serializeDOM (PER-8053).

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	snapshot.py:504 uses .get('snapshot', {}), which returns None (not {}) when the CLI healthcheck sends snapshot: null; {**None, **kwargs} then raises TypeError, silently dropping the snapshot in the outer except. The codebase's own pattern at snapshot.py:202 uses (config.get('snapshot') or {}) for exactly this reason.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	The added code introduces no new swallowing; however the null-config crash (above) would be swallowed by the existing outer except, masking the failure. Driven by the Finding-1 fix.
High	Correctness	No race conditions or concurrency issues	Pass	Pure local dict merge; no shared/concurrent state.
Medium	Testing	New code has corresponding tests	Fail	No tests in tests/test_snapshot.py cover the merge: config-only option forwarded, per-call override precedence, or the snapshot: null non-crash case. mock_healthcheck(config=...) already supports adding them.
Medium	Testing	Error paths and edge cases tested	Fail	The snapshot: null edge case (Finding 1) is untested and currently crashes.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive to the serialize path; no existing test assertions appear affected.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single in-memory dict merge per snapshot; negligible.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK snapshot call.
Medium	Quality	Follows existing codebase patterns	Fail	Inconsistent with the established defensive (config.get('snapshot') or {}) pattern used at snapshot.py:202 and :233; the new line uses the fragile .get('snapshot', {}) form.
Medium	Quality	Changes are focused (single concern)	Pass	Tightly scoped to merging config into the serialize kwargs.
Low	Quality	Meaningful names, no dead code	Pass	config_options / merged_kwargs are clear. capture_responsive_dom:437 minHeight fallback becomes partly redundant but is not newly introduced.
Low	Quality	Comments explain why, not what	Pass	Comment at :503 states intent and the precedence rule.
Low	Quality	No unnecessary dependencies added	Pass	Inline merge; no JS-utility dependency, matching the stated design.

Findings

• File: percy/snapshot.py:504

• Severity: High

• Reviewer: stack:code-review

• Issue: config_options = data['config'].get('snapshot', {}) returns None when the healthcheck payload contains snapshot: null (the key is present with a null value, so the {} default is not used). The next line merged_kwargs = {**config_options, **kwargs} then raises TypeError: 'NoneType' object is not a mapping, which is caught by the outer except Exception and logged as "Could not take DOM snapshot", silently dropping the snapshot. The same module already guards against this at line 202 ((config.get('snapshot') or {})) and documents (lines 198–200) that the CLI may send None and it "should degrade to {}, not raise".

• Suggestion: Use config_options = data['config'].get('snapshot') or {} for consistency with the existing defensive pattern.

• File: percy/snapshot.py:503-519

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No test coverage for the new merge behavior: (a) a config-only snapshot option is forwarded to serialize when absent from kwargs, (b) per-call kwargs override the same config key, (c) snapshot: null does not crash the call. tests/test_snapshot.py already drives config via mock_healthcheck(config=...), so the harness exists.

• Suggestion: Add three focused tests covering merge-forwarding, per-call precedence, and the null-config edge case.

• File: percy/snapshot.py:524

• Severity: Low

• Reviewer: stack:code-review

• Issue: The snapshot POST body is built from the original kwargs (post_kwargs), not merged_kwargs. This is intentional and correct for this PR's scope — the CLI already owns the global .percy.yml config (it is the source of data['config']), so re-sending merged config options in the POST body would be redundant. Flagged only to confirm the asymmetry is deliberate; the merge is meant to influence in-browser PercyDOM.serialize, not the CLI POST payload.

• Suggestion: No change required; consider a one-line comment noting that the POST body deliberately carries only per-call options since the CLI already has the global config.

---

Verdict: FAIL

[rishigupta1599]

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

[rishigupta1599]

[Medium] Guard config itself against null, not just `config['snapshot']

This fixes snapshot being null, but data['config'] itself can be None: is_percy_enabled() does data.get('config', {}), which only defaults when the key is absent — a healthcheck returning {"config": null} makes data['config'] None, and .get('snapshot') then raises AttributeError. The codebase's own _resolve_readiness_config defends against exactly this (config = percy_config or {}, see snapshot.py:198-200).

Suggestion:

Suggested change

	config_options = data['config'].get('snapshot') or {}
	config_options = (data['config'] or {}).get('snapshot') or {}

Reviewer: stack:code-review

[rishigupta1599]

[Medium] Missing tests for the new config-merge behavior

No test covers that config-level snapshot options flow into serializeDOM, that per-call kwargs override config options, or that a null snapshot config degrades to {} on the percy_snapshot path. The existing null-config test (test_snapshot.py:505) only exercises _resolve_readiness_config, a different function.

Suggestion: Add percy_snapshot tests with mocked healthcheck config {'snapshot': {...}} asserting the option reaches the serialize path, a kwargs-override case, and a {'snapshot': None} (and config: None) case.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #223 • Head: 4e94aef • Reviewers: stack:code-review

Summary

Merges .percy.yml snapshot config options into per-call snapshot options before DOM serialization so config-level options reach the serializeDOM path (per-call kwargs win), and adds an or {} guard so a null snapshot config no longer raises a TypeError/AttributeError during the merge.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens skipped per orchestrator policy.
High	Security	Authentication/authorization checks present	N/A	Security lens skipped per orchestrator policy.
High	Security	Input validation and sanitization	N/A	Security lens skipped per orchestrator policy.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped per orchestrator policy.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped per orchestrator policy.
High	Correctness	Logic is correct, handles edge cases	Pass	config_options = data['config'].get('snapshot') or {} correctly coerces a null/absent snapshot to {}; the prior FAIL (TypeError on null snapshot config) is resolved. Merge order {**config_options, **kwargs} correctly lets per-call kwargs win.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No new exception swallowing; the surrounding try/except is unchanged. Note the residual edge case in Findings (config itself being null).
High	Correctness	No race conditions or concurrency issues	Pass	No shared mutable state introduced; merged_kwargs is a fresh local dict.
Medium	Testing	New code has corresponding tests	Fail	No test exercises the new config→serializeDOM merge or the null-snapshot guard on the percy_snapshot path. See Findings.
Medium	Testing	Error paths and edge cases tested	Fail	The null-config edge case is only covered for _resolve_readiness_config (test_snapshot.py:505), not for the new merge code path.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive to kwarg handling; existing percy_snapshot tests pass unchanged kwargs through merged_kwargs.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single dict merge per snapshot; 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 _resolve_readiness_config or {} defensive style.
Medium	Quality	Changes are focused (single concern)	Pass	Diff is scoped to one function; both commits address the same merge concern.
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	The inline comment explains intent (config merge, snapshot options take priority).
Low	Quality	No unnecessary dependencies added	Pass	Dependency-free; no imports added.

Findings

• File: percy/snapshot.py:504

• Severity: Medium

• Reviewer: stack:code-review

• Issue: The guard handles data['config']['snapshot'] being null (... .get('snapshot') or {}), but does not guard data['config'] itself being None. is_percy_enabled() does config = data.get('config', {}), which only defaults when the key is absent; if the CLI healthcheck returns {"config": null}, data['config'] is None and data['config'].get('snapshot') raises AttributeError. The codebase's own _resolve_readiness_config explicitly defends against this with config = percy_config or {} (and documents it at snapshot.py:198-200).

• Suggestion: Coerce config first, e.g. config_options = (data['config'] or {}).get('snapshot') or {}, to match the established defensive pattern.

• File: percy/snapshot.py:504-505

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No test covers the new merge behavior. There is no assertion that config-level snapshot options flow into the serializeDOM call, that per-call kwargs override config options, or that a null snapshot config degrades to {} on the percy_snapshot path. The existing null-config test (test_snapshot.py:505) only exercises _resolve_readiness_config, a different function.

• Suggestion: Add percy_snapshot tests with a mocked healthcheck config {'snapshot': {...}} asserting the option reaches the serialize/POST path, a kwargs-override case, and a {'snapshot': None} (and ideally config: None) case.

• File: percy/snapshot.py:524-526

• Severity: Low

• Reviewer: stack:code-review

• Issue: The snapshot POST body (post_kwargs) is still built from the raw kwargs, not merged_kwargs. So config-level snapshot options now influence DOM serialization but are NOT forwarded to the CLI snapshot endpoint. This is likely intentional (the CLI applies .percy.yml itself), but the asymmetry is worth confirming so options like minHeight behave consistently between the serialize path and the POST.

• Suggestion: Confirm intended asymmetry; if config options should also reach the POST body, use merged_kwargs (minus readiness) there too. Otherwise no change needed.

---

Verdict: PASS — the null-config guard resolves the prior FAIL and the core merge logic is correct; remaining items are Medium/Low (test coverage and a latent null-config edge) and do not block.

[rishigupta1599]

Claude Code PR Review

PR: #223 • Head: 178d599 • Reviewers: stack:code-review

Summary

Merges .percy.yml config.snapshot options with per-snapshot keyword arguments before DOM serialization so config-level options (e.g. enableJavaScript, percyCSS) reach PercyDOM.serialize, with per-call kwargs taking precedence; adds a unit test covering the merge precedence and a null-config guard.

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 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	Pass	Merge {**config_options, **kwargs} correctly gives per-call kwargs priority; or {} guards a null/missing snapshot config (covered by commit 4e94aef).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No new error handling introduced; existing try/except in percy_snapshot unchanged.
High	Correctness	No race conditions or concurrency issues	Pass	Pure dict-merge of local data; no shared/mutable state.
Medium	Testing	New code has corresponding tests	Pass	TestConfigPerCallMergePrecedence asserts a config-only key reaches serialize and a per-call kwarg overrides the config value.
Medium	Testing	Error paths and edge cases tested	Partial	Precedence + config-forwarding covered; the null/missing-config guard (or {}) has no dedicated test (see Findings).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive to the merge path; existing call signatures preserved.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	One extra in-memory dict merge per snapshot; 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	Uses existing kwargs/data['config'] conventions; consistent with is_responsive_snapshot_capture config handling.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config<->kwargs merge for serialization plus its test.
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	Inline comment states intent (priority of snapshot options).
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: percy/snapshot.py:524

• Severity: Low

• Reviewer: stack:code-review

• Issue: The snapshot POST body (post_kwargs) is still derived from the original kwargs, not merged_kwargs. As a result, .percy.yml config.snapshot options now flow into PercyDOM.serialize but are NOT included in the snapshot POST payload. This is most likely intentional — the CLI applies .percy.yml config on its side for the POST — but the asymmetry between the serialize path (merged) and the POST path (un-merged) is non-obvious and could surprise a future maintainer.

• Suggestion: Add a one-line comment near post_kwargs clarifying that config options are intentionally NOT merged into the POST body because the CLI already applies .percy.yml for the upload, leaving only the serialize path needing the merge. No code change required if the CLI-side handling is confirmed.

• File: tests/test_snapshot.py:40

• Severity: Low

• Reviewer: stack:code-review

• Issue: The null/missing-config guard added in commit 4e94aef (data['config'].get('snapshot') or {}) — which prevents a TypeError when config.snapshot is null or absent — has no dedicated regression test. The new test always mocks a populated config.snapshot.

• Suggestion: Add a small test case where mock_healthcheck returns a config with snapshot: None (or no snapshot key) and assert percy_snapshot still serializes without raising, to lock in the guard against regressions.

---

Verdict: PASS — focused, well-tested config/kwargs merge with correct precedence; only minor documentation/test-coverage nits.

[rishigupta1599]

Claude Code PR Review

PR: #223 • Head: d2339f9 • Reviewers: stack:code-review

Summary

Replaces the shallow .percy.yml config ↔ per-call kwargs merge in percy_snapshot with a recursive deep merge via a new _deep_merge helper (nested dicts merge, per-call wins at leaves, lists/scalars replace), and adds two unit tests covering nested-leaf merging and top-level per-call precedence.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	No secrets; pure merge logic.
High	Security	Authentication/authorization checks present	N/A	Not applicable to a config-merge helper.
High	Security	Input validation and sanitization	N/A	Inputs are SDK kwargs/CLI config dicts; no external untrusted input.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	_deep_merge recurses only when BOTH existing and value are dicts (isinstance(existing, dict) and isinstance(value, dict)); otherwise the override value replaces. Lists/scalars replace; None on either side falls to the else value branch (never recurses), so no dict(None)/None.items() is reachable. Call site config_options = data['config'].get('snapshot') or {} guarantees base is a dict.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No new exception handling; existing flow unchanged.
High	Correctness	No race conditions or concurrency issues	Pass	Pure function, no shared mutable state.
Medium	Testing	New code has corresponding tests	Pass	TestConfigDeepMerge covers nested-leaf merge (sibling kept, overridden leaf wins); TestConfigPerCallMergePrecedence covers top-level per-call precedence + config-only key forwarding.
Medium	Testing	Error paths and edge cases tested	Pass (note)	Happy-path nesting + precedence covered. List-replacement and None-value edge cases are not unit-tested directly — non-blocking coverage gap (those paths are correct by inspection).
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive; existing serialize/responsive paths now receive merged_kwargs (superset of prior kwargs).
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No I/O in the merge.
Medium	Performance	Long-running tasks use background jobs	N/A	Synchronous in-memory merge.
Medium	Quality	Follows existing codebase patterns	Pass	Module-level private helper consistent with file style.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to the config↔per-call merge plus its tests.
Low	Quality	Meaningful names, no dead code	Pass	_deep_merge / merged_kwargs / config_options are clear.
Low	Quality	Comments explain why, not what	Pass	Inline comment notes snapshot options take priority.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

No blocking findings. Non-blocking notes below (none introduced by this diff as a regression).

• Adjudicated FALSE POSITIVE (N/A): A reviewer flagged that the snapshot POST body is built from raw kwargs rather than merged_kwargs, supposedly dropping config-only keys. This is a verified false positive: the Percy CLI merges config.snapshot server-side (percy/cli packages/core/src/snapshot.js getSnapshotOptions), so config-inherited options are not required in the SDK POST body. Not a defect.
• Note (non-blocking, pre-existing): data['config'].get('snapshot') would AttributeError only if the healthcheck returned config: null; data['config'] was already dereferenced upstream (is_responsive_snapshot_capture(data['config'], ...)) before this PR, so this is pre-existing, not introduced here.
• Note (non-blocking, coverage): List-replacement and None-leaf cases are correct by inspection but lack dedicated unit tests; adding a local._deep_merge unit test for list replacement and a None override value would tighten coverage.

---

Verdict: PASS — the deep merge is correct (recurses only when both sides are dicts, lists/scalars replace, null-safe on all reachable paths) and is covered by new tests. No new correctness regression in this diff; remaining items are an adjudicated false positive and non-blocking coverage notes.