[codex] Add unified and smart adgroup flags

[axisrow]

Summary

• add canonical UNIFIED_AD_GROUP support for adgroups add with typed --offer-retargeting
• add typed adgroups update support for SmartAdGroup.AdTitleSource, SmartAdGroup.AdBodySource, and UnifiedAdGroup.OfferRetargeting
• reject mixed subtype update blocks so a single update item cannot combine Smart/Unified/Dynamic/Mobile subtype flags
• update dry-run tests, WSDL parity routing/audit, and single-line README examples

Docs boundary

• adgroups.add lists UnifiedAdGroup.OfferRetargeting and existing SmartAdGroup add fields: https://yandex.com/dev/direct/doc/en/adgroups/add
• adgroups.update lists SmartAdGroup.AdTitleSource, SmartAdGroup.AdBodySource, and UnifiedAdGroup.OfferRetargeting: https://yandex.ru/dev/direct/doc/ru/adgroups/update

Validation

• python3 -m pytest tests/test_dry_run.py -k 'adgroups and (unified or smart)'
• python3 -m pytest tests/test_dry_run.py -k adgroups
• python3 scripts/build_wsdl_optional_field_audit.py --check
• 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
• mypy .
• git diff --check

Closes #283

[axisrow]

@codex @claude review. Focus on critical issues: bugs, API contract mismatches, logical errors, data loss risks, performance problems, missing tests, docs/audit inconsistencies, and issue #283 scope gaps. Do not nitpick cosmetic style unless it affects correctness or maintainability.

[claude]

Claude finished @axisrow's task in 4m 14s —— View job

---

Review of PR #329

• Read PR diff vs origin/main
• Inspect adgroups.py add/update routing
• Check dry-run tests
• Check WSDL parity gate + audit changes
• Verify README and audit consistency
• Cross-check issue adgroups add/update: add UNIFIED_AD_GROUP and SMART_AD_GROUP parity #283 scope

---

Summary

PR looks correct end-to-end. The new UNIFIED_AD_GROUP add path, the SmartAdGroup/UnifiedAdGroup update blocks, and the mixed-subtype-flag rejection are all consistent with the WSDL schema in tests/wsdl_cache/adgroups.xml (lines 317–346, 356–358). Audit deltas (supported 569→576, missing_followup 2649→2642) line up with the four newly covered WSDL paths plus inherited rows. No critical bugs found.

Issue #283 scope — covered

• UnifiedAdGroupAdd.OfferRetargeting (minOccurs=1) → --offer-retargeting, internally enforced via UsageError (direct_cli/commands/adgroups.py:795-799).
• UnifiedAdGroupUpdate.OfferRetargeting (minOccurs=0) → optional --offer-retargeting on update (adgroups.py:1084-1085).
• SmartAdGroupUpdate.AdTitleSource / AdBodySource → --ad-title-source / --ad-body-source on update (adgroups.py:1077-1083).
• _reject_mixed_update_subtype_flags (adgroups.py:386-402) blocks Smart/Unified/Dynamic/Mobile combos within a single update item — good, this is the real data-loss guard since Yandex would silently apply only one block otherwise.
• Audit table drops the four #283 missing_followup entries; counts in tests/WSDL_OPTIONAL_FIELD_AUDIT.md are consistent with the change.

Parity gate — clean

• get_required_item_fields (direct_cli/wsdl_coverage.py:575-590) only collects top-level container item fields, so UnifiedAdGroupAdd.OfferRetargeting's minOccurs=1 doesn't have to be re-listed in INTERNAL_VALIDATION. The internal UsageError is the right pattern here (matches existing SmartAdGroup.FeedId handling).
• OPTIONAL_FIELD_CLI_OPTIONS adds the right add/update entries (tests/test_wsdl_parity_gate.py:690-691, 881-888).

Add-path correctness

• _reject_incompatible_flags (line 693) now sees --offer-retargeting in the universe of subtype flags, and the per-type allowlist only permits it for UNIFIED_AD_GROUP (line 685). Test test_adgroups_add_rejects_unified_flag_for_text_group confirms it bounces for other types — good silent-data-loss guard.
• --region-ids remains globally required=True (line 522), so the unified path inherits the WSDL AdGroupAddItem.RegionIds minOccurs=1 requirement without needing extra plumbing.

Update-path correctness

