feat(sitelinks): support escaped pipe + --sitelink-json/--sitelinks-from-file (#221)

[axisrow]

Summary

• direct sitelinks add --sitelink TITLE|HREF[|DESCRIPTION] ломался на URL с литеральным | (UTM-шаблоны Яндекс.Директа вида cid|{campaign_id}|gid|{gbid}). Теперь литеральный pipe экранируется как \|; обычные spec-строки без \| ведут себя bit-for-bit как раньше.
• Добавлены два альтернативных структурных канала по образцу keywords add (feat(keywords): batch add via --from-file / --keywords-json (#203) #218): --sitelink-json '<JSON-array>' и --sitelinks-from-file <path.jsonl>. Источники взаимоисключающие; ровно один обязателен; ошибки про отсутствие Title/Href сообщают индекс строки.
• ValueError из parse_sitelink_specs оборачивается в click.UsageError — все три входа возвращают exit code 2 на невалидный ввод.

Verification

• pytest — 888 passed, 45 skipped.
• pytest tests/test_dry_run.py -k sitelinks — 9/9 зелёные (1 страховка backwards-compat + 8 новых).
• pytest tests/test_wsdl_parity_gate.py — sitelinks.add/Sitelinks зарегистрировано в INTERNAL_VALIDATION (теперь validated в теле команды, а не через Click required=True).
• black --check — clean.
• flake8 direct_cli tests — без новых замечаний.

Test plan

• \| round-trips через Href без изменения payload (UTM-шаблон сохраняется)
• Невалидный pipe spec → UsageError с hint про \\|
• --sitelink-json с массивом объектов
• --sitelinks-from-file с JSONL
• Mixed sources → UsageError
• No source → UsageError
• JSON без Href → UsageError с индексом строки
• JSON не-массив → UsageError
• Существующий test_sitelinks_add_parses_links_array проходит без правок (backwards-compat)
• WSDL parity gate зелёный

Closes #221
Closes #220

🤖 Generated with Claude Code

[axisrow]

@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]

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

---

Claude PR Review — focused on critical issues

• Load review tools
• Read changed files (sitelinks.py, utils.py, tests, parity gate)
• Analyze parser/escape logic
• Analyze JSON / file input paths
• Cross-check tests & WSDL gate
• Post findings

---

Verdict: no critical issues found

I reviewed the change with a focus on bugs, security, logical errors, data-loss risks, and performance. The implementation is sound and the test coverage is broad enough for the scope.

What I checked and why it's safe

1. _split_sitelink_spec state machine (utils.py:460) — backward compatible.
The escape branch only triggers on the exact two-char sequence \|. For any spec that does not contain \|, the loop is byte-equivalent to spec.split("|") (and the existing test_sitelinks_add_parses_links_array proves it). Trailing lone \ is preserved as a literal because of the i + 1 < len(spec) guard — no IndexError risk.

2. Source mutex in add (sitelinks.py:154-172) — correctly enforced.
sitelinks_specs or None handles the empty-tuple-from-multiple=True case. Empty string for --sitelink-json is also falsy, so a stray --sitelink-json "" falls into "no source" rather than silently sending an empty payload. Both error paths are exercised by tests.

3. JSON / JSONL ingestion (sitelinks.py:16-66) — defensive enough.

• json.loads("null") → decoded is None → caught by isinstance(decoded, list) check.
• Missing/empty Title and Href raise UsageError with row index.
• Non-dict rows raise with type name in the message.
• Blank lines in JSONL are skipped, and an all-blank file raises "Input contains no sitelink rows."
• File path is validated by click.Path(exists=True, dir_okay=False, readable=True) before the handler runs — no path-traversal surface beyond what Click already accepts.

4. Dry-run gate (sitelinks.py:199-201) — returns before creating the client. No accidental live calls.

5. Exception envelope (sitelinks.py:212-216) — click.UsageError is re-raised so Click can map it to exit code 2; only unexpected exceptions fall through to print_error + Abort.

6. WSDL parity gate registration (test_wsdl_parity_gate.py) — entry added to both INTERNAL_VALIDATION and INTERNAL_VALIDATION_PROBES for the new validation path, matching the CLAUDE.md instruction for moving away from required=True. The probe (["sitelinks", "add"] with no source flags) trips the "Provide exactly one of:" branch, so the gate sees the validation.

Non-critical observations (not requesting changes)

• A binary or non-UTF-8 file passed to --sitelinks-from-file would raise UnicodeDecodeError (not caught explicitly in _load_sitelinks_from_file), bubble to the generic except Exception, and exit with Abort (status 1) rather than UsageError (status 2). Behaviour-wise still safe — no payload is sent — just a slightly non-canonical exit code for "bad user input". Not worth churning for.
• _normalize_sitelink_row coerces with str(row["Title"]), so a JSON "Title": [] would stringify to "[]" and pass validation. Realistic only for hand-rolled garbage input; the API would reject it. Not a correctness issue worth fixing.

Security / data-loss / performance

• No shell or eval surfaces; JSON parsing is stdlib only.
• No write/delete paths added; delete subcommand untouched.
• File is read with read_text() — fine for sitelink-set sized inputs; not a concern.
• Dry-run isolation prevents accidental live writes during the new validation path; live add still requires explicit sandbox/prod credentials.

LGTM from a critical-issues standpoint.
· branch worktree-fix-221-sitelinks-pipe-escape

[Copilot]

Pull request overview

This PR improves direct sitelinks add input handling by (1) supporting literal | in the legacy TITLE|HREF[|DESCRIPTION] spec via escaping and (2) adding structured batch inputs (--sitelink-json / --sitelinks-from-file) with consistent UsageError behavior on invalid user input.

Changes:

• Add an escape-aware splitter for --sitelink specs so \| round-trips as a literal pipe in Href.
• Extend sitelinks add with mutually-exclusive JSON array (--sitelink-json) and JSONL file (--sitelinks-from-file) inputs, plus internal validation and UsageError wrapping.
• Expand test coverage (dry-run + WSDL parity gate + API coverage payload probes) for the new behaviors.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
direct_cli/utils.py	Adds escape-aware splitting for sitelink spec parsing and improves the invalid-spec error hint.
direct_cli/commands/sitelinks.py	Implements new JSON/JSONL input modes and internal validation for sitelinks add.
tests/test_dry_run.py	Adds regression and feature tests for escaped pipes and JSON/JSONL input modes.
tests/test_wsdl_parity_gate.py	Registers sitelinks.add/Sitelinks as internally validated for the parity gate.
tests/api_coverage_payloads.py	Adds payload probes to keep API coverage checks exercising the new input paths.

[axisrow]

Triage по двум non-critical observation от @claude:

1. UnicodeDecodeError → Abort (exit 1) vs UsageError (exit 2) в _load_sitelinks_from_file — SKIP. Сам ревьюер пометил как «not worth churning for», поведение безопасное (никаких payload-ов не уходит), edge-case для не-UTF-8 sitelinks.jsonl исчезающе редкий.

2. str([]) → "[]" стрингификация в _normalize_sitelink_row — SKIP. Ревьюер пометил как «not a correctness issue worth fixing». Списки в Title/Href пройти не могут: isinstance(row, dict) обрезает целые non-dict ряды на верхнем уровне (commands/sitelinks.py:17); внутри dict значение-список действительно стрингифицируется, но это запрос на «реалистичный hand-rolled garbage», который API всё равно отклонит.

Copilot inline-замечания #1 и #2 закрыты коммитом 6993009 (unknown-key rejection + source-selection tightening, обе правки выравнивают sitelinks под паттерн keywords add).

[axisrow]

@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]

Claude finished @axisrow's task in 41s —— View job

---

Triage acknowledged

Confirming the decisions on both non-critical observations from my earlier review:

1. UnicodeDecodeError → Abort (exit 1) vs UsageError (exit 2) — SKIP ✅

Agreed. Reasoning holds:

• No payload is ever constructed on the decode failure path, so no data-loss or live-write risk.
• A non-UTF-8 sitelinks.jsonl is an extreme edge case — JSONL itself is by spec UTF-8.
• Catching UnicodeDecodeError just to remap exit code 1 → 2 is pure cosmetic churn against an input the user almost certainly didn't intend.

2. str([]) → "[]" stringification in _normalize_sitelink_row — SKIP ✅

Agreed, and the rationale is now even stronger after commit 6993009:

• isinstance(row, dict) on commands/sitelinks.py:17 rejects non-dict rows wholesale.
• Inside a dict, a list value for Title/Href would stringify to e.g. "[]", but the Yandex API will reject it on the server side — no silent data loss in the Direct CLI layer.
• Reaching this codepath requires a hand-rolled malformed JSON object, not a realistic user mistake.
• More importantly, the new unknown-key guard (see below) catches the much more probable failure mode (typos) before bad payloads form.

---

Copilot inline comments — verified closed by 6993009

I checked the commit against the two Copilot findings:

(a) Unknown-key rejection — direct_cli/commands/sitelinks.py:15-30 now computes set(row) - set(_SITELINK_FIELDS) and raises UsageError with the offending key and the allowed list. Matches the _normalize_keyword_row precedent in direct_cli/commands/keywords.py:80. ✅ Closes silent-data-loss concern on typos like "Decsription".

(b) Source-selection by presence — direct_cli/commands/sitelinks.py:163-167 swapped truthiness for explicit presence checks (is not None for the string options, length-based for the --sitelink tuple). --sitelink-json "" now routes to "invalid JSON" instead of "Provide exactly one of: ...", which is the correct error class for that input. ✅

Both regression tests (typo-key + empty-string-JSON) are in tests/test_dry_run.py per the commit diff.

LGTM — PR is in a good state to merge from a critical-issues standpoint.
· branch worktree-fix-221-sitelinks-pipe-escape

[claude]

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

---

Claude PR Review — critical issues focus

• Load review tools
• Read changed files (sitelinks.py, utils.py, tests)
• Analyze parser/escape logic and JSON paths
• Check Copilot comments status (addressed in 6993009)
• Post findings

---

Verdict: no critical issues — Copilot's two flags already resolved in 6993009

I re-reviewed the head of the branch (commit 6993009) against the original brief: bugs, security, logical errors, data-loss risks, performance. The two real risks Copilot flagged in its inline review are already fixed; nothing new rises to that bar.

Copilot's two inline comments — status

1. Truthiness in sources_used (sitelinks.py) — FIXED.
Now uses presence checks (sitelinks.py:164-168):

sources_used = (
    (1 if sitelinks_specs else 0)
    + (1 if sitelinks_json is not None else 0)
    + (1 if sitelinks_from_file is not None else 0)
)

--sitelink-json "" now falls into _load_sitelinks_from_inline and produces "invalid JSON" (the correct user-facing error), not "Provide exactly one of…". sitelinks_specs is a tuple from multiple=True, so bool(()) is fine — counted only when at least one --sitelink was passed. Mixed-source detection is correctly preserved. Regression test test_sitelinks_add_empty_json_rejected locks the behaviour.

2. Silent unknown-key drop in _normalize_sitelink_row — FIXED.
Mirrors the keywords.add precedent (sitelinks.py:24-29):

unknown = sorted(set(row) - set(_SITELINK_FIELDS))
if unknown:
    allowed = ", ".join(_SITELINK_FIELDS)
    raise click.UsageError(
        f"Unknown field {unknown[0]!r} in sitelink #{index}; allowed: {allowed}"
    )

A typo like "Decsription" now raises with the offending key quoted and the allowed list shown. Closes the silent-data-loss path. Regression test test_sitelinks_add_rejects_unknown_field locks it.

Re-verified critical paths (still clean)

• Escape splitter (utils.py:460-483) — byte-equivalent to spec.split("|") when \| is absent (proven by the unchanged test_sitelinks_add_parses_links_array). The i + 1 < len(spec) guard keeps a trailing lone \ literal and rules out IndexError.
• Empty inputs — --sitelink-json '[]', all-whitespace JSONL, and an empty file all hit the "Input contains no sitelink rows." branch (sitelinks.py:192-193) and never construct a payload.
• Dry-run isolation (sitelinks.py:205-207) — returns before create_client. No accidental live calls during validation tests.
• Exception envelope (sitelinks.py:218-222) — UsageError re-raised (exit 2), everything else goes through print_error + Abort (exit 1). Matches CLI conventions.
• WSDL parity gate — sitelinks.add/Sitelinks registered in INTERNAL_VALIDATION + INTERNAL_VALIDATION_PROBES, with the no-source-flags probe exercising the validation path. Conforms to the CLAUDE.md guidance on moving away from Click required=True.

Security / data-loss / performance

• No shell, eval, subprocess, or dynamic-import surfaces introduced.
• JSON parsing is stdlib only; no pickle, no yaml.load.
• --sitelinks-from-file is gated by click.Path(exists=True, dir_okay=False, readable=True); no path-traversal surface beyond what Click already accepts.
• Inputs are sitelink-set sized — read_text() is fine; no DoS exposure.
• No write/delete paths added; delete subcommand untouched.

Previously raised non-critical observations

The author's triage comment closed both prior non-criticals (UnicodeDecodeError exit code; str([]) stringification). I agree — neither is a correctness or safety issue worth churning the patch for.

LGTM from a critical-issues standpoint. Safe to merge.

---

· branch worktree-fix-221-sitelinks-pipe-escape