@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

Claude finished @axisrow's task in 2m 24s —— View job

Review — no critical issues found ✅

Scope per request: bugs, security, logic errors, data-loss risk, performance. Style / naming / nits skipped intentionally.

• Read changed files
• Diff against origin/main
• Verify mutex logic, FieldNames validation, edge cases
• Cross-check WSDL fixture
• Post review

What I checked

1. Mutex enforcement (direct_cli/commands/changes.py:59-70)
The "exactly one of" check happens before the try/except Exception wrapper, so users get a clean UsageError (exit 2 with Usage hint) instead of Abort (exit 1 + colored stderr). Counter-style sources_used correctly handles 0, 2, and 3 simultaneous flags. ✅

2. FieldNames enum (changes.py:17, :72-85)
_CHECK_FIELD_NAMES = {"CampaignIds", "AdGroupIds", "AdIds", "CampaignsStat"} is byte-identical to the WSDL CheckFieldEnum enumeration at tests/wsdl_cache/changes.xml:67-74. Validation runs against this enum before the API call, which is exactly the behavior the PR description promises. ✅

3. Order of validation
mutex → fields → ID parsing → API call. This order is correct: it surfaces the most actionable user-facing error first (which flag combo to pass), then field typos, then parse errors. ✅

4. Edge cases

• --fields , → f.strip() filter yields [] → explicit UsageError("empty list...") instead of WSDL minOccurs=1 violation server-side. ✅
• --campaign-ids , → parse_ids raises ValueError("Invalid ID: ''. ..."), caught at changes.py:97 and re-raised as UsageError (exit 2). ✅
• --fields "CampaignIds, ,CampaignsStat" (whitespace token between commas) → stripped + filtered → passes correctly.
• Case-sensitive comparison against enum — typos like campaignids rejected. Matches API behavior; correct. ✅

5. Fixture correction (tests/test_low_coverage_payloads.py:66-78)
Old fixture used FieldNames=["CampaignId", "ChangesIn"] — neither is a valid CheckFieldEnum member; those are response-payload field names. New fixture ["CampaignIds", "CampaignsStat"] is WSDL-correct. This change was load-bearing — without it, the new validation would have made that test fail. The fix is right. ✅

6. Integration test (tests/test_integration.py:609-623)
Still uses --campaign-ids only — passes mutex (exactly one filter). No regression. ✅

7. Backward compatibility
The only previously-supported invocation was --campaign-ids <ids>. Still works identically. Two new flags are additive. No breaking change for existing callers. ✅

Things that are not bugs but worth noting

• field_names = get_default_fields("changes") returns a live reference to COMMON_FIELDS["changes"], not a copy. If tapi_yandex_direct ever mutated params["FieldNames"] during serialization, it would corrupt the module-level default for subsequent calls. This is pre-existing behavior in direct_cli/utils.py:598 and affects every command using get_default_fields, not just this PR. Out of scope for this fix, but worth a follow-up if anyone audits utils.py.
• The new mutex tests assert result.exit_code != 0 rather than == 2. The PR description explicitly claims "exit code 2" is the contract being established (vs. legacy exit 1). Tests would pass even if Click silently changed to exit 1. Tightening to == 2 would lock in the intent — but this is a test-strictness suggestion, not a production risk.

Summary

Clean fix. The WSDL-derived enum is correctly extracted, the mutex matches the documented Yandex Direct contract, and the order of UsageError checks (mutex before try/except) gives the right exit code. The corrected fixture in test_low_coverage_payloads.py is necessary and accurate.

No blocking issues. Safe to merge.

--- · worktree-fix-228-changes-mutex-ids