• Empty-payload guard updated to include the three new flags (line 1095-1096), so direct adgroups update --id N with only a subtype flag is correctly not rejected, while --id N alone still is. Parity-gate Rule 1 stays satisfied.
• Mixed-subtype rejection runs before the per-subtype block builders, so it short-circuits before any payload is built.

---

Non-blocking notes

1. _reject_mixed_update_subtype_flags only reports the first two violating subtypes (adgroups.py:395-401). If three or more subtype groups are passed simultaneously (e.g. --domain-url, --target-device-types, --offer-retargeting), the message names only the first pair. UX-only — the command is still rejected. Worth widening the message to list all populated subtypes for clarity.
   Fix this →

2. Help-text inconsistency (cosmetic): on add, --ad-title-source reads "Smart ad group title source" (adgroups.py:586), while on update it reads "SmartAdGroup.AdTitleSource update value" (adgroups.py:869). Same flag, different style. Aligning them — preferably with the WSDL-typed form on both — would match the convention used by --offer-retargeting (UnifiedAdGroup.OfferRetargeting ...).

3. No nillable clear for SmartAdGroupUpdate.AdTitleSource / AdBodySource. The WSDL marks both nillable="true" (adgroups.xml:343-344), so the documented way to clear either is to send xsi:nil. The PR doesn't expose this — passing an empty string would send "" instead, which is a different semantic. Out of adgroups add/update: add UNIFIED_AD_GROUP and SMART_AD_GROUP parity #283 scope (no other CLI option exposes nillable clearing), but worth tracking as a follow-up so a future user isn't surprised.

4. PAYLOAD_CASES gap (minor). tests/api_coverage_payloads.py doesn't add a UNIFIED_AD_GROUP add case or a Smart/Unified update case. CLAUDE.md's "Strict WSDL parity" rule treats PAYLOAD_CASES as the per-operation parity fixture; since the operations themselves (adgroups.add / adgroups.update) are already represented, this isn't a gate violation, but adding one fixture row per new subtype would make the dry-run JSON shape part of the regression suite alongside the new test_dry_run.py assertions.

Tests

Coverage looks appropriate for the changes:

• positive paths (test_adgroups_add_unified_payload_omits_type, test_adgroups_update_smart_payload_without_type, test_adgroups_update_unified_payload_without_type),
• negative paths (required-field, incompatible-type, mixed-subtype),
• empty-payload guard updated to include all three new flags.

Verdict

LGTM. No correctness blockers; the four findings above are non-critical polish/follow-ups.
· branch: codex/issue-283-unified-smart-adgroups

[Copilot]

Pull request overview

This PR closes parity gaps for adgroups add/update by adding typed CLI flags for Unified and Smart ad group subtype fields, and by enforcing that a single adgroups update item cannot mix subtype-specific update blocks.

Changes:

• Add UNIFIED_AD_GROUP support to adgroups add with typed --offer-retargeting (emits UnifiedAdGroup.OfferRetargeting).
• Add typed adgroups update support for SmartAdGroup.AdTitleSource, SmartAdGroup.AdBodySource, and UnifiedAdGroup.OfferRetargeting, with validation rejecting mixed subtype flag combinations per update item.
• Update dry-run tests, WSDL optional-field audit status/routing, and README single-line examples to reflect the new flags.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
direct_cli/commands/adgroups.py	Adds UNIFIED_AD_GROUP handling and --offer-retargeting; adds Smart/Unified update flags and rejects mixed subtype updates.
tests/test_dry_run.py	Adds dry-run payload/validation tests for unified add, smart/unified updates, and mixed-subtype rejection.
tests/test_wsdl_parity_gate.py	Marks the newly supported WSDL optional paths as covered by the new flags (and removes prior missing-followup routing).
tests/WSDL_OPTIONAL_FIELD_AUDIT.md	Updates the generated audit to reflect newly supported Unified/Smart adgroup optional paths and updated counts.
README.md	Adds concise one-line examples for unified add and smart/unified update flags.

[chatgpt-codex-connector]

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

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[axisrow]

Pushed a follow-up fix for the documented UNIFIED_AD_GROUP endpoint: unified adgroups add/update now route through json/v501/adgroups, with regression tests proving unified payloads use adgroups_v501 and smart payloads keep regular adgroups.

Local verification after the fix:

