@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 finished @axisrow's task in 2m 52s —— View job

Code Review — Group-4 get migration to make_get_command

• Read the diff vs origin/main
• Review _get.py factory ordering change
• Review migrated command modules (adgroups, keywords, creatives, strategies, audiencetargets)
• Check regression test coverage
• Post review

Reviewed for the requested focus only (bugs, logic errors, data-loss, performance). No critical issues found — this is a faithful, behavior-preserving migration.

Factory ordering change (_get.py) — correct

The move of nested-*FieldNames parsing to before enforce_criteria_array_limits and the require_criteria_message guard (_get.py:197-208) is the right fix and matches every pre-migration hand-rolled body:

• The parsed dict is still merged after build_common_params (_get.py:218-219), so payload key order is unchanged. ✅
• if parsed_nested: replacing if nested_specs: is safe — parse_nested_field_names returns {} when nothing was provided, and params.update({}) was already a no-op, so the result is identical. ✅
• Error precedence now resolves to the nested error for --<nested> "" + over-limit and --<nested> "" + no-filter, pinned by the new test_get_empty_nested_field_names_precedes_criteria_limit regression test. ✅

Per-module migrations — faithful

I traced each criteria_builder against its original inline body:

• adgroups / keywords — the --status/--statuses mutual-exclusion guard correctly lives inside the criteria_builder (adgroups.py:427, keywords.py), which runs before nested parsing — preserving the original "mutual-excl first" ordering. criteria_limits → require ordering preserved.
• keywords nested error message — the old bespoke UsageError ("Provide a non-empty comma-separated <key> list.") is byte-identical to what parse_field_names_option (utils.py:88-93) raises, so the i18n key is unchanged. ✅
• creatives / strategies — no criteria_limits in the originals, correctly omitted; Types casing (parse_csv_upper for creatives vs plain add_criteria_csv for strategies) and the --is-archived choice/default are preserved.
• audiencetargets — long require-message passed through t() identically; option order (--ids → extras → get_options) preserved.

Minor / non-blocking

• Client resolution now happens only on the live path, so --dry-run becomes token-free for these five commands. This shifts error ordering on the live path (validation errors now precede a missing-token error). The PR documents this as intentional and it's consistent with all prior factory migrations — flagging only for visibility, not a defect.

Verification note

I could not execute pytest/pip install in this sandbox (commands blocked by the environment), so I relied on static tracing rather than a live run. The PR reports the full offline suite green (2554 passed) and the logic checks out on inspection. If CI is green on this branch, I'd consider the behavioral invariants confirmed.

Verdict: ✅ Looks good — net −278 lines, no functional or data-loss risk identified.
· refactor/issue-588-group4-clean-fits

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 6c756fc449