[codex] Add retargeting description flags by axisrow · Pull Request #260 · axisrow/direct-cli

axisrow · 2026-05-24T03:44:24Z

Summary

Add typed --description support to retargeting add and update.
Validate RetargetingList*.Description at the documented 4096 character max.
Mark RetargetingList Description and existing --rule argument paths supported in the WSDL audit.

Verification

python3 -m pytest tests/test_dry_run.py -k 'retargeting and description'
python3 -m pytest tests/test_wsdl_parity_gate.py
python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py

axisrow · 2026-05-24T03:44:45Z

@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, API contract mismatches, and missing tests. 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 · 2026-05-24T03:44:56Z

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

Review of PR #260 — retargeting `--description`

Read PR diff against feat/issue-239-optional-fields-gate
Review retargeting.py changes
Review test_dry_run.py changes
Review WSDL audit + parity gate changes
Post review

Verdict: LGTM — no critical issues found

Focused review per request (bugs, security, data loss, API contract, missing tests). No blocking issues.

Findings

1. Empty-string description silently drops on add — minor UX nit, not a data-loss bug (direct_cli/commands/retargeting.py:125)

if description:
    list_data["Description"] = description

A user passing --description "" on add will have it silently omitted from the payload. The update path is protected by the no-op guard (and you added a test for it — tests/test_dry_run.py:4671), but add has no equivalent safety. In practice this is harmless because add also requires --rule, so an empty --description just means "no description sent" — which matches the WSDL minOccurs=0. Not a correctness bug; flagging only because it's an inconsistency between the two commands.

2. _validate_description ordering in add runs before the --rule check (direct_cli/commands/retargeting.py:117-119)

If a user invokes add --name X --description "<huge>" with no --rule, they'll get the length error first instead of the missing-rule error. Cosmetic ordering issue only — both are UsageError, both are recoverable, both messages are accurate.

3. WSDL audit reclassification is accurate (tests/WSDL_OPTIONAL_FIELD_AUDIT.md)

Marking Rules.Arguments.MembershipLifeSpan and Rules.Arguments.ExternalId as supported via --rule is correct — parse_retargeting_rule_specs in direct_cli/utils.py:446-448 already emits both fields. Not new coverage from this PR, just honest reclassification of pre-existing behavior. Good cleanup.

4. 4096-char limit enforced as character count, not byte count

Yandex Direct's documented limits for retargeting list descriptions are character-based, so len(description) is the right unit. No issue.

5. Test coverage is solid

Boundary cases all covered:

4096 chars accepted (test_retargeting_add_description_accepts_4096_chars)
4097 chars rejected on both add and update
add payload includes Description when supplied
update payload includes Description and Id only
update --description "" correctly triggers no-op guard

The parity gate entries for Description / Rules.Arguments.* line up with the audit reclassification.

Security / data-loss / API-contract: no issues found.

Performance: N/A.

• Branch: codex/issue-256-retargeting-description

Copilot

Pull request overview

Adds typed --description support to the retargeting CLI so RetargetingList Description is explicitly supported, validated, and reflected in WSDL parity/audit tooling.

Changes:

Add --description to direct retargeting add and direct retargeting update, with a 4096-character validation guard.
Extend dry-run tests to assert Description payload behavior and max-length rejection.
Mark Description (and existing --rule-covered nested rule paths) as supported in WSDL parity gate + optional-field audit; update README examples.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
direct_cli/commands/retargeting.py	Adds `--description` options and validates Description length before building request payloads.
tests/test_dry_run.py	Adds dry-run coverage for Description payload inclusion and length-limit enforcement.
tests/test_wsdl_parity_gate.py	Declares `Description` (and `--rule` nested paths) as supported to satisfy parity gate expectations.
tests/WSDL_OPTIONAL_FIELD_AUDIT.md	Updates audit classification counts and marks retargeting Description/rule paths as supported.
README.md	Updates English/Russian usage examples to include `--description`.

