@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 6m 12s —— View job

Claude PR Review — Critical-issues only

• Read PR diff
• Inspect each changed command file
• Verify empty-payload guards and per-type allow-lists against WSDL
• Check strategy enum / strategies update flow against WSDL
• Post findings

I cross-checked each of the 12 fixes against the WSDL cache in tests/wsdl_cache/. 11 of 12 fixes are correct and a real improvement. I found one potential over-constraint in H12 and one harmless code-style nit; no security issues, no data-loss risks, no performance regressions.

🟡 Potential bug — H12: strategies update may over-constrain --goal-id vs. WSDL

direct_cli/commands/strategies.py:354-358:

if strategy_type:
    # GoalId is minOccurs=1 inside Strategy*Update for the
    # goal-id family (issue #198 H12). Mirror the add command's
    # validation here so updates do not silently drop GoalId.
    if strategy_type in GOAL_ID_STRATEGY_TYPES and goal_id is None:
        raise click.UsageError("Provide --goal-id for this strategy type")

The inline comment's WSDL claim does not match the cached schema. In tests/wsdl_cache/strategies.xml:

• StrategyAverageCpaAddItem.GoalId → minOccurs="1" (required on add — correct)
• StrategyAverageCpaBase.GoalId → minOccurs="0" (and every Strategy*UpdateItem is <extension base="ns:Strategy*Base"/> with no override)

The same pattern holds for StrategyPayForConversionBase, StrategyAverageCpaPerCampaignBase, StrategyAverageCrrBase, StrategyPayForConversionCrrBase. So per WSDL, GoalId is optional inside every Strategy*UpdateItem. The justification "mirroring add" therefore mixes contracts that differ on purpose: WSDL lets update change --average-cpa (or any other base field) without re-specifying GoalId.

Concrete regression this introduces, if the WSDL is authoritative:

direct strategies update --id 123 --type AverageCpa --average-cpa 1500000
# was accepted; now → UsageError: Provide --goal-id for this strategy type

The user has to look up and repeat the existing GoalId even when they are not changing it.

Two ways to resolve, depending on what the audit actually observed:

1. If the live API really rejects updates without GoalId (despite the WSDL): keep the validation, but rewrite the comment so it doesn't claim minOccurs=1. E.g. # Empirically, the Yandex live API rejects Strategy*Update without GoalId even though the WSDL marks it minOccurs=0 (issue #198 H12). That preserves the future-proofing for whoever wonders why we're stricter than the schema.
2. If the audit didn't actually verify this against the live API: drop the H12 validation. The WSDL is the contract and the gate already accepts the no-validation state (the parity gate looks at top-level StrategyUpdateItem required fields, which don't include GoalId).

Fix this →

🟢 Other fixes — verified correct

🔵 Minor (non-blocking)

• direct_cli/commands/keywordbids.py:99-101 — implicit string concatenation:

  raise click.UsageError(
      "keywordbids set requires at least one of " "--search-bid or --network-bid."
  )

  Two adjacent string literals get joined to "...of --search-bid..." which is correct output, just an unusual idiom. Not a bug; can be left as-is or flattened.

Summary

The only finding worth gating on is H12: the inline comment makes a WSDL claim that the cached schema disproves, and the validation may reject valid update payloads that the WSDL allows. Either rewrite the rationale to point at live-API behavior or drop the check. Everything else looks good — empty-payload guards close real silent-no-op bugs, the per-type allow-list eliminates the silent-data-loss class, and STRATEGY_TYPES is now exactly the WSDL set.
· Branch

@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 33s —— View job

Claude PR Review — Critical-issues only (re-review after 83f3636)

• Read PR diff and recent commits
• Inspect strategies.py after the follow-up commit
• Verify each of the 12 fixes against WSDL cache
• Cross-check copilot-suppressed findings against current code
• Post findings

I re-reviewed after 83f3636 "fix(strategies): align with WSDL on update GoalId and Crr-family". That commit cleanly resolves my prior H12 concern and two of copilot's three suppressed findings (PayForConversionCrr moved to CRR_STRATEGY_TYPES, PayForConversionMultipleGoals added to GOAL_ID_STRATEGY_TYPES). 11 of 12 audit items are now correctly fixed.

However one suppressed copilot finding is still a live bug: strategies add --type AverageCpcPerFilter produces a WSDL-invalid payload.

🔴 Critical — strategies add --type AverageCpcPerFilter emits wrong field name

direct_cli/commands/strategies.py:98-99:

if average_cpc is not None:
    fields["AverageCpc"] = average_cpc

This is unconditional regardless of strategy_type. The WSDL however defines two different field names for the two CPC strategy subtypes (tests/wsdl_cache/strategies.xml:294-309):

So a natural invocation like:

