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

@codex @claude review. Focus on critical issues only: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, CLI contract violations, WSDL/API mismatch, milestone-scope gaps, and incorrect issue closure/triage. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, violate the public CLI contract, or make milestone 18 incomplete.

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

Review — PR #266 (milestone 18 optional-field gate)

Scope-filtered review per trigger: bugs, security, logic errors, data-loss risks, perf, CLI-contract, WSDL/API mismatch, milestone-scope gaps, triage. Style/naming/format intentionally skipped.

Verdict: no blocking issues. Two minor concerns worth flagging.

Verified clean

• WSDL/JSON shape for ArrayOf* matches tests/wsdl_cache/imports/general.xsd — NegativeKeywords (general:ArrayOfString) and NegativeKeywordSharedSetIds (general:ArrayOfLong) are correctly wrapped as {"Items": [...]} in adgroups.py:257,263,357,362. RegionIds stays bare (different schema: xsd:long w/ maxOccurs="unbounded"), consistent with existing code.
• WSDL required-combination invariants enforced: PointOnMap (X/Y/X1/Y1/X2/Y2 all minOccurs=1 per vcards.xml:48-57) → _build_point_on_map requires all six. InstantMessenger.MessengerClient/MessengerLogin both minOccurs=1 → _build_instant_messenger rejects partial. Both correct.
• PriceExtensionUpdateItem truly optional per ads.xml:437-444 (all four children minOccurs=0); partial payload test at tests/test_dry_run.py:1449 is WSDL-accurate (vs. PriceExtensionAddItem where Price/PriceQualifier/PriceCurrency are required — but that path isn't part of this PR).
• --clear-feed-login / --clear-feed-password semantics match UrlFeedUpdate nillable="true" in feeds.xml:94-95. Mutual-exclusion check at feeds.py:184-191 is correct.
• UsageError propagation: all new code paths use except click.UsageError: raise or validate before the try: block, so user-facing error messages aren't swallowed into click.Abort().
• Empty-subtype no-op guards updated to include new flags (adgroups.py:369, feeds.py:207). ads update still trips when only an incompatible flag is given.
• Issue triage / audit doc: tests/WSDL_OPTIONAL_FIELD_AUDIT.md removes every closed-issue row (adgroups add/update: add TrackingParams typed flags #242, adgroups add/update: add ad-group negative keyword typed flags #243, ads update TEXT_AD: add VideoExtension and PriceExtension typed flags #245, vcards add: add InstantMessenger and PointOnMap typed flags #246, feeds add/update: complete UrlFeed and FileFeed WSDL parity #253-urlfeed, retargeting lists: complete optional Description WSDL parity #256, sitelinks add: expose TurboPageId with typed CLI input #257), repoints FileFeed rows to feeds add/update: add FileFeed typed upload support #264 (matching PR-body statement that file-feed upload is split out). tests/test_wsdl_parity_gate.py OPTIONAL_FIELD_CLI_OPTIONS and OPTIONAL_FIELD_AUDIT mirror this consistently. Milestone-18 scope claim ("10 closed / 0 open") is supported by the diff.

Minor — not blocking, worth a follow-up

1. parse_sitelink_specs accepts negative TurboPageId via the pipe-spec path — direct_cli/utils.py:524 uses int(parts[3]), which accepts -1, +12, etc. The JSON path's _coerce_turbo_page_id (direct_cli/commands/sitelinks.py:18) rejects non-digit strings via .isdigit() but accepts negative ints from JSON. Inconsistent guardrails; the live API will reject negative IDs either way, so user impact is small. Fix this →

2. No way to clear AdGroup.NegativeKeywords / NegativeKeywordSharedSetIds — WSDL marks both nillable="true" (adgroups.xml:127-128), but the CLI uses if parsed_negative_keywords: so an empty list silently drops. The feeds module exposes --clear-feed-login/password for the analogous case; the adgroup module does not. Not part of the issue scope (adgroups add/update: add ad-group negative keyword typed flags #243 only asked for typed flags), so probably fine to defer — but worth tracking. Same applies to --tracking-params "" (also nillable in update path).

Things I deliberately did not flag

• README example replaces the basic adgroups update --name … --status … --region-ids … line with a new-flag-only line. Doc-only, not a CLI-contract change.
• retargeting --description "" produces Description: "" in payload while adgroups --tracking-params "" does not — asymmetric but covered by explicit tests; intentional.
• int() for --price-extension-price accepts negatives; price units are documented as × 1,000,000 and the API will reject.
  · branch feat/issue-239-optional-fields-gate

Addressed independent scope-review blocker in a5dbb45: CHANGELOG.md and CLAUDE.md now describe optional WSDL audit depth as unbounded, matching tests/WSDL_OPTIONAL_FIELD_AUDIT.md and the generator. Verification: rg confirms no stale 'depth 2' text; python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py -> 379 passed, 6 skipped.\n\n@codex @claude review. Focus on critical issues only: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, CLI contract violations, WSDL/API mismatch, milestone-scope gaps, and incorrect issue closure/triage. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, violate the public CLI contract, or make milestone 18 incomplete.

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

Review of PR #266 — milestone 18 optional-field scope

• Pull diff vs origin/main and inspect changed files
• Critical bugs / logical errors
• CLI contract & WSDL/API mismatch
• Milestone-scope gaps & issue triage
• Post review

Critical findings

1. Milestone-scope gap — CHANGELOG.md 0.3.12 section does not describe the seven typed-flag additions this PR ships

pyproject.toml:3 is already at version = "0.3.12", and the PR description says this milestone closes #242, #243, #245, #246, #253, #256, #257. The 0.3.12 section in CHANGELOG.md:3-27, however, only documents:

• direct ads update --callouts-* (ads update: support CalloutSetting (ADD/REMOVE/SET) for managing callouts on existing ads #238) — shipped by an earlier commit
• the soft WSDL optional-field audit (audit: WSDL parity gate skips optional fields — audit all *Update/*Add types for missed CLI flags (CalloutSetting pattern) #239)

None of the seven flag additions ship a public CHANGELOG entry, even though they ship under 0.3.12 as a public PyPI release:

This is a real publish-readiness gap: PyPI users upgrading to 0.3.12 will see a one-bullet release that omits every flag this PR is named after. Either each child PR's CHANGELOG bullet needs to land in this branch's 0.3.12 section, or the milestone-rollup commit on this branch should add them in a single batch before tagging.

Fix this →

Non-blocking observations (verified clean)

• a5dbb45 docs alignment is correct. OPTIONAL_FIELD_AUDIT_MAX_DEPTH = None in tests/test_wsdl_parity_gate.py:612, the generator emits unbounded (scripts/build_wsdl_optional_field_audit.py:159-162), and tests/WSDL_OPTIONAL_FIELD_AUDIT.md:4 shows unbounded. CHANGELOG.md and CLAUDE.md now match.
• Subtype gates hold. ads.py rejects --video-extension-* / --price-extension-* on non-TEXT_AD types via _reject_incompatible_flags (direct_cli/commands/ads.py:626-712), confirmed by test_ads_update_text_ad_price_extension_flags_rejected_for_mobile_app_ad and the TEXT_IMAGE_AD variant.
• PriceExtension enum values match WSDL. PriceCurrencyEnum (RUB, UAH, BYN, USD, EUR, KZT, TRY, CHF, UZS) and PriceQualifierEnum (NONE, FROM, UP_TO) in tests/wsdl_cache/ads.xml:311-330 are mirrored 1:1 in the click Choice enums.
• Empty-payload no-ops fire correctly. feeds update raises if neither --name, --url, --remove-utm-tags, credential set nor clear flag is given (direct_cli/commands/feeds.py:204-209, locked by test_feeds_update_without_fields_errors). adgroups update does the same with the extended message (direct_cli/commands/adgroups.py:368-374). ads update empty-subtype gate still trips even when only one extension flag is set, since the if text_ad: ad_data["TextAd"] = text_ad branch promotes the dict (direct_cli/commands/ads.py:736-775, locked by test_ads_update_text_ad_extension_flags_count_as_updatable_fields).
• UrlFeed credential-clear semantics. _url_feed_payload(...) emits {"Login": None} / {"Password": None} for clear-flags, and --feed-login + --clear-feed-login together are rejected with click.UsageError before any payload is built (direct_cli/commands/feeds.py:170-180). The conflict path is covered by test_feeds_update_rejects_setting_and_clearing_login.
• vcards add PointOnMap requires all six WSDL minOccurs=1 coordinates if any single one is supplied (direct_cli/commands/vcards.py:39-66), which matches tests/WSDL_OPTIONAL_FIELD_AUDIT.md:6076-6082.
• Sitelinks Href relaxation is correct API parity. _normalize_sitelink_row and the pipe-spec parser both require Href OR TurboPageId (no row drops a previously required field silently) — direct_cli/commands/sitelinks.py:43-48, direct_cli/utils.py:527-530. Existing Title|Href[|Description] callers stay unchanged.
• WSDL parity registry covers everything new. Every new field-path is added to OPTIONAL_FIELD_CLI_OPTIONS in tests/test_wsdl_parity_gate.py:635-897, and #253 only remains as a missing_followup rationale for FileFeed rows (correctly split to feeds add/update: add FileFeed typed upload support #264).
• Validation paths re-raise UsageError. adgroups.add and vcards.add both wrap user-facing validation with explicit except click.UsageError: raise ahead of the generic Exception handler, so character-limit and pair-incompleteness errors propagate as exit-code 2 rather than getting Abort()-swallowed.

I could not run pytest in this environment (no install permission). The trigger comment reports 379 passed, 6 skipped on tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py, which is consistent with the registry being kept in lock-step with the command additions.

Bottom line

No code-level critical bugs, security vulnerabilities, data-loss paths, CLI-contract violations or WSDL/API mismatches found. The only milestone-completeness blocker is the missing 0.3.12 CHANGELOG bullets for the seven typed-flag groups shipped on this branch — the release tag would otherwise misrepresent what 0.3.12 actually contains for PyPI users.
· Branch

Codex Review: Didn't find any major issues. You're on a roll.

Addressed Claude's CHANGELOG publish-readiness blocker in ebffa64: 0.3.12 now documents all milestone-18 typed-flag groups shipped by this branch (#242, #243, #245, #246, #253 URL-feed portion, #256, #257) plus the existing #238/#239 entries. Verification: python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py -> 379 passed, 6 skipped.\n\n@codex @claude review. Focus on critical issues only: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, CLI contract violations, WSDL/API mismatch, milestone-scope gaps, incorrect issue closure/triage, and release-note blockers. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, violate the public CLI contract, or make milestone 18 incomplete.

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

Review of PR #266 — milestone 18 optional-field scope (post-ebffa64)

• Inspect ebffa64 CHANGELOG diff vs prior review's blocker list
• Cross-check CHANGELOG bullets against actual shipped flags on this branch
• Re-verify no new critical issues introduced
• Post final verdict

Verdict: no remaining critical issues. Publish-readiness blocker resolved.

ebffa64 closes the CHANGELOG gap. The 0.3.12 section now ships an Added: bullet for every typed-flag group on this branch, plus the prior #238 (callouts) entry and the #239 audit-soft-gate entry under Changed:. pyproject.toml:7 is version = "0.3.12", so the public release will match what landed.

Important correction — my prior review had the issue→surface mapping wrong

My earlier comment listed:

• #242 → adgroups --negative-keywords (wrong)
• #246 → adgroups --tracking-params (wrong)
• #243 → retargeting --description (wrong)
• #256 → vcards InstantMessenger + PointOnMap (wrong)

The new CHANGELOG ships the correct mapping, which I verified against the original tests/WSDL_OPTIONAL_FIELD_AUDIT.md rows (git history of the doc, before the closed-row removal):

The CHANGELOG is correct as written; my prior table was the one with the scrambled labels. Apologies for the noise — the author addressed the real blocker (missing entries), not a phantom one. Nothing to change here.

Re-verified clean (unchanged from prior review)

• WSDL ArrayOf* wrapping for NegativeKeywords/NegativeKeywordSharedSetIds correct (adgroups.py).
• Subtype gates for TEXT_AD-only price/video extension flags (ads.py:626-712).
• Required-children invariants on PointOnMap (six coords) and InstantMessenger (client + login) enforced via UsageError before request build (vcards.py:39-66).
• UrlFeed credential clear semantics emit {"Login": None} / {"Password": None}; set+clear mutual-exclusion raised before payload build (feeds.py:170-191).
• Empty-subtype no-op guards still fire on feeds update (feeds.py:204-209), adgroups update (adgroups.py:368-374), ads update even when only an extension flag is set (ads.py:736-775).
• UsageError re-raise pattern preserved across all new code paths so validation errors propagate as exit-code 2 rather than being swallowed into Abort().
• Audit doc / parity gate / OPTIONAL_FIELD_CLI_OPTIONS registry mirror the shipped flags consistently; only FileFeed rows remain as missing_followup pointing to #264.

The reporter's pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py → 379 passed / 6 skipped is consistent with no regressions, but I could not re-run pytest locally in this environment.

Non-blocking carryover from prior review (still unaddressed, still not in milestone-18 scope)

1. parse_sitelink_specs pipe path uses int(parts[3]) which accepts negative TurboPageId; the JSON path's _coerce_turbo_page_id uses .isdigit() which also still accepts negatives once parsed as int. Live API will reject either way — defer.
2. No clear flag for AdGroup.NegativeKeywords / NegativeKeywordSharedSetIds / TrackingParams even though WSDL marks them nillable="true". Out of scope for #243/#242/#246; worth tracking as a follow-up but not a milestone blocker.
   · Branch

Codex Review: Didn't find any major issues. You're on a roll.