@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 3m 10s —— View job

Claude PR Review

• Read changed files
• Analyze parse_priority_goals_spec logic
• Check campaigns.py wiring for both add/update
• Review test coverage for new validations
• Look for edge cases (sibling code paths, other strategies)

Verdict

The campaigns-side fix is sound and well-tested. Validation mirrors MicroRublesParamType.convert exactly (same threshold, same "Did you mean N×1,000,000?" hint, value_int == 0 still allowed for parity with sibling money flags). Help text is updated on both add and update decorator stacks (lines 768 and 4193). The "KNOWN ISSUE #387" comment block in _bidding_strategy.py is safely removed. New reject tests cover the two failure modes (tests/test_dry_run.py:11268 and :11290).

No issues flagged in the diff itself.

Critical finding — sibling bug NOT addressed (same #387 root cause)

direct strategies add / direct strategies update build PriorityGoals.Items through a separate parser, _parse_priority_goal at direct_cli/commands/strategies.py:255, which still forwards Value as a raw int with no MICRO_RUBLE_MIN or non-negative guard:

# direct_cli/commands/strategies.py:255-281
def _parse_priority_goal(spec: str) -> dict:
    ...
    try:
        item = {"GoalId": int(parts[0]), "Value": int(parts[1])}
    except ValueError:
        ...

That parser is called from both strategy mutations:

• strategies.py:567-569 (add)
• strategies.py:727-729 (update)

The WSDL evidence this PR's description leans on — tests/wsdl_cache/strategies.xml lines 503-509 (PriorityGoalsItem.Value is xsd:long in advertiser currency × 1,000,000) — applies to the standalone Strategy* items just as it does to campaigns. So:

direct strategies add --type AverageCpaMultipleGoals \
    --name X --average-cpa 1000000 \
    --priority-goal 1:500

still emits Value: 500 after this PR (≈0.0005 RUB instead of the user's likely-intended 500 RUB). That is the exact same data-loss class as #387 — silent 4-orders-of-magnitude underpricing — on a different command surface.

This PR is explicitly scoped to direct campaigns per its title and CHANGELOG entry, so I'm not flagging this as a blocker for this PR — but #387 is not fully closed by merging it, and "Closes #387" in the PR body is misleading. Recommend either:

1. Narrow the closing keyword to Refs #387 / Part of #387 and open a follow-up issue for strategies, or
2. Add the same < 0 and 0 < v < MICRO_RUBLE_MIN checks to _parse_priority_goal in this PR (3-line change, plus 2 reject tests parallel to the campaigns ones).

The two parsers have different signatures (strategies uses multiple=True --priority-goal singular, campaigns uses comma-separated --priority-goals plural), so unifying them is non-trivial — but the validation slice can be lifted into a shared _validate_priority_goal_value(value_int) helper called from both. Fix this →

Nits (not blockers)

• value_int == 0 is permitted (check uses 0 < value_int). Matches MicroRublesParamType.convert behavior, so consistent; API will reject Value=0 if it's invalid there. No action needed.
  • Branch: fix/priority-goals-micro-rubles

@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 2m 18s —— View job

Claude PR Review

• Read PR diff (origin/main...HEAD)
• Verify strategies.py _parse_priority_goal validation matches campaigns parser
• Confirm shared helper used consistently
• Review new strategies reject tests
• Look for any other priority-goal code paths still affected
• Post final verdict

Verdict — LGTM, no critical issues

The follow-up commit 7149917 cleanly addresses the sibling bug I flagged in my previous review pass. Closes #387 is now accurate. No bugs, security issues, data-loss risks, or perf regressions found.

What I verified

1. Shared helper is the single source of truth. validate_priority_goal_value(value_int, error_prefix) (direct_cli/utils.py:242-263) mirrors MicroRublesParamType.convert exactly (utils.py:200-214): same threshold (MICRO_RUBLE_MIN = 100_000), same < 0 reject, same 0 < v < threshold band, same val * 1_000_000 hint. value_int == 0 is allowed by both, which is intentional consistency with --budget/--average-cpa.

2. All PriorityGoals.Items emitters route through the helper. Greped every PriorityGoal writer in direct_cli/ — only two parsers populate Items:

• parse_priority_goals_spec (utils.py:266) — campaigns surface; calls validator at utils.py:315.
• _parse_priority_goal (strategies.py:260) — strategies surface; calls validator at strategies.py:279.

No third path exists. _bidding_strategy.py and the campaign update code only forward items already produced by parse_priority_goals_spec.

3. Tests cover both surfaces and both failure modes.

• Campaigns: test_campaigns_add_rejects_priority_goals_value_below_micro_min (tests/test_dry_run.py:11268) and test_campaigns_add_rejects_priority_goals_negative_value (:11290).
• Strategies (new in 7149917): test_strategies_add_rejects_priority_goal_value_below_micro_min (tests/test_dry_run.py:17852) and test_strategies_update_rejects_priority_goal_negative_value (tests/test_dry_run.py:17878).

The strategies update test reaches the parser correctly: with only --id 77 --priority-goal …, strategy_fields is empty so the --type-required check at strategies.py:716 doesn't fire, and priority_goals is processed at strategies.py:742-745, triggering the validator before the no-op guard at :749.

4. CHANGELOG correctly scopes the breaking change to both campaigns add/update and strategies add/update, names the threshold, documents the "Did you mean N×1,000,000?" hint, and notes negative values are now rejected up-front.

5. Existing fixture migration is consistent. Every prior priority_goals fixture in tests/test_dry_run.py was bumped by ×1,000,000 (e.g. 1:80 → 1:80000000, 9:1000 → 9:1000000000) — checked the entire diff, no fixture left at raw-ruble values that would now spuriously fail.

Notes (non-blocking)

• Value == 0 is permitted on both surfaces. The Yandex API will likely reject it for MULTIPLE_GOALS strategies, but consistency with MicroRublesParamType (which also permits 0) is more valuable than diverging here. No action.
• The strategies-side test for "below micro min" exercises AverageCrr with --average-crr 10. That's correct because --average-crr is a cost-revenue percentage, not a money flag, so 10 is not itself a micro-currency suspect. Good fixture choice.

Branch: fix/priority-goals-micro-rubles —

@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 3m 5s —— View job

Claude PR Review

• Read PR diff (origin/main...HEAD)
• Verify shared validate_priority_goal_value helper logic
• Confirm both parse_priority_goals_spec (campaigns) and _parse_priority_goal (strategies) route through it
• Enumerate every PriorityGoals.Items writer to confirm no third path bypasses validation
• Verify the strategies update reject test reaches the parser (no earlier guard short-circuits it)
• Confirm api_coverage_payloads fixture migration (×1,000,000 across all priority-goals cases)

Verdict — LGTM, no critical issues

No bugs, security vulnerabilities, logical errors, data-loss risks, or perf regressions found. The earlier sibling-bug call-out has been fully addressed by commit 7149917, so Closes #387 is accurate.

What I verified

1. Single source of truth. validate_priority_goal_value(value_int, error_prefix) at direct_cli/utils.py:242-263 mirrors MicroRublesParamType.convert at utils.py:200-214 byte-for-byte: same threshold (MICRO_RUBLE_MIN = 100_000), same < 0 reject, same 0 < v < threshold reject band, same v * 1_000_000 hint. Value == 0 remains allowed on both surfaces — intentional consistency with --budget/--average-cpa (the API will reject Value=0 itself if invalid for the strategy).

2. Both parsers wired through the helper.

• parse_priority_goals_spec (utils.py:266) calls validator at utils.py:315.
• _parse_priority_goal (strategies.py:260) calls validator at strategies.py:279, between the int() coercion and the IsMetrikaSourceOfValue check — correct ordering (Value is validated regardless of whether the 3rd part is supplied).

3. No third PriorityGoals.Items writer exists. Greped PriorityGoals and priority_goals_items across direct_cli/:

• Campaigns surface — only parse_priority_goals_spec produces items (campaigns.py:2684, :6233), which are then placed by _bidding_strategy.py and commands/campaigns.py without further mutation.
• Strategies surface — only _parse_priority_goal (strategies.py:580, :744).

No bypass path.

4. Strategies-update reject test correctly reaches the parser. For --id 77 --priority-goal "123:-1000000" (no other field flags), the _build_strategy_fields returns empty, strategy_type is None, so:

• The if strategy_fields and not strategy_type guard at strategies.py:716 doesn't fire (strategy_fields empty).
• The if strategy_type and not strategy_fields guard at strategies.py:720 doesn't fire (strategy_type None).
• The if priority_goals: branch at strategies.py:742 triggers _parse_priority_goal, which raises before the no-op guard at :749.

Confirmed.

5. Fixture migration is complete. Every prior priority_goals fixture in tests/api_coverage_payloads.py and tests/test_dry_run.py was scaled by ×1,000,000 (e.g. 80→80000000, 20→20000000, 500→500000000). No raw-ruble fixture left that would now spuriously trip the new validator.

6. The --average-crr 10 choice in the strategies reject test is intentional and correct — --average-crr is declared as plain type=int (a CRR percentage, not micro-currency), so 10 legitimately satisfies its own field type and does not itself trip a money-flag validator. The test isolates the priority-goal validation cleanly.

Notes (non-blocking, no action required)

• Both parsers permit Value == 0. Matches sibling money-flag behavior (MicroRublesParamType also allows 0), so diverging here would be inconsistent. The API is the appropriate gate for "0 is invalid for this strategy."
• I could not run the test suite locally (pip install requires permission), but I confirmed the test logic and fixture migrations via static reading. Per the PR body, the author has run pytest tests/test_dry_run.py (1003 passed).
  • Branch: fix/priority-goals-micro-rubles