• python3 -m pytest tests/test_cli.py -k "adgroups and (v501 or unified or smart)"
• python3 -m pytest tests/test_dry_run.py -k "adgroups and (unified or smart)"
• python3 scripts/build_wsdl_optional_field_audit.py --check
• 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
• mypy .
• git diff --check

@codex @claude review. Focus on critical issues: bugs, API contract mismatches, logical errors, data loss risks, performance problems, missing tests, docs/audit inconsistencies, and issue #283 scope gaps. Do not nitpick cosmetic style unless it affects correctness or maintainability.

[claude]

Claude finished @axisrow's task in 4m 15s —— View job

---

Review of v501 routing follow-up (commit 517c84c)

• Inspect new v501 routing in adgroups.py
• Check resource_mapping.py + .pyi stub changes
• Verify smart vs unified path discrimination
• Check new test_cli.py tests
• Cross-check API contract (WSDL vs runtime)
• Verify WSDL parity + audit consistency
• Final verdict

---

Summary

The v501 routing closes the runtime-rejection gap I would have raised on the first pass: unified add/update payloads now hit json/v501/adgroups, smart payloads stay on json/v5/adgroups, and delete/get remain on v5 (correct — v5 WSDL declares UnifiedAdGroupGet/UnifiedAdGroupFieldNames, so reads + ID-only delete work). Sandbox routing is preserved because endpoints.py only switches the host, not the resource path. No correctness blockers.

Critical / API-contract review

1. Routing predicate is correct for today's CLI
   _uses_unified_adgroup_endpoint (direct_cli/commands/adgroups.py:405-418) keys off "UnifiedAdGroup" in adgroup for any item in params.AdGroups. The defensive type checks (isinstance(params, dict), isinstance(adgroups_payload, list)) gracefully fall back to v5 — so if a future caller passes a malformed body the routing is non-fatal.

2. WSDL/runtime mismatch is the actual justification — call it out in code
   The cached WSDL is v5/adgroups (tests/wsdl_cache/adgroups.xml:5) and declares UnifiedAdGroupAdd at line 148, UnifiedAdGroupUpdate at line 358, and UnifiedAdGroupFieldNames at line 378. Per the WSDL alone, unified groups are valid v5 inputs. The PR routes to v501 because Yandex rejects them at the v5 runtime endpoint even though the WSDL allows them — this is the same WSDL-vs-runtime divergence pattern RUNTIME_DEPRECATED_METHODS (direct_cli/wsdl_coverage.py:139-148) was built to document.

   • Add a short comment on _uses_unified_adgroup_endpoint citing the Yandex docs page that says unified add/update must use v501. Otherwise a future maintainer reading wsdl_cache/adgroups.xml will conclude (incorrectly) that v5 should handle it and "simplify" the routing back.
   • Stronger option: register ("adgroups", "add", "UnifiedAdGroup") and ("adgroups", "update", "UnifiedAdGroup") in a sibling structure to RUNTIME_DEPRECATED_METHODS — call it RUNTIME_ROUTED_METHODS — so the rationale lives next to the deprecation registry instead of buried in a command-module helper.
     Fix this →

