fix(cli): enforce micro-ruble money inputs by axisrow · Pull Request #400 · axisrow/direct-cli

axisrow · 2026-05-27T14:11:25Z

Summary

Enforce API-native micro-ruble inputs for campaign money flags and ad price-extension fields.
Remove the ruble-to-micro conversion type and update CLI help/README contract wording.
Update dry-run/API coverage fixtures and guards so decimal currency inputs are rejected.

Test plan

pytest tests/test_dry_run.py
pytest tests/test_cli.py
pytest tests/test_wsdl_parity_gate.py tests/test_api_coverage.py
rg guards for old ruble conversion wording and RUBLES_TO_MICRO_RUBLES

Made with Cursor

Co-authored-by: Cursor <[email protected]>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

axisrow · 2026-05-27T14:14:39Z

@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-27T14:14:59Z

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

Claude PR Review — critical-issues pass

Read full diff vs origin/main (9 files, +813/-796)
Inspected each changed file for bugs / security / data-loss risks
Verified dry-run + WSDL parity fixtures match the new contract
Posted findings

Summary

Focused on bugs, security, logic, data loss, perf. Found one real defect (stale help text that actively contradicts the new contract) and no security/data-loss concerns. The contract flip itself is well-tested.

Findings

🟠 1. Four flags ship stale help text that contradicts the new contract — `direct_cli/commands/campaigns.py`

--text-search-roi-coef, --text-search-profitability, --text-network-roi-coef, --text-network-profitability all switched from RUBLES_TO_MICRO_RUBLES (which multiplied by 1M) to MICRO_RUBLES (which does not multiply — it expects the raw integer). Their help text was not updated and still tells the user the opposite:

direct_cli/commands/campaigns.py:1488-1503 (text-search, update block)
direct_cli/commands/campaigns.py:1631-1646 (text-network, update block)
direct_cli/commands/campaigns.py:4501-4516 (text-search, add block)
direct_cli/commands/campaigns.py:4651-4666 (text-network, add block)

All eight occurrences still read:

"Input is multiplied by 1,000,000 to match the Yandex API wire format (e.g. 1 → 1000000)."

After the type change this is wrong:

A user following the help types 1 → MICRO_RUBLES rejects with "1 seems too low for micro-rubles (min 100000 = 0.1 RUB). Did you mean 1000000?" — confusing, because the help just told them 1 was the correct input.
Net data-loss risk is small because MICRO_RUBLE_MIN = 100_000 (direct_cli/utils.py:192) blocks the wrong-magnitude value and the suggestion happens to land on the right answer. But the user-facing contract is contradictory.

Also worth noting: the guard test test_direct_money_flags_use_micro_rubles_only (tests/test_dry_run.py:18173) explicitly forbids the strings RUBLES_TO_MICRO_RUBLES, Decimal("1000000"), human-readable money, and converted to API long in these source files — but the literal phrase "multiplied by 1,000,000" is not in forbidden_snippets, which is why this slipped through CI. Adding that to the guard would prevent future regressions of the same shape.

Fix this →

Non-issues verified

Removal of RUBLES_TO_MICRO_RUBLES (direct_cli/utils.py) — grep -rn across direct_cli/, tests/, scripts/ shows no leftover importers; the only remaining textual match is the deliberately broken "RUBLES_TO_MICRO" + "_RUBLES" sentinel inside the guard test at tests/test_dry_run.py:18174. Clean.
direct_cli/v4/money.py — only the docstring of parse_v4_money_sum changed; the function body is identical. v4 Sum is genuinely a decimal in the API, so calling it "API-native decimal amount" is accurate.
_build_price_extension_add (direct_cli/commands/ads.py) — the per-call decimal parser/multiplier was removed; values now arrive pre-multiplied via the Click MICRO_RUBLES type. The "Price + Qualifier + Currency are required when any PriceExtension flag is used" invariant is preserved (still raised inside _build_price_extension_add), and --price-extension-old-price without --price-extension-price is still rejected (test test_ads_add_text_ad_price_extension_requires_mandatory_fields). Behavior preserved.
Test coverage — test_micro_rubles_rejects_float, test_campaigns_add_money_flag_rejects_decimal_rubles, test_ads_update_text_ad_price_extension_rejects_decimal_rubles, and the cross-file guard test_direct_money_flags_use_micro_rubles_only collectively pin the new contract.
PAYLOAD_CASES and dry-run fixtures (tests/api_coverage_payloads.py, tests/test_dry_run.py) — every CLI invocation that previously passed a decimal value now passes the equivalent micro-ruble integer; spot-checked ~20 fixtures and the math is consistent (e.g. "5" → "5000000", "4.5" → "4500000", "0.2" → "200000").

· branch fix/micro-ruble-money-contract

