refactor(commands): make_get_command factory for the v5 get skeleton (#582)

[axisrow]

Closes #582. Part of epic #584 (аудит дублирования кода).

Что сделано

Введена фабрика direct_cli/commands/_get.py::make_get_command — общий скелет тела v5 get-команды (разрешить поля → собрать SelectionCriteria → общие params → dry-run печать или post + format), по образцу make_lifecycle_command. На неё переведены 5 самых простых get-команд: advideos, businesses, negativekeywordsharedsets, vcards, turbopages.

Вариативность ресурсов фабрика покрывает небольшими «ручками»: default_fields_key, ids_help/ids_required, extra_options (--bound-with-hrefs у turbopages) и опциональный criteria_builder (строковые Ids у advideos, BoundWithHrefs у turbopages).

Patchability — строго лучше lifecycle-фабрики

Lifecycle-фабрика захватывает create_client в замыкании при загрузке модуля, из-за чего patch.object(module, "create_client") не перехватывает live-путь (латентный дефект; lifecycle-команды покрыты только через --dry-run). Здесь это исправлено: новый api.resolve_module_create_client перечитывает create_client из живого модуля команды в момент вызова, поэтому patch.object(<module>, "create_client", ...) — используемый юнит-тестами и coverage wire-payload capture (scripts/build_api_coverage_report.py) — перехватывает live-путь. Именно это держит зелёным schema-parity coverage-гейт.

Байт-идентичность (проверено)

• --help (порядок опций, i18n-ключи) и --dry-run payload байт-идентичны для всех 5 команд (сверка с эталоном до рефакторинга, включая строковые vs целочисленные Ids и BoundWithHrefs).
• VCR-cassette read-тесты (test_read_cassettes.py) для этих 5 команд проходят — live-путь не изменился.
• Полный офлайн-прогон: 2535 passed, 23 skipped; ruff чисто.
• /simplify: 4 ревьюера; применён вынос resolve_module_create_client в api.py, приватизация _default_ids_criteria, удаление неиспользуемого service; декларативный criteria-механизм отложен в follow-up под реальные требования групп 2-3 (YAGNI).

Follow-up (под эпиком #584)

Остальная миграция вынесена отдельными issue (расширяют фабрику под свои паттерны с собственной верификацией байт-идентичности):

• Группа 2 (вложенные *FieldNames + add_criteria_csv): adextensions, adimages, leads, retargeting, sitelinks, feeds, clients, agencyclients.
• Группа 3 (enforce_criteria_array_limits): dynamicads, smartadtargets, dynamicfeedadtargets, bids, bidmodifiers, reports, keywordbids.
• Группа 4 (сложные: конфликты флагов, обязательные фильтры): keywords, campaigns, ads, adgroups, strategies, creatives, audiencetargets.
• Хелперы fix(ads): stop sending implicit Type field in add command #11 (lifecycle-обёртки), fix+test: stop sending implicit Type field in add commands; add dry-run coverage #12 (make_set_bids_command), feat(ads get): add --text-ad-fields for type-specific field retrieval #13 (execute_add).

Diff: +198 / −214 (нетто −16).

🤖 Generated with Claude Code

[axisrow]

@claude @codex review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. This is a dedup refactor (#582) that introduces a make_get_command factory and migrates 5 simple get-commands; it must preserve the CLI surface 1:1 — pay special attention to (a) byte-level divergence in --help option order, i18n help keys, or --dry-run payloads vs pre-refactor; (b) the dynamic create_client resolution via api.resolve_module_create_client (patchability for tests + coverage wire-payload capture); (c) the --help option ordering produced by applying extra_options/--ids around get_options. Scope is intentionally Group 1 only — Groups 2-4 and sibling helpers are tracked as follow-ups #587/#588/#589. Do NOT nitpick style, naming, or formatting.

[claude]

Claude finished @axisrow's task in 5m 0s —— View job

---

Review: make_get_command factory (#586)

Focused on correctness/behavioral divergence per the request. Static analysis only — python/pip execution is blocked in this sandbox (permission-gated), so I verified the Click option-ordering and payload semantics by reading the code rather than running --help/--dry-run. The author's reported 2535 passed + VCR-cassette evidence covers the runtime side.

Verdict: no critical bugs. The CLI surface (option order, i18n help strings, dry-run payloads) is preserved, and the patchability mechanism is sound and genuinely better than the lifecycle factory. One behavioral divergence is worth calling out (item 1) — it's almost certainly an improvement, but it's not "byte-identical behavior" and the PR text glosses over it.

1. Behavioral divergence: --dry-run no longer resolves credentials (not a payload change)

In all 5 pre-refactor commands, the client was built before the dry_run check:

def get(...):
    client = client_from_ctx(ctx, create_client)   # <-- ran even in dry-run
    ...
    if dry_run:
        format_output(body, "json", None); return

client_from_ctx → create_client → get_credentials, which raises ValueError when no token is configured (auth.py:899). So pre-refactor, advideos get --dry-run with no creds errored out. The factory returns before client_from_ctx (_get.py:93-103), so dry-run now succeeds without credentials.

• The printed payload is byte-identical — this only changes whether creds are required.
• It's consistent with how add/update dry-run already behaves and with CLAUDE.md ("print request JSON without calling the API"), so it reads as a positive fix, not a regression.
• It doesn't break tests: _read_dry_run (tests/test_dry_run.py:74) still injects dummy creds (now unused for these commands), and _dry_run for writes never passed creds.

Suggestion (non-blocking): lock the new behavior in with a test asserting a migrated get --dry-run succeeds with no credentials, and/or drop the now-unnecessary dummy-cred injection for these commands. Otherwise the old "dry-run needs a token" contract silently disappears with nothing asserting the new one.

2. Option ordering — verified correct ✅

Click reverses __click_params__, so display order = reverse of decorator-application order. The factory applies get_options → reversed(extra_options) → --ids (_get.py:116-119), reproducing the original top-to-bottom stack --ids, [resource options], then the six get_options entries. The reversed(extra_options) is necessary and correct for the multi-extra-option case (turbopages' single --bound-with-hrefs lands between --ids and --limit, matching the original).

3. Patchability via resolve_module_create_client — sound ✅

module_name = group.callback.__module__ resolves to the real command module (direct_cli.commands.advideos, etc.), and getattr(sys.modules[module_name], "create_client", fallback) is re-read at call time (api.py:108-111), so patch.object(<module>, "create_client", …) intercepts the live path. This is strictly better than the lifecycle closure capture, as claimed. The fallback guard for a missing module/attr is reasonable defensive code.

4. Payload / criteria semantics — match originals ✅

• advideos: string Ids via parse_csv_strings(ids) or [] (required=True) — preserved.
• businesses / vcards / negativekeywordsharedsets: integer Ids via parse_ids, omitted when absent (_default_ids_criteria) — preserved.
• turbopages: Ids (parse_ids) + BoundWithHrefs (parse_csv_strings) — preserved; extra option threaded through **kwargs → criteria_builder.
• field_names, build_common_params, fetch_all/extract branches identical.

Minor notes (non-blocking)

• help_text=None default would yield blank help if a future caller forgets it; the inner get has no docstring to fall back to. All 5 current callers pass it, so fine for Group 1.
• Module-level get = make_get_command(...) binding is unused (registration happens via group.command); harmless, mirrors the lifecycle pattern.

---

• Read factory _get.py and api.resolve_module_create_client
• Verify --help option order & i18n keys (static)
• Verify --dry-run payload identity (incl. string vs int Ids, BoundWithHrefs)
• Audit dynamic create_client resolution / patchability
• Post review
  · branch refactor/582-make-get-command

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 3dba156a56

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[axisrow]

Thanks @claude @codex. Triage:

Item 1 — dry-run no longer resolves credentials. Confirmed and intentional: the factory builds the client after the --dry-run guard, which is exactly what the established _lifecycle.py factory already does (if dry_run: ... return; client = client_from_ctx(...)), and matches CLAUDE.md ("print request JSON without calling the API"). Note the test suite already encodes the old wart — get-commands used a dedicated _read_dry_run helper that injects dummy creds because they used to resolve credentials in dry-run. Locked the new behavior in with a4214f5: test_make_get_command_dry_run_needs_no_credentials invokes all 5 migrated commands via _dry_run (empty env) and asserts success, so credential resolution can't silently creep back. Updated the PR framing — the payload is byte-identical; this side-effect is a deliberate alignment with the sibling factory.

Minor notes (non-blocking, left as-is):

• help_text=None blank-help risk — all current callers pass it; the follow-up groups will too. Acceptable for Group 1.
• Unused module-level get = make_get_command(...) binding — harmless, mirrors the lifecycle factory's delete = make_lifecycle_command(...) pattern (registration happens via group.command).

Codex: no issues. Full offline suite green (2536 passed), ruff clean. Proceeding to merge once CI is green.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[claude]

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

---

Triage confirmed — resolution is sound ✅

Thanks @axisrow. I checked the lock-in commit a4214f5 and your triage holds up.

Item 1 — dry-run no longer resolves credentials. Agreed on all points:

• The factory building the client after the --dry-run guard mirrors _lifecycle.py exactly and matches CLAUDE.md ("print request JSON without calling the API"). This is an intentional alignment, not a regression — the printed payload stays byte-identical.
• test_make_get_command_dry_run_needs_no_credentials (tests/test_dry_run.py:99) pins it correctly: it routes all 5 migrated commands through _dry_run (which, unlike _read_dry_run, injects no dummy creds — test_dry_run.py:63-71) and asserts exit 0 + body["method"] == "get". So credential resolution can't silently creep back into the dry-run path. Good catch on keeping --ids 1 for advideos (its Ids is required=True).

Minor notes — agree, leave as-is:

• help_text=None blank-help risk is theoretical; all current callers pass it. Fine for Group 1.
• The unused module-level get = make_get_command(...) binding mirrors the lifecycle factory's delete = make_lifecycle_command(...) convention. Harmless.

No remaining concerns from my side — no critical bugs, CLI surface preserved 1:1, and the resolve_module_create_client patchability mechanism is genuinely better than the lifecycle closure capture. 👍 to merge once CI is green.
· branch refactor/582-make-get-command