+def _validate_description(description: Optional[str]) -> None:
+    """Validate RetargetingList*.Description documented API constraints."""
+    if (
+        description is not None
+        and len(description) > _RETARGETING_DESCRIPTION_MAX_LENGTH
+    ):
+        raise click.UsageError(
+            "--description must be at most 4096 characters for "
+            "RetargetingList*.Description"
+        )


chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f25fd48ad2

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

chatgpt-codex-connector · 2026-05-24T03:47:47Z

+        if description:
+            list_data["Description"] = description


Honor explicit empty --description on retargeting update

The update path drops --description "" because it only forwards Description when description is truthy, so an explicit empty value is silently ignored. In practice, direct retargeting update --id <id> --name ... --description "" updates the name but cannot clear an existing description, even though the CLI accepts the flag. This creates an API contract mismatch for typed input and can leave stale data in production when operators expect a field reset.

Useful? React with 👍 / 👎.

axisrow · 2026-05-24T03:50:31Z

Addressed the actionable review feedback in 7163b6c:

--description now preserves explicit empty strings on add and update instead of silently dropping the provided typed flag;
the max-length UsageError now uses _RETARGETING_DESCRIPTION_MAX_LENGTH and matches the existing wording style.

Verification:

python3 -m pytest tests/test_dry_run.py -k 'retargeting and description'
python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py

@codex @claude review. Please re-check only blocking issues: bugs, contract mismatches, and missing tests.

claude · 2026-05-24T03:50:41Z

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

Re-review of PR #260 — addressing feedback in `7163b6c`

Read latest diff against feat/issue-239-optional-fields-gate
Review retargeting.py empty-description handling
Review test coverage for empty-string preservation
Verify max-length constant usage
Post findings

Verdict: LGTM — feedback fully addressed, no blocking issues

Focused re-review per request (bugs, contract mismatches, missing tests).

What changed in `7163b6c`

direct_cli/commands/retargeting.py:125,180 — if description: → if description is not None: on both add and update. Explicit --description "" is now forwarded to the API instead of being silently dropped.
direct_cli/commands/retargeting.py:78-81 — UsageError message now uses _RETARGETING_DESCRIPTION_MAX_LENGTH via f-string (no more hard-coded 4096) and drops the RetargetingList*.Description suffix to match the established CLI error wording.
tests/test_dry_run.py:3269 — new test_retargeting_add_empty_description_payload asserts Description == "" is included on add.
tests/test_dry_run.py:4687 — old test_retargeting_update_empty_description_no_op_rejected correctly replaced by test_retargeting_update_empty_description_payload, which asserts the payload becomes {"Id": 55, "Description": ""} — exactly the contract the Codex P2 comment flagged.

Re-check against blocking-issue criteria

API contract mismatch — Resolved. The Codex finding that update --description "" could not clear a stored description is fixed; the typed flag now reaches the API. The previous add-vs-update inconsistency I flagged is also gone (both now use is not None).

Bugs — None spotted.

_validate_description correctly skips when description is None (flag not provided), accepts "", accepts exactly 4096 chars, rejects 4097.
update no-op guard (len(list_data) == 1) is unaffected: when only --id is passed, description is None so Description is not added, length stays 1, UsageError still fires. The behavioral change only kicks in when the user explicitly passes --description.

Missing tests — None. Boundary coverage is complete:

