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
• Adds MergeSnapshotOptions() helper that extracts cliConfig.snapshot properties and merges 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

[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: Failed - see PR comment.

[rishigupta1599]

[Low] Empty catch swallows config-merge errors

catch (Exception) { } discards every error during config extraction with no log, so a malformed cliConfig silently contributes nothing and the user gets no signal.

Suggestion: Log at debug, matching the existing pattern: Log($"Error reading snapshot config: {e.Message}", "debug");

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #430 • Head: a6fd4fa • Reviewers: stack:code-review

Summary

Adds MergeSnapshotOptions() to Percy.cs, which builds a Dictionary<string,object> from cliConfig.snapshot (coercing JsonElement scalars to typed values) and overlays per-call options on top (per-call priority), then routes the merged options into isResponsiveSnapshotCapture, CaptureResponsiveDom, and getSerializedDom before serializeDOM (PER-8053).

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	Fail	Config-supplied widths (a JSON array) is stored as a raw JsonElement by the _ => prop.Value branch, then CaptureResponsiveDom line 774 casts options["widths"] to (List<int>) → InvalidCastException. Merge now feeds config keys into a code path that assumes List<int>.
High	Correctness	Error handling is explicit, no swallowed exceptions	Fail	catch (Exception) { } (Percy.cs:859) silently swallows all merge errors; a malformed cliConfig yields an empty config contribution with no log, masking misconfig.
High	Correctness	No race conditions or concurrency issues	Pass	Operates on local dict + static read-only cliConfig; no new shared mutable state.
Medium	Testing	New code has corresponding tests	Fail	No tests added for MergeSnapshotOptions despite non-trivial JsonElement→typed coercion and per-call-priority overlay.
Medium	Testing	Error paths and edge cases tested	Fail	No coverage for config-only widths, nested/array config values, null options, or per-call override precedence.
Medium	Testing	Existing tests still pass (no regressions)	N/A	Not run in this review; CI status authoritative.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single pass over snapshot properties and options; negligible cost, once per snapshot.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to SDK snapshot path.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the existing (JsonElement)cliConfig / TryGetProperty pattern already used in isResponsiveSnapshotCapture and ResolveResponsiveTargetHeight.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to the merge feature; only Percy.cs touched.
Low	Quality	Meaningful names, no dead code	Pass	MergeSnapshotOptions / mergedOptions are clear; removed dead string opts serialize.
Low	Quality	Comments explain why, not what	Pass	The merge-priority comment at the call site is useful.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: Percy/Percy.cs:774 (interaction with Percy/Percy.cs:854)

• Severity: High

• Reviewer: stack:code-review

• Issue: MergeSnapshotOptions coerces only scalar JSON kinds; arrays/objects fall through _ => prop.Value and are stored as raw JsonElement. If widths is configured in .percy.yml (snapshot.widths) but not passed per-call, the merged widths is a JsonElement. isResponsiveSnapshotCapture(mergedOptions) can then be true and CaptureResponsiveDom runs (List<int>)options["widths"] (line 774), which throws InvalidCastException at runtime. Before this PR widths only ever came from per-call options (already List<int>), so config-supplied widths is a new, broken path — and is squarely within the feature's stated goal of merging config into options.

• Suggestion: In MergeSnapshotOptions, handle JsonValueKind.Array for widths (and any int-list option) by materializing a List<int> (e.g. prop.Value.EnumerateArray().Select(e => e.GetInt32()).ToList()), or make CaptureResponsiveDom tolerant of a JsonElement/IEnumerable widths value instead of a hard (List<int>) cast.

• File: Percy/Percy.cs:859

• Severity: Low

• Reviewer: stack:code-review

• Issue: catch (Exception) { } swallows every error during config extraction with no log, so a malformed cliConfig silently contributes nothing to the merge and the user gets no signal. (Consistent with the existing swallow in isResponsiveSnapshotCapture, hence Low.)

• Suggestion: Log at debug (matching ResolveResponsiveTargetHeight's Log($"Error reading ... {e.Message}", "debug")) so misconfigured config is diagnosable.

• File: Percy/Percy.cs:835 (test coverage)

• Severity: Medium

• Reviewer: stack:code-review

• Issue: No unit tests accompany MergeSnapshotOptions, despite JsonElement→typed coercion, the array/object fall-through, and per-call-priority overlay all being correctness-sensitive.

• Suggestion: Add tests for: per-call option overriding a config value; config-only scalar passthrough; config-only widths array; and null/empty options.

---

Verdict: FAIL — config-supplied widths triggers an InvalidCastException in CaptureResponsiveDom, and the new merge logic ships with no tests.

[rishigupta1599]

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

[rishigupta1599]

[Low] Empty config widths: [] re-triggers the InvalidCastException

ConvertJsonArray only returns List<int> when items.Count > 0. An empty config array falls through to the second branch returning an empty List<object>. Since MergeSnapshotOptions still adds the widths key, CaptureResponsiveDom (line 774, ContainsKey("widths") true) then runs (List<int>)options["widths"] on a List<object> and throws — the same failure this PR fixes, for the empty-widths edge case.

Suggestion: Return List<int> for empty numeric arrays too, e.g. if (items.Count == 0 || items.All(item => item.ValueKind == JsonValueKind.Number && item.TryGetInt32(out _))), or make line 774 defensive.

Reviewer: stack:code-review

[rishigupta1599]

[Medium] No tests for the new merge/array-typing logic

MergeSnapshotOptions and ConvertJsonArray (including the widths-typing fix that was the prior FAIL) ship with no unit tests, though the repo has a Percy.Test project. Regressions in array typing or merge precedence would go uncaught.

Suggestion: Add tests covering: config widths array converting to List<int> consumable by CaptureResponsiveDom; per-call options overriding config; empty and mixed-type arrays; and absent/non-object snapshot config falling back cleanly.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #430 • Head: 6dabd1f • Reviewers: stack:code-review

Summary

Adds MergeSnapshotOptions to Percy.Snapshot, merging .percy.yml cliConfig.snapshot options with per-call snapshot options (per-call wins), and routes the merged dictionary into isResponsiveSnapshotCapture, CaptureResponsiveDom, and getSerializedDom. A new ConvertJsonArray helper converts JSON config arrays of integers (e.g. widths) into a typed List<int>, so the existing (List<int>)options["widths"] cast in CaptureResponsiveDom (the prior FAIL) no longer throws when widths come from config.

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.
High	Security	Input validation and sanitization	N/A	Security lens skipped.
High	Security	No IDOR — resource ownership validated	N/A	Security lens skipped.
High	Security	No SQL injection (parameterized queries)	N/A	Security lens skipped.
High	Correctness	Logic is correct, handles edge cases	Pass	Core fix is correct: ConvertJsonArray returns List<int> for all-int arrays, so the (List<int>) cast at Percy/Percy.cs:774 succeeds for config-sourced widths. One residual edge case noted (empty widths: [] in config) — see Findings (Low).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	catch (Exception) { } at Percy/Percy.cs:860 swallows silently, but mirrors the existing isResponsiveSnapshotCapture pattern (line 913) and degrades safely to per-call-only options. Acceptable.
High	Correctness	No race conditions or concurrency issues	Pass	Pure transform over static cliConfig; no new shared mutable state.
Medium	Testing	New code has corresponding tests	Fail	No tests added for MergeSnapshotOptions/ConvertJsonArray or the widths-conversion fix; PR touches only Percy/Percy.cs. See Findings (Medium).
Medium	Testing	Error paths and edge cases tested	Fail	Empty-array and mixed-type array paths in ConvertJsonArray, and config/per-call merge precedence, are untested.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Change is additive/behavior-preserving for per-call options; existing log-based tests unaffected.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single pass over config snapshot props; no I/O added.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to a client SDK option-merge path.
Medium	Quality	Follows existing codebase patterns	Pass	Uses the same JsonElement/ValueKind switch and try/catch style already present in the file.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to option merging + array typing for the widths fix.
Low	Quality	Meaningful names, no dead code	Pass	MergeSnapshotOptions/ConvertJsonArray are clear; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comment at line 938 explains the precedence rationale.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies; uses existing System.Text.Json/LINQ.

Findings

• File: Percy/Percy.cs:873

• Severity: Low

• Reviewer: stack:code-review

• Issue: ConvertJsonArray only returns List<int> when items.Count > 0. An empty config array (widths: []) falls through to the second branch and returns an empty List<object>. Because MergeSnapshotOptions still adds the widths key, CaptureResponsiveDom at line 774 (ContainsKey("widths") is true) then executes (List<int>)options["widths"] on a List<object>, throwing InvalidCastException — the same class of failure this PR fixes, for the empty-widths edge case.

• Suggestion: Treat an empty numeric array as List<int> too, e.g. return List<int> when items.Count == 0 || items.All(int); or make line 774 defensive (options["widths"] as List<int> ?? widths.OfType<int>()...). Empty config arrays are uncommon but valid in .percy.yml.

• File: Percy/Percy.cs:835

• Severity: Medium

• Reviewer: stack:code-review

• Issue: The new MergeSnapshotOptions and ConvertJsonArray logic (including the widths-typing fix that was the prior FAIL) ships with no unit tests. The repo has a Percy.Test project, but the PR diff touches only Percy/Percy.cs. Regressions in array typing or merge precedence would go uncaught.

• Suggestion: Add tests covering: (a) config widths array converting to List<int> consumable by CaptureResponsiveDom, (b) per-call options overriding config, (c) empty and mixed-type arrays, (d) non-object/absent snapshot config falling back cleanly.

---

Verdict: PASS — the widths conversion fix is correct and resolves the prior cast FAIL; remaining items (missing tests, empty-array edge case) are non-blocking Medium/Low.

[rishigupta1599]

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

[rishigupta1599]

[Low] Silently swallowed merge error

catch (Exception) { } discards any failure while reading cliConfig.snapshot, so a misconfigured .percy.yml silently drops all config-seeded options with no diagnostic.

Suggestion: Log at debug level inside the catch (the file already has a Log(..., "debug") helper) so misconfiguration is diagnosable while still degrading gracefully.

Reviewer: stack:code-review

[rishigupta1599]

[Low] ConvertJsonArray is untested

The added test only covers scalar (bool/string) merge precedence; the widths-style array conversion path has no direct coverage.

Suggestion: Add a case where cliConfig.snapshot.widths is a JSON number array and assert it converts to a typed List<int> and survives the merge.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #430 • Head: 4f2e554 • Reviewers: stack:code-review

Summary

Merges .percy.yml config options (cliConfig.snapshot) with per-snapshot options before DOM serialization in Percy.Snapshot, with per-snapshot options taking precedence. Adds MergeSnapshotOptions/ConvertJsonArray helpers and a deterministic reflection-based unit test asserting the merge precedence (config-only key inherited; conflicting key won by per-call value).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	Security lens out of scope for this re-run; no secrets in diff.
High	Security	Authentication/authorization checks present	N/A	Out of scope; no auth surface touched.
High	Security	Input validation and sanitization	N/A	Out of scope; merge consumes already-parsed JsonElement config.
High	Security	No IDOR — resource ownership validated	N/A	Out of scope; no resource access.
High	Security	No SQL injection (parameterized queries)	N/A	Out of scope; no DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	Merge seeds from cliConfig.snapshot, overlays per-call options; per-call wins on conflict. Null config and null options handled. JSON kinds mapped (bool/number/string/array). mergedOptions now also feeds isResponsiveSnapshotCapture, fixing config-set widths/responsive detection.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	catch (Exception) {} swallows merge errors silently (Low nit below), but degrades to per-call-only options — same defensive pattern as existing isResponsiveSnapshotCapture. No new exception escapes.
High	Correctness	No race conditions or concurrency issues	Pass	Static helpers read static cliConfig; no new shared mutable state. Consistent with existing static design.
Medium	Testing	New code has corresponding tests	Pass	PerSnapshotOptionWinsOverConfigDuringMerge exercises MergeSnapshotOptions via reflection — acceptable since Selenium 4 WebDriver is not mockable; deterministic, no browser/CLI needed.
Medium	Testing	Error paths and edge cases tested	Partial	Conflict + config-only inheritance covered. ConvertJsonArray (widths array typing) and the empty/null-config paths are untested. Low.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Test wraps state mutation in try/finally with ResetInternalCaches(); no shared-state leak into other tests.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Single in-memory enumeration of config + options; no I/O.
Medium	Performance	Long-running tasks use background jobs	N/A	No long-running work.
Medium	Quality	Follows existing codebase patterns	Pass	Switch-on-ValueKind mapping and catch-and-degrade match existing helpers in the file.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config<->snapshot option merge plus its test.
Low	Quality	Meaningful names, no dead code	Pass	MergeSnapshotOptions/ConvertJsonArray clear; removed dead opts serialize line.
Low	Quality	Comments explain why, not what	Pass	Comments justify precedence rules and the reflection approach.
Low	Quality	No unnecessary dependencies added	Pass	Only System.Reflection added in the test project.

Findings

• File: Percy/Percy.cs:860

• Severity: Low

• Reviewer: stack:code-review

• Issue: catch (Exception) { } silently swallows any failure while reading cliConfig.snapshot. If the config shape is unexpected, the merge silently drops all config-seeded options with no diagnostic.

• Suggestion: Log at debug level inside the catch (the file already has a Log(..., "debug") helper used elsewhere) so misconfigured .percy.yml is diagnosable, while still degrading gracefully.

• File: Percy/Percy.cs:870

• Severity: Low

• Reviewer: stack:code-review

• Issue: ConvertJsonArray (used for widths-style arrays) has no direct test; the added test only covers scalar (bool/string) precedence.

• Suggestion: Add a case where cliConfig.snapshot.widths is a JSON number array to assert it is converted to a typed List<int> and survives the merge.

---

Verdict: PASS — small, focused fix with correct precedence semantics, a deterministic test, and only Low-severity polish nits.

[rishigupta1599]

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

[rishigupta1599]

[High] Config-seeded widths crashes responsive capture

This now passes mergedOptions into CaptureResponsiveDom. When widths comes from .percy.yml, MergeSnapshotOptions/JsonElementToObjectDeep convert it to List<object>, but CaptureResponsiveDom (line 774) hard-casts options["widths"] to List<int> and throws InvalidCastException. It's swallowed by Snapshot's outer catch, so a config-driven responsive snapshot that previously fell back to default widths now produces no snapshot at all.

Suggestion: Coerce widths at the consumption site regardless of element type:

List<int> widths = new List<int>();
if (options != null && options.TryGetValue("widths", out var raw)) {
    if (raw is List<int> ints) widths = ints;
    else if (raw is List<object> objs) widths = objs.Select(Convert.ToInt32).ToList();
}

or route array conversion through the (currently unused) ConvertJsonArray, which already yields List<int> for homogeneous int arrays.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #430 • Head: 17070dd • Reviewers: stack:code-review

Summary

Replaces the prior per-call-wins-wholesale merge of .percy.yml (cliConfig.snapshot) into per-snapshot options with a recursive deep merge: JsonElementToObjectDeep converts config JsonElement objects/arrays into Dictionary<string,object>/List<object>, DeepMerge recurses into nested objects (per-call wins at leaves; arrays/scalars replace), and MergeSnapshotOptions wires the two together. Percy.Snapshot now passes the merged options to isResponsiveSnapshotCapture/CaptureResponsiveDom/getSerializedDom. Adds a nested-merge unit test.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	No secrets touched.
High	Security	Authentication/authorization checks present	N/A	No auth surface in this change.
High	Security	Input validation and sanitization	N/A	Internal option-merge only.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No DB/query code.
High	Correctness	Logic is correct, handles edge cases	Fail	JsonElementToObjectDeep converts config widths array to List<object>; CaptureResponsiveDom (line 774) hard-casts options["widths"] to List<int>. With widths set in .percy.yml + responsive capture, the merged dict now carries List<object> → InvalidCastException. New regression introduced by routing merged config options into the responsive path.
High	Correctness	Error handling is explicit, no swallowed exceptions	Fail	The above InvalidCastException is swallowed by Snapshot's outer catch(Exception) → snapshot silently dropped (logs "Could not take DOM snapshot", returns null) instead of producing a snapshot with default widths as before. Silent functional regression.
High	Correctness	No race conditions or concurrency issues	Pass	Pure synchronous dictionary transforms; static cliConfig read pattern unchanged.
Medium	Testing	New code has corresponding tests	Pass	Adds MergePrecedenceTests (flat precedence + nested deep-merge). Reflection-based but valid; test project targets net10 so CI runs it.
Medium	Testing	Error paths and edge cases tested	Fail	No test exercises a config-seeded array (widths) flowing into CaptureResponsiveDom — the exact path that regresses. Pre-existing test-gap pattern; non-blocking per adjudication.
Medium	Testing	Existing tests still pass (no regressions)	N/A	net10 test project not runnable locally; CI runs it.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Recursion bounded by config depth; negligible.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to SDK merge code.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the JS sdk-utils deep-merge; consistent with existing helpers.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config/option merge.
Low	Quality	Meaningful names, no dead code	Fail	ConvertJsonArray (lines 930–946) is added with zero callers — dead code. It also diverges from JsonElementToObjectDeep (returns List<int> for homogeneous int arrays), and is precisely the coercion that would have prevented the widths regression had it been wired in. Low severity.
Low	Quality	Comments explain why, not what	Pass	Helpers and test carry clear intent comments.
Low	Quality	No unnecessary dependencies added	Pass	Only System.Reflection in the test; no new package deps.

Findings

• File: Percy/Percy.cs:774 (introduced via the merged-options wiring at Percy/Percy.cs:1005)

• Severity: High

• Reviewer: stack:code-review

• Issue: CaptureResponsiveDom casts options["widths"] directly to List<int>. Before this PR it received raw per-call options, where widths was either a caller-supplied List<int> or absent (falling back to GetResponsiveWidths defaults). After this PR it receives mergedOptions, into which MergeSnapshotOptions/JsonElementToObjectDeep copy a .percy.yml-configured widths array as List<object> (boxed ints). The cast then throws InvalidCastException. The exception is caught by Snapshot's outer catch, so the snapshot is silently dropped — a config-driven responsive snapshot that previously worked (with default widths) now produces no snapshot at all whenever widths is set in config rather than per-call.

• Suggestion: Coerce widths regardless of element type at the consumption site, e.g.:

  List<int> widths = new List<int>();
  if (options != null && options.TryGetValue("widths", out var raw)) {
      if (raw is List<int> ints) widths = ints;
      else if (raw is List<object> objs) widths = objs.Select(Convert.ToInt32).ToList();
  }

  (Equivalently, route array conversion through the existing ConvertJsonArray, which already yields List<int> for homogeneous int arrays — and delete it otherwise.)

• File: Percy/Percy.cs:930-946

• Severity: Low

• Reviewer: stack:code-review

• Issue: ConvertJsonArray is added but never called. It also disagrees with JsonElementToObjectDeep's array handling (List<int> vs List<object>), inviting future inconsistency.

• Suggestion: Either delete it, or use it for array conversion inside JsonElementToObjectDeep (which would also fix the widths regression above).

---

Verdict: FAIL — the deep-merge core is correct, but routing config-seeded options into the responsive path introduces a config-triggerable widths cast failure that silently drops responsive snapshots. Coerce widths (or wire in ConvertJsonArray) before merge.

[rishigupta1599]

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

[rishigupta1599]

[Low] Dead code: ConvertJsonArray is never called

ConvertJsonArray is a private static method with no call site. JsonElementToObjectDeep (used by MergeSnapshotOptions) already implements the all-integer→List<int> logic, so this method is unused and may trigger unused-member analyzer warnings.

Suggestion: Delete ConvertJsonArray, or route the array branch of JsonElementToObjectDeep through it to remove the duplication.

Reviewer: stack:code-review

[rishigupta1599]

Claude Code PR Review

PR: #430 • Head: 042d9c8 • Reviewers: stack:code-review

Summary

Adds .percy.yml (cliConfig.snapshot) ⇆ per-snapshot option merging to Percy.Snapshot. A new private MergeSnapshotOptions seeds merged options from config and deep-merges per-call options on top (per-call wins at leaves; nested objects recurse; arrays/scalars replace). The prior regression — config widths being converted to List<object> and then InvalidCastException-ing on the (List<int>)options["widths"] cast in CaptureResponsiveDom — is fixed: JsonElementToObjectDeep now special-cases all-integer arrays to List<int>, so config widths keeps the type the downstream consumer expects. Two reflection-based unit tests exercise config-only inheritance, per-call override precedence, and nested deep-merge.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	N/A	No secrets/credentials in scope.
High	Security	Authentication/authorization checks present	N/A	SDK option-merge; no auth surface.
High	Security	Input validation and sanitization	N/A	Local config deserialization only.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	All-integer arrays → List<int> resolves the widths cast at Percy.cs:774; deep-merge precedence (config seeds, per-call overlays, nested recurse) is correct and matches the JS sdk-utils behavior. Empty/null and mixed-type arrays handled.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	MergeSnapshotOptions wraps config parsing in try/catch and degrades to per-call-only merge, consistent with sibling methods (isResponsiveSnapshotCapture, minHeight reader). Intentional defensive swallow, not a new gap.
High	Correctness	No race conditions or concurrency issues	Pass	Static, synchronous, no shared mutable state introduced beyond existing cliConfig.
Medium	Testing	New code has corresponding tests	Pass	MergePrecedenceTests covers config-only inheritance, per-call override, and nested deep-merge via reflection. Selenium 4 WebDriver is not mockable, so testing the private merge surface directly is a reasonable approach.
Medium	Testing	Error paths and edge cases tested	Partial	Pass — non-blocking gap. No direct test for the all-integer widths → List<int> conversion (the exact prior regression) or the malformed-config catch path. Test gap, not a defect.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Test project targets net10 and runs in CI (can't run locally per adjudication). No changes to existing tests; behavior change is additive.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No queries. Recursion bounded by config depth.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to SDK merge path.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors existing cliConfig JsonElement handling and the JS sdk-utils deep-merge contract.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to config/option merge and the supporting test.
Low	Quality	Meaningful names, no dead code	Fail	ConvertJsonArray (Percy.cs:935) is defined but never called — dead code. JsonElementToObjectDeep supersedes it. Low severity, non-blocking.
Low	Quality	Comments explain why, not what	Pass	Comments explain the precedence rules, the List<int> rationale, and why WebDriver isn't mockable.
Low	Quality	No unnecessary dependencies added	Pass	Only System.Reflection added in the test file; no runtime deps.

Findings

• File: Percy/Percy.cs:935

• Severity: Low

• Reviewer: stack:code-review

• Issue: ConvertJsonArray is a private static method with no call site. JsonElementToObjectDeep (the recursive converter actually used by MergeSnapshotOptions) duplicates its all-integer→List<int> logic, leaving ConvertJsonArray as dead code that may trigger unused-member analyzer warnings.

• Suggestion: Delete ConvertJsonArray (lines 935–951), or route the array branch of JsonElementToObjectDeep through it to remove the duplication.

• File: Percy.Test/Percy.Test.cs:33

• Severity: Low

• Reviewer: stack:code-review

• Issue: Tests cover the merge precedence and nested deep-merge but not the specific all-integer widths → List<int> conversion that caused the prior InvalidCastException, nor the malformed-config catch path. Non-blocking test gap.

• Suggestion: Add a case asserting InvokeMerge returns List<int> for {"snapshot":{"widths":[375,1280]}}, locking in the regression fix.

Adjudication applied: "merged options not in POST body" is intended (the CLI merges server-side); the test project targeting net10 runs in CI rather than locally; pre-existing patterns and test gaps are non-blocking. The prior FAIL is verified fixed — config widths stays List<int> so the (List<int>)options["widths"] cast in CaptureResponsiveDom no longer throws, and the deep-merge precedence remains correct. No new regression.

---

Verdict: PASS