@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, documentation or test contract regressions, and issue-scope gaps. Do not nitpick style, naming, minor formatting, or subjective preferences.

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

Claude Code Review

• Read PR diff and changed files
• Verify scalar autotargeting flag handling in keywords.py
• Check batch-mode rejection logic and tests
• Check dry-run payload emission and tests
• Check WSDL audit / parity gate updates
• Post review findings

Focused review on bugs / contract regressions / scope gaps. Skipping style.

1. README example regression — loss of "normal keyword" demonstration

README.md:425 and README.md:1137 were the only one-line demos of keywords add for an ordinary keyword ("buy laptop" / "купить ноутбук") with --bid + --context-bid + user params. The PR overwrites both with an autotargeting-only example using the placeholder text ---autotargeting:

-direct keywords add --adgroup-id 12345 --keyword "buy laptop" --bid 10500000 --context-bid 5250000 --user-param-1 segment-a --user-param-2 segment-b --dry-run
+direct keywords add --adgroup-id 12345 --keyword "---autotargeting" --bid 10500000 --context-bid 5250000 --autotargeting-search-bid-is-auto YES --priority HIGH --user-param-1 segment-a --user-param-2 segment-b --dry-run

After this change the README has no example of adding a regular keyword. Recommend keeping the original line and adding the autotargeting line next to it, rather than overwriting — otherwise every user who reads the README to figure out how to add a normal search keyword now sees only the special autotargeting placeholder, which is confusing for the common case.

Fix this →

2. WSDL audit marks both fields supported but batch mode has zero coverage

tests/WSDL_OPTIONAL_FIELD_AUDIT.md flips both rows to supported with rationale "--autotargeting-search-bid-is-auto" / "--priority". That is true for single-item mode only:

• direct_cli/commands/keywords.py:346-351 raises UsageError if the flag is passed alongside --from-file / --keywords-json.
• _KEYWORD_ROW_FIELDS (direct_cli/commands/keywords.py:32-39) does NOT include AutotargetingSearchBidIsAuto / StrategyPriority, so per-row JSONL/JSON values trigger the unknown-field error in _normalize_keyword_row (line 85-91).

Other fields currently marked supported (Bid, ContextBid, UserParam1, UserParam2) are reachable through both the single-item flag and the JSONL row schema. The two new rows are not — the typed flag exists, but a user with a 50-row JSONL has no way to set these fields per item. The audit's supported label therefore overstates real coverage for these two rows.

