@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, API contract mismatches, and missing tests. 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 3m 44s —— View job

Claude Code Review

Scope per request: critical correctness only — no nitpicks.

• Read PR diff vs. feat/issue-239-optional-fields-gate
• Review direct_cli/commands/adgroups.py
• Review tests/test_dry_run.py
• Review tests/test_wsdl_parity_gate.py
• Review docs/README/audit updates
• Post review

Verdict

No blocking issues. The payload shape, WSDL parity bookkeeping, and dry-run coverage are correct. Three minor observations below, none blocking.

Correctness checks that passed

• Payload shape matches WSDL. tests/wsdl_cache/adgroups.xml declares both NegativeKeywords as general:ArrayOfString and NegativeKeywordSharedSetIds as general:ArrayOfLong; tests/wsdl_cache/imports/general.xsd defines those as { Items: [...] } envelopes. Wrapping the parsed values in {"Items": [...]} (adgroups.py:249,254,348,351) is the right JSON shape. Do not be misled by negativekeywordsharedsets.py:84 which sends a bare list — that resource’s WSDL declares NegativeKeywords as a repeating xsd:string, not an ArrayOfString, so the two shapes are correctly different.
• WSDL parity gate stays consistent. Removing the four missing_followup entries from OPTIONAL_FIELD_AUDIT and adding eight OPTIONAL_FIELD_CLI_OPTIONS entries (container + .Items, ×2 fields × 2 ops) matches the audit-table count delta (2848 → 2840 missing, 393 → 401 supported, Δ=8).
• Top-level placement is correct. Both fields live on AdGroupAddItem / AdGroupUpdateItem (lines 127–128 and 138–139 of adgroups.xml), not on a subtype, so writing to adgroup_data (top-level) rather than into DynamicTextAdGroup/SmartAdGroup is right; nothing in invariant H2 (silent data loss) is broken.
• Empty-payload no-op still rejected. update length check at adgroups.py:358 is updated and the rejection-message test at tests/test_dry_run.py:1769–1772 asserts both new flag names appear in the error. Good.
• No conflict introduced with --type. These flags are subtype-agnostic, so _reject_incompatible_flags correctly does not list them.

Minor observations (non-blocking)

1. update validates IDs outside the try/except (pre-existing pattern, now reused).
   At direct_cli/commands/adgroups.py:349, parse_ids(negative_keyword_shared_set_ids) runs before the try: at line 365. A malformed value like --negative-keyword-shared-set-ids "10,abc" raises a raw ValueError traceback rather than the project’s standard print_error() + click.Abort() path. This already happens for --region-ids on the same command (line 345), so this PR isn’t a regression — but if you want clean errors everywhere, both lines could be moved into the existing try or wrapped to re-raise as click.UsageError. Fix this →

2. No way to clear the list. The WSDL marks NegativeKeywords on AdGroupUpdateItem as nillable="true" (adgroups.xml:127), meaning the API supports clearing by sending null. The current update only sends the field when there’s non-empty content (if parsed_negative_keywords:), so the CLI can add/replace but not clear. This matches the project’s existing convention; if you want a --clear-negative-keywords flag, it’s a separate follow-up. Not a defect in this PR.

3. Replacement semantics are implicit. Yandex Direct’s update for these list fields replaces the entire list rather than appending. Worth a one-line note in the help text for both flags so users don’t accidentally drop existing negatives by passing only the new ones. Documentation polish, not a bug.

Tests

Coverage looks adequate: both add and update paths get a dry-run payload assertion per flag, and the rejection message is asserted to list the new options. The WSDL parity gate change is purely declarative bookkeeping and the four removed missing_followup entries match the eight new OPTIONAL_FIELD_CLI_OPTIONS mappings. No missing-test gaps for critical paths.
· Branch: codex/issue-243-adgroups-negative-keywords

Codex Review: Didn't find any major issues. Bravo.

Addressed the actionable invalid-ID feedback in a9eb191:

• adgroups add/update now wrap --region-ids and --negative-keyword-shared-set-ids parsing with click.UsageError instead of leaking ValueError;
• added dry-run rejection tests for invalid shared-set IDs on add/update and invalid --region-ids on update.

Verification:

• python3 -m pytest tests/test_dry_run.py -k 'adgroups and (negative or region_ids or tracking_params)'
• python3 -m pytest tests/test_wsdl_parity_gate.py
• python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py

@codex @claude review. Please re-check only blocking issues: bugs, contract mismatches, and missing tests.

Claude finished @axisrow's task in 1m 31s —— View job

Claude Code Review — re-check after a9eb191

• Pull latest branch state and diff vs. previous review
• Re-check adgroups.py UsageError wrapping
• Re-check new dry-run rejection tests
• Post verdict

Verdict

No blocking issues. The previously-flagged minor (raw ValueError traceback on bad --region-ids / --negative-keyword-shared-set-ids in update) is now fixed, and the fix is properly tested.

What I verified

Fix is correct and centralized. _parse_ids_option(value, option_name) at direct_cli/commands/adgroups.py:38 wraps parse_ids() and re-raises as click.UsageError(f"{option_name}: {exc}"). It's applied at all four call sites:

• add → --region-ids (line 254), --negative-keyword-shared-set-ids (line 258)
• update → --region-ids (line 354), --negative-keyword-shared-set-ids (line 358)

UsageError propagation is sound in both commands.

• In add, the helper is called inside try:, but the except click.UsageError: raise at line 299 prevents the broad except Exception from swallowing the usage error into print_error + Abort. ✓
• In update, the helper is called before the try: at line 376, so the UsageError propagates directly to Click's exit handler. ✓ (No regression introduced — the empty-payload no-op rejection at line 369 also runs before the try:, same pattern.)

Test coverage is complete and targeted. Three new _rejected() cases at tests/test_dry_run.py:1540, 1739, 1753:

• add with --negative-keyword-shared-set-ids 10,nope
• update with --negative-keyword-shared-set-ids 10,nope
• update with --region-ids 225,nope

Each asserts both the exact prefixed message (--<opt>: Invalid ID: 'nope') and absence of Traceback in the output — directly nailing the failure mode the previous review described.

No contract drift. The fix is purely a CLI surface improvement (exception → UsageError); payload shape, WSDL parity bookkeeping, and COMMAND_WSDL_MAP entries from the previous round are unchanged.

Things I checked and ruled out

• The helper preserves cause via from exc, so any debug-level inspection still gets the chain. Not user-visible noise.
• --region-ids in add was already inside try: in the original PR (not a previous-review concern), but the fix wraps it anyway for consistency. Symmetric and correct.
• No interaction with the empty-payload no-op rejection in update: that check operates on adgroup_data keys, not on raw --*-ids input, and runs after the parsing helpers, so invalid IDs still fail with the new clean error before the no-op check.

Tests / contracts

• No missing critical-path tests. Both add and update parse paths have invalid-ID rejection assertions.
• Replacement-semantics observation from the previous round (Yandex update replaces the whole list rather than appending) is still a doc-polish item, not a defect.

Ship it.
· Branch: codex/issue-243-adgroups-negative-keywords

Codex Review: Didn't find any major issues. More of your lovely PRs please.