@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, API contract mismatches, tests/docs/audit integrity, and issue scope completeness. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, API correctness, or the accepted issue scope.

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

Claude PR Review

• Gather PR diff and changed-file context
• Read direct_cli/commands/ads.py changes (ShoppingAd / ListingAd)
• Verify WSDL audit + parity gate changes
• Cross-check dry-run tests and CLI help tests
• Check official Yandex Direct API contract for ShoppingAd / ListingAd
• Post final review

Verdict

Looks good — no critical correctness, security, or data-loss bugs found. Payload shapes match WSDL (ShoppingAdAdd/ListingAdAdd use flat arrays, not the {"Items": [...]} wrapper the corresponding *Update types use), subtype allow-listing is wired through FEED_BASED_ADD_FIELDS, and the empty-subtype-no-op invariant is preserved via the missing-fields UsageError in _build_feed_based_ad_add. Audit math is internally consistent (2473→2449 = −24, 745→769 = +24, matching the 24 rows reclassified from missing_followup to supported).

A few non-blocking issues worth flagging:

1. Asymmetric FeedFilterConditions cap between add and update

direct_cli/commands/ads.py:491-495 rejects >30 filters on add:

if feed_filter_conditions:
    if len(feed_filter_conditions) > 30:
        raise click.UsageError(
            f"{container_name}.FeedFilterConditions accepts at most 30 filters."
        )

But _build_feed_based_ad_update at direct_cli/commands/ads.py:430-436 does not enforce the same cap. The WSDL declares maxOccurs="unbounded" for both. Either the 30-cap is a documented API constraint and should apply to both paths, or it is not and should be removed from the add path (let the API return the constraint). Internal asymmetry between add and update for the same nested type will eventually bite. Fix this →

2. DefaultTexts single-value rule is enforced only on add

_build_feed_based_ad_add at direct_cli/commands/ads.py:473-477 rejects len(parsed_default_texts) != 1, but _build_feed_based_ad_update (ads.py:444-446) accepts an unbounded list and forwards it. The "exactly one element" rule is the documented Yandex contract for DefaultTexts, so the add behavior is correct. The update path will silently send a multi-element array if a user passes --default-texts "A,B"; consider mirroring the guard or noting why update is permissive. Fix this →

3. --default-texts cannot represent prose with literal commas

_parse_required_csv_strings in direct_cli/commands/ads.py:299-308 always splits on ,. Combined with the new "exactly one value" guard, any plausible default product text containing a comma — for example "Fast delivery, no fees" — is now rejected with ShoppingAd.DefaultTexts accepts exactly one value. Pre-existing CSV pattern, but ShoppingAd/ListingAd are the first ad subtypes where the field is user-visible prose rather than enum-like tokens. Worth tracking as a follow-up (allow --default-text repeated or accept a single non-split string). Not a blocker.

Other checks

