Add 1Password auth and simplify build config#2
Conversation
Remove setuptools_scm, set static version in pyproject.toml, simplify setup.py to a minimal shim. Add destructive commands warnings to README and copilot-instructions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add project instructions for Claude Code. Document which commands are intentionally excluded from automated integration testing and why. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add op_read() to auth.py for reading secrets via 1Password CLI. New --op-token-ref and --op-login-ref global options in cli.py pass references through env vars so command modules stay untouched. Priority: CLI flags > env vars > .env > 1Password. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@claude review |
1 similar comment
|
@claude review |
|
Claude finished @axisrow's task in 1m 49s —— View job PR Review: Add 1Password auth and simplify build config
Overall this is a solid, focused PR. The 1Password integration is straightforward and secure (no shell injection, uses Issues1.
try:
result = subprocess.run([op_path, "read", ref], ...)
except subprocess.TimeoutExpired:
raise RuntimeError("1Password CLI timed out after 10 seconds")2. CLI flags bypass the documented priority order (medium)
A cleaner approach: store 3.
# Docstring says:
# 1. Direct arguments
# 2. Environment variables
# 3. .env file
# Missing: 4. 1Password (via YANDEX_DIRECT_OP_TOKEN_REF / op_token_ref)4.
@click.option(
"--op-token-ref",
envvar="YANDEX_DIRECT_OP_TOKEN_REF", # add this
help="1Password secret reference for token (e.g. op://vault/item/token)",
)Suggestions5. Unused import in test file (nit)
6. No test for CLI flag → env var path There's no test covering the cli.py code path where What looks good
|
- Catch subprocess.TimeoutExpired in op_read and re-raise as RuntimeError - Pass op_token_ref/op_login_ref explicitly through get_credentials() and create_client() params instead of relying solely on os.environ - Store op refs in ctx.obj for consistency with token/login - Add envvar= to --op-token-ref/--op-login-ref Click options - Update get_credentials() docstring with 4-level priority - Remove unused subprocess import from tests - Add timeout test, explicit param test, and CLI integration tests Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@claude review |
|
Claude finished @axisrow's task in 1m 54s —— View job PR Review: Add 1Password auth and simplify build config
Good progress from the previous review. Previous issues #1–#4 have all been addressed. However, there is one critical regression introduced by the fix for issue #2 that prevents Issues1.
# campaigns.py:45-49 — same pattern in all ~25 command files
client = create_client(
token=ctx.obj.get("token"),
login=ctx.obj.get("login"),
sandbox=ctx.obj.get("sandbox"),
# op_token_ref and op_login_ref are never passed
)Result: Two options to fix this: Option A — Pass from client = create_client(
token=ctx.obj.get("token"),
login=ctx.obj.get("login"),
sandbox=ctx.obj.get("sandbox"),
op_token_ref=ctx.obj.get("op_token_ref"),
op_login_ref=ctx.obj.get("op_login_ref"),
)Option B — Resolve credentials in the top-level # In cli.py, after setting ctx.obj:
from .auth import get_credentials
try:
resolved_token, resolved_login = get_credentials(
token=token, login=login,
op_token_ref=op_token_ref, op_login_ref=op_login_ref,
)
ctx.obj["token"] = resolved_token
ctx.obj["login"] = resolved_login
except ValueError:
pass # let subcommands fail naturally if no tokenOption B is preferred — it centralizes auth resolution, keeps all subcommands unchanged, and makes the credential flow explicit. 2.
What was fixed from the previous review
What looks good
| Branch |
Resolve credentials early in the top-level cli() callback so all subcommands receive the final token/login via ctx.obj without needing to pass op refs individually. Fixes --op-token-ref CLI flag being silently ignored by subcommands. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@claude review |
|
Claude finished @axisrow's task in 2m 35s —— View job PR Review: Add 1Password auth and simplify build config
Good progress — the critical regression from the previous review (Issue #1: Issues1.
Before this fix, errors surfaced inside each subcommand's Fix: catch try:
resolved_token, resolved_login = get_credentials(
token=token, login=login,
op_token_ref=op_token_ref, op_login_ref=op_login_ref,
)
ctx.obj["token"] = resolved_token
ctx.obj["login"] = resolved_login
except RuntimeError as e:
raise click.ClickException(str(e))
except ValueError:
# No token provided — let subcommands fail naturally
ctx.obj["token"] = token
ctx.obj["login"] = login2. No test for 1Password error path via CLI flag (minor)
@patch("direct_cli.auth.op_read", side_effect=RuntimeError("1Password CLI not found"))
def test_op_token_ref_cli_error_surfaces_cleanly(self, mock_op_read, monkeypatch):
monkeypatch.delenv("YANDEX_DIRECT_TOKEN", raising=False)
runner = CliRunner()
result = runner.invoke(cli, ["--op-token-ref", "op://vault/item/token", "campaigns", "get"])
assert result.exit_code != 0
assert "1Password CLI not found" in result.outputWhat was fixed from the previous review
What looks good
| Branch |
Surface op_read errors (op not found, timeout, item not found) as clean ClickException messages instead of raw tracebacks. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@claude review |
|
Claude finished @axisrow's task in 2m 5s —— View job PR Review: Add 1Password auth and simplify build config
All issues from the previous review rounds have been addressed. This PR is ready to merge. What was fixed in the latest round
Complete picture: what's in the PR
Build simplification
What looks good end-to-end
Branch | |
…via CLI Addresses Codex review finding #2 on #179: write-mutating tests were inheriting the v4_live_read file-level marker, breaking the trust boundary between read-only and write test suites. - Removes test_v4_live_wordstat_lifecycle_contract_opt_in_write and test_v4_live_forecast_lifecycle_contract_opt_in_write from tests/test_v4_live_contracts.py (which is correctly marked v4_live_read). - Adds test_live_draft_v4wordstat_lifecycle and test_live_draft_v4forecast_lifecycle to tests/test_integration_live_write.py under the integration_live_write marker. - Rewrites them to use the CLI (via _invoke_live / CliRunner) instead of native call_v4, matching the convention of every other live-write test in that file. - Drops in-test YANDEX_DIRECT_LIVE_WRITE checks — the file-level skipif already enforces the env gate. Verified: pytest --collect-only -m v4_live_read tests/test_v4_live_contracts.py → 9 read-only tests (no lifecycle probes). pytest --collect-only -m integration_live_write tests/test_integration_live_write.py → both v4 lifecycle tests included. env -u YANDEX_DIRECT_LIVE_WRITE pytest <…lifecycle…> → SKIPPED via file-level skipif. Codex review finding #1 (bare `pytest` hits live API when profile is active) is intentionally deferred: project decision is that v4 live read tests should run whenever a profile is available. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…177) * test(v4_live): add live contracts for v4forecast, v4tags, v4wordstat Refs #174. - wordstat lifecycle (create/list/delete) under v4_live_read marker - forecast lifecycle (create/list/delete) under v4_live_read marker - tags GetCampaignsTags read-only via existing _campaign_id helper - tags GetBannersTags read-only via existing _campaign_id helper GetForecast and GetWordstatReport are out of scope (require polling). UpdateBannersTags / UpdateCampaignsTags are out of scope (write). Tests auto-skip without YANDEX_DIRECT_TOKEN / YANDEX_DIRECT_LOGIN. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(v4_live): gate wordstat/forecast lifecycle behind LIVE_WRITE opt-in Addresses Codex review on #177: CreateNew*/Delete* are write operations and must not run as part of the read-only v4_live_read tier. Follows the existing pattern of test_v4_live_create_invoice_contract_opt_in_write (line 140) — env-gate inside the test body with explicit _opt_in_write suffix in the function name. Behaviour: - Without YANDEX_DIRECT_LIVE_WRITE=1: tests skip (even with token+login). - With YANDEX_DIRECT_LIVE_WRITE=1: tests run and mutate the live account with a single create/delete cycle. Tags read-only tests (GetCampaignsTags, GetBannersTags) are unchanged — they remain pure reads under v4_live_read. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test: relax 'No such option' asserts for Click 8.x format Click 8.x changed the missing-option error format from "No such option: --foo" to "No such option '--foo'. Did you mean '--bar'?" This broke three legacy-flag rejection tests on Python 3.11/3.13 CI runners (3.9 still ships an older Click). Split each assertion into two checks — "No such option" + the flag name — which match both the old and new formats. Fixes the CI failure inherited from main on #177. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(v4_live): apply Claude review polish (next/single-arg/TODOs) Addresses Claude review on #177: - Use next() to fetch the just-created report/forecast from the list and validate its schema directly, instead of asserting on reports[0] which may belong to a prior run. - Drop redundant third argument to call_v4(..., "List*", None) — param defaults to None. - Add TODOs flagging two assertions to tighten after first live run: Delete* return value (== 1 per docs) and GetBannersTags field set (likely {BannerID, TagIDS}, mirroring UpdateBannersTags writes). Behaviour unchanged for both skip path and live path; lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(v4_live): resolve credentials from direct auth profile when env is unset Companion fix to #179 (auth-profile fallback in WRITE_SANDBOX runners). Previously _credentials() silently skipped all v4_live_read tests for profile-only users — manual pytest -m v4_live_read showed no signal. Now: when env vars are missing, try direct_cli.auth.get_credentials() to resolve the profile (matches the priority used by every CLI subcommand). Skip message also tells the user both options. Refs #178. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(v4_live): move write lifecycle probes to integration_live_write via CLI Addresses Codex review finding #2 on #179: write-mutating tests were inheriting the v4_live_read file-level marker, breaking the trust boundary between read-only and write test suites. - Removes test_v4_live_wordstat_lifecycle_contract_opt_in_write and test_v4_live_forecast_lifecycle_contract_opt_in_write from tests/test_v4_live_contracts.py (which is correctly marked v4_live_read). - Adds test_live_draft_v4wordstat_lifecycle and test_live_draft_v4forecast_lifecycle to tests/test_integration_live_write.py under the integration_live_write marker. - Rewrites them to use the CLI (via _invoke_live / CliRunner) instead of native call_v4, matching the convention of every other live-write test in that file. - Drops in-test YANDEX_DIRECT_LIVE_WRITE checks — the file-level skipif already enforces the env gate. Verified: pytest --collect-only -m v4_live_read tests/test_v4_live_contracts.py → 9 read-only tests (no lifecycle probes). pytest --collect-only -m integration_live_write tests/test_integration_live_write.py → both v4 lifecycle tests included. env -u YANDEX_DIRECT_LIVE_WRITE pytest <…lifecycle…> → SKIPPED via file-level skipif. Codex review finding #1 (bare `pytest` hits live API when profile is active) is intentionally deferred: project decision is that v4 live read tests should run whenever a profile is available. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(v4_live): record VCR cassettes for v4wordstat/v4forecast lifecycle Adds @pytest.mark.vcr to the two new v4 lifecycle tests and commits their recorded cassettes so `pytest -m integration_live_write` no longer hits the production API on every run, matching the contract documented in README and pyproject.toml. Also extends `_before_record_request` in tests/conftest.py to redact the OAuth token from the request body: the v4 JSON API embeds `"token":"..."` inside the payload (not just the Authorization header), so the existing header-only filter would have leaked it into the recorded YAML. Addresses Codex adversarial review feedback on #177. * test(v4_live): move v4 report lifecycle to opt-in tier, add orphan store Codex's second adversarial review flagged two real issues confirmed by live verification: 1. tests/test_v4_live_contracts.py:_credentials() falls back to the active `direct auth` profile when env vars are missing. This is intentional but was never documented, and contradicts CLI's own priority chain (where the profile wins over env). Document the inverted, tests-only contract in _credentials() docstring, CLAUDE.md and README.md (EN+RU). No code change — behaviour is already correct (env wins; profile is fallback; otherwise skip). 2. test_live_draft_v4{wordstat,forecast}_lifecycle in tests/test_integration_live_write.py created account-level v4 reports under a marker that promises "only disposable draft resources". Move the two tests to tests/test_v4_live_contracts.py under the existing `_opt_in_write` pattern, gated by YANDEX_DIRECT_V4_LIVE_REPORT_WRITE=1. Delete the two corresponding cassettes (no replay for this tier — opt-in live only). Rename the remaining 18 v5 tests and cassettes from `test_live_draft_*` to `test_v5_live_draft_*` so v5 vs v4 is immediately visible at the test name level. Add tests/_orphan_store.py: a small atomic JSON store at ~/.direct-cli/test-orphans.json that records created v4 report IDs. Each opt-in test calls drain() on entry to retry deletions left over from an interrupted previous run, add() right after create, and remove() after a successful delete. If delete fails (network drop, SIGKILL), the ID stays in the store and gets cleaned up next time. Atomic write pattern modeled on direct_cli/auth.py:144-155. Verified: - pytest -m integration_live_write replay: 10 passed + 7 skipped (the same 18-test surface, minus the 2 removed v4 tests; cassettes resolve via new v5_ names). - pytest tests/test_v4_live_contracts.py without env: both new tests skip as expected. - orphan store: simulated network failure scenario — ID persists after failed delete, gets picked up by drain() on next run, store empties. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(v5): rename test_integration_live_write to test_v5_live_write Align the file name with the test names. After the previous commit renamed the 18 functions to `test_v5_live_draft_*`, the file itself still said `test_integration_live_write.py` — function prefix and file name now point at different things ("v5" vs. nothing). Renaming makes the file/test/cassette directory all say v5. The pytest marker `integration_live_write` is kept untouched — it describes the run contract (`-m integration_live_write`), not the file name, and renaming it would break the CI/dev invocation surface. git mv: - tests/test_integration_live_write.py → tests/test_v5_live_write.py - tests/cassettes/test_integration_live_write/ → tests/cassettes/test_v5_live_write/ (pytest-recording derives the cassette directory from the module name, so the directory has to track the file). Doc references updated: - tests/API_ISSUE_AUDIT.md - tests/API_COVERAGE.md Verified: 18 tests collect under the new path, replay returns the same 10 passed + 7 skipped as before, no leftover refs to the old name in the repo (only in `direct_cli.egg-info/SOURCES.txt`, which is a build artifact and not committed). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test: broaden exception handlers in test bootstrap and orphan store Address two real defects flagged in PR #177 review on commit a200daa. 1. tests/test_v4_live_contracts.py::_credentials() previously caught only (ValueError, RuntimeError, ImportError) from direct_cli.auth.get_credentials(None, None). Tracing the call path showed two reachable leak points after a successful OAuth refresh: - OSError from save_auth_store → _write_json (os.chmod + mkstemp on ~/.direct-cli/ — unconditional, no swallow); - json.JSONDecodeError from parsing a malformed refresh-token response body (auth.py:534). Either would turn a "skip when creds unavailable" bootstrap into a hard test error on a dev machine with a stale profile but no env vars. Widen the except to include OSError and json.JSONDecodeError, and add a comment so the next reader sees why these two specifically. The other failure modes Copilot mentioned (HTTPError/URLError, KeyError/TypeError) are already trapped inside auth.py and cannot escape — confirmed by reading load_auth_store, get_oauth_profile, refresh_access_token, and validate_oauth_profile. 2. tests/_orphan_store.py::_read() had its list comprehension `[int(x) for x in v if isinstance(x, (int, str))]` outside the try/except, so a corrupted bucket like {"v4wordstat": ["abc"]} would raise ValueError up through add/remove/drain — contradicting the module docstring promise that "all read/write errors are swallowed silently so tests are never broken by store corruption." Move the comprehension inside the try and extend except with (ValueError, TypeError). Also drop redundant FileNotFoundError (subclass of OSError). Verified with hand-corrupted JSON: "abc" yields {}, [null, 456] yields {"v4wordstat": [456]}, plain garbage yields {}. Codex's P2 (skip v4 tests entirely without explicit env credentials) is left unaddressed: the env > profile > skip contract is the documented and intended behavior — see CLAUDE.md, README.md (EN+RU), and the _credentials() docstring. Reply posted on the thread. Copilot's other comments triaged: - "PR description doesn't reflect actual scope": process feedback, not a code issue. - "Weak BannerID assert": intentional per the file's "confirm at first live run" pattern (matches sibling v4 read-only tests). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: axisrow <[email protected]> Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Addresses PR #208 review: * claude[bot] H12 over-constraint: StrategyAverageCpaBase, StrategyPayForConversionBase, StrategyAverageCrrBase, StrategyPayForConversionCrrBase, StrategyAverageCpaPerCampaignBase all carry GoalId minOccurs=0, and every Strategy*UpdateItem is an empty extension of its *Base. Per strict WSDL parity the update command MUST accept payloads without --goal-id, so the issue #198 H12 validation block is removed (with a comment explaining why the add-time minOccurs=1 check still stands). * copilot #1: PayForConversionCrrAddItem schema is Crr + GoalId (no Cpa). Moved PayForConversionCrr out of PAY_FOR_CONVERSION_STRATEGY_TYPES into CRR_STRATEGY_TYPES so --average-crr now maps to Crr, not the WSDL-unknown Cpa. * copilot #2: StrategyPayForConversionMultipleGoalsAddItem.GoalId is minOccurs=1, so the type is added to GOAL_ID_STRATEGY_TYPES; the add command now rejects payloads without --goal-id for that subtype. AverageCpaMultipleGoals stays out — its AddItem has no GoalId field at all. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
* fix(cli): close 12 High bugs from #198 audit + drop xfail markers Per the systemic-audit issue #198, the WSDL parity gate landed in PR #205 marked 8 commands as xfail-strict pending their fixes. This PR applies the fixes one by one and drops every xfail / KNOWN_*_BUGS exemption, so the gate now passes outright (37 passed, 8 skipped). Patterns and fixes: * H1, H5, H8, H9, H10 — empty-payload no-op guard: - ``ads update``, ``adgroups update``, ``keywords update``: if the built payload has only ``Id``, raise UsageError listing the available update fields. - ``bids set``, ``keywordbids set``: require at least one bid value before constructing the request. * H2 — silent data loss on ``ads update``: - Per-type allow-list (TEXT_AD / TEXT_IMAGE_AD / MOBILE_APP_AD) rejects incompatible flags up front (e.g. ``--image-hash`` with ``--type TEXT_AD``) instead of silently dropping them. * H4 — ``adgroups add --region-ids`` is now ``required=True`` (WSDL ``AdGroupAddItem.RegionIds`` minOccurs=1). * H6 — ``campaigns add --type SMART_CAMPAIGN --counter-id`` is now required (WSDL ``SmartCampaignAddItem.CounterId`` minOccurs=1). * H7 — ``dynamicads add`` no longer requires ``--condition``; the WSDL field is minOccurs=0 and the CLI was over-constraining. * H11 — ``STRATEGY_TYPES`` in ``direct_cli/commands/strategies.py`` is rebuilt from ``StrategyAddItem``: dropped 5 names that were not in the WSDL (WbMaximumClicksPerBid, WbMaximumConversionRatePerBid, AverageCrrPerCampaign, MaxProfitPerFilter, MaxProfitPerCampaign), added 5 that were missing (AverageCpcPerCampaign, AverageCpcPerFilter, PayForConversionCrr, AverageCpaMultipleGoals, PayForConversionMultipleGoals). Membership sets (CPA / CRR / PFC / multi-goal) updated accordingly. * H12 — ``strategies update`` now validates ``--goal-id`` for the goal-id strategy family, mirroring ``strategies add``. * H13 — verified false-positive. WSDL ``ClientUpdateItem`` does NOT declare ``ClientId`` minOccurs=1; the login goes through the Client-Login header, and ``clients update`` already enforces "Provide at least one field to update". No code change here. The parity gate (``tests/test_wsdl_parity_gate.py``) drops every ``xfail`` marker and the ``KNOWN_REQUIRED_FIELD_BUGS`` exemption set, so any regression of these patterns will now fail CI on the spot. Three pre-existing tests updated to match the new validation surface: - ``test_adgroups_add_case_insensitive_default_type`` now passes ``--region-ids`` (now required). - ``test_campaigns_add_smart_requires_filter_average_cpc`` now passes ``--counter-id`` (now required, validated before ``--filter-average-cpc``). - ``test_dynamicads_add_requires_condition`` rewritten as ``test_dynamicads_add_without_condition_omits_conditions_field`` to reflect the new minOccurs=0 behaviour. Refs #198. Closes PR 2 of milestone 0.3.9. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(strategies): align with WSDL on update GoalId and Crr-family Addresses PR #208 review: * claude[bot] H12 over-constraint: StrategyAverageCpaBase, StrategyPayForConversionBase, StrategyAverageCrrBase, StrategyPayForConversionCrrBase, StrategyAverageCpaPerCampaignBase all carry GoalId minOccurs=0, and every Strategy*UpdateItem is an empty extension of its *Base. Per strict WSDL parity the update command MUST accept payloads without --goal-id, so the issue #198 H12 validation block is removed (with a comment explaining why the add-time minOccurs=1 check still stands). * copilot #1: PayForConversionCrrAddItem schema is Crr + GoalId (no Cpa). Moved PayForConversionCrr out of PAY_FOR_CONVERSION_STRATEGY_TYPES into CRR_STRATEGY_TYPES so --average-crr now maps to Crr, not the WSDL-unknown Cpa. * copilot #2: StrategyPayForConversionMultipleGoalsAddItem.GoalId is minOccurs=1, so the type is added to GOAL_ID_STRATEGY_TYPES; the add command now rejects payloads without --goal-id for that subtype. AverageCpaMultipleGoals stays out — its AddItem has no GoalId field at all. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(strategies): route --average-cpc to FilterAverageCpc for AverageCpcPerFilter Addresses PR #208 re-review: * claude[bot] critical: StrategyAverageCpcPerFilterAddItem has no AverageCpc field — its WSDL schema is FilterAverageCpc + WeeklySpendLimit + ... . The CLI used to emit {AverageCpc: N} unconditionally, producing a WSDL-invalid payload for `--type AverageCpcPerFilter`. Added a FILTER_CPC_STRATEGY_TYPES branch mirroring the existing FILTER_CPA_STRATEGY_TYPES handling so `--average-cpc` maps to `FilterAverageCpc` for that subtype only. * claude[bot] observation: AverageCpaMultipleGoals and PayForConversion- MultipleGoals AddItems have no Cpa/AverageCpa field at all. Now `_build_strategy_fields` raises `UsageError` if `--average-cpa` is passed with a multi-goal subtype, instead of silently emitting an unknown key. Tests: * test_strategies_add_average_cpc_per_filter_payload_uses_filter_field locks the new dispatch. * test_strategies_add_pay_for_conversion_crr_payload_uses_crr_field confirms the earlier commit's CRR-family classification still routes through `Crr`. * test_strategies_add_multi_goal_rejects_average_cpa exercises the multi-goal guard. * test_strategies_add_pay_for_conversion_multi_goal_requires_goal_id exercises the GOAL_ID_STRATEGY_TYPES extension from the prior commit. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(strategies): reject empty-subtype no-op on update PR #208 re-review found one more silent no-op of the same class this PR closes for ads/adgroups/bids/keywordbids/keywords: ``` direct strategies update --id 42 --type AverageCpa ``` …built `{Id: 42, AverageCpa: {}}` and the live API accepted it as a 200 OK no-op. A user mistyping a flag name got a successful response with no diff applied. Two guards now reject this: * if `--type X` is present but `_build_strategy_fields(...)` returned `{}`, the command errors with "strategies update requires at least one field for --type X."; * if after all flag merging `strategy_data` still contains only `Id` (no --name, no --counter-ids, no --priority-goal, no --attribution-model), the command errors with "strategies update requires at least one updatable field." Also locked the regression down in the WSDL parity gate: added a `strategies.update` probe to `EMPTY_PAYLOAD_PROBES` alongside the existing five so future refactors cannot silently re-open the no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: axisrow <[email protected]> Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
* fix(auth): stop --help and unit tests from hanging on client-login network call A regression from #480 made get_credentials() resolve the bare Client-Login via a network clients.get on every CLI invocation when an OAuth profile with an email login lacked the login_migration_checked flag — including plain `<group> --help`. That call had no timeout (neither the vendored client nor tapi2 set one), so a slow link or a Yandex SmartCaptcha gateway could hang the CLI indefinitely. Under CliRunner this also hung the unit suite (it stalled on test_registered_mapped_groups_show_docs_url, which fires ~30 `--help` calls). Three independent guards: - auth.py: cap the best-effort resolver with LOGIN_RESOLVE_TIMEOUT_SECONDS=8 and disable retries, so it can never wait forever. - cli.py: skip the resolver entirely on --help/--version/-h passes (allow_login_resolve=False) — no command runs, so no login is needed. - tests/conftest.py: autouse fixture neutralizes the network resolver for the whole unit suite; the module-level test-credential probe no longer triggers it. Adds regression tests asserting a --help pass makes zero resolver calls and that allow_login_resolve=False suppresses the migration. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix: resolve 13 confirmed bugs from the #483 bug hunt Verified each finding against the live API, the WSDL cache, and the official Yandex Direct docs before fixing. Bugs 10 and 14 were false positives (no UsageError source inside the try — style only) and are intentionally skipped. Functional fixes: - bids/keywordbids get: reject empty SelectionCriteria with a UsageError before the request (live API error 4001) (#1, #2). - bids get: re-raise UsageError so the new guard surfaces with exit code 2 (#8). - bids set-auto: require exactly one of CampaignId/AdGroupId/KeywordId via add_single_id_selector (docs: the three are mutually exclusive) (#4). - vendored to_columns: pad short report rows instead of IndexError (#3). - vendored error handler: error_data.get("error_detail") to avoid KeyError (#11). - reports build_report_request: reject empty FieldNames (live API error 8000) (#12). Swallowed-validation fixes (add `except click.UsageError/ClickException: raise` before the bare `except Exception`): balance (#6), strategies get (#7), campaigns get (#9), ads get (#13), negativekeywordsharedsets update (#5). Type annotation: parse_priority_goals_spec items -> List[Dict[str, Any]] (#15). build_api_coverage_report: supply --campaign-ids for bids/keywordbids get wire-capture so the empty-criteria guard does not break schema parity. Closes #483 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix: broaden #483 coverage and add CHANGELOG (cherry-picked from #484) Incorporates the genuinely-better parts of the parallel PR #484 into this branch, which already carries the regression tests, the #15 annotation fix, and the localized messages: - ads: re-raise click.UsageError in all lifecycle handlers (update, delete, archive, unarchive, suspend, resume, moderate) in addition to get — so the whole resource matches bug #13's "get + 8 lifecycle" scope. Defensive: these handlers have no pre-API UsageError source today, but the guard keeps them consistent and future-proof. - retargeting get/delete, advideos get: same defensive re-raise for uniformity (issue #483 items 10/14; not live defects, but harmless consistency). - bids/keywordbids get: the empty-criteria message now also lists --serving-statuses, which likewise populates SelectionCriteria (kept the t() localization wrapper). - CHANGELOG.md: document the bug-hunt fixes (#483) and the --help network-hang follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --------- Co-authored-by: axisrow <[email protected]> Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
…ns (#488) * fix(vendor): recover from tapi_yandex_direct 2026.5.29 bump regressions The 2026.5.29 vendor bump (1ee8c2a) rebuilt direct_cli/_vendor via update_vendor.sh's rm -rf + cp -R of the fork, wiping local patches and desyncing the vendor from the tests. That introduced four regressions; CI surfaced only the first because Quality fails on mypy before the test step: 1. timeout stub (mypy): the .pyi for YandexDirectClientExecutor.get/post dropped the `timeout` kwarg auth.py:631 passes, so mypy rejected the call even though tapi2 forwards it to requests at runtime. 2. to_columns IndexError: upstream reverted the short-row guard; report rows with fewer cells than the header crashed. 3. transport hosts: upstream turned DIRECT_API_PRODUCTION_ROOT into a {tld} template (v5 -> .com, v4 -> .ru); test_transport_contract compared against the un-substituted constant. 4. VCR cassettes: the v5 host moved .ru -> .com but all cassettes are .ru, so the host matcher failed and 42 replay tests broke under --record-mode=none. Fixes: - Re-apply #1 and #2 and make them self-healing in patch_vendor_imports.py (_patch_stub_text for the .pyi, _patch_runtime_text for to_columns) so the next vendor bump cannot silently drop them again; both are idempotent. - Add two gates in test_vendor_imports.py (ast check on the shipped stub + idempotency of _patch_stub_text). - Update test_transport_contract.py to the {tld} contract. - Register a TLD-insensitive VCR host matcher in conftest.py so .ru cassettes replay against .com requests and vice versa. Verified: mypy clean, ruff clean, pytest -m "not integration" all green (2035 passed), self-heal restores both patches on a stripped tmp copy and is a no-op on re-run. Closes #487 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * refactor(vendor): boundary-aware class scope in _patch_stub_text Review follow-up (#488): _patch_stub_text scoped the stub class with line.startswith("class YandexDirectClientExecutor"), which also matches the prefix-sharing class YandexDirectClientExecutorResponse. Harmless today (that Response body has no get/post or :param data:), but a latent fragility if upstream ever reorders classes. Replace the prefix check with a boundary-aware regex ^class YandexDirectClientExecutor[:\(] so only the exact class is treated as in-scope, and add a gate test feeding a prefix-sharing decoy class with the exact executor signature (would fail on the old startswith check). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --------- Co-authored-by: axisrow <[email protected]> Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
Summary
--op-token-ref/--op-login-refCLI options and env var fallback (YANDEX_DIRECT_OP_TOKEN_REF,YANDEX_DIRECT_OP_LOGIN_REF) for reading secrets viaopCLI. Priority: CLI flags > env vars > .env > 1Passwordsetuptools_scm, set static version inpyproject.toml, reducesetup.pyto a minimal shimCLAUDE.md, document destructive commands in README and copilot-instructions, document excluded integration testsTest plan
pytest— all 35 tests pass (29 existing + 6 new)pytest tests/test_auth_op.py -v— 1Password unit testsdirect-cli --help— shows--op-token-refand--op-login-refdirect-cli --op-token-ref "op://vault/item/token" campaigns get --dry-run🤖 Generated with Claude Code