add empty / non-empty / 4096 / 4097
update empty / non-empty / 4097
update no-arg still rejected via existing len(list_data) == 1 guard (not part of this PR's surface, but unchanged)

Parity gate / audit — tests/test_wsdl_parity_gate.py:786,793 and tests/WSDL_OPTIONAL_FIELD_AUDIT.md correctly reclassify Description and Rules.Arguments.* as supported. Counts move 387→393 supported, 2854→2848 missing_followup. Math checks out (6 entries moved).

Performance / security / data loss — N/A.

Note on API behavior (informational, not blocking)

The change assumes Yandex Direct accepts Description: "" on update as "clear the field." That matches the standard v5 pattern for optional string fields (minOccurs=0), and is the contract that operators expect when typing the flag explicitly. If the live API rejects empty Description on add, the error surfaces through the normal print_error path — no silent loss. No action required here; flagging only so the team is aware that the actual clear-on-update behavior should be confirmed in a sandbox smoke if not already done.
• Branch: codex/issue-256-retargeting-description

chatgpt-codex-connector · 2026-05-24T03:52:23Z

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

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

* [codex] Add adgroup tracking params flags (#258) * Add adgroup tracking params flags * test: cover adgroup tracking params add rejection --------- Co-authored-by: axisrow <[email protected]> * [codex] Add sitelink TurboPageId typed input (#259) * feat(sitelinks): support turbo page id * test(sitelinks): cover turbo page json input --------- Co-authored-by: axisrow <[email protected]> * [codex] Add retargeting description flags (#260) * feat(retargeting): add description flags * fix(retargeting): preserve empty description --------- Co-authored-by: axisrow <[email protected]> * [codex] Add adgroups negative keyword flags (#261) * feat(adgroups): add negative keyword flags * fix(adgroups): report invalid negative set ids as usage errors --------- Co-authored-by: axisrow <[email protected]> * [codex] Add vcards nested contact flags (#262) * feat(vcards): add nested contact fields * fix(vcards): preserve grouped flag usage errors --------- Co-authored-by: axisrow <[email protected]> * feat(ads): add text ad extension update flags (#263) Co-authored-by: axisrow <[email protected]> * feat(feeds): add url feed detail flags (#265) Co-authored-by: axisrow <[email protected]> * docs: align optional WSDL audit depth notes * docs: document milestone 18 typed flags --------- Co-authored-by: axisrow <[email protected]>

feat(retargeting): add description flags

f25fd48

Copilot AI review requested due to automatic review settings May 24, 2026 03:44

Copilot started reviewing on behalf of axisrow May 24, 2026 03:44 View session

Copilot AI reviewed May 24, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed May 24, 2026

View reviewed changes

fix(retargeting): preserve empty description

7163b6c

axisrow merged commit dcd9e4e into feat/issue-239-optional-fields-gate May 24, 2026
6 checks passed

axisrow deleted the codex/issue-256-retargeting-description branch May 24, 2026 03:56

axisrow mentioned this pull request May 24, 2026

retargeting lists: complete optional Description WSDL parity #256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[codex] Add retargeting description flags#260

[codex] Add retargeting description flags#260
axisrow merged 2 commits into
feat/issue-239-optional-fields-gatefrom
codex/issue-256-retargeting-description

axisrow commented May 24, 2026

Uh oh!

axisrow commented May 24, 2026

Uh oh!

claude Bot commented May 24, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 24, 2026

Uh oh!

axisrow commented May 24, 2026

Uh oh!

claude Bot commented May 24, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented May 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

axisrow commented May 24, 2026

Summary

Verification

Uh oh!

axisrow commented May 24, 2026

Uh oh!

claude Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of PR #260 — retargeting --description

Verdict: LGTM — no critical issues found

Findings

Security / data-loss / API-contract: no issues found.

Performance: N/A.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 24, 2026

Choose a reason for hiding this comment

Uh oh!

axisrow commented May 24, 2026

Uh oh!

claude Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Re-review of PR #260 — addressing feedback in 7163b6c

Verdict: LGTM — feedback fully addressed, no blocking issues

What changed in 7163b6c

Re-check against blocking-issue criteria

Note on API behavior (informational, not blocking)

Uh oh!

chatgpt-codex-connector Bot commented May 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented May 24, 2026 •

edited

Loading

Review of PR #260 — retargeting `--description`

claude Bot commented May 24, 2026 •

edited

Loading

Re-review of PR #260 — addressing feedback in `7163b6c`

What changed in `7163b6c`