If the intent is "single-item flag is enough to close #285 and #289 tracks batch parity," at minimum the audit/note should reflect that partial state (e.g. keep missing_followup with #289, or add a note: qualifier), otherwise the audit becomes ambiguous as a contract surface. The PR body says "reject the new scalar autotargeting flags in batch modes so they are not silently ignored before #289" — that confirms batch coverage is deliberately deferred, but WSDL_OPTIONAL_FIELD_AUDIT.md no longer records that fact anywhere.

Fix this →

3. Batch-rejection test only asserts one of the two flags

tests/test_dry_run.py::test_keywords_add_rejects_scalar_autotargeting_flags_in_batch_mode only invokes the CLI with --priority. The rejection check at keywords.py:347 uses an or, so a regression that drops the autotargeting_search_bid_is_auto is not None half of the condition would not be caught. Suggest either a second test case or a parametrize over both flags.

Fix this →

Looks good

• case_sensitive=False on the click.Choice plus .upper() on the value correctly normalizes user input before insertion into the payload — no risk of mixed-case enums reaching the API.
• Unknown JSONL row fields raise UsageError rather than silently dropping (_normalize_keyword_row), so users who write AutotargetingSearchBidIsAuto per row get a clear failure, not silent data loss — this matches the WSDL parity gate's "silent data loss" invariant.
• The dry-run payload assertion in tests/test_dry_run.py:2820-2826 pins the exact shape including absence of Bid / ContextBid when not passed — good coverage.
• OPTIONAL_FIELD_CLI_OPTIONS in tests/test_wsdl_parity_gate.py:934-942 correctly registers the new CLI flags against the WSDL fields.

· Branch: codex/issue-285-keywords-scalar-autotargeting

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

Addressed Copilot feedback in 1c180c6: batch mode now rejects all single-item-only flags (--bid, --context-bid, --autotargeting-search-bid-is-auto, --priority, --user-param-1, --user-param-2) instead of silently ignoring them. Re-ran: python3 -m pytest tests/test_dry_run.py -k "keywords_add and batch_mode"; python3 -m pytest tests/test_dry_run.py -k "keywords_add and scalar_autotargeting"; python3 scripts/build_wsdl_optional_field_audit.py --check; 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; mypy .

@codex @claude review. Re-run after addressing batch-mode single-item flag feedback. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, documentation or test contract regressions, and issue-scope gaps. Do not nitpick style, naming, minor formatting, or subjective preferences.

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

Claude Code Re-Review

• Read updated diff & commit 1c180c6
• Verify batch-mode single-item flag rejection
• Check test coverage parametrization
• Re-check WSDL audit row claims
• Re-check README example status

Re-reviewed after 1c180c6. The batch-mode fix correctly addresses the previous data-loss concern. Notes below.

Looks good in the fix

• direct_cli/commands/keywords.py:346-362 — the rejection now covers all single-item-only top-level flags (--bid, --context-bid, --autotargeting-search-bid-is-auto, --priority, --user-param-1, --user-param-2) via a flag→value dict, which closes the silent-ignore data-loss gap Copilot flagged. This converts a previously silent UX trap into an explicit UsageError, matching the project's "silent data loss" WSDL parity invariant.
• The error message names every offending flag, so users running 50-row JSONL batches with an accidental top-level --bid get a clear, actionable message instead of a missing field on the server.
• keywords.py:336/341 keeps _KEYWORD_ROW_FIELDS as the per-row contract; --bid / --context-bid / --user-param-1 / --user-param-2 remain reachable per-row in JSONL — only the top-level form is rejected. That's the right scoping.

Remaining critical concerns (carried over)

1. README example regression — still not addressed

README.md:425 and README.md:1137 still overwrite the only one-line demos of keywords add for an ordinary keyword ("buy laptop" / "купить ноутбук") with an autotargeting-only example using the placeholder text ---autotargeting. After this PR the README has no example of adding a regular search keyword. This was the previous review's point #1 and is unchanged on this branch.

Fix this →

2. Batch-rejection test still doesn't exercise --autotargeting-search-bid-is-auto

tests/test_dry_run.py:2820-2829 (test_keywords_add_rejects_scalar_autotargeting_flags_in_batch_mode) still only invokes the CLI with --priority HIGH. The new bundle test at tests/test_dry_run.py:2832-2854 (test_keywords_add_rejects_single_item_flags_in_batch_mode) covers --bid, --context-bid, --user-param-1 but also omits --autotargeting-search-bid-is-auto and --user-param-2.

The current implementation iterates a dict so the risk is lower than the previous or chain, but a regression that drops the --autotargeting-search-bid-is-auto row from single_item_flags would still pass every existing test. Suggest extending the bundle test to include all six flags, or @pytest.mark.parametrize over each flag individually.

Fix this →

3. WSDL audit still marks both fields supported without recording the single-item-only constraint

tests/WSDL_OPTIONAL_FIELD_AUDIT.md:5577,5579 rationale is --autotargeting-search-bid-is-auto / --priority — same shape as Bid / ContextBid / UserParam1 / UserParam2. The asymmetry is that the four older rows are reachable both via the single-item flag and per-row JSONL (they're in _KEYWORD_ROW_FIELDS at direct_cli/commands/keywords.py:32-39), while the two new ones are reachable only via single-item flag — batch users with a 50-row JSONL cannot set them per item, and the per-row schema actively rejects them (_normalize_keyword_row at keywords.py:85-91).

The PR body says "reject the new scalar autotargeting flags in batch modes so they are not silently ignored before #289" — confirming batch parity is deliberately deferred — but the audit (the contract surface) no longer records that fact. Recommend either re-adding a missing_followup row referencing #289 for batch coverage, or adding a note: qualifier to the supported row, so a future reader can tell the supported-state apart from fields with full single-item + batch reach.

Fix this →

4. Behavior change to existing --bid / --context-bid / --user-param-1 / --user-param-2 flags is unstated

The fix newly rejects four pre-existing flags (--bid, --context-bid, --user-param-1, --user-param-2) when combined with --from-file / --keywords-json. Previously those were silently dropped. Converting silent loss → explicit error is the right call (it matches the WSDL parity gate's silent-data-loss invariant), but this is a user-visible CLI contract change that isn't called out in the PR body, README, or commit message — anyone whose scripts pass --bid alongside --from-file (a stale habit, but the CLI used to accept it) will now exit non-zero. Worth mentioning in the PR description / CHANGELOG so users aren't surprised. Not a code change — just a doc/disclosure gap.

Items 2 and 3 are unresolved from the previous review. Item 1 (README) and Item 4 (disclosure) are not blocking on their own, but item 1 leaves the docs strictly worse than before this PR.
· Branch: codex/issue-285-keywords-scalar-autotargeting

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

Addressed Claude review feedback in e29281e: restored regular keyword README examples and added autotargeting examples as adjacent lines; made scalar batch rejection cover both new flags explicitly; annotated WSDL audit supported rows as single-item support with batch/from-file parity tracked in #289. Re-ran: python3 -m pytest tests/test_dry_run.py -k "keywords_add and (batch_mode or scalar_autotargeting)"; python3 scripts/build_wsdl_optional_field_audit.py --check; 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; mypy .

@codex @claude review. Re-run after addressing README, batch coverage test, and audit note feedback. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, documentation or test contract regressions, and issue-scope gaps. Do not nitpick style, naming, minor formatting, or subjective preferences.

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

Claude re-review for @axisrow

• Pull latest changes and re-read PR diff
• Re-verify README example fix
• Re-verify batch-mode rejection covers both flags
• Re-verify WSDL audit note reflects single-item-only support
• Look for any new bugs / contract regressions / scope gaps
• Post findings

Re-reviewed at e29281e against the three previous findings + the Copilot batch-silent-data-loss note. All four are addressed; I have no remaining critical issues.

Previous findings — status

1. README regular-keyword example regression — fixed.
README.md:425 and README.md:1136 keep the original "buy laptop" / "купить ноутбук" one-liners; the autotargeting example is now added as a separate adjacent line at README.md:426 / README.md:1137. Users still get a regular-keyword demo.

2. WSDL audit overstating coverage — fixed.
tests/WSDL_OPTIONAL_FIELD_AUDIT.md:5577 and :5579 mark both rows supported with a note that explicitly carves out batch/from-file parity to #289:

"Single-item typed flag is supported; batch/from-file parity is tracked separately. [#289]"

The same notes mirror the in-code audit dict (tests/test_wsdl_parity_gate.py:1468-1488). The audit is no longer ambiguous about partial coverage. (I was unable to fetch issue #289 to confirm it actually tracks batch parity for these fields — worth a quick sanity check that the linked issue exists and reflects that scope, since the audit will surface it as the single source of truth for the gap.)

3. Batch-rejection test asymmetry — fixed.
tests/test_dry_run.py:2830-2851 now loops over both ("--priority", "HIGH") and ("--autotargeting-search-bid-is-auto", "YES"), asserts exit_code != 0, "single-item mode" in output, and that the offending flag name is echoed back. A regression that drops either half of the or-condition in the check would now be caught.

Copilot finding (silent batch drop of --bid / --context-bid / --user-param-*) — fixed

direct_cli/commands/keywords.py:346-362 now collects all six single-item-only flags into one dict and raises a single UsageError listing every offender, instead of dropping them on the floor before _bulk_add. tests/test_dry_run.py:2853-2875 exercises this with --bid, --context-bid, and --user-param-1 together. This closes the silent-data-loss surface for the entire batch path, not just the two new flags — that's the right call given the WSDL parity gate's "silent data loss" invariant. --adgroup-id is (correctly) not in the rejection list since _bulk_add already honors it as a per-row default.

Spot checks that still hold from the prior pass

• case_sensitive=False on click.Choice plus .upper() keeps the wire-format enums canonical regardless of user casing.
• _normalize_keyword_row still rejects unknown JSONL fields, so a user who tries to set AutotargetingSearchBidIsAuto per row gets a loud UsageError rather than silent drop — consistent with the deferred-batch-parity story for #289.
• OPTIONAL_FIELD_CLI_OPTIONS at tests/test_wsdl_parity_gate.py:937-942 registers both flags so the parity gate verifies CLI coverage matches the audit.
• Dry-run payload assertion at tests/test_dry_run.py:2820-2826 pins the exact dict shape, including the absence of Bid/ContextBid when not passed.

Minor / non-blocking observations

• OPTIONAL_FIELD_AUDIT reorders StrategyPriority after AutotargetingBrandOptions (vs. its previous position immediately after AutotargetingSearchBidIsAuto). Pure dict ordering, no behavioral effect, but readers scanning the file by topic may find it slightly noisy. Not worth changing.
• In single-item mode if user_param_1: (truthy) is still used instead of is not None, so an explicit empty string would be silently dropped in single mode but rejected in batch mode. This is pre-existing behavior, not introduced by this PR, and outside the issue scope.

Verdict

The PR is in good shape for #285. No critical bugs, security issues, data-loss risks, or contract regressions introduced by the latest revision. The one remaining loose end is verifying that #289 actually exists and is scoped to batch parity for these fields, since the audit now points there as the followup tracker.

--- · Branch: codex/issue-285-keywords-scalar-autotargeting

Addressed remaining stale review/disclosure items in 6024002 and PR body: batch rejection test now also covers --user-param-2, PR description explicitly calls out the user-visible batch-mode rejection for all single-item-only flags, and previous e29281e already restored regular README examples plus audit #289 notes. Re-ran full local suite and mypy.

@codex @claude review. Final re-run after adding user-param-2 batch rejection coverage and updating the PR body disclosure. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, documentation or test contract regressions, and issue-scope gaps. Do not nitpick style, naming, minor formatting, or subjective preferences.

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

Claude Code Final Re-Review

• Pull latest changes and re-read PR diff (through 6024002)
• Re-verify README example fix (regular + autotargeting adjacent)
• Re-verify batch-mode rejection covers all six single-item flags + test coverage
• Re-verify WSDL audit note reflects single-item-only support / keywords batch inputs: support or explicitly defer autotargeting fields #289 link
• Look for any new bugs / contract regressions / scope gaps
• Post findings

All four items from the previous review are resolved. No new critical issues found. The PR is in good shape for merge.

Resolved

1. README regression — fixed. README.md:425 and README.md:1137 now keep the original "buy laptop" / "купить ноутбук" regular-keyword examples and add the "---autotargeting" example as an adjacent line, so the README still demonstrates the common case.
2. Batch test coverage — fixed. tests/test_dry_run.py::test_keywords_add_rejects_scalar_autotargeting_flags_in_batch_mode now iterates ("--priority", "HIGH") and ("--autotargeting-search-bid-is-auto", "YES"), asserting each flag string appears in the error output — a regression that drops either entry from the single_item_flags dict in direct_cli/commands/keywords.py:347-354 would now fail. test_keywords_add_rejects_single_item_flags_in_batch_mode covers --bid, --context-bid, --user-param-1, and --user-param-2 together. All six single-item-only flags have explicit coverage.
3. WSDL audit asymmetry — fixed. tests/WSDL_OPTIONAL_FIELD_AUDIT.md:5577,5579 and the OPTIONAL_FIELD_AUDIT entry in tests/test_wsdl_parity_gate.py:1468-1488 both annotate the two new supported rows with Single-item typed flag is supported; batch/from-file parity is tracked separately. [#289]. The audit now accurately distinguishes these from Bid/ContextBid/UserParam1/UserParam2, which have full single-item + batch reach. Header counts updated consistently (supported 476→478, missing_followup 2765→2763).
4. User-visible behavior change disclosure — fixed. The PR body now explicitly calls out the batch-mode rejection of --bid, --context-bid, --autotargeting-search-bid-is-auto, --priority, --user-param-1, --user-param-2, so users whose scripts combined these with --from-file / --keywords-json are warned.

Verified looks-good in final state

• direct_cli/commands/keywords.py:346-362 — flag→value dict + explicit list of unsupported flags in the error message converts silent data loss into actionable UsageError, matching the project's WSDL parity gate invariant.
• direct_cli/commands/keywords.py:385-390 — case_sensitive=False on click.Choice + .upper() correctly normalizes user input before insertion into the payload.
• tests/test_dry_run.py:2808-2826 pins the exact single-item payload shape including absence of Bid/ContextBid when not passed.
• tests/test_wsdl_parity_gate.py:937-942 registers --autotargeting-search-bid-is-auto and --priority in OPTIONAL_FIELD_CLI_OPTIONS against the correct WSDL field names.
• _KEYWORD_ROW_FIELDS (direct_cli/commands/keywords.py:32-39) intentionally omits the two new fields, and _normalize_keyword_row at keywords.py:85-91 raises UsageError on unknown row fields — so a per-row AutotargetingSearchBidIsAuto in JSONL fails loudly rather than silently dropping, until keywords batch inputs: support or explicitly defer autotargeting fields #289 lands batch parity.

No blocking issues. Approves intent (not a formal GitHub approval, which I can't issue).

· Branch: codex/issue-285-keywords-scalar-autotargeting

Codex Review: Didn't find any major issues. Another round soon, please!

Final review triage: FIX - Copilot batch-mode silent-ignore feedback addressed in 1c180c6 and covered further in 6024002. FIX - Claude README/test/audit/disclosure feedback addressed in e29281e, 6024002, and PR body update. Final Claude re-review confirms all items resolved; Codex found no major issues; independent scope review was COMPLETE; CI is green. No blocking/actionable comments remain. Merging.