direct strategies add --name "Per-filter CPC" --type AverageCpcPerFilter --average-cpc 1000000

produces:

{ "AverageCpcPerFilter": { "AverageCpc": 1000000 } }

…but the WSDL StrategyAverageCpcPerFilterAddItem has no AverageCpc field — only FilterAverageCpc, WeeklySpendLimit, CustomPeriodBudget, BidCeiling. The live API will reject this (or silently ignore the unknown field and create a strategy with no CPC set — either way, broken).

Parallel proof inside the same repo: direct_cli/commands/campaigns.py:324 correctly uses FilterAverageCpc for the SMART_CAMPAIGN → AVERAGE_CPC_PER_FILTER path, and tests/test_dry_run.py:984 asserts that field name explicitly. The strategies path is the inconsistent one.

The PAYLOAD_CASES set for strategies.add (tests/test_low_coverage_payloads.py:209-307) covers only AverageCpa, AverageCrr, PayForConversion — nothing exercises AverageCpcPerFilter (or AverageCpaMultipleGoals/PayForConversionMultipleGoals), which is why the gate doesn't catch this. Copilot's first suppressed comment flagged exactly this; the follow-up commit fixed the other two but missed this one.

Suggested fix — same shape as the existing PAY_FOR_CONVERSION / FILTER_CPA branches:

FILTER_CPC_STRATEGY_TYPES = {"AverageCpcPerFilter"}

if average_cpc is not None:
    if strategy_type in FILTER_CPC_STRATEGY_TYPES:
        fields["FilterAverageCpc"] = average_cpc
    else:
        fields["AverageCpc"] = average_cpc

…and add a payload test analogous to test_strategies_add_pay_for_conversion_payload_uses_cpa_field so the parity gate locks the field name down.

Fix this →

🟢 Other 11 fixes — verified correct

🔵 Lower-severity observations (non-blocking)