• WSDL parity gate (Pattern C, required nested fields): FeedId and DefaultTexts are minOccurs=1 in ShoppingAdAdd/ListingAdAdd, but get_required_item_fields only inspects the top-level AdAddItem row, so the gate does not require them in INTERNAL_VALIDATION. The PR correctly puts these in OPTIONAL_FIELD_CLI_OPTIONS and uses a body-side UsageError. Coverage test (test_ads_add_shopping_ad_requires_feed_id_and_default_texts) exercises the rejection — good.
• WSDL parity gate (Pattern B, silent data loss): FEED_BASED_ADD_FIELDS is wired into type_fields for both subtypes and --feed-id is plumbed through provided/flag_for. test_ads_add_listing_ad_rejects_other_subtype_flags covers the reverse direction (LISTING_AD + --href). The forward direction (e.g. TEXT_AD + --feed-id) is not directly probed but is structurally covered by _reject_incompatible_flags.
• Audit integrity: OPTIONAL_FIELD_CHILD_PREFIX_FOLLOWUPS entries for ("ads","add","ShoppingAd") and ("ads","add","ListingAd") removed in lockstep with the supported-flip of all 24 audit rows. Counts match.
• Issue scope (ads add: add SHOPPING_AD and LISTING_AD typed flags #275): SHOPPING_AD and LISTING_AD add, top-level ShoppingAd/ListingAd blocks with flat add arrays, required FeedId, single DefaultTexts — matches the PR summary and the audit transitions.
  • branch: codex/issue-275-shopping-listing-add

@codex @claude review. Follow-up after addressing the DefaultTexts comma handling feedback. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, API contract mismatches, tests/docs/audit integrity, and issue scope completeness. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, API correctness, or the accepted issue scope.

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

Claude PR Review (follow-up)

• Pull latest branch state and check diff vs origin/main
• Inspect direct_cli/commands/ads.py — verify DefaultTexts comma handling fix
• Re-verify the previous review's open items
• Cross-check tests/docs/audit integrity after the fix
• Post review

Verdict

The comma-handling fix in c04aa05 is correct. _build_feed_based_ad_add (direct_cli/commands/ads.py:473-480) now treats --default-texts as a single raw string, wraps it in a one-element list, and still enforces "exactly one value" (you can't pass several values at all). The new dry-run tests cover both the positive path (Sale, today → ["Sale, today"]) and the empty-string rejection. Audit math still ties out (2473→2449 = −24 missing_followup; 745→769 = +24 supported). No new correctness, security, or data-loss bugs introduced by the fix commit.

One follow-up from the previous review is now more pronounced and worth a separate ticket, plus the other two from the previous review still stand:

1. Update path silently splits --default-texts on commas (new asymmetry from this fix)

After this fix, add correctly treats --default-texts "Sale, today" as one value. But _build_feed_based_ad_update at direct_cli/commands/ads.py:444-446 still calls _parse_required_csv_strings, and --default-texts help text at ads.py:1469-1472 is still documented as "Comma-separated ShoppingAd/ListingAd.DefaultTexts values".

Result: identical user input (--default-texts "Sale, today") produces different payloads on add vs. update — the update path silently sends ["Sale", "today"] to Yandex with no warning. That's the same data-loss class of bug that was just fixed on add. Out of scope for issue #275 (which scopes ads add), but worth a tracking issue so the symmetric fix lands soon. Fix this →

2. FeedFilterConditions cap is enforced only on add (carryover)

_build_feed_based_ad_add (ads.py:489-493) rejects >30 filters; _build_feed_based_ad_update (ads.py:430-436) does not. WSDL says maxOccurs="unbounded" for both. Either both paths cap or neither does. Not regression-blocking, still asymmetric.

3. Type field correctly omitted from feed-based add payload

Confirmed by test_ads_add_shopping_ad_payload (tests/test_dry_run.py:1312 — assert "Type" not in ad). Matches the documented Yandex shape where the subtype is conveyed by the ShoppingAd/ListingAd key rather than Type.

Other checks (unchanged from previous review)

• WSDL parity gate Pattern A (empty subtype no-op): preserved — _build_feed_based_ad_add raises UsageError before payload construction when --feed-id or --default-texts is missing.
• WSDL parity gate Pattern B (silent data loss): FEED_BASED_ADD_FIELDS is wired into both subtypes' type_fields entries; --feed-id/--default-texts/--feed-filter-condition/--title-sources/--text-sources all flow through provided/flag_for. Forward-direction test (e.g. TEXT_AD + --feed-id) is structurally covered by _reject_incompatible_flags; reverse direction explicitly tested in test_ads_add_listing_ad_rejects_other_subtype_flags.
• WSDL parity gate Pattern C (required nested fields): FeedId and DefaultTexts are minOccurs=1 in ShoppingAdAdd/ListingAdAdd but live below the top-level AdAddItem row that get_required_item_fields inspects, so the gate uses OPTIONAL_FIELD_CLI_OPTIONS + body-side UsageError instead — correctly exercised by test_ads_add_shopping_ad_requires_feed_id_and_default_texts.
• Audit integrity: 24 rows transitioned from missing_followup → supported; counts match exactly. OPTIONAL_FIELD_CHILD_PREFIX_FOLLOWUPS entries for ("ads","add","ShoppingAd") and ("ads","add","ListingAd") removed in lockstep.
• Issue scope (ads add: add SHOPPING_AD and LISTING_AD typed flags #275): matches — SHOPPING_AD/LISTING_AD add, top-level ShoppingAd/ListingAd blocks, flat add arrays, required FeedId, single DefaultTexts.

No blockers for merge of this PR; issue #1 above should be filed as a follow-up (it's out of scope for #275 but newly conspicuous).
• branch: codex/issue-275-shopping-listing-add

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

Review triage:

• FIX: Codex P2 on add DefaultTexts comma handling was addressed in c04aa05; --default-texts "Sale, today" now stays one raw text value and is covered by dry-run tests.
• FOLLOW-UP: Claude follow-up notes about ads update DefaultTexts comma handling and FeedFilterConditions cap are outside ads add: add SHOPPING_AD and LISTING_AD typed flags #275, which is scoped to ads add ShoppingAd/ListingAd. They do not block this PR.
• NO-OP: Claude/Codex final reviews report no blockers; CI is green.