refactor: migrate agencyclients/adextensions get to make_get_command (part of #587)

[axisrow]

What

Finish the #587 get migration with the two structurally-divergent carve-outs. Follows #597 (criteria-limits) and #598 (nested *FieldNames).

Two more optional factory parameters:

• include_ids (default True) — set False for resources with no id filter. agencyclients get filters by --logins / --archived and has no --ids; it supplies a custom criteria_builder building {Archived, Logins?} from its own options.
• adextensions_wire_layout (default False) — a single-command flag reproducing adextensions's exact recorded wire layout: always emit SelectionCriteria (even empty) and order nested *FieldNames before Page.

The get callback signature moves ids to a trailing defaulted kwarg so both include_ids modes work (Click passes by keyword; absent --ids → None).

Why the empty SelectionCriteria is preserved (not "fixed")

It's a recorded API contract, not a latent bug:

• tests/cassettes/test_read_cassettes/test_read_command[adextensions_get].yaml replays {"method":"get","params":{"SelectionCriteria":{},"FieldNames":[...]}}.
• adextensions is deliberately excluded from test_dry_run._SELECTION_CRITERIA_REQUIRED_GET_COMMANDS — the live API accepts an empty criteria here, unlike adgroups/ads/keywords (which 4001 on it).

So adextensions_wire_layout preserves a contract; the docstring and code comments flag it as a do-not-normalize target.

Byte-identity

Verified each command against pre-migration baselines — all 11 snapshots identical, including the empty-SelectionCriteria case, the nested-*FieldNames-before-Page ordering, the --archived [YES|NO] [default: NO] rendering, and the provided-but-empty *FieldNames UsageError.

Tests

• Full offline suite green (2552 passed); test_read_cassettes, test_dry_run, test_api_coverage, test_comprehensive included.
• ruff check clean.

Remaining carve-out

• leads get — its --datetime-from / --datetime-to options sit between --limit and --fetch-all, a bespoke option order the shared get_options stack can't reproduce without a one-off insertion slot. Not worth contorting the factory for a single command; it stays hand-rolled. With this PR, make_get_command covers 10 of the 11 get commands in make_get_command: мигрировать get с вложенными полями + criteria-limit (Группы 2-3) #587's scope.

🤖 Generated with Claude Code

[axisrow]

@claude @codex 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 finished @axisrow's task in 1m 31s —— View job

---

Review: make_get_command carve-outs (agencyclients / adextensions)

• Reviewed _get.py factory changes
• Reviewed agencyclients.py migration
• Reviewed adextensions.py migration
• Verified wire-layout & error-message parity

Verdict: no critical issues found. This is a faithful, behavior-preserving refactor. Scope was limited to bugs / logical errors / data-loss as requested.

What I verified

1. ids signature move is safe. Moving ids from a positional 2nd arg to a trailing ids=None kwarg works in both modes because Click always invokes the callback with keyword arguments (only ctx is positional via @click.pass_context). When include_ids=False, no --ids option is registered, so Click never passes ids and it correctly defaults to None → empty Ids filter. ✅

2. adextensions wire layout — key ordering preserved. _get.py:182-198 reproduces the exact original sequence:

• {SelectionCriteria, FieldNames} → CalloutFieldNames (nested update) → Page (only when limit).
• Original (adextensions.py): same SC, FieldNames, CalloutFieldNames, Page order.
• Empty SelectionCriteria: {} is always emitted, matching the recorded cassette contract. ✅

3. agencyclients non-wire path unchanged. Still routes through build_common_params (adds Page when limit), then params.update(nested) appends the *FieldNames after Page — byte-identical to the pre-migration body. Archived is always present (option default="NO"), so criteria is never empty. ✅

4. Empty-CSV UsageError message is byte-identical. The original adextensions hand-rolled check raised "Provide a non-empty comma-separated CalloutFieldNames list.". The factory now routes through parse_nested_field_names → parse_field_names_option (utils.py:88-93), which formats "Provide a non-empty comma-separated {wsdl_key} list." with wsdl_key="CalloutFieldNames" → identical string. It also still raises before the request is built (covers both --dry-run and live paths). ✅

5. No data-loss / silent-drop risk. Every extra option and nested-field kwarg is funneled through **kwargs → build_criteria(ids, **kwargs) (absorbed via **_) and kwargs[kwarg] for nested specs. No option is dropped on the floor.

Minor (non-blocking) note

include_ids=False is only safe in combination with a custom criteria_builder — the default builder would produce an empty criteria with no way to filter. The docstring already states this dependency (_get.py:125-129), so it's documented rather than a latent trap. Nothing to change.

The byte-identity claims in the PR description hold up against the code. LGTM.
• feat/587-carveouts-noids-adext

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: c6a9de51bf

ℹ️ 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".