Skip to content

test: automated API coverage check via WSDL introspection - #34

Merged
axisrow merged 10 commits into
mainfrom
feat/wsdl-coverage-test
Apr 13, 2026
Merged

test: automated API coverage check via WSDL introspection#34
axisrow merged 10 commits into
mainfrom
feat/wsdl-coverage-test

Conversation

@axisrow

@axisrow axisrow commented Apr 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add WSDL-based coverage tests that detect missing CLI commands and methods by comparing against Yandex Direct API v5 service definitions (closes test: automated API coverage check via WSDL introspection #32)
  • direct_cli/wsdl_coverage.py: WSDL fetching, parsing, CLI↔API service/method mapping with cached XML files for offline CI
  • tests/test_api_coverage.py: Phase 1 (method coverage per service) and Phase 2 (new service discovery) tests
  • 27 cached WSDL XML files in tests/wsdl_cache/
  • api_coverage pytest marker added to pyproject.toml

Detected gaps (tracked in KNOWN_* constants):

  • Missing service: advideos
  • 15 missing API methods (setAuto, setBids, getGeoRegions, etc.)
  • 4 extra CLI methods without WSDL counterpart (keywords archive/unarchive, dynamicads update, keywordsresearch get)

Test plan

  • pytest tests/test_api_coverage.py -v — 2/2 passed (offline, from cache)
  • pytest -m api_coverage -v — marker filtering works
  • Full suite pytest — 120 passed, no regressions
  • black --check / flake8 — clean
  • Remove an entry from KNOWN_MISSING_METHODS → test fails with clear message
  • Add a fake service to CANONICAL_API_SERVICEStest_no_missing_services fails

🤖 Generated with Claude Code

Add WSDL-based coverage tests that detect missing CLI commands and
methods by comparing against Yandex Direct API v5 service definitions.

- wsdl_coverage.py: WSDL fetching, parsing, CLI↔API service/method mapping
- test_api_coverage.py: Phase 1 (method coverage) and Phase 2 (service discovery) tests
- wsdl_cache/: 27 cached WSDL XML files for offline CI
- pyproject.toml: add api_coverage pytest marker

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 12, 2026 16:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds automated WSDL-driven test coverage checks to compare the CLI’s registered commands/subcommands against Yandex Direct API v5 service definitions, using a committed WSDL cache to keep CI offline.

Changes:

  • Introduces direct_cli/wsdl_coverage.py utilities to fetch/cache WSDL XML and map CLI subcommands to API operations.
  • Adds tests/test_api_coverage.py (pytest api_coverage marker) to detect missing API methods per service and newly introduced API services.
  • Commits cached WSDL XML files under tests/wsdl_cache/ and registers the api_coverage marker in pyproject.toml.

Reviewed changes

Copilot reviewed 28 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
direct_cli/wsdl_coverage.py WSDL fetch/parse + CLI↔API service/method mapping utilities used by coverage tests
tests/test_api_coverage.py Coverage tests for missing/extra methods and missing/new services using cached WSDLs
pyproject.toml Registers api_coverage pytest marker
tests/wsdl_cache/adextensions.xml Cached WSDL for offline coverage checks (adextensions)
tests/wsdl_cache/adgroups.xml Cached WSDL for offline coverage checks (adgroups)
tests/wsdl_cache/adimages.xml Cached WSDL for offline coverage checks (adimages)
tests/wsdl_cache/ads.xml Cached WSDL for offline coverage checks (ads)
tests/wsdl_cache/advideos.xml Cached WSDL for offline coverage checks (advideos)
tests/wsdl_cache/agencyclients.xml Cached WSDL for offline coverage checks (agencyclients)
tests/wsdl_cache/audiencetargets.xml Cached WSDL for offline coverage checks (audiencetargets)
tests/wsdl_cache/bidmodifiers.xml Cached WSDL for offline coverage checks (bidmodifiers)
tests/wsdl_cache/bids.xml Cached WSDL for offline coverage checks (bids)
tests/wsdl_cache/businesses.xml Cached WSDL for offline coverage checks (businesses)
tests/wsdl_cache/campaigns.xml Cached WSDL for offline coverage checks (campaigns)
tests/wsdl_cache/changes.xml Cached WSDL for offline coverage checks (changes)
tests/wsdl_cache/clients.xml Cached WSDL for offline coverage checks (clients)
tests/wsdl_cache/creatives.xml Cached WSDL for offline coverage checks (creatives)
tests/wsdl_cache/dictionaries.xml Cached WSDL for offline coverage checks (dictionaries)
tests/wsdl_cache/dynamictextadtargets.xml Cached WSDL for offline coverage checks (dynamictextadtargets)
tests/wsdl_cache/feeds.xml Cached WSDL for offline coverage checks (feeds)
tests/wsdl_cache/keywordbids.xml Cached WSDL for offline coverage checks (keywordbids)
tests/wsdl_cache/keywords.xml Cached WSDL for offline coverage checks (keywords)
tests/wsdl_cache/keywordsresearch.xml Cached WSDL for offline coverage checks (keywordsresearch)
tests/wsdl_cache/leads.xml Cached WSDL for offline coverage checks (leads)
tests/wsdl_cache/negativekeywordsharedsets.xml Cached WSDL for offline coverage checks (negativekeywordsharedsets)
tests/wsdl_cache/retargetinglists.xml Cached WSDL for offline coverage checks (retargetinglists)
tests/wsdl_cache/sitelinks.xml Cached WSDL for offline coverage checks (sitelinks)
tests/wsdl_cache/smartadtargets.xml Cached WSDL for offline coverage checks (smartadtargets)
tests/wsdl_cache/turbopages.xml Cached WSDL for offline coverage checks (turbopages)
tests/wsdl_cache/vcards.xml Cached WSDL for offline coverage checks (vcards)

Comment on lines +120 to +127
import requests

url = WSDL_BASE_URL.format(service=service_name)
resp = requests.get(url, timeout=30)
resp.raise_for_status()

xml_text = resp.text

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch_wsdl imports and uses requests, but requests is not listed in project dependencies. This will raise ModuleNotFoundError whenever the cache is missing or use_cache=False (e.g., running refresh_all_caches()), and it won’t be caught by the offline tests.

Consider either switching to the stdlib (urllib.request) for the HTTP GET, or adding requests as an explicit runtime dependency and ensuring it’s available in CI environments that refresh the cache.

Suggested change
import requests
url = WSDL_BASE_URL.format(service=service_name)
resp = requests.get(url, timeout=30)
resp.raise_for_status()
xml_text = resp.text
from urllib.request import urlopen
url = WSDL_BASE_URL.format(service=service_name)
with urlopen(url, timeout=30) as resp:
xml_text = resp.read().decode("utf-8")

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +13
import xml.etree.ElementTree as ET
from pathlib import Path

WSDL_BASE_URL = "https://api.direct.yandex.com/v5/{service}?wsdl"
CACHE_DIR = Path(__file__).resolve().parent.parent / "tests" / "wsdl_cache"

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CACHE_DIR points into a tests/wsdl_cache directory relative to the installed package. In an installed environment this path likely won’t exist (tests aren’t included in the package) and may also be unwritable (site-packages), causing unexpected network fetches and/or write failures.

Consider making the cache directory configurable (parameter/env var) and defaulting to a user-writable cache location (e.g., XDG cache dir), or moving cached WSDLs into package data and reading them via importlib.resources when use_cache=True.

Suggested change
import xml.etree.ElementTree as ET
from pathlib import Path
WSDL_BASE_URL = "https://api.direct.yandex.com/v5/{service}?wsdl"
CACHE_DIR = Path(__file__).resolve().parent.parent / "tests" / "wsdl_cache"
import os
import xml.etree.ElementTree as ET
from pathlib import Path
WSDL_BASE_URL = "https://api.direct.yandex.com/v5/{service}?wsdl"
def _get_cache_dir():
"""Return a writable cache directory for downloaded WSDL files."""
cache_dir = os.environ.get("DIRECT_CLI_WSDL_CACHE_DIR")
if cache_dir:
return Path(cache_dir).expanduser()
xdg_cache_home = os.environ.get("XDG_CACHE_HOME")
if xdg_cache_home:
return Path(xdg_cache_home).expanduser() / "direct_cli" / "wsdl_cache"
return Path.home() / ".cache" / "direct_cli" / "wsdl_cache"
CACHE_DIR = _get_cache_dir()

Copilot uses AI. Check for mistakes.
Comment thread tests/test_api_coverage.py Outdated
Comment on lines +21 to +25
# API methods that exist in WSDL but the CLI does not implement.
# When one is fixed, remove it from this set and the test will still pass.
KNOWN_MISSING_METHODS = {
("agencyclients", "addPassportOrganization"),
("agencyclients", "addPassportOrganizationMember"),

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suppression sets (KNOWN_MISSING_SERVICES/KNOWN_MISSING_METHODS/KNOWN_EXTRA_CLI_METHODS/ALLOWED_EXTRA_METHODS) can mask future regressions if they become stale. If a missing method/service is later implemented but not removed from the relevant KNOWN_* set, a subsequent regression that removes it again would be ignored.

Consider adding assertions that each KNOWN_MISSING_* entry is currently missing (and each KNOWN_EXTRA_* entry is currently extra), so the lists must be updated when coverage changes.

Copilot uses AI. Check for mistakes.
codex and others added 7 commits April 12, 2026 16:24
…VICES

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Item (Demographics/Retargeting/Regional/SerpLayout/IncomeGrade)
Реализует оставшиеся 6 пунктов из follow-up плана issue #32
для "100% covered supported API surface".

Инфраструктура (direct_cli/wsdl_coverage.py):
- CLI_ALIAS_GROUPS для dynamictargets/smarttargets/negativekeywords
- NON_WSDL_SERVICE_POLICIES — явная политика для reports и др. non-SOAP surface
- INTENTIONAL_EXTRA_METHODS с reasons (заменяет KNOWN_MISSING_METHODS-стиль)
- расширены METHOD_NAME_OVERRIDES
- registry-driven schema: get_api_coverage_policy, get_operation_request_schema

Команды (22 файла): добавлен --dry-run во все lifecycle-команды
(delete/archive/unarchive/suspend/resume/moderate). Новые команды:
agencyclients add-passport-organization/add-passport-organization-member/update,
audiencetargets set-bids, dynamicads set-bids. reports: выделена чистая
build_report_request() функция.

Тесты (tests/test_api_coverage.py, tests/test_dry_run.py):
- 70+ PAYLOAD_CASES с реальными dry-run прогонами и проверкой формы запроса
- DRY_RUN_PAYLOAD_EXCLUSIONS с обоснованиями
- test_non_wsdl_services_have_explicit_coverage_policy
- test_alias_groups_resolve_to_real_cli_groups
- test_api_coverage_report_script_matches_strict_parity_contract
- end-to-end contract тесты reports (build_report_request + client.reports().post)

Скрипты и CI:
- scripts/build_api_coverage_report.py — JSON-отчёт strict parity / summary
- scripts/check_wsdl_drift.py — сравнение cached WSDL с live API
- .github/workflows/api-coverage.yml — fast suite + report artifact + weekly drift check

Integration test: убран dynamicads.update (операция отсутствует в WSDL
dynamictextadtargets, это часть приведения CLI к strict WSDL parity).

Docs: README секция "API Coverage And Drift Monitoring" с таблицей
четырёх surfaces и определением "100% coverage".

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@axisrow

axisrow commented Apr 13, 2026

Copy link
Copy Markdown
Owner Author

@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

claude Bot commented Apr 13, 2026

Copy link
Copy Markdown

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


PR Review — Critical Issues

Tasks:

  • Gather context and understand request
  • Read direct_cli/wsdl_coverage.py
  • Read tests/test_api_coverage.py
  • Read modified command files for critical bugs
  • Check GitHub Actions workflow
  • Post review findings

Bug 1 — requests is an undeclared dependency (breaks CI) Fix this →

wsdl_coverage.py:147 does a lazy import requests that runs whenever use_cache=False. Both scripts/check_wsdl_drift.py and scripts/refresh_wsdl_cache.py call fetch_wsdl(..., use_cache=False), and the monitor-live-wsdl CI job runs check_wsdl_drift.py on every scheduled/manual trigger.

requests is not in pyproject.toml — neither under dependencies nor dev. The weekly scheduled CI job will fail with ModuleNotFoundError. (tapi-yandex-direct likely brings requests as a transitive dep, but relying on that is fragile and will silently break if the upstream ever switches to httpx.)

Fix: add "requests" to [project.optional-dependencies] dev in pyproject.toml.


Bug 2 — int() truncates bid microcurrency values for common inputs Fix this →

All new bid commands in this PR use int(bid * 1_000_000) to convert to microcurrency. Python floating-point representation makes 0.3 * 1_000_000 == 299_999.999…, so int() truncates it to 299_999 instead of 300_000. This will send the wrong bid amount to the API for any value that has a non-representable decimal component (0.3, 1.3, 0.7, etc.).

Affected new code in this PR:

File Line Pattern
bids.py 144 int(max_bid * 1000000) in set-auto
audiencetargets.py 93 int(bid * 1000000) in add
audiencetargets.py 137 int(context_bid * 1000000) in set-bids
smartadtargets.py 274 int(average_cpc * 1000000) in set-bids
smartadtargets.py 277 int(average_cpa * 1000000) in set-bids
dynamicads.py 203, 205 int(bid * 1000000) in set-bids
keywordbids.py 84, 85 int(search_bid * 1000000) in set

Fix: use round(bid * 1_000_000) (or equivalently int(round(bid * 1_000_000))). The same bug exists in the pre-existing bids.set and bids.get commands but those predate this PR.


Bug 3 — keywordsresearch sends wrong method name casing Fix this →

The cached WSDL (tests/wsdl_cache/keywordsresearch.xml) defines operations as:

hasSearchVolume
deduplicate

(lowercase camelCase, consistent with all other Yandex Direct SOAP operations)

But keywordsresearch.py sends:

# line 31
body = {"method": "HasSearchVolume", ...}
# line 58
body = {"method": "Deduplicate", ...}

These are PascalCase, not camelCase. The API likely rejects or ignores them. Every other command in the codebase uses lowercase-initial names (get, add, setBids, setAuto, etc.).

The WSDL coverage test test_service_method_coverage does not catch this because get_cli_methods_for_service builds the method set from METHOD_NAME_OVERRIDES (which correctly maps "has-volume""hasSearchVolume"), but never validates the string actually present in the command's request body. This bug predates this PR but the new coverage tests are silently failing to detect it.


Observation — _collect_complex_type_fields has unbounded recursion

wsdl_coverage.py:224–253 recursively follows XSD type inheritance with no cycle guard. Valid schemas can't be circular, but a corrupted or adversarially-crafted WSDL cache file would cause infinite recursion and crash the test process. Low severity given these are committed files, but worth noting.

scripts/check_wsdl_drift.py и scripts/refresh_wsdl_cache.py вызывают
direct_cli.wsdl_coverage.fetch_wsdl(use_cache=False), который делает
lazy `import requests`. Пакет приходит транзитивно через
tapi-yandex-direct, но явная декларация защищает от будущего перехода
upstream на другой HTTP-клиент (например httpx).

Addresses feedback from PR #34 review (claude[bot]).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@axisrow

axisrow commented Apr 13, 2026

Copy link
Copy Markdown
Owner Author

Триаж ревью

Bug 1 — requests undeclared depпринято как defensive fix (3e3f0e3).
requests приходит транзитивно через tapi-yandex-direct (прямая зависимость):

$ pip show tapi-yandex-direct | grep Requires
Requires: orjson, requests, tapi-wrapper2

Поэтому CI monitor-live-wsdl зелёный сейчас. Но чтобы защититься от возможной смены upstream на httpx — добавил "requests>=2.0" в [project.optional-dependencies] dev явно.


Bug 2 — int(bid * 1_000_000) FP truncationне воспроизводится. Проверено:

$ python3 -c "print(int(0.3*1_000_000), round(0.3*1_000_000))"
300000 300000

Проверка по диапазону 0.01–99.99 показывает, что int() и round() дают одинаковый результат для всех реальных 2-decimal bid values. На шкале 1e6 float-residue для 0.3 положительный (0.3 * 1_000_000 == 300000.00000000006), truncate и round совпадают. Рецензент, видимо, перепутал с int(0.3*10) == 2 — там residue отрицательный. На шкале 1e6 этой проблемы нет. Фикс не требуется.


Bug 3 — keywordsresearch PascalCaseреальный баг, но pre-existing в main, вне scope PR #34.
Подтверждаю: keywordsresearch.py:31,58 шлёт "HasSearchVolume"/"Deduplicate", WSDL объявляет hasSearchVolume/deduplicate, все остальные команды репо используют camelCase (setBids, getGeoRegions, checkCampaigns и т.д.). Однако изменения в keywordsresearch.py в этом PR — только удаление мёртвой get-команды (-30 строк), PascalCase-method существовал в main до этого PR. Открою отдельный issue — не блокирует merge #34.


Observation — unbounded recursion в _collect_complex_type_fields — accepted как known low-severity limitation. Input — доверенные WSDL-файлы Яндекса, адверсариальный сценарий отсутствует. Если когда-нибудь проявится — добавим cycle guard.

@axisrow

axisrow commented Apr 13, 2026

Copy link
Copy Markdown
Owner Author

@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

claude Bot commented Apr 13, 2026

Copy link
Copy Markdown

Claude finished @axisrow's task in 7m 16s —— View job


PR Review — Critical Issues

Tasks:

  • Gather context and understand request
  • Read direct_cli/wsdl_coverage.py
  • Read tests/test_api_coverage.py
  • Read new/modified command files for critical bugs
  • Check CI workflow and scripts
  • Post review findings

Previous review items — triaged correctly

All three bugs from the prior review were handled appropriately. The one remaining open item is Bug 3 (PascalCase keywordsresearch method names), which the author confirmed is pre-existing on main and out of scope for this PR. No objection.


New Finding 1 — Schema validator silently skips nested type validation for imported XSD namespaces [Medium severity]

wsdl_coverage.py:292-294 builds complex_types only from local schema elements:

complex_types = {
    ctype.get("name"): ctype for ctype in schema.findall("xsd:complexType", ns)
}

When _collect_complex_type_fields (called at line 317) looks up a type like gc:NotificationAdd or general:CurrencyEnum, it resolves via _local_name() to NotificationAdd / CurrencyEnum, finds nothing in complex_types (those types live in imported namespaces — generalclients.xsd, general.xsd), and silently returns [].

Result: _assert_body_matches_wsdl never validates nested required fields for any field whose type comes from an xsd:import. In agencyclients.add this affects Notification (type gc:NotificationAdd), Settings (type gc:ClientSettingAddItem), and Grants (type gc:GrantItem). The test passes "Notification":{} (empty object) and no assertion fires, even if NotificationAdd has required sub-fields.

This isn't a production bug in the CLI commands themselves — the commands are correct. But the test_dry_run_payload_schema_coverage tests overstate their own coverage: they can only validate top-level param names and one level of inline-type fields. Any deeper structure silently passes. Worth documenting this in the test's docstring or a comment so future contributors don't assume nested validation is complete.


New Finding 2 — get_operation_request_schema drops inherited base-type fields [Low severity, latent]

wsdl_coverage.py:304-305:

extension = complex_type.find("xsd:complexContent/xsd:extension", ns)
sequence = extension.find("xsd:sequence", ns) if extension is not None else complex_type.find("xsd:sequence", ns)

When a request element's inline complexType uses xsd:extension (inheriting from a base), only the extension's own sequence is captured — the base type's fields are discarded. This is the inverse of _collect_complex_type_fields which does follow inheritance.

Today this doesn't bite any of the PAYLOAD_CASES because all the tested add/update/delete/setBids request elements use direct xsd:sequence (no extension). But GetRequest elements commonly extend general:GetRequestGeneral, and if a get operation were ever added to PAYLOAD_CASES, its inherited required fields would be missing from required_fields in the assertion.

Since get operations are excluded from PAYLOAD_CASES by design, this is latent. No action needed now, but if the validator is ever extended to cover get, this would produce false passes on missing required base-type fields.


New Finding 3 — Stale INTENTIONAL_EXTRA_METHODS entries are silently tolerated

(Copilot raised this too.) test_service_method_coverage filters out extra methods that are in ALLOWED_EXTRA_METHODS, but never asserts the reverse: that every entry in INTENTIONAL_EXTRA_METHODS is actually present in the CLI as an extra method. If keywords archive is removed from the CLI in a future PR without updating INTENTIONAL_EXTRA_METHODS, no test fails — the stale allowlist entry becomes dead weight and silently masks any future regression that re-introduces a spurious method with the same name.

Suggested guard in test_service_method_coverage (or a separate test):

# Assert no stale entries in INTENTIONAL_EXTRA_METHODS
for (cli_name, method), _ in INTENTIONAL_EXTRA_METHODS.items():
    api_service = CLI_TO_API_SERVICE[cli_name]
    wsdl_ops = set(parse_wsdl_operations(fetch_wsdl(api_service)))
    cli_ops = get_cli_methods_for_service(cli_name)
    assert method in cli_ops, f"Stale INTENTIONAL_EXTRA_METHODS entry: {cli_name}.{method} not in CLI"
    assert method not in wsdl_ops, f"Stale INTENTIONAL_EXTRA_METHODS entry: {cli_name}.{method} now exists in WSDL"

Observation — check_wsdl_drift.py exit-code behaviour is correct but implicit

check_wsdl_drift.py returns exit 1 when drift is detected. The CI step Check live WSDL drift doesn't have continue-on-error: true, so the job fails on drift. The "Publish WSDL drift summary" and artifact upload steps run anyway (if: always()), and the JSON report is fully written before the non-zero exit, so reports are never truncated. Behaviour is correct; just noting it for anyone reading the workflow.


Summary

No new production-breaking bugs found beyond those in the previous review. The three findings above are all test-coverage quality issues:

  • Findings 1 & 2: The WSDL schema validator gives weaker guarantees than its name implies for imported types and inherited base types. The commands themselves are correct per WSDL inspection.
  • Finding 3: A simple counter-assertion would make INTENTIONAL_EXTRA_METHODS self-documenting and regression-proof.

None of these block merge; they're improvements to the test suite's own correctness guarantees.

Делает ограничения `get_operation_request_schema` и
`_assert_body_matches_wsdl` явными, а не скрытыми:

1. Типы из xsd:import не резолвятся — item_fields пустые для
   cross-namespace полей. Nested validation работает только для
   локально-определённых inline типов (agencyclients Notification,
   Settings, Grants проходят с пустыми dict-ами без ошибки).

2. xsd:complexContent/xsd:extension у request element теряет поля
   базового типа. Латентный риск для get* операций, которых сейчас
   нет в PAYLOAD_CASES.

Addresses review Findings 1 & 2 on PR #34 (claude[bot]).
Не меняет runtime, production-код корректен (проверено вручную
против WSDL). Сами команды валидируются на CLI dry-run уровне.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@axisrow

axisrow commented Apr 13, 2026

Copy link
Copy Markdown
Owner Author

Триаж ревью #2

Finding 1 (Medium) — imported XSD types не валидируютсяпринято, задокументировано (389df1b).
Подтверждаю: get_operation_request_schema смотрит только на локальные <xsd:complexType>, типы из <xsd:import> (gc:NotificationAdd, general:*) не резолвятся, _collect_complex_type_fields возвращает []. Production-команды корректны (проверено вручную против WSDL), но валидатор даёт более слабые гарантии, чем подразумевает его имя. Полное решение (резолв xsd:import) — отдельная задача; сейчас сделано минимальное: явно задокументировал ограничение в docstring get_operation_request_schema и в комментарии _assert_body_matches_wsdl.

Finding 2 (Low, latent) — xsd:extension теряет поля базызадокументировано (389df1b).
Проверено: ни один из текущих PAYLOAD_CASES не использует extension-based request type (все — direct xsd:sequence). Ревьюер сам отмечает latent. Docstring get_operation_request_schema теперь содержит предупреждение для будущих контрибьюторов.

Finding 3 — stale INTENTIONAL_EXTRA_METHODS — SKIP.
Counter-assertion защитила бы от мёртвого груза, но риск низкий: allowlist короткий и ведётся вручную, при удалении CLI-метода запись в INTENTIONAL_EXTRA_METHODS почти гарантированно заметна в diff-ревью. Добавлять 5 строк теста без явного сценария регрессии — over-engineering. Если когда-нибудь появится реальная регрессия — добавим тогда.

Observation — принято к сведению, никаких действий.

@axisrow

axisrow commented Apr 13, 2026

Copy link
Copy Markdown
Owner Author

@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

claude Bot commented Apr 13, 2026

Copy link
Copy Markdown

Claude finished @axisrow's task in 6m 19s —— View job


PR Review — Round 3

Tasks:

  • Gather context and understand request
  • Read new command files introduced in this PR
  • Read wsdl_coverage.py and test_api_coverage.py current state
  • Examine CI workflow
  • Check scripts/ for issues
  • Post review findings

Previously flagged items — all resolved

All bugs and findings from rounds 1 and 2 have been correctly addressed or documented. No action needed on those.


New Finding — agencyclients.add breaking-change is actually a correct fix

agencyclients.py diff changed:

# OLD (wrong — wrapped in non-existent Clients array)
body = {"method": "add", "params": {"Clients": [json.loads(client_json)]}}

# NEW (correct — matches WSDL AddRequest flat structure)
body = {"method": "add", "params": json.loads(client_json)}

The WSDL AddRequest for agencyclients is a flat object (Login, FirstName, LastName, Currency, Notification, …) — not wrapped in a Clients array. The old code was silently sending a malformed request that the API would have rejected. The new code is correct per the WSDL.

The updated test confirms this: assert body["params"]["Login"] == "client-login" and assert "Clients" not in body["params"]. No action needed — documenting here so reviewers don't interpret this as a regression.


New Finding — bids.set-auto correctly guards Scope, but guard ordering is worth noting

bids.py:set_auto checks:

if "Scope" not in bid_data:
    raise click.UsageError(...)

The WSDL confirms Scope is minOccurs="1" on BidSetAutoItem, so the guard is correct and necessary. The except click.UsageError: raise pattern is properly implemented to prevent the UsageError from being swallowed by the generic except Exception handler. No issue — noting this for completeness.


New Finding — keywordbids.set-auto omits target-ID validation [no action needed]

keywordbids.set-auto accepts all three of --campaign-id, --adgroup-id, --keyword-id as optional, with no check that at least one is provided. However, the KeywordBidSetAutoItem WSDL shows all three IDs as minOccurs="0" — the API explicitly allows calling setAuto without specifying a target (it would apply to all keywords matching the billing rule). No validation gap: CLI correctly mirrors the WSDL contract.


New Finding — dictionaries.get_geo_regions uses result.data [correct]

dictionaries.py:get_geo_regions does format_output(result.data, ...) which reviewers might flag as inconsistent with the result().extract() pattern used in most commands. This is the established pattern for this module: dictionaries.get, changes.check*, and keywordsresearch.* all use result.data. No issue.


New Finding — dynamicads.update removal is correct

The update subcommand was removed from dynamicads.py. The dynamictextadtargets WSDL only defines: add, get, delete, suspend, resume, setBids. There is no update operation. The removal is correct. The INTENTIONAL_EXTRA_METHODS entry is gone and the DRY_RUN_PAYLOAD_EXCLUSIONS entry does not reference it, so the self-consistency test (test_all_canonical_dry_run_commands_have_payload_coverage_or_exclusion) will pass cleanly.


Summary

No new production-breaking bugs, security vulnerabilities, or data-loss risks found in round 3. The changes in this PR are:

  1. Correct API fixesagencyclients.add payload shape was wrong; now matches the WSDL.
  2. New commands — all verified against cached WSDLs: advideos get/add, bids set-auto, keywordbids set-auto, audiencetargets set-bids, smartadtargets set-bids/suspend/resume, dynamicads set-bids/suspend/resume, agencyclients update/add-passport-organization/add-passport-organization-member.
  3. Lifecycle --dry-run — added to delete/archive/unarchive/suspend/resume across campaigns, ads, keywords, and others. Pattern is consistent.
  4. WSDL coverage infrastructure — correctly documented limitations (xsd:import, xsd:extension inheritance), CI workflow is safe.

PR is ready to merge.

@axisrow
axisrow merged commit bd98c65 into main Apr 13, 2026
3 checks passed
@axisrow
axisrow deleted the feat/wsdl-coverage-test branch April 13, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: automated API coverage check via WSDL introspection

3 participants