feat(keywords): batch add via --from-file / --keywords-json (#203) - #218
Conversation
`direct keywords add` now accepts batch input through either:
- `--from-file PATH` — JSONL, one keyword object per line
- `--keywords-json TEXT` — inline JSON array of keyword objects
CLI chunks input into groups of 10 (Yandex Direct API limit for
`keywords.add`; the WSDL declares `maxOccurs="unbounded"`, the cap
comes from the documentation), preserves input order, calls the API
chunk-by-chunk, and merges all `AddResults` into a single response.
Item-level errors no longer abort the whole batch — successful Ids
and per-item errors land in the merged output together.
Row keys mirror the WSDL `KeywordAddItem` CamelCase exactly:
`Keyword`, `AdGroupId`, `Bid`, `ContextBid`, `UserParam1`,
`UserParam2`. Unknown keys are rejected with the row number.
`--adgroup-id` is now optional in batch mode and acts as a default
that rows may override via per-row `AdGroupId`. `--dry-run` prints
the first chunk's payload plus `{chunks, totalItems, chunkSize}`.
Single-item mode (`--keyword`) is unchanged, including its existing
behaviour of failing the command on any `Errors` from the API.
WSDL parity gate: `--keyword` and `--adgroup-id` are no longer
Click-required, so `Keyword` and `AdGroupId` are registered in
`INTERNAL_VALIDATION` with matching probes — running `keywords add`
with no flags still raises a `UsageError` covering both fields.
Closes #203
There was a problem hiding this comment.
Pull request overview
Adds batch input support to direct keywords add so users can upload multiple keywords via JSONL (--from-file) or inline JSON (--keywords-json), auto-chunked to the API limit (10/request), with merged AddResults and per-row validation aligned to WSDL field names.
Changes:
- Implemented batch mode for
keywords addwith JSONL/inline JSON ingestion, validation, chunking, and merged results output. - Extended dry-run and WSDL parity gate tests to cover the new batch behavior and internal required-field validation.
- Documented batch keyword upload in README (EN/RU) and added release notes to the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| direct_cli/commands/keywords.py | Implements batch mode ingestion/validation/chunking and merged AddResults for keywords add. |
| tests/test_dry_run.py | Adds dry-run/unit tests for batch payload shaping, chunking, validation, and mode mutex behavior. |
| tests/test_wsdl_parity_gate.py | Updates parity gate internal validation mappings/probes for keywords add required fields. |
| README.md | Documents batch keyword upload usage (JSONL and inline JSON) in English and Russian sections. |
| CHANGELOG.md | Adds an unreleased 0.3.9 entry describing the new batch keywords add feature. |
| if kind == "int": | ||
| if isinstance(raw_value, bool) or not isinstance(raw_value, int): | ||
| try: | ||
| return int(raw_value) | ||
| except (TypeError, ValueError): | ||
| raise click.UsageError( | ||
| f"Row {row_index} field {field!r}: expected integer, " | ||
| f"got {raw_value!r}" | ||
| ) |
There was a problem hiding this comment.
Fixed in commit e7ba3ee. _coerce_keyword_field now rejects isinstance(raw_value, bool) at the top, before any int() or MICRO_RUBLES.convert conversion attempt. Test: test_keywords_add_rejects_bool_in_row.
| """Add one or many keywords (batch via --from-file / --keywords-json).""" | ||
| modes_used = sum(1 for value in (keyword, from_file, keywords_json) if value) | ||
| if modes_used == 0: | ||
| raise click.UsageError( | ||
| "Provide exactly one of: --keyword (single), --from-file (JSONL), " | ||
| "or --keywords-json (inline JSON array)." | ||
| ) | ||
| if modes_used > 1: | ||
| raise click.UsageError( | ||
| "Provide exactly one of: --keyword, --from-file, or " | ||
| "--keywords-json — they are mutually exclusive." | ||
| ) |
There was a problem hiding this comment.
Fixed in commit e7ba3ee. Mode-detection now uses is not None instead of truthiness, so --keyword "" registers as mode-provided and reaches the normal payload-building path. Test: test_keywords_add_empty_string_keyword_counts_as_mode.
| text = file_path.read_text(encoding="utf-8") | ||
| except OSError as exc: | ||
| raise click.UsageError(f"Cannot read --from-file {path!r}: {exc}") | ||
|
|
||
| for line_number, raw_line in enumerate(text.splitlines(), start=1): | ||
| line = raw_line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| rows.append(json.loads(line)) | ||
| except json.JSONDecodeError as exc: | ||
| raise click.UsageError(f"Row {line_number}: invalid JSON: {exc.msg}") |
There was a problem hiding this comment.
Not changing — the Yandex Direct keywords.add endpoint is capped at 10 items per request, and an account-level limit caps practical batches at tens of thousands of keywords. A realistic JSONL fits in memory long before any other constraint binds, and the second-pass bot review explicitly agreed that streaming is YAGNI here. Adding it would complicate the code path that already needs per-row index tracking for error reporting.
|
@claude 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 14s —— View job Code review — critical issues onlyFocused on bugs, data-loss, security, perf per the request. Three findings, ranked by impact. 🔴 1. Data loss on chunk-level failure (high impact)
all_results: List[Any] = []
for index, chunk in enumerate(chunks, start=1):
click.echo(f"Sending chunk {index}/{len(chunks)}: {len(chunk)} items", err=True)
body = {"method": "add", "params": {"Keywords": chunk}}
response = client.keywords().post(data=body) # <- raises
all_results.extend(_normalize_add_results(response().extract()))
print(format_json({"AddResults": all_results}, indent=2)) # <- never reachedThe PR description claims "item-level errors no longer abort the whole batch", which is true for per-item For a batch mutation tool this is the most important property to get right. At minimum, on exception print whatever except Exception as e:
if all_results:
click.echo(
"Partial success before failure (re-running will duplicate these):",
err=True,
)
click.echo(format_json({"AddResults": all_results}, indent=2), err=True)
print_error(str(e))
raise click.Abort()🟠 2. Boolean values silently coerced to 1/0 for integer fields (real bug)
if kind == "int":
if isinstance(raw_value, bool) or not isinstance(raw_value, int):
try:
return int(raw_value) # int(True) -> 1
except (TypeError, ValueError):
raise click.UsageError(...)
return raw_valueThe explicit Practical blast radius is bounded because Yandex will likely reject if kind == "int":
if isinstance(raw_value, bool):
raise click.UsageError(
f"Row {row_index} field {field!r}: expected integer, got bool"
)
if not isinstance(raw_value, int):
try:
return int(raw_value)
...This is the same issue Copilot flagged at line 48; agreeing because the existing code reads as if the author already noticed the bool/int subclass gotcha but didn't complete the fix. 🟡 3. Mode detection mis-classifies explicit empty strings (low impact)
Real-world blast radius is small (empty keyword/file path is nonsense), but the fix is one line — Looked at but not flagged
Summary1 high-impact data-loss issue (#1) and 1 real validation bug (#2) worth fixing before merge. #3 is a polish nit. Otherwise the batch design is sound: WSDL-aligned field names, sane chunking constant with a doc URL inline, single-mode regression preserved.
|
Address three review findings on PR #218: 1. Partial-batch data loss on chunk failure (high impact) When a later chunk raised (network blip, 5xx, rate-limit), Ids from earlier successful chunks were already created in Yandex Direct but silently dropped from output, so a retry would create duplicates. On exception, write any accumulated AddResults to stderr with a recovery note before failing. 2. Boolean values silently coerced to 1/0 (real bug) `isinstance(True, int) is True` in Python, so the explicit `isinstance(raw_value, bool)` guard was routing bools into `int(True) -> 1` instead of raising. Reject bools at the top of `_coerce_keyword_field` for every kind (int, micro, str), before any conversion attempt. 3. Mode detection mis-classified explicit empty strings `sum(1 for v in (...) if v)` treated `--keyword ""` as "not provided"; switch to `is not None` so empty strings reach normal validation paths instead of the "Provide exactly one of" error. Tests cover all three: partial-success-on-stderr (mocked chunk-2 RuntimeError after chunk-1 returns 10 Ids), bool rejection per row, and empty-string-as-mode acceptance.
|
Addressed all three reviewer findings in commit e7ba3ee: 1. Partial-batch data loss (bot finding #1, high impact) — Fixed in 2. Bool coerced to 1/0 (bot finding #2 / Copilot inline comment, real bug) — Fixed by moving the bool guard to the top of 3. Mode detection truthiness (bot finding #3 / Copilot inline comment) — Switched from Copilot inline comment on streaming JSONL (line 112) — Not changed. The 10-item-per-request API limit caps a realistic batch at tens of thousands of keywords, which fits in memory long before any other constraint binds. Streaming is YAGNI here; the bot review explicitly agreed. All 21 |
|
@claude 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 24s —— View job Follow-up review — verifying fixes in e7ba3eeRe-reviewed the three findings from my previous round against ✅ Fix #1 — Partial-batch data loss (high impact)
Test: Minor follow-up consideration (not blocking): the dumped ✅ Fix #2 — Bool coerced to 1/0 (real bug)
Test: ✅ Fix #3 — Mode detection (low impact polish)
Test: Other things scanned, not flagged
VerdictShip it. All three fixes are correct, narrowly scoped, and well-tested. The PR also picked up two additional defensive tests in 8d982f4 (empty JSON array, non-object row, file+inline mutex) that close adjacent gaps.
|
…recovery test - Add `KEYWORDS_PER_ADGROUP_LIMIT = 200` (Yandex Direct per-ad-group cap). When the input distributes >200 keywords to a single `AdGroupId`, print a stderr warning before any chunk is sent. Doesn't block — the API already rejects the excess as per-item errors and the existing partial-recovery surfaces them. - Drop `CliRunner(mix_stderr=False)` in the partial-success test: that kwarg is Click 8.2+; the project pins `click>=8.0.0` and CI runs an older Click. The CliRunner default mixes stderr into `result.output`, which works for the same assertion across all supported Click versions. Tests: - `test_keywords_add_batch_warns_when_over_200_per_adgroup` — input with 201 kw for adgroup 1 + 1 kw for adgroup 2 → warning lists only adgroup 1 with `(1 over the limit)`. - `test_keywords_add_batch_no_warning_under_200` — 150 kw for one adgroup → no warning. - `test_keywords_add_batch_partial_success_on_failure` — rewritten to use CliRunner's combined-output default; same assertion semantics. README (EN/RU) + CHANGELOG now mention the per-ad-group limit, the partial-success-to-stderr behaviour, and the explicit boolean rejection.
|
Follow-up commit 1eefede: Pre-flight 200/adgroup warning (per user feedback) — added CI fix — the partial-success test used README (EN/RU) and CHANGELOG updated. 23/23 |
`direct_cli.commands.__init__` rebinds the submodule name `keywords` to
the Click Group object, so `from direct_cli.commands import keywords as
keywords_module` (and `import direct_cli.commands.keywords as ...`)
return the Group, not the module. Python 3.11+ tolerated this in
unittest.mock.patch's dotted resolution; Python 3.9 doesn't, hence:
AttributeError: <Group keywords> has no attribute 'create_client'
Use `importlib.import_module(\"direct_cli.commands.keywords\")` to
unambiguously get the module object — the same pattern already used in
tests/test_api_coverage.py for `reports_module`.
…rom-file (#221) (#223) * feat(sitelinks): support escaped pipe + --sitelink-json/--sitelinks-from-file (#221) UTM templates in Yandex Direct (e.g. cid|{campaign_id}|gid|{gbid}) contain literal '|' characters in the URL, which broke parse_sitelink_specs's unbounded split('|') and rejected valid sitelinks with "Invalid sitelink". URL-encoding the pipe as %7C is not a workaround because Yandex does not expand macros inside URL-encoded templates. This change introduces two independent, backwards-compatible improvements: 1. `\|` is recognised as a literal pipe inside --sitelink spec strings. The new posix-style scanner _split_sitelink_spec ignores backslash- escaped pipes and unescapes them in the output. Specs without `\|` behave bit-for-bit identically to before, so existing scripts and the existing payload coverage test pass unchanged. 2. Two structural sources are added for callers that don't want to fight the pipe convention at all, mirroring the keywords-add #218 pattern: * --sitelink-json '<JSON-array>' (inline) * --sitelinks-from-file <path.jsonl> (one object per line) Sources are mutually exclusive; exactly one must be provided. Missing Title/Href in a structural row is rejected with the row index. The previously-required Click option --sitelink is now validated manually, so the new INTERNAL_VALIDATION entry in test_wsdl_parity_gate.py keeps the WSDL parity gate honest. api_coverage_payloads.py grows two extra PAYLOAD_CASES (escape variant + JSON variant) for the same reason. ValueError from parse_sitelink_specs is wrapped as click.UsageError so all three input paths return exit code 2 on invalid input. Closes #221 Closes #220 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(sitelinks): reject unknown JSON keys + tighten source-selection (PR #223 review) Copilot review on PR #223 flagged two real risks; both align with the ``keywords.add`` precedent in this codebase: 1. ``_normalize_sitelink_row`` silently dropped unknown keys, so a typo like ``"Decsription"`` would produce a sitelink without a description and no warning to the user — silent data loss. Mirror ``_normalize_keyword_row`` (``direct_cli/commands/keywords.py:80``): compute the unknown-key set, raise ``UsageError`` with the offending key and the allowed list. 2. ``sources_used`` used boolean truthiness, so ``--sitelink-json ""`` was treated as "no source" and produced "Provide exactly one of: ..." instead of "invalid JSON". Match the ``keywords.add`` mutex (``is not None`` for string options, length-based for the ``--sitelink`` tuple) so an explicitly-empty JSON argument falls into the JSON-validation error path. Two regression tests added: typo-key rejection with offending key quoted; ``--sitelink-json ""`` → "invalid JSON". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
PR #212 wired the API item-level error renderer into format_output so every mutating command exits non-zero with a human-readable message when the response contains embedded Errors. The bulk-add path introduced by PR #218 lands its final result via print(format_json(...)) and the per-chunk extract via _normalize_add_results — neither goes through format_output, so item-level Errors in any chunk were swallowed and the CLI exited 0 with raw JSON. That was the one remaining mutating bypass of the renderer. This change wires raise_for_api_result_errors into the per-chunk loop and replaces the final print(format_json(...)) with format_output so the same renderer fires: * After each chunk response, only items without per-item Errors are appended to all_results — they are the keywords Yandex actually accepted. The partial-success diagnostic ("these keywords were already created in Yandex Direct (retrying will duplicate them)") therefore lists only success items, never the failed item that triggered the abort. * raise_for_api_result_errors then raises on the raw (unfiltered) chunk_results, propagating the 8800 Client-Login guidance and the full Errors payload through the existing except Exception handler. * The final happy-path printout goes through format_output, so a hypothetical regression that leaks Errors past the per-chunk check still gets caught at the boundary. Tests added in tests/test_cli.py (mock client, no HTTP): * test_keywords_bulk_add_surfaces_item_errors — single chunk with only Errors → non-zero exit, "Yandex Direct API returned errors", "Error 8800: Object not found", "Details: Ad group not found", and the current Client-Login/account guidance. * test_keywords_bulk_add_multi_chunk_partial_success — chunk 1 all success, chunk 2 mixed (one Id + one Errors). The Id from the mixed chunk appears in the partial-success diagnostic; the Errors entry does not. * test_keywords_bulk_add_all_failure_second_chunk — chunk 1 all success, chunk 2 all Errors. The diagnostic shows only chunk 1's ids; the failing chunk contributes nothing. Closes #211 Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Summary
direct keywords addnow accepts batch input (--from-file PATHJSONL or--keywords-json TEXTinline JSON array), pages through chunks of 10 (the Yandex Direct API limit forkeywords.add), preserves order, and merges allAddResultsinto one response.KeywordAddItemCamelCase exactly (Keyword,AdGroupId,Bid,ContextBid,UserParam1,UserParam2); unknown keys are rejected with the row number.--adgroup-idis now optional in batch mode and acts as a default; rows may override it. Single-item mode (--keyword) is unchanged, including the existing fail-on-Errors behaviour.Contract fidelity (Yandex Direct WSDL 1:1)
tests/wsdl_cache/keywords.xml:114-127declaresKeywordswithmaxOccurs="unbounded". The 10-item cap is a documentation-level runtime limit (https://yandex.ru/dev/direct/doc/dg/objects/keyword.html), recorded asKEYWORDS_ADD_MAX_BATCH = 10with the doc URL inline.KeywordAddItemminOccurs=1fields:Keyword,AdGroupId— both registered inINTERNAL_VALIDATIONwith matchingINTERNAL_VALIDATION_PROBESso the parity gate keeps enforcing them after the Clickrequired=Trueflags went away.StrategyPriority/AutotargetingSettingsetc. is left to a follow-up issue to avoid scope creep.Out of scope (intentional)
Autotargeting*types that don't map cleanly to a flat header; if needed, a separate issue.StrategyPriority,Autotargeting*) — handled in a separate feature request.keywords update— issue keywords add: support batch input (--from-file / --keywords-json) respecting API 10-per-request limit #203 only coversadd.Test plan
pytest tests/test_dry_run.py -k keywords_add -v— 15 new tests pass (chunking, mutex, per-row validation, unknown-field reject, mode-mode rejection,--format other than jsonrejection, single-mode regression on Errors).pytest tests/test_wsdl_parity_gate.py— 48 passed, 6 skipped (parity gate enforcesKeyword/AdGroupIdthroughINTERNAL_VALIDATION).pytest -m "not integration and not integration_write and not integration_live_write"— 791 passed, 11 skipped, no regressions.black .clean on changed files;flake8on changed files — no new warnings (the three pre-existing E501s in_DEPRECATED_KEYWORDS_UPDATE_OPTIONSlines pre-date this PR).direct keywords add --helpshows the new options.direct keywords add --adgroup-id N --keyword X --dry-runpayload is byte-identical to the pre-PR single-mode payload.--dry-run→chunks=2, totalItems=12, firstChunkLen=10.Closes #203
🤖 Generated with Claude Code