3. docs URL on adgroups_v501 is identical to v5's
   direct_cli/_vendor/tapi_yandex_direct/resource_mapping.py:11-17 reuses the v5 docs anchor for adgroups_v501. Either point it at the v501-specific section (the PR description's yandex.com/dev/direct/doc/en/adgroups/add would be more accurate), or add a comment noting the docs page covers both versions. As-is, this row is the only reasonable place a future reader would look for "why a separate resource entry exists" and the field is a dead-end.

4. Batch-routing invariant is undocumented
   _uses_unified_adgroup_endpoint uses any(). The CLI builds single-item AdGroups: [adgroup_data] batches today (adgroups.py:837 and :1123), so the invariant holds. If multi-id batching ever lands, a mixed batch (e.g. UnifiedAdGroup + MobileAppAdGroup in the same call) would route the entire payload through v501. If v501 doesn't accept non-unified items, that silently corrupts the non-unified mutation. Suggest either documenting the single-item assumption inline or actively rejecting heterogeneous batches when they arrive.

5. WSDL parity gate has no v501 schema cached
   tests/wsdl_cache/ only contains v5 WSDL. If v501 adds a new minOccurs=1 field (or a new enum value, or renames OfferRetargeting), the parity gate will not catch the drift — it'll keep validating against v5 and the CLI will fail at runtime. Worth filing a follow-up to either:

   • Cache the v501 adgroups WSDL alongside v5 and run parity against both, or
   • Add a documented assertion that v501 adgroups schema is a strict superset of v5 (and grep-test it).
     Not a blocker for adgroups add/update: add UNIFIED_AD_GROUP and SMART_AD_GROUP parity #283 scope, but the moment Yandex evolves v501, this PR's audit confidence drops to zero.

Tests

The four new test_cli.py cases (adgroups.py:120-219 in the diff) cover what matters:

• test_adgroups_v501_resource_mapping_exists_for_unified_groups — pins the resource string and method allowlist.
• test_adgroups_add_unified_uses_v501_endpoint — golden path for add routing + payload shape.
• test_adgroups_update_unified_uses_v501_endpoint — golden path for update routing.
• test_adgroups_add_smart_keeps_v5_endpoint — the negative discrimination that proves routing is targeted, not a default switch.

What's not tested (non-blocking):

• A SmartAdGroup update (only Smart fields, no Unified) staying on v5. The existing test only covers add for smart. Symmetric coverage for update would close the matrix.
• A combined unified-update with --name/--status (i.e. unified subtype + base fields in one payload) to confirm the predicate fires from a nested presence, not from being a single-field payload. The current test sends --offer-retargeting alone, which makes UnifiedAdGroup the only block — coincidentally the same shape the predicate trivially matches.

Audit / docs consistency

• tests/WSDL_OPTIONAL_FIELD_AUDIT.md rows for adgroups.add / adgroups.update UnifiedAdGroup* are still routed to --type / --offer-retargeting — consistent with the v5 WSDL (which is what the audit script reads).
• OPTIONAL_FIELD_CLI_OPTIONS in tests/test_wsdl_parity_gate.py:687-692, 882-888 matches the new flags.
• direct_cli/smoke_matrix.py:77-79 registers only adgroups.add / .update / .delete — not a separate adgroups_v501 smoke entry. Reasonable: smoke is keyed on CLI command, not on transport endpoint. Worth confirming sandbox-write smoke (scripts/test_sandbox_write.sh) doesn't try to exercise a sandbox v501 endpoint that Yandex may not host yet — if the live sandbox doesn't expose json/v501/adgroups, sandbox-write smoke against direct --sandbox adgroups add --type UNIFIED_AD_GROUP … will 404. That's worth verifying before merge.

--dry-run UX (cosmetic)

--dry-run short-circuits before _post_adgroups, so users dry-running --type UNIFIED_AD_GROUP see the body JSON but no indication it would route to v501. The body shape (UnifiedAdGroup block present) is the only hint. Not a bug, but documenting "unified payloads route to json/v501/adgroups" near the help text or README single-line example would close the loop.

Verdict

LGTM on correctness for #283 scope. The four numbered findings above are documentation / future-proofing rather than blockers — the route logic itself is sound and well-covered by the new tests. The most useful follow-up is finding #2 (comment / RUNTIME_ROUTED_METHODS) so the WSDL-vs-runtime rationale doesn't get accidentally optimized away.
· branch: codex/issue-283-unified-smart-adgroups

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[axisrow]

Addressed the non-blocking follow-up from the v501 review:

• documented why unified adgroups route to v501 even though the v5 WSDL declares UnifiedAdGroup
• documented the adgroups_v501 mapping rationale
• added a smart-only update routing regression test so Smart update stays on regular v5
• expanded unified update routing coverage to include a base field plus UnifiedAdGroup

Local verification after this commit:

• python3 -m pytest tests/test_cli.py -k "adgroups and (v501 or unified or smart)"
• python3 -m pytest tests/test_dry_run.py -k "adgroups and (unified or smart)"
• python3 scripts/build_wsdl_optional_field_audit.py --check
• 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
• mypy .
• git diff --check

@codex @claude review. Focus on critical issues: bugs, API contract mismatches, logical errors, data loss risks, performance problems, missing tests, docs/audit inconsistencies, and issue #283 scope gaps. Do not nitpick cosmetic style unless it affects correctness or maintainability.

[claude]

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

---

Review of documentation/test follow-up (commit b78192a)

• Read latest commit diff vs prior review
• Inspect new documentation in adgroups.py and resource_mapping.py
• Verify the new smart-update and base-field+unified routing tests
• Check WSDL audit consistency
• Final verdict

---

Summary

Tight, focused follow-up. Both of the documentation gaps from the prior round are closed and the two test-coverage gaps are filled. No correctness blockers; no new findings of any severity. This is ready to merge from my side.

Verification of prior findings

Prior finding	Status in this commit
#2 — Document WSDL-vs-runtime rationale for v501 routing	Closed via inline comment at direct_cli/commands/adgroups.py:415-417. Explains that v5 WSDL declares UnifiedAdGroup but the runtime requires v501, and also documents the single-item-batch assumption inline (which subsumes my finding #4).
#3 — Document adgroups_v501 mapping rationale	Closed at direct_cli/_vendor/tapi_yandex_direct/resource_mapping.py:13 with a one-line comment. Minor: the comment doesn't include a docs URL — future maintainers would still have to grep adgroups.py for the why. Not worth a follow-up.
Test gap — Smart-only update stays on v5	Closed by test_adgroups_update_smart_keeps_v5_endpoint (tests/test_cli.py:225-256). Asserts both the resource ("adgroups") and that the SmartAdGroup block survives intact.
Test gap — Unified update with base field + subtype block	Closed by extending test_adgroups_update_unified_uses_v501_endpoint to send --name "Updated Unified" alongside --offer-retargeting NO (tests/test_cli.py:177-192). This proves _uses_unified_adgroup_endpoint keys on the nested UnifiedAdGroup block, not on payload shape.

Routing predicate review (unchanged in this commit, re-verified)

_uses_unified_adgroup_endpoint (adgroups.py:405-421) and _post_adgroups (:424-428) — sound:

• Predicate is any(... "UnifiedAdGroup" in adgroup ...) over params.AdGroups. With the new comment, the single-item-batch assumption is now explicit.
• Defensive type guards (isinstance(params, dict), isinstance(adgroups_payload, list)) preserve a benign fallback to v5 for malformed bodies.
• Both call sites (_post_adgroups(client, body) at :852 for add and :1138 for update) flow through the same routing helper — no path skips it.

Audit / parity gate consistency

• tests/WSDL_OPTIONAL_FIELD_AUDIT.md:2744-2747, 2797-2804 rows are unchanged and still correctly marked supported against the new flags. No drift introduced by this commit.
• direct_cli/_vendor/tapi_yandex_direct/tapi_yandex_direct.pyi:136 exposes adgroups_v501(), matching the resource mapping — type-checked path is consistent.
• direct_cli/smoke_matrix.py still registers only adgroups.{add,update,delete,get} and no --type UNIFIED_AD_GROUP invocation in scripts/test_sandbox_write.sh — so the sandbox-v501 404 concern I raised in the previous round remains theoretical (unified-on-sandbox isn't auto-exercised today). Leave as-is unless/until sandbox-write smoke grows a unified case.

Standing follow-ups (still non-blocking, unchanged)

Re-flagging only so they don't get lost — none gate this PR:

1. RUNTIME_ROUTED_METHODS registry — finding Add 1Password auth and simplify build config #2 was addressed with a comment; the structural option (sibling to RUNTIME_DEPRECATED_METHODS in direct_cli/wsdl_coverage.py:139-148) is still open if a third runtime-routing case ever lands. One-off comment is fine for one case.
2. v501 WSDL not cached — parity gate validates against v5 only. If Yandex ships a new minOccurs=1 field or renames OfferRetargeting in v501, the gate won't catch it. Worth a tracking issue when convenient.
3. PAYLOAD_CASES fixtures — tests/api_coverage_payloads.py still has no UNIFIED/Smart-update fixture rows. Not a parity-gate violation (operations are already represented), but adding them would lock the dry-run JSON shapes into the regression suite alongside the existing test_dry_run.py assertions.
4. nillable clear for SmartAdGroup.AdTitleSource / AdBodySource — WSDL nillable="true" (tests/wsdl_cache/adgroups.xml:343-344) is not exposed by the CLI. Out of adgroups add/update: add UNIFIED_AD_GROUP and SMART_AD_GROUP parity #283 scope.

Verdict

LGTM. Documentation and test coverage gaps from the prior round are closed; the routing logic, parity audit, and pyi stub are all internally consistent. Issue #283 scope is fully covered with appropriate guards against silent data loss and mis-routing.
· branch: codex/issue-283-unified-smart-adgroups

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".