feat(campaigns): typed flags for CPA strategies (Goals, Notification, TimeTargeting) (#204)#219
Conversation
… TimeTargeting) Adds typed flags to `direct campaigns add` for issue #204: - `--goal-id` / `--priority-goals goal_id:value,…` — single goal vs. WSDL `TextCampaignAddItem.PriorityGoals.Items[]` (PriorityGoalsItem with GoalId/Value, not Id/Weight). - `--average-cpa MICRO_RUBLES`, `--bid-ceiling MICRO_RUBLES`, `--counter-ids` for Text/Dynamic. - `--notification JSON` (CampaignBase.Notification with SmsSettings/EmailSettings shape validation). - `--time-targeting JSON` (CampaignAddItem.TimeTargeting with HolidaysSchedule shape validation). Strategy-subtype compatibility enforced via UsageError at CLI: * --average-cpa rejected outside AVERAGE_CPA / AVERAGE_CPA_PER_CAMPAIGN * --bid-ceiling rejected for subtypes without BidCeiling (PayForConversionCrr, PayForConversionMultipleGoals) * --priority-goals rejected outside *_MULTIPLE_GOALS subtypes * --counter-ids rejected for SMART_CAMPAIGN (uses singular --counter-id) Issue-requested `--goals` (array) and `--network-settings` intentionally NOT shipped: WSDL has no Goals array (only scalar GoalId, multi-goal via PriorityGoals) and no NetworkSettings under CampaignAddItem. CHANGELOG documents this. Closes #204. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@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 4m 38s —— View job Claude PR Review — Critical Issues Only
I focused only on bugs, logical errors, and data-loss / API-rejection risks — not style. 1.
|
| Invocation | What CLI sends | WSDL says |
|---|---|---|
--search-strategy AVERAGE_CPA (no --average-cpa, no --goal-id) |
Search: {BiddingStrategyType: "AVERAGE_CPA"} — no AverageCpa subblock at all (_place only writes the block if any field landed in it, campaigns.py:198) |
AverageCpa and GoalId both minOccurs=1 on StrategyAverageCpaAdd |
--search-strategy AVERAGE_CPA_MULTIPLE_GOALS (no --priority-goals) |
Search.AverageCpaMultipleGoals = {} and TextCampaign.PriorityGoals absent |
PriorityGoalsArray.Items is minOccurs=1 maxOccurs=unbounded once PriorityGoals is used; without PriorityGoals the multi-goal strategy is unusable |
--search-strategy PAY_FOR_CONVERSION_MULTIPLE_GOALS (no --priority-goals) |
Same | Same |
--search-strategy AVERAGE_CPA_PER_CAMPAIGN / AVERAGE_CPA_PER_FILTER (any combo) |
See issue #3 below | n/a |
The "guarantee" in the PR description is false. The fix is one of:
- Mark
_STRATEGY_REQUIRES_PRIORITY_GOALSsubtypes as actually requiring--priority-goals(raiseUsageErrorif absent). - For
AverageCpa/PayForConversionCrr, require--goal-id(and--average-cpa/--crr) when the strategy is chosen. - Document these as
INTERNAL_VALIDATIONentries intests/test_wsdl_parity_gate.pyso the gate covers them.
This is the most likely real-world failure mode — a user picks the strategy, forgets one of the typed flags, and gets a confusing API error from the wire instead of a clear CLI message. Fix this →
3. _NETWORK_STRATEGY_TO_WSDL_SUBTYPE maps to subtypes that don't exist on TextCampaign / DynamicTextCampaign
direct_cli/commands/campaigns.py:36–40:
_NETWORK_STRATEGY_TO_WSDL_SUBTYPE = {
"AVERAGE_CPA_PER_CAMPAIGN": "AverageCpaPerCampaign",
"AVERAGE_CPA_PER_FILTER": "AverageCpaPerFilter",
}But TextCampaignStrategyAddBase and DynamicTextCampaignStrategyAddBase (the bases used by TextCampaignNetworkStrategyAdd / DynamicTextCampaignNetworkStrategyAdd) do not declare AverageCpaPerCampaign or AverageCpaPerFilter fields — those exist only on SmartCampaignStrategyAddBase. And the SMART_CAMPAIGN branch (campaigns.py:718) doesn't call _apply_cpa_strategy_fields.
So today: passing --type TEXT_CAMPAIGN --network-strategy AVERAGE_CPA_PER_CAMPAIGN --goal-id 5 --average-cpa 1000000 builds
"Network": {
"BiddingStrategyType": "AVERAGE_CPA_PER_CAMPAIGN",
"AverageCpaPerCampaign": {"AverageCpa": 1000000, "GoalId": 5}
}— a key that isn't in the WSDL for TextCampaignNetworkStrategyAdd. Either:
- delete these two entries from
_NETWORK_STRATEGY_TO_WSDL_SUBTYPE(they only ever apply to SmartCampaign, which doesn't route here), or - if the intent was to support these for SmartCampaign network strategy, wire
_apply_cpa_strategy_fieldsinto the SMART_CAMPAIGN branch and gate accordingly. Fix this →
Looked at but not flagged
parse_priority_goals_spec(utils.py) — handles bad-shape, empty, non-int, trailing-comma, no-separator. Errors all surface as--priority-goals. ✓_validate_notification_shape/_validate_time_targeting_shape— type-checked, empty rejected, unknown top-level / nested keys rejected. ✓BidCeilingcorrectly excluded fromStrategyPayForConversionCrrAddandStrategyPayForConversionMultipleGoalsAdd(the two PR-noted WSDL audit catches). ✓PriorityGoalsplaced on the TextCampaign / DynamicTextCampaign block, not on the strategy — matchesTextCampaignAddItem.PriorityGoalsin WSDL. ✓--counter-idsaccepted on Text/Dynamic, rejected for SMART_CAMPAIGN (singular--counter-idonly). ✓- No secrets, injection vectors, or destructive paths introduced.
Summary
Three real issues, all in the strategy-construction code. Issue #1 (PAY_FOR_CONVERSION_CRR with no --crr) and issue #2 (incomplete CPA subblocks pass silently) directly contradict the PR's "invalid combinations never reach the API" promise — downstream MCP callers will hit API rejections that the CLI is supposed to prevent. Issue #3 is dead/misleading code that produces an invalid payload if a user trips into it.
• Branch: worktree-issue-204-campaigns-cpa-flags
CI quality job's `test_readme_direct_examples_are_single_line_canonical_commands` rejects backslash line-continuations in README `direct ...` examples. Refold the new CPA / multi-goal / Notification+TimeTargeting examples into single lines (matching surrounding examples). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Adds new typed CLI flags to direct campaigns add to support CPA-oriented bidding strategies and campaign-level Notification / TimeTargeting blocks, aiming to prevent invalid combinations from reaching the API and to improve downstream automation compatibility.
Changes:
- Added new
campaigns addoptions (--goal-id,--priority-goals,--average-cpa,--bid-ceiling,--counter-ids,--notification,--time-targeting) and new payload composition/validation helpers. - Introduced
parse_priority_goals_spec()indirect_cli/utils.pyforgoal_id:value,…parsing into WSDLPriorityGoalsItem[]. - Expanded dry-run and WSDL-coverage payload fixtures/tests plus README + changelog documentation for the new surfaces.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
direct_cli/commands/campaigns.py |
Adds new Click flags, CPA-strategy payload injection, and JSON shape validators for Notification/TimeTargeting. |
direct_cli/utils.py |
Adds parse_priority_goals_spec() helper for PriorityGoals parsing. |
tests/test_dry_run.py |
Adds dry-run tests for new flags, payload shapes, and invalid combinations/inputs. |
tests/api_coverage_payloads.py |
Adds new campaigns add payload cases for WSDL parity/coverage gate. |
README.md |
Documents new CPA/Notification/TimeTargeting usage examples (EN + RU). |
CHANGELOG.md |
Documents the new campaigns add typed flags and notes about issue/WSDL deviations. |
Comments suppressed due to low confidence (4)
README.md:361
- These new examples use shell line continuations (
\) and multi-line commands, but this README section explicitly documents a contract that canonical examples must be single-line (and disallows line continuations). Please rewrite the added commands as single-line examples to match the documented CLI/README contract.
This issue also appears on line 1044 of the same file.
# CPA strategy (single goal): --goal-id required, --average-cpa / --bid-ceiling are micro-rubles
direct campaigns add --name "CPA Campaign" --start-date 2026-06-01 --type TEXT_CAMPAIGN --search-strategy AVERAGE_CPA --network-strategy SERVING_OFF --goal-id 1234567 --average-cpa 500000000 --bid-ceiling 1000000000 --counter-ids 111,222 --dry-run
# Multi-goal CPA via PriorityGoals (goal_id:value pairs, WSDL PriorityGoalsItem)
direct campaigns add --name "Multi-Goal CPA" --start-date 2026-06-01 --type TEXT_CAMPAIGN --search-strategy AVERAGE_CPA_MULTIPLE_GOALS --network-strategy SERVING_OFF --priority-goals 1234567:80,9876543:20 --bid-ceiling 1000000000 --dry-run
# Notification (Sms/Email) and TimeTargeting accept JSON with WSDL CamelCase keys
direct campaigns add --name "Notify+Schedule" --start-date 2026-06-01 --type TEXT_CAMPAIGN --search-strategy HIGHEST_POSITION --network-strategy SERVING_OFF --notification '{"EmailSettings":{"Email":"[email protected]","SendWarnings":"YES"}}' --time-targeting '{"Schedule":["1A0123456789ABCDEFGHIJKL"],"ConsiderWorkingWeekends":"YES"}' --dry-run
# Update / lifecycle
direct campaigns update --id 12345 --name "New Name" --status SUSPENDED --budget 100000000 --start-date 2024-02-10 --end-date 2024-03-01
direct campaigns suspend --id 12345
direct campaigns resume --id 12345
direct campaigns archive --id 12345
direct campaigns unarchive --id 12345
direct campaigns delete --id 12345
**README.md:1061**
* Same as the English section: these added Russian examples use shell line continuations (`\`) and multi-line commands, but the README’s documented contract says canonical examples must be single-line and must not use line continuations. Please rewrite these examples as single-line commands.
direct campaigns update --id 12345 --name "Новое название" --status SUSPENDED --budget 100000000 --start-date 2024-02-10 --end-date 2024-03-01
direct campaigns suspend --id 12345
direct campaigns resume --id 12345
direct campaigns archive --id 12345
direct campaigns unarchive --id 12345
direct campaigns delete --id 12345
#### Группы объявлений
```bash
direct adgroups get --campaign-ids 1,2,3 --limit 50
direct adgroups add --name "Группа 1" --campaign-id 12345 --region-ids 1,225 --dry-run
direct adgroups add --name "Динамическая группа" --campaign-id 12345 --type DYNAMIC_TEXT_AD_GROUP --region-ids 1,225 --domain-url example.com --dry-run
direct adgroups add --name "Смарт-группа" --campaign-id 12345 --type SMART_AD_GROUP --region-ids 1,225 --feed-id 170 --ad-title-source FEED_NAME --ad-body-source FEED_NAME --dry-run
direct adgroups update --id 67890 --name "Новое название" --status SUSPENDED --region-ids 1,225
direct adgroups delete --id 67890
**README.md:361**
* The README’s “Documentation Contract” section says invalid examples include command lines that pass raw JSON flags, but the added `--notification` / `--time-targeting` examples embed raw JSON strings. Either adjust that contract text (to allow these specific JSON-typed flags) or change the examples to avoid raw JSON in canonical snippets.
This issue also appears on line 1056 of the same file.
direct campaigns resume --id 12345
direct campaigns archive --id 12345
direct campaigns unarchive --id 12345
direct campaigns delete --id 12345
README.md:1061
- Same issue in the Russian examples: the README contract states raw JSON flags are invalid in canonical snippets, but the new
--notification/--time-targetingexample includes inline JSON strings. Consider updating the contract text or changing the example format.
direct adgroups add --name "Группа 1" --campaign-id 12345 --region-ids 1,225 --dry-run
direct adgroups add --name "Динамическая группа" --campaign-id 12345 --type DYNAMIC_TEXT_AD_GROUP --region-ids 1,225 --domain-url example.com --dry-run
direct adgroups add --name "Смарт-группа" --campaign-id 12345 --type SMART_AD_GROUP --region-ids 1,225 --feed-id 170 --ad-title-source FEED_NAME --ad-body-source FEED_NAME --dry-run
direct adgroups update --id 67890 --name "Новое название" --status SUSPENDED --region-ids 1,225
direct adgroups delete --id 67890
</details>
| if not value: | ||
| return None |
| def _validate_time_targeting_shape(time_targeting: object) -> None: | ||
| """Reject malformed --time-targeting JSON before sending to API.""" | ||
| if not isinstance(time_targeting, dict) or not time_targeting: | ||
| raise click.UsageError( | ||
| "--time-targeting must be a non-empty JSON object with " | ||
| f"keys from {sorted(_TIME_TARGETING_KEYS)}" | ||
| ) | ||
| unknown = set(time_targeting) - _TIME_TARGETING_KEYS | ||
| if unknown: | ||
| raise click.UsageError( | ||
| f"--time-targeting contains unknown key(s) {sorted(unknown)}; " | ||
| f"allowed: {sorted(_TIME_TARGETING_KEYS)}" | ||
| ) | ||
| holidays = time_targeting.get("HolidaysSchedule") | ||
| if holidays is not None: | ||
| if not isinstance(holidays, dict): | ||
| raise click.UsageError( | ||
| "--time-targeting.HolidaysSchedule must be a JSON object" | ||
| ) | ||
| unknown_h = set(holidays) - _HOLIDAYS_SCHEDULE_KEYS | ||
| if unknown_h: | ||
| raise click.UsageError( | ||
| "--time-targeting.HolidaysSchedule contains unknown " | ||
| f"key(s) {sorted(unknown_h)}; allowed: " | ||
| f"{sorted(_HOLIDAYS_SCHEDULE_KEYS)}" | ||
| ) | ||
|
|
| # WSDL: BiddingStrategyType enum value → Strategy*Add subtype field name | ||
| # in TextCampaignSearch/Network/SmartCampaign… containers. | ||
| # Only CPA-shaped subtypes that accept --average-cpa / --goal-id / | ||
| # --bid-ceiling / --priority-goals are listed; legacy types | ||
| # (HIGHEST_POSITION etc.) do not carry these fields and must not get | ||
| # a nested subtype block at all. | ||
| _SEARCH_STRATEGY_TO_WSDL_SUBTYPE = { | ||
| "AVERAGE_CPA": "AverageCpa", | ||
| "PAY_FOR_CONVERSION_CRR": "PayForConversionCrr", | ||
| "AVERAGE_CPA_MULTIPLE_GOALS": "AverageCpaMultipleGoals", | ||
| "PAY_FOR_CONVERSION_MULTIPLE_GOALS": "PayForConversionMultipleGoals", | ||
| } | ||
| _NETWORK_STRATEGY_TO_WSDL_SUBTYPE = { | ||
| # Smart-only subtype lives on Network side as well. | ||
| "AVERAGE_CPA_PER_CAMPAIGN": "AverageCpaPerCampaign", | ||
| "AVERAGE_CPA_PER_FILTER": "AverageCpaPerFilter", | ||
| } | ||
|
|
||
| _STRATEGY_SUPPORTS_AVERAGE_CPA = { | ||
| "AverageCpa", | ||
| "AverageCpaPerCampaign", | ||
| } | ||
| _STRATEGY_SUPPORTS_GOAL_ID = { | ||
| "AverageCpa", | ||
| "PayForConversionCrr", | ||
| "AverageCpaPerCampaign", | ||
| "AverageCpaPerFilter", | ||
| } | ||
| _STRATEGY_SUPPORTS_BID_CEILING = { | ||
| "AverageCpa", | ||
| "AverageCpaPerCampaign", | ||
| "AverageCpaPerFilter", | ||
| "AverageCpaMultipleGoals", | ||
| } | ||
| _STRATEGY_REQUIRES_PRIORITY_GOALS = { | ||
| "AverageCpaMultipleGoals", | ||
| "PayForConversionMultipleGoals", | ||
| } |
| # Single-goal CPA strategies must reject --priority-goals; | ||
| # only *_MULTIPLE_GOALS subtypes carry PriorityGoals. | ||
| if priority_goals_items is not None: | ||
| chosen_subtype = search_subtype or network_subtype | ||
| if chosen_subtype not in _STRATEGY_REQUIRES_PRIORITY_GOALS: | ||
| raise click.UsageError( | ||
| "--priority-goals is only valid with " | ||
| "AVERAGE_CPA_MULTIPLE_GOALS / " | ||
| "PAY_FOR_CONVERSION_MULTIPLE_GOALS strategies; " | ||
| f"got --search-strategy={search_strategy!r}, " | ||
| f"--network-strategy={network_strategy!r}" | ||
| ) | ||
| sub_campaign_block["PriorityGoals"] = {"Items": priority_goals_items} |
| f"got --search-strategy={search_strategy!r}, " | ||
| f"--network-strategy={network_strategy!r}" | ||
| ) | ||
|
|
…network map Address all three @claude review findings on PR #219: 1. PAY_FOR_CONVERSION_CRR was advertising itself but had no way to populate the WSDL-required Crr field. Add --crr (xsd:int) with gating: rejected outside PAY_FOR_CONVERSION_CRR. 2. Add CLI-level minOccurs=1 enforcement via _STRATEGY_REQUIRED_TYPED_FLAGS. Picking a CPA strategy without the required typed flags now raises UsageError up-front instead of silently sending a half-configured payload that the API would reject. Affects AVERAGE_CPA (needs --average-cpa + --goal-id), PAY_FOR_CONVERSION_CRR (needs --crr + --goal-id), and the two *_MULTIPLE_GOALS strategies (need --priority-goals). 3. Empty _NETWORK_STRATEGY_TO_WSDL_SUBTYPE: AverageCpaPerCampaign and AverageCpaPerFilter live only on SmartCampaignStrategyAddBase, not on Text/Dynamic network strategy. Keeping them in the network map for TEXT_CAMPAIGN/DYNAMIC_TEXT_CAMPAIGN would silently emit WSDL-invalid keys. Adds 6 negative regression tests and updates the existing test_campaigns_add_pay_for_conversion_crr_search_payload to pass --crr alongside --goal-id. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@claude review. All three FIX findings addressed in 9ca8852: added --crr flag for PAY_FOR_CONVERSION_CRR (WSDL Crr minOccurs=1), added CLI-level minOccurs=1 enforcement via _STRATEGY_REQUIRED_TYPED_FLAGS for all CPA subtypes, and emptied _NETWORK_STRATEGY_TO_WSDL_SUBTYPE so Per-Campaign/Per-Filter (Smart-only in WSDL) can no longer leak into Text/Dynamic network strategy. 6 new negative regression tests cover each gap. Focus on critical issues only. |
|
Claude finished @axisrow's task in 3m 36s —— View job Claude PR Review — Verifying fix commit 9ca8852
All three prior FIX findings are properly resolved. No new critical issues. Detail below; per request, style / doc-drift nits are not flagged. Verification of prior findings✅ Finding #1 —
|
Summary
Adds typed flags to
direct campaigns addfor issue #204 — minimum-viable surface for the CPA / PayForConversion strategy use-case that downstream MCP (yandex-direct-mcp-pluginB3) currently can't complete without manual cleanup in the Direct web UI.New flags (1:1 with WSDL)
--goal-id INTStrategy*Add.GoalId(single)AVERAGE_CPA,PAY_FOR_CONVERSION_CRR,AVERAGE_CPA_PER_CAMPAIGN,AVERAGE_CPA_PER_FILTER--priority-goals goal_id:value,…TextCampaign.PriorityGoals.Items[](PriorityGoalsItem)*_MULTIPLE_GOALSsubtypes — note WSDL fields are GoalId/Value, not Id/Weight--average-cpa MICRO_RUBLESStrategy*Add.AverageCpaAVERAGE_CPA/AVERAGE_CPA_PER_CAMPAIGNonly--bid-ceiling MICRO_RUBLESStrategy*Add.BidCeilingPayForConversionCrrandPayForConversionMultipleGoalsper WSDL)--counter-ids INT,…TextCampaign/DynamicTextCampaign.CounterIds--counter-idinstead--notification JSONCampaignBase.Notification{SmsSettings:{Events,TimeFrom,TimeTo}, EmailSettings:{Email,CheckPositionInterval,WarningBalance,SendAccountNews,SendWarnings}}— unknown keys rejected--time-targeting JSONCampaignAddItem.TimeTargeting(TimeTargetingAdd){Schedule, ConsiderWorkingWeekends, HolidaysSchedule:{SuspendOnHolidays,BidPercent,StartHour,EndHour}}— unknown keys rejectedStrategy compatibility is enforced via
UsageErrorat CLI level — invalid combinations never reach the API.Issue-requested but NOT shipped (WSDL audit)
--goals(array) — WSDLStrategy*Adddeclares only scalarGoalId; multi-goal CPA is viaPriorityGoals.Items[]instead, so the array surface is--priority-goals.--network-settings— noNetworkSettingsfield exists onCampaignAddItem/TextCampaignAddItem/DynamicTextCampaignAddItem/SmartCampaignAddItemintests/wsdl_cache/campaigns.xml. Probably a misnomer in the issue (possibly confused withSettingsorPlacementTypes).The CHANGELOG
**Notes:**section documents the deviation from the original issue wording.Tests (34 new)
PAYLOAD_CASESfortests/api_coverage_payloads.py(CPA single-goal, multi-goal, Notification+TimeTargeting).Two verifier-found WSDL bugs caught and fixed before PR:
BidCeilingremoved from_STRATEGY_SUPPORTS_BID_CEILING["PayForConversionCrr"](no such field inStrategyPayForConversionCrrAdd).BidCeilinginjection forPayForConversionMultipleGoalsgated by_STRATEGY_SUPPORTS_BID_CEILING(no such field inStrategyPayForConversionMultipleGoalsAdd).Test plan
pytest tests/test_dry_run.py -k campaigns_add— 34 PASSpytest tests/test_api_coverage.py tests/test_wsdl_parity_gate.py— 209 PASSflake8 direct_cli/commands/campaigns.py direct_cli/utils.py tests/test_dry_run.py— cleanpython -m direct_cli.cli campaigns add --help— new flags visible,--goals/--network-settingsabsentCloses #204.
🤖 Generated with Claude Code