1. AverageCpaMultipleGoals with --average-cpa — StrategyAverageCpaMultipleGoalsAddItem has no Cpa/AverageCpa field at all. If a user passes --average-cpa to --type AverageCpaMultipleGoals, _build_strategy_fields falls through to the else branch and emits AverageCpa, producing an invalid payload. Less likely than the H1 bug above (the option name doesn't suggest this type) but the same fix shape applies — either reject the flag for this subtype or omit it.

2. keywordbids.py:99-101 — implicit string concatenation across two literals. Not a bug; cosmetic.

Summary

One real bug remains: strategies add --type AverageCpcPerFilter produces a WSDL-invalid payload because the field-name dispatch in _build_strategy_fields doesn't account for FilterAverageCpc. Same pattern as the PayForConversion → Cpa and AverageCpaPerFilter → FilterAverageCpa branches that the function already handles correctly. The other 11 audit fixes are verified correct against the cached WSDL. PR is mergeable once that one field-name mapping is added (and ideally a payload test alongside it).
· Branch

@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 6m 4s —— View job

Claude PR Review — Critical-issues only (re-review after 2d890b7)

• Read PR diff and recent commits (2d890b7, 83f3636, 278ae8e)
• Inspect strategies.py after the latest commit
• Verify the AverageCpcPerFilter fix lands on the right code paths
• Check AverageCpaMultipleGoals / PayForConversionMultipleGoals subtypes
• Post findings

I re-reviewed after 2d890b7 "route --average-cpc to FilterAverageCpc for AverageCpcPerFilter". That commit closes the field-name bug I flagged previously, and also adds an explicit UsageError for --average-cpa against the MultiGoal subtypes (good — that was the second copilot-suppressed concern). The 12 audit fixes all check out against the cached WSDL.

I found one remaining bug of the same class the PR exists to fix (silent no-op) — not in the audit's original 12, but caught by the same pattern.

🟠 Bug — strategies update still has an empty-subtype no-op

direct_cli/commands/strategies.py:358-380:

strategy_fields = _build_strategy_fields(strategy_type, ...)
if strategy_fields and not strategy_type:
    raise click.UsageError("Provide --type when setting strategy-specific fields")
if strategy_type:
    strategy_data[strategy_type] = strategy_fields

Reproducer:

direct strategies update --id 42 --type AverageCpa --dry-run

• strategy_fields = {} (no field flags supplied)
• first guard: if strategy_fields and not strategy_type: → False (falsy strategy_fields short-circuits)
• second branch: strategy_data["AverageCpa"] = {} is set
• payload sent: {"Strategies": [{"Id": 42, "AverageCpa": {}}]} — exactly the empty subtype no-op that H1 / H5 / H8 / H9 / H10 close in ads.py, adgroups.py, bids.py, keywordbids.py, keywords.py. Yandex accepts this as a successful no-op, so the user sees no error and no change.

This is a real production hazard for the same reason the other five guards exist: a user trying to update e.g. --bid-ceiling and mistyping the flag name gets a 200 OK with no diff applied.

Why the parity gate didn't catch it: EMPTY_PAYLOAD_PROBES in tests/test_wsdl_parity_gate.py:56-82 includes ads.update, adgroups.update, bids.set, keywordbids.set, keywords.update — strategies.update is missing.

Fix shape (mirrors keywords update):

# Reject empty-subtype no-ops (issue #198 pattern, H5/H10 sibling).
if strategy_type and not strategy_fields:
    raise click.UsageError(
        f"strategies update requires at least one field for "
        f"--type {strategy_type}."
    )
if len(strategy_data) == 1 and not (counter_ids or priority_goals or attribution_model):
    raise click.UsageError(
        "strategies update requires at least one updatable field."
    )

…plus an entry in EMPTY_PAYLOAD_PROBES so the parity gate locks it down.

Fix this →

🟡 Class-of-bug observation (non-blocking, broader than #198 H-list)

_build_strategy_fields still silently emits or drops fields when a strategy-specific flag is paired with a subtype that doesn't define that field. The PR only handled the MultiGoal+--average-cpa case explicitly. The same H2-class issue remains for, e.g.:

The live API will either reject these (best case) or accept them and silently ignore the unknown field (worst case — user gets a strategy without the CPC/CPA they intended). The ads.py H2 fix solved the equivalent for ads update via a per-type allow-list table — strategies would benefit from the same pattern. Out of scope for this PR (issue #198 didn't enumerate these), but worth filing as a follow-up.

🟢 The 12 audit fixes — verified correct

Also verified the follow-up commits resolved both prior copilot/me findings: PayForConversionCrr is correctly in CRR_STRATEGY_TYPES (Crr+GoalId, no Cpa), PayForConversionMultipleGoals is correctly in GOAL_ID_STRATEGY_TYPES, and AverageCpcPerFilter correctly emits FilterAverageCpc with a payload test at tests/test_low_coverage_payloads.py:346-359.

Summary

The PR is one short guard away from done. Blocking: the strategies update empty-subtype no-op described above — it's exactly the bug class this PR was created to close, just not on the audit's hit list. Once that guard lands (plus a probe entry in the parity gate so it can't regress), this is mergeable. Everything in the listed 12 H-bugs checks out against the cached WSDL.
· Branch

@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 1m 52s —— View job

Claude PR Review — Critical-issues only (re-review after 7945109)

• Read recent commits and identify deltas since last review
• Verify strategies update empty-subtype no-op guard
• Verify parity-gate probe entry locks it down
• Spot-check other 12 audit fixes
• Post findings

I re-reviewed after 7945109 "fix(strategies): reject empty-subtype no-op on update". That commit cleanly closes the only blocker I flagged in my prior pass, and the fix matches the shape I'd suggested exactly. No remaining critical issues — PR is mergeable from a correctness standpoint.

🟢 Blocker from prior review — resolved

direct_cli/commands/strategies.py:372-380 adds the missing guard:

if strategy_type and not strategy_fields:
    raise click.UsageError(
        f"strategies update requires at least one field for "
        f"--type {strategy_type}."
    )

…and strategies.py:401-406 adds the broader "Id-only" guard:

if len(strategy_data) == 1:
    raise click.UsageError(
        "strategies update requires at least one updatable field."
    )

Both fire before payload construction. I traced the relevant cases:

Parity-gate lock-down is also in place — tests/test_wsdl_parity_gate.py:82-86 adds:

(
    "strategies.update",
    ["strategies", "update", "--id", "1", "--type", "AverageCpa"],
    "at least one",
),

So the regression cannot silently re-open in a future refactor.

🟢 All 12 audit fixes — verified correct

🟡 Class-of-bug observation (non-blocking, broader than #198 H-list)

_build_strategy_fields (strategies.py:88-133) still silently emits a few flags against subtypes that don't define them. The 2d890b7/7945109 commits fixed two cases (AverageCpcPerFilter → FilterAverageCpc, MULTI_GOAL+--average-cpa → hard reject), but these remain:

The live API will either reject these (best case) or silently ignore the unknown key (worst case — user gets a strategy without the CPC/CPA they intended). The ads.py H2 fix solved the equivalent via a per-type allow-list; strategies would benefit from the same pattern. Out of scope for this PR (the audit didn't enumerate these), but worth filing as a follow-up — particularly because the new STRATEGY_TYPES list is larger now, so the cross-product of "wrong flag + wrong subtype" silent emits has grown too.

Summary

No blockers. The empty-subtype no-op I flagged last round is fixed and lock-down is in place. All 12 audit fixes verified against cached WSDL. The remaining _build_strategy_fields silent-emit class is out-of-scope for #198 but worth tracking as a follow-up issue with the same shape as ads.py H2.
· Branch