The four AVERAGE_ROI flags (--text-search-roi-coef, --text-search-profitability, --text-network-roi-coef, --text-network-profitability) switched from RUBLES_TO_MICRO_RUBLES to MICRO_RUBLES but their help text still said "Input is multiplied by 1,000,000", which contradicted the new contract: MICRO_RUBLES expects the pre-multiplied wire-format integer and does no conversion. Rewrite the help to say the value is supplied directly in micro-rubles wire format (matches the wording used by the neighboring *-exploration-min-budget flags), and extend the test_direct_money_flags_use_micro_rubles_only guard to forbid "multiplied by 1,000,000" so the same drift is caught next time. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

axisrow · 2026-05-27T14:23:25Z

@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-27T14:23:41Z

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

Claude PR Review — critical-issues pass

Read full diff vs origin/main (9 files, +834/-816)
Inspect each changed file for bugs / security / data-loss risks
Verify dry-run + WSDL parity fixtures match the new contract
Re-check the previous review's flagged issue

Summary

No critical issues. The stale help-text defect flagged in my previous pass (f030c47) has been fixed: all four --text-*-roi-coef / --text-*-profitability flags now correctly describe the micro-rubles wire format, and the cross-file guard test_direct_money_flags_use_micro_rubles_only was extended with the phrase "multiplied by 1,000,000" so the same drift cannot recur.

The contract flip itself remains clean — see verifications below.

Verified

All RUBLES_TO_MICRO_RUBLES → MICRO_RUBLES swaps are 1:1 — git diff over campaigns.py shows every changed type= line is the same pair; no flag was widened, narrowed, or repurposed.
No stragglers — grep -rn "RUBLES_TO_MICRO|multiplied by 1,000,000|human-readable money|converted to API long" across direct_cli/, tests/, scripts/ returns only the deliberately-broken sentinel inside the guard test (tests/test_dry_run.py:18174).
_build_price_extension_add invariants preserved — the "Price + Qualifier + Currency required when any PriceExtension flag is used" check still raises UsageError (direct_cli/commands/ads.py:307-317); the test test_ads_add_text_ad_price_extension_requires_mandatory_fields still pins it.
MICRO_RUBLE_MIN = 100_000 guard (direct_cli/utils.py:208) blocks the most common wrong-magnitude footgun (a user typing 1 after the contract flip) and the suggestion Did you mean {val * 1_000_000}? lands on the correct answer.
Removed code is fully unreferenced — Decimal/InvalidOperation imports in direct_cli/commands/ads.py are gone with the helper; direct_cli/utils.py still uses them elsewhere so they stay imported.
direct_cli/v4/money.py — only the docstring changed; body identical. v4 Sum is genuinely an API-native decimal, so the new wording is accurate.
PAYLOAD_CASES and dry-run fixtures — every CLI invocation that previously passed a decimal now passes the equivalent micro-ruble integer; math spot-checks consistently ("5" → "5000000", "4.5" → "4500000", "0.2" → "200000", "1.5" → "1500000", "300" → "300000000").
Test coverage — test_micro_rubles_rejects_float, test_micro_rubles_rejects_negative, test_campaigns_add_money_flag_rejects_decimal_rubles, test_ads_update_text_ad_price_extension_rejects_decimal_rubles, and the cross-file guard collectively pin the new contract.

Observation (not blocking, pre-existing, out of scope)

--dyn-network-roi-coef / --dyn-network-profitability / --dyn-search-roi-coef / --dyn-search-profitability (direct_cli/commands/campaigns.py:1068-1075, 1160-1167) are typed click.IntRange(0) and their help text does not mention micro-rubles, while their --text-* and --smart-* siblings are MICRO_RUBLES. The dry-run fixture at tests/test_dry_run.py:10309 sends "Profitability": 25 (raw integer, not micro-rubles) while the sibling text-campaign fixture at line 12152 sends "Profitability": 25000000.

This inconsistency exists on origin/main and is unchanged by this PR — every type= line touched in campaigns.py is just a RUBLES_TO_MICRO_RUBLES → MICRO_RUBLES rename. Mentioning only so the contract drift between --dyn-* and --text-*/--smart-* can be tracked as a follow-up if not already covered by an existing issue.

Recommendation

LGTM for merge on the critical-issues criteria you asked for: no bugs, no security concerns, no data-loss risk, no perf regression. Contract flip is well-tested and behaviorally tight.

· branch fix/micro-ruble-money-contract

