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

Review — focused on critical issues only

I read the full diff against origin/main, the new gate, the updated wsdl_coverage.py helpers, the smoke matrix (WRITE_SANDBOX vs DANGEROUS), and confirmed the INTERNAL_VALIDATION expected substrings against the actual UsageError strings in direct_cli/commands/*.py.

Bottom line: no production-impacting bugs, security issues, data-loss risks, or performance problems. The gate is test-only (read-path) and the helpers added to wsdl_coverage.py are pure XML traversal. Below are the items I'd consider before relying on this gate as the green-light for PR 2.

1. xfail tests only assert exit_code != 0 — risk of green-for-the-wrong-reason in PR 2

test_empty_payload_no_op_rejected (tests/test_wsdl_parity_gate.py:74-78) and test_silent_data_loss_rejected (tests/test_wsdl_parity_gate.py:116-120) both check only result.exit_code != 0. With xfail(strict=True), that means PR 2 will be "validated" if any change makes the command exit non-zero — not necessarily because the Pattern A / Pattern B guard was added.

Concrete failure mode: if PR 2 adds a new required=True Click option to e.g. ads update for an unrelated reason, ["ads", "update", "--id", "1", "--type", "TEXT_AD"] will fail with Missing option ..., the strict-xfail flips green, and we declare the empty-payload bug fixed without actually having added a guard.

The INTERNAL_VALIDATION pattern you already use (substring match on the expected error string) handles this correctly for Pattern C. Worth applying the same pattern here — e.g. assert the output mentions "no fields" / "empty" / "specify" for Pattern A, and "incompatible with --type" for Pattern B. Otherwise the gate's PR 2 sign-off is partly cosmetic.

Fix this →

2. Footgun: RUNTIME_DEPRECATED_METHODS filter uses CLI group names, but the registry is keyed by API service names

tests/test_wsdl_parity_gate.py:407-412:

relevant = {
    cmd
    for cmd in _mutating_commands()
    if (cmd.split(".", 1)[0], cmd.split(".", 1)[1])
    not in RUNTIME_DEPRECATED_METHODS
}

cmd.split(".", 1)[0] is a CLI group name (e.g. retargeting, dynamicads), but RUNTIME_DEPRECATED_METHODS in direct_cli/wsdl_coverage.py:139-148 is keyed by the API service name (e.g. retargetinglists, dynamictextadtargets). Today the only deprecated entry is ("agencyclients", "add") where CLI=API, and agencyclients.add is in DANGEROUS not WRITE_SANDBOX, so it never reaches this filter — it works by accident.

The moment someone adds a new entry like ("retargetinglists", "add") to RUNTIME_DEPRECATED_METHODS, the filter silently fails to exclude retargeting.add and the coverage test starts demanding a COMMAND_WSDL_MAP entry for a runtime-rejected command. Worth either translating through CLI_TO_API_SERVICE here, or normalizing RUNTIME_DEPRECATED_METHODS to CLI keys.

Fix this →

3. _wsdl_strategy_subtype_names hardcodes the scalar field set

tests/test_wsdl_parity_gate.py:350-358:

scalar_fields = {"AttributionModel", "Name", "CounterIds", "PriorityGoals"}
return [itf["name"] for itf in (strategies_field.get("item_fields") or [])
        if itf["name"] not in scalar_fields]

If Yandex adds a new scalar field to StrategyAddItem (e.g. Budget, CurrencyCode), it will be returned as a strategy subtype name and the PR 2 fix in STRATEGY_TYPES will look wrong. This is a maintenance-burden trap, not a correctness bug — but the gate's whole point is to catch WSDL drift, and this particular check goes blind to the kind of drift it's supposed to detect.

A more durable filter would be the xsd:choice element marker inside StrategyAddItem (since the per-strategy subtype block is a choice, not a sequence of scalars). If that's too much complexity for PR 1, at minimum a # TODO comment so a future reader knows the scalar list must be updated alongside WSDL refreshes.

Nits (non-blocking)

• next(f for f in schema["fields"] if f["name"] == "Strategies") in _wsdl_strategy_subtype_names raises StopIteration (→ RuntimeError on py3.7+) if the field is missing. Use next(..., None) + an explicit assertion so a WSDL schema rename produces a readable failure.
• INTERNAL_VALIDATION_PROBES[("creatives", "add", "VideoExtensionCreative")] runs ["creatives", "add"] and expects "Missing option '--video-id'". This works today only because --video-id is the only required option on creatives add. If someone adds another required=True Click option to creatives add later, Click will report that one instead and the probe will break. Adding a second mock argument that satisfies any future required field would make this less fragile.
• tests/test_wsdl_parity_gate.py:147 registers ("dynamicads", "add"): ("dynamictextadtargets", "add", "Webpages") — correct, but worth noting in CLAUDE.md that the gate's (cli_group, cli_op) → (api_service, wsdl_op, container) triple is where this CLI/API renaming gets recorded for the parity check; the conventions block in your diff documents the map but doesn't call out that CLI-vs-API name divergence is the reason the value tuple isn't redundant.

Verified clean

• INTERNAL_VALIDATION substrings all match actual UsageError strings in direct_cli/commands/adimages.py:98, bidmodifiers.py:312-315, retargeting.py:96.
• get_required_item_fields correctly handles the min_occurs default (defaults to 1 if absent, matching XSD semantics).
• Coverage test correctly excludes *-passport-organization, set-bids, set-auto, and all non-add/update/set ops via the exact-match filter on op in {"add","update","set"}.
• xfail(strict=True) is the right choice — flipping to passing in PR 2 forces removal of the marker, no silent skip.
• No new code paths run against the live API; everything goes through cached WSDLs in tests/wsdl_cache/ and CliRunner with --dry-run. No data-loss surface.

Recommendation

Merge-blocking only if you want the PR 2 sign-off to be substantive: tighten #1 (assert on error substrings, not just exit_code). Items #2 and #3 are footguns that won't bite today but will bite later — I'd at least add # TODO comments so the next person finds them before they rot.

• Read changed files
• Inspect related modules
• Look for critical issues
• Post review
  · branch: worktree-milestone-0.3.9-pr1-wsdl-gate

Triage of review nits:

Nit 2 — creatives.add probe fragility: SKIP. The probe only invokes ["creatives", "add"] (no options), and INTERNAL_VALIDATION matches on the substring "Missing option '--video-id'". If a new required=True option is added to creatives add later, Click will report that one instead, the substring won't match, and test_internal_validation_rejects_missing_field[creatives.add/VideoExtensionCreative] will fail loudly — not silently. The gate fails explicitly, which is the intended behavior. Not a footgun.

Nit 3 — CLAUDE.md note that CLI-vs-API rename lives in the tuple: SKIP. The convention block already documents COMMAND_WSDL_MAP as the canonical place to register a new mutating command, and the in-file docstring of the constant explains the triple shape. Adding "and by the way this is where renames go" reads as redundant against the tuple type itself (tuple[str, str, str] — three distinct identifiers). Happy to add it if a future maintainer trips on it, but I'd rather not preempt.

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

Review — focused on critical issues (re-run after fix commit 36841b4)

Re-read the diff against origin/main and the fix(test): address PR #205 review feedback commit (36841b4). All three load-bearing items from the previous review have been addressed cleanly; the two triaged nits are reasonable to skip.

Item #1 — xfail substring assertion ✅ fixed

tests/test_wsdl_parity_gate.py:56-108 and :119-161 now carry an expected_error substring per probe:

• Pattern A probes assert "at least one".lower() in result.output.lower() — verified against the codebase: grep -n "at least one" direct_cli/commands/*.py returns 19 occurrences, so this matches the established wording convention across clients.py, campaigns.py, retargeting.py, bids.py, etc. PR 2 will pick up the convention naturally.
• Pattern B probe pins on "--image-hash" — any per-type validation error that mentions the offending flag passes; an unrelated Missing option '--foo' will not.

Net effect: strict-xfail can no longer flip green from an unrelated UsageError introduced for a different reason in PR 2. This was the only merge-blocking concern from the prior review and it's resolved.

Item #2 — RUNTIME_DEPRECATED_METHODS filter ✅ fixed

tests/test_wsdl_parity_gate.py:456-467 adds _is_runtime_deprecated() which routes cli_group through CLI_TO_API_SERVICE before the membership check, with an identity fallback for groups whose name matches the API service. test_command_wsdl_map_covers_known_mutating_commands now calls this helper instead of inlining the raw split. A future entry like ("retargetinglists", "add") in RUNTIME_DEPRECATED_METHODS will now correctly skip the CLI retargeting.add command.

Item #3 — hardcoded scalar field set ✅ fixed

tests/test_wsdl_parity_gate.py:391-414 replaces the hardcoded scalar-field set with a WSDL type-pattern filter: (itf.get("type") or "").endswith("AddItem"). Verified against tests/wsdl_cache/strategies.xml:515-538:

• Scalar fields use types AttributionModelEnum, string, ArrayOfLong, PriorityGoalsArray — none end in AddItem, correctly excluded.
• All 16 subtype fields use Strategy*AddItem types — correctly included.

Also replaced next(...) with next(..., None) + explicit assertion ("WSDL drift: strategies.add request schema has no top-level 'Strategies' field. Refresh tests/wsdl_cache/strategies.xml and update the gate.") so a WSDL rename of Strategies gives a readable failure instead of StopIteration/RuntimeError.

Residual risk: if Yandex ever introduces a per-strategy block whose type does not end in AddItem, the gate silently under-reports it. That would also break Yandex's own naming convention — acceptable.

Triaged nits — agree with skip

• Nit 2 (creatives.add probe fragility): Agree. A new required=True option on creatives add would make Click report the new option's name, the INTERNAL_VALIDATION substring "Missing option '--video-id'" wouldn't match, and test_internal_validation_rejects_missing_field[creatives.add/VideoExtensionCreative] fails loudly. Not a footgun — the gate fails closed.
• Nit 3 (CLAUDE.md note on CLI↔API rename): Agree. The tuple[str, str, str] shape is self-documenting; the entry ("dynamicads", "add"): ("dynamictextadtargets", "add", "Webpages") already encodes the divergence visibly. No need to preempt.

Verified clean (re-check)

• No new code paths run against the live API — fetch_wsdl() reads from tests/wsdl_cache/, CliRunner().invoke(... --dry-run) doesn't touch the network. No data-loss surface.
• get_required_fields / get_required_item_fields (added in direct_cli/wsdl_coverage.py:536-564) are pure XML traversal with sane defaults (min_occurs defaults to 1, matching XSD semantics).
• xfail(strict=True) keeps PR 2's sign-off honest — flipping to passing forces marker removal, no silent skip.
• Coverage filter (tests/test_wsdl_parity_gate.py:449-453) keeps exact-match on {"add","update","set"} so set-bids/set-auto/add-passport-organization aren't pulled in.
• Substring assertions are case-insensitive (.lower() in .lower()), so PR 2 isn't pinned to exact casing.

Recommendation

No further changes required. The fix commit is a substantive response to the prior critical-issue review; the gate's PR 2 sign-off is now substantive rather than cosmetic. Safe to merge.

• Inspect current branch state and fix commit (36841b4)
• Re-check critical issues Add Claude Code GitHub Workflow #1 (xfail substring), Add 1Password auth and simplify build config #2 (deprecated filter), feat: add Bitwarden CLI integration #3 (strategy scalar set)
• Verify no regressions and no new data-loss/security surface
• Post final review
  · branch: worktree-milestone-0.3.9-pr1-wsdl-gate