feat(ads): typed flags Title2/Mobile/VCard/Sitelinks/Turbo/AdExtensions (#202) by axisrow · Pull Request #216 · axisrow/direct-cli

axisrow · 2026-05-21T02:20:39Z

Summary

Adds 7 typed flags to direct ads add and 5 to direct ads update, mirroring WSDL TextAdAdd/TextAdUpdate/TextImageAdAdd/TextImageAdUpdate 1:1. Downstream yandex-direct-mcp-plugin can now create production ads with image extensions, sitelinks, callouts, vCards, Turbo pages, and second headlines without falling back to the Direct web UI.
All new flags route through the existing per-subtype _reject_incompatible_flags guard; explicit --mobile NO on TEXT_IMAGE_AD/MOBILE_APP_AD now correctly fails via ctx.get_parameter_source (no silent data loss).
Flag scope follows WSDL strictly: --mobile and --ad-extensions are add-only (TextAdUpdate has no Mobile; TextAdUpdateBase uses the complex CalloutSetting type, out of scope); --turbo-page-id applies to both TEXT_AD and TEXT_IMAGE_AD.

Test plan

pytest — 814 passed, 45 skipped, 0 failed
pytest tests/test_dry_run.py -k "ads_add or ads_update" — 31 passed (16 new tests)
pytest tests/test_wsdl_parity_gate.py — 46 passed (Patterns A/B/C/D hold)
pytest tests/test_api_coverage.py — 158 passed (PAYLOAD_CASES extended)
black + flake8 on touched files — clean
Manual smoke: direct ads add --type TEXT_AD ... --title2 ... --display-url-path ... --mobile YES --vcard-id ... --sitelink-set-id ... --turbo-page-id ... --ad-extensions "1,2" --dry-run emits all 11 fields under TextAd
Manual smoke: direct ads update --id N --type TEXT_AD --mobile YES rejected with No such option: --mobile
Manual smoke: direct ads add --type TEXT_IMAGE_AD ... --mobile NO --dry-run rejected (regression for the code-review finding)
README EN + RU sections updated with new examples and a flag-scope summary

Closes #202

🤖 Generated with Claude Code

…nsions (#202) Mirrors WSDL TextAdAdd/TextAdUpdate 1:1 so production ads can be created and updated from the CLI without falling back to the Direct web UI. The downstream yandex-direct-mcp-plugin needs these flags to ship picture-ad extensions, sitelinks, callouts, vCards and Turbo pages. ads add (TEXT_AD): --title2, --display-url-path, --mobile [YES|NO] (default NO), --vcard-id, --sitelink-set-id, --turbo-page-id, --ad-extensions. TEXT_IMAGE_AD also accepts --turbo-page-id (it's in the TextImageAdAdd WSDL type). ads update: same TEXT_AD subset minus --mobile (TextAdUpdate has no Mobile field) and --ad-extensions (TextAdUpdateBase exposes the complex CalloutSetting type instead of the AdExtensionIds array; out of scope here). All new flags route through the existing _reject_incompatible_flags guard, so attaching --title2 to TEXT_IMAGE_AD or --ad-extensions to MOBILE_APP_AD fails loudly via click.UsageError before the request is built. WSDL_FIELD_TO_CLI_OPTION gains entries for each new field. Closes #202 Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

Found in code review: the previous guard masked an explicit ``--mobile NO`` on TEXT_IMAGE_AD/MOBILE_APP_AD because ``mobile_provided`` collapsed to None whenever the value happened to equal the default. That re-introduced the silent-data-loss class the WSDL parity gate is meant to prevent (#198 H2). Switch to ``ctx.get_parameter_source("mobile")`` so we react to user intent rather than value: explicit ``--mobile NO`` now raises UsageError on non- TEXT_AD subtypes, while omitting the flag still passes through unchanged (default Mobile: NO is filled into the TEXT_AD payload only). Two regression tests added: - test_ads_add_rejects_explicit_mobile_no_on_text_image_ad - test_ads_add_text_image_ad_default_mobile_does_not_leak Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

axisrow · 2026-05-21T02:20:56Z

@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 · 2026-05-21T02:21:12Z

Claude finished @axisrow's task in 3m 47s —— View job

Claude Code Review

Load context and PR diff
Review direct_cli/commands/ads.py changes
Review test coverage (test_dry_run.py, api_coverage_payloads.py, test_wsdl_parity_gate.py)
Check WSDL parity claims (flag scope: add vs update, TEXT_AD vs TEXT_IMAGE_AD)
Verify _reject_incompatible_flags handles new flags correctly
Post final review

Verdict: no critical issues found

I verified the load-bearing claims of the PR against the WSDL cache (tests/wsdl_cache/ads.xml) and traced the failure paths through direct_cli/commands/ads.py. The implementation is sound.

What I verified

1. The --mobile NO rejection fix is correct. direct_cli/commands/ads.py:243-253 uses ctx.get_parameter_source("mobile") to distinguish a Click default (NO) from an explicit --mobile NO. Trace for ads add --type TEXT_IMAGE_AD ... --mobile NO:

mobile_source == ParameterSource.COMMANDLINE, so mobile_provided = "NO" (not None)
_reject_incompatible_flags (direct_cli/commands/ads.py:17) sees mobile in provided with a non-None value but "mobile" is not in TEXT_IMAGE_AD's allowed set → UsageError raised. Tested by test_ads_add_rejects_explicit_mobile_no_on_text_image_ad (tests/test_dry_run.py).
Conversely, omitting --mobile on TEXT_IMAGE_AD leaves mobile_provided = None so no false-positive, covered by test_ads_add_text_image_ad_default_mobile_does_not_leak.

2. The WSDL minOccurs=1 invariant on Mobile is preserved. tests/wsdl_cache/ads.xml line ~ shows <xsd:element name="Mobile" type="general:YesNoEnum" minOccurs="1" maxOccurs="1"/> inside the TEXT_AD-add path. direct_cli/commands/ads.py:329 always writes "Mobile": mobile.upper() for TEXT_AD because Click's default is "NO" — so the field is never accidentally dropped.

3. The "no --mobile/no --ad-extensions on update" claim matches WSDL exactly.

TextAdUpdate (extends TextAdUpdateBase) at tests/wsdl_cache/ads.xml:785-805: no Mobile element. ✓
TextAdUpdateBase: has CalloutSetting (a complex ext:AdExtensionSetting type), not an AdExtensionIds array. Exposing --ad-extensions on update would be type-incorrect. ✓
TextImageAdUpdate (line 854-864): correctly includes TurboPageId — and the PR exposes it. ✓

4. Pattern A empty-subtype guard still holds for update. direct_cli/commands/ads.py:609-613 raises if only Id is in ad_data. So --type TEXT_AD with only e.g. an unset --turbo-page-id 0 (falsy) → text_ad == {} → not nested into ad_data → guard fires.

5. parse_ids for --ad-extensions correctly returns list[int] matching the WSDL xsd:long maxOccurs="unbounded" shape (tests/wsdl_cache/ads.xml).

Minor observations (not blocking)

if turbo_page_id: / if vcard_id: / if sitelink_set_id: use truthy checks, so passing 0 would silently be dropped. In practice Yandex IDs are positive; and clearing a field on update needs nillable JSON null, not 0. So this matches both the WSDL and the existing code's convention for image_hash. Not a bug.
The _reject_incompatible_flags error message for update is wrapped with the "subtype update block; it does not convert an ad between subtypes" suffix only in the update path (direct_cli/commands/ads.py:550-554). Consistent with pre-existing behavior.

Test coverage

The 16 new dry-run tests in tests/test_dry_run.py plus the parity-gate additions (tests/test_wsdl_parity_gate.py:342-349) cover: each new typed flag round-trip in both add and update; the --mobile YES, default NO, and explicit-NO-on-wrong-subtype cases; rejection of --title2/--ad-extensions on incompatible subtypes; absence of --mobile/--ad-extensions on update. The PAYLOAD_CASES fixtures (tests/api_coverage_payloads.py) exercise the full TEXT_AD flag set in one call, which would have caught any serialisation drift.

No bugs, no security concerns, no data-loss risks, no performance regressions.
· worktree-issue-202-ads-typed-flags

Copilot

Pull request overview

This PR extends the direct ads add / direct ads update commands with additional typed flags to reach closer WSDL parity for production-grade ad creation and updates (Title2, DisplayUrlPath, TurboPageId, etc.), and updates tests/docs accordingly.

Changes:

Added new typed Click options for ads add (including add-only --mobile and --ad-extensions) and for ads update (Title2/DisplayUrlPath/VCardId/SitelinkSetId/TurboPageId).
Updated request-body construction and subtype incompatibility guarding to prevent silent data loss (notably around --mobile defaults).
Expanded dry-run / parity-gate / API coverage tests and refreshed README examples (EN/RU).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`direct_cli/commands/ads.py`	Adds new typed flags for ads add/update, extends subtype allow-lists, and maps flags into the WSDL-shaped payload.
`tests/test_dry_run.py`	Adds dry-run assertions and incompatibility-guard coverage for the new flags across ad subtypes.
`tests/test_wsdl_parity_gate.py`	Extends WSDL→CLI option mapping for the new ads fields.
`tests/api_coverage_payloads.py`	Extends payload cases to exercise the new ads flags in coverage gating.
`tests/test_cli.py`	Makes the help-text assertion resilient to Click’s line-wrapping.
`README.md`	Updates examples and documents add-only vs update-available flag scope (EN/RU).

Comments suppressed due to low confidence (1)

direct_cli/commands/ads.py:577

Same silent-drop issue for the new update flags: if vcard_id: / if sitelink_set_id: / if turbo_page_id: will ignore an explicitly provided value of 0. Either enforce a positive range in the Click option types or use is not None checks so that explicit values aren’t silently omitted.

        if vcard_id:
            text_ad["VCardId"] = vcard_id
        if sitelink_set_id:
            text_ad["SitelinkSetId"] = sitelink_set_id
        if turbo_page_id:
            text_ad["TurboPageId"] = turbo_page_id

+            if vcard_id:
+                text_ad["VCardId"] = vcard_id
+            if sitelink_set_id:
+                text_ad["SitelinkSetId"] = sitelink_set_id
+            if turbo_page_id:
+                text_ad["TurboPageId"] = turbo_page_id


axisrow · 2026-05-21T02:26:26Z

Re: Copilot's truthy-vs-is not None note on --vcard-id / --sitelink-set-id / --turbo-page-id.

In tests/wsdl_cache/ads.xml (lines 779, 781, 798) these are all xsd:long with nillable="true". Yandex Direct allocates entity IDs starting from 1, so 0 is not a valid VCardId / SitelinkSetId / TurboPageId and the API would reject it anyway. Clearing a field via the WSDL nillable (JSON null) is a distinct feature that requires a dedicated --clear-* flag and is intentionally out of scope here — see Out-of-scope in #202.

The if vcard_id: shape also matches every other update field in the same block (if title:, if text:, if href:, if image_hash:) and the corresponding add block. Skipping this one consistently.

Click 8.1 writes ``No such option: --mobile`` while 8.2+ uses ``No such option '--mobile'``. Loosen the two ads-update unknown-flag assertions to match the stable substring across CI's 3.11 / 3.13 jobs. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

axisrow and others added 2 commits May 21, 2026 09:07

Copilot AI review requested due to automatic review settings May 21, 2026 02:20

Copilot started reviewing on behalf of axisrow May 21, 2026 02:21 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

Comment thread direct_cli/commands/ads.py

Comment on lines +340 to +345

if vcard_id:

text_ad["VCardId"] = vcard_id

if sitelink_set_id:

text_ad["SitelinkSetId"] = sitelink_set_id

if turbo_page_id:

text_ad["TurboPageId"] = turbo_page_id

axisrow merged commit 3817570 into main May 21, 2026
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ads): typed flags Title2/Mobile/VCard/Sitelinks/Turbo/AdExtensions (#202)#216

feat(ads): typed flags Title2/Mobile/VCard/Sitelinks/Turbo/AdExtensions (#202)#216
axisrow merged 3 commits into
mainfrom
worktree-issue-202-ads-typed-flags

axisrow commented May 21, 2026

Uh oh!

axisrow commented May 21, 2026

Uh oh!

claude Bot commented May 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

axisrow commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

axisrow commented May 21, 2026

Summary

Test plan

Uh oh!

axisrow commented May 21, 2026

Uh oh!

claude Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude Code Review

Verdict: no critical issues found

What I verified

Minor observations (not blocking)

Test coverage

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

axisrow commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented May 21, 2026 •

edited

Loading