Bump version to 0.3.13 for PyPI release. CHANGELOG covers: - Breaking: micro-ruble money inputs for campaign bidding-strategy and ads price-extension flags (#399, #400). - All 0.3.12 additions (callouts on ads update, tracking params and negative keywords on adgroups, video/price extensions on ads, UnifiedCampaign/SmartCampaign/TextCampaign/DynamicTextCampaign/MobileApp /CPMBanner bidding strategy typed flags, milestone 19 audit close). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

* fix(campaigns): priority-goals Value enforced as micro-currency Per Yandex Direct API contract (add-text-campaign, strategies-types), PriorityGoalsItem.Value is xsd:long in advertiser currency × 1,000,000 — the same micro-currency contract as --budget and other money flags after #399/#400. Previously parse_priority_goals_spec accepted raw integers and forwarded them unchanged, so --priority-goals 1:500 emitted Value: 500 which the API interprets as 0.0005 currency units. - direct_cli/utils.py: parse_priority_goals_spec rejects negative values and values below MICRO_RUBLE_MIN with a helpful "Did you mean N×1,000,000?" suggestion. - direct_cli/commands/campaigns.py: --priority-goals help text now documents the micro-currency contract on both option declarations (add + update). - direct_cli/_bidding_strategy.py: removed the obsolete "KNOWN ISSUE #387" comment. - tests/test_dry_run.py: bumped all priority-goals fixture values to realistic micro-currency amounts (20→20000000 etc.) and added two reject tests: sub-MICRO_RUBLE_MIN value and negative value. Closes #387. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(strategies): apply priority-goal micro-currency validation `direct strategies add` / `direct strategies update` build PriorityGoals.Items via a separate parser (`_parse_priority_goal`) which forwarded `Value` as a raw int — same #387 data-loss class as the campaigns side. Per WSDL (tests/wsdl_cache/strategies.xml lines 503-509), PriorityGoalsItem.Value is xsd:long advertiser currency × 1,000,000 for standalone Strategy* items too, so the contract is shared and the validation must be too. - Extract `validate_priority_goal_value(value_int, error_prefix)` in utils.py from the campaign-side parser (single source of truth for the < 0 and 0 < v < MICRO_RUBLE_MIN guards, plus the `Did you mean N×1,000,000?` hint). - Call it from `_parse_priority_goal` in strategies.py so `direct strategies add ... --priority-goal 1:500` is now rejected at CLI parse time instead of silently sending Value=500 (≈0.0005 RUB). - Update help text on both `--priority-goal` decorators to document the micro-currency contract. - Add `test_strategies_add_rejects_priority_goal_value_below_micro_min` and `test_strategies_update_rejects_priority_goal_negative_value` parallel to the campaigns reject tests. Now genuinely Closes #387 (review-found sibling bug on the strategies surface). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test: update api_coverage priority-goals fixtures to micro-currency Six PAYLOAD_CASES fixtures in tests/api_coverage_payloads.py still used raw-ruble Value (e.g. ``1234567:80``) which the new parse_priority_goals_spec validator rejects as below MICRO_RUBLE_MIN. Bump them to ``1234567:80000000,9876543:20000000`` to match the post-#387 contract — same change the first commit made for tests/test_dry_run.py fixtures. This unblocks CI: test_dry_run_payload_schema_coverage[ campaigns-add-argv48,53,54,55,56] and test_dry_run_payload_schema_coverage[campaigns-update-argv57], plus the three cascading schema-gate tests. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: axisrow <[email protected]> Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>

fix(cli): enforce micro-ruble money inputs

f7a6477

Co-authored-by: Cursor <[email protected]>

Copilot AI review requested due to automatic review settings May 27, 2026 14:11

Copilot AI reviewed May 27, 2026

axisrow merged commit d3b2a82 into main May 27, 2026
6 checks passed

axisrow deleted the fix/micro-ruble-money-contract branch May 27, 2026 14:28

This was referenced May 27, 2026

fix(campaigns): priority-goals Value enforced as micro-currency #401

Merged

campaigns add/update: complete optional WSDL field parity #250

Closed

claude Bot mentioned this pull request May 28, 2026

test: integration suite cleanup — graceful skip, explicit creds, dedupe, dynamic counts #404

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): enforce micro-ruble money inputs#400

fix(cli): enforce micro-ruble money inputs#400
axisrow merged 2 commits into
mainfrom
fix/micro-ruble-money-contract

axisrow commented May 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

axisrow commented May 27, 2026

Uh oh!

claude Bot commented May 27, 2026 •

edited

Loading

Uh oh!

axisrow commented May 27, 2026

Uh oh!

claude Bot commented May 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

axisrow commented May 27, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

axisrow commented May 27, 2026

Uh oh!

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

Uh oh!

Claude PR Review — critical-issues pass

Summary

Findings

🟠 1. Four flags ship stale help text that contradicts the new contract — direct_cli/commands/campaigns.py

Non-issues verified

Uh oh!

axisrow commented May 27, 2026

Uh oh!

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

Uh oh!

Claude PR Review — critical-issues pass

Summary

Verified

Observation (not blocking, pre-existing, out of scope)

Recommendation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented May 27, 2026 •

edited

Loading

🟠 1. Four flags ship stale help text that contradicts the new contract — `direct_cli/commands/campaigns.py`

claude Bot commented May 27, 2026 •

edited

Loading