Skip to content

Add 1Password auth and simplify build config#2

Merged
axisrow merged 6 commits into
mainfrom
feature/1password-auth-and-improvements
Mar 13, 2026
Merged

Add 1Password auth and simplify build config#2
axisrow merged 6 commits into
mainfrom
feature/1password-auth-and-improvements

Conversation

@axisrow

@axisrow axisrow commented Mar 13, 2026

Copy link
Copy Markdown
Owner

Summary

  • 1Password authentication: new --op-token-ref / --op-login-ref CLI options and env var fallback (YANDEX_DIRECT_OP_TOKEN_REF, YANDEX_DIRECT_OP_LOGIN_REF) for reading secrets via op CLI. Priority: CLI flags > env vars > .env > 1Password
  • Build simplification: remove setuptools_scm, set static version in pyproject.toml, reduce setup.py to a minimal shim
  • Docs: add CLAUDE.md, document destructive commands in README and copilot-instructions, document excluded integration tests

Test plan

  • pytest — all 35 tests pass (29 existing + 6 new)
  • pytest tests/test_auth_op.py -v — 1Password unit tests
  • direct-cli --help — shows --op-token-ref and --op-login-ref
  • Manual: direct-cli --op-token-ref "op://vault/item/token" campaigns get --dry-run

🤖 Generated with Claude Code

axisrow and others added 3 commits March 13, 2026 14:17
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]>
@axisrow

axisrow commented Mar 13, 2026

Copy link
Copy Markdown
Owner Author

@claude review

1 similar comment
@axisrow

axisrow commented Mar 13, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Mar 13, 2026

Copy link
Copy Markdown

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


PR Review: Add 1Password auth and simplify build config

  • Gather context and read changed files
  • Review auth.py changes
  • Review cli.py changes
  • Review test coverage
  • Review build config changes
  • Post final review

Overall this is a solid, focused PR. The 1Password integration is straightforward and secure (no shell injection, uses shutil.which, proper timeout). The build simplification is clean. A few issues worth addressing before merging:


Issues

1. subprocess.TimeoutExpired not caught in op_read (medium)

auth.py:34-44 — if the op CLI hangs for 10 seconds, subprocess.TimeoutExpired is raised and propagates as an unhandled exception with a confusing traceback. It should be caught and re-raised as a user-friendly RuntimeError:

try:
    result = subprocess.run([op_path, "read", ref], ...)
except subprocess.TimeoutExpired:
    raise RuntimeError("1Password CLI timed out after 10 seconds")

Fix this →


2. CLI flags bypass the documented priority order (medium)

cli.py:62-65--op-token-ref and --op-login-ref are stored into os.environ rather than passed as parameters to get_credentials(). This has two problems:

  • ctx.obj stores token and login directly, but the op refs are smuggled through global state. The two mechanisms are inconsistent.
  • If subcommands call get_credentials(token=ctx.obj["token"]), the CLI --token flag takes priority over 1Password — but there's no way to pass --op-token-ref at the same level, because it's silently set as an env var. The effective priority becomes: --token / env var → 1Password ref env var, with the CLI ref flag converted to the same env var path as the env var fallback.

A cleaner approach: store op_token_ref in ctx.obj and pass it explicitly to get_credentials() (adding it as a parameter). This makes the data flow explicit. Fix this →


3. get_credentials() docstring doesn't mention 1Password (minor)

auth.py:60-75 — the docstring lists only 3 priority levels but the implementation has 4. The comment inline is correct but the docstring Args/description is stale:

# Docstring says:
# 1. Direct arguments
# 2. Environment variables
# 3. .env file
# Missing: 4. 1Password (via YANDEX_DIRECT_OP_TOKEN_REF / op_token_ref)

Fix this →


4. --op-token-ref missing envvar= in Click option (minor)

cli.py:47-54 — unlike --token (which declares envvar="YANDEX_DIRECT_TOKEN"), the --op-token-ref option has no envvar= declaration. The env var still works because get_credentials() reads it directly, but --help won't show users which env var to set, which is inconsistent with the rest of the CLI:

@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)",
)

Fix this →


Suggestions

5. Unused import in test file (nit)

tests/test_auth_op.py:3import subprocess is imported but never used. Can be removed.

6. No test for CLI flag → env var path

There's no test covering the cli.py code path where --op-token-ref sets os.environ["YANDEX_DIRECT_OP_TOKEN_REF"]. The existing tests go directly through the auth module. A CLI integration test using CliRunner would close this gap.


What looks good

  • op_read uses shutil.which + list-form subprocess.run — no shell injection possible
  • 10-second timeout on the op CLI call is appropriate
  • Test coverage for the three main scenarios (success, CLI not found, CLI error) is solid
  • setup.py reduction to a minimal shim is correct modern practice
  • pyproject.toml build simplification is clean and correct
  • .env.example shows the 1Password vars are commented-out (opt-in), which is the right UX
  • Double load_dotenv() (at import time in cli.py and inside get_credentials) is pre-existing, not introduced here

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

axisrow commented Mar 13, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Mar 13, 2026

Copy link
Copy Markdown

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


PR Review: Add 1Password auth and simplify build config

  • Gather context and read changed files
  • Review auth.py changes
  • Review cli.py changes
  • Review test coverage
  • Review build config changes
  • Post final review

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 --op-token-ref from working at all in actual subcommands.


Issues

1. --op-token-ref CLI flag is silently broken for all subcommands (critical)

cli.py:62-63 — The fix for the previous issue #2 correctly moved op_token_ref into ctx.obj instead of os.environ. But the subcommands never read it back out. Every create_client() call across all command modules only passes token, login, and sandbox from ctx.obj:

# 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: direct-cli --op-token-ref "op://vault/item/token" campaigns get silently fails to use the ref. The env var path (YANDEX_DIRECT_OP_TOKEN_REF) still works because get_credentials() reads it directly, but the CLI flag does nothing.

Two options to fix this:

Option A — Pass from ctx.obj in every command (noisy, ~25 files):

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 cli() group and store the resolved token/login in ctx.obj (cleaner, single change in cli.py):

# 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 token

Option B is preferred — it centralizes auth resolution, keeps all subcommands unchanged, and makes the credential flow explicit.

Fix this →


2. test_op_token_ref_stored_in_ctx doesn't test what it says (minor)

tests/test_auth_op.py:109-114 — The test invokes --help which exits before ctx.obj is populated from the CLI params. It verifies exit_code == 0 but not that the ref was actually stored. A more meaningful test would pass the flag to a real subcommand (with op_read mocked) and assert that op_read was called with the right ref.


What was fixed from the previous review


What looks good

  • op_read security: shutil.which + list-form subprocess, no shell injection possible
  • Timeout handling is now complete and user-friendly
  • get_credentials priority logic is correct — env vars fall through to 1Password cleanly
  • Build simplification (setup.py → minimal shim, static version in pyproject.toml) is correct modern practice
  • pyproject.toml classifiers and Python version range are accurate

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

axisrow commented Mar 13, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Mar 13, 2026

Copy link
Copy Markdown

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


PR Review: Add 1Password auth and simplify build config

  • Gather context and read changed files
  • Review auth.py changes
  • Review cli.py changes
  • Review test coverage
  • Post final review

Good progress — the critical regression from the previous review (Issue #1: --op-token-ref silently broken for subcommands) is now properly fixed using Option B. The fix is clean: cli() resolves credentials early and subcommands pick up the resolved values from ctx.obj. One new issue was introduced by that fix, and one minor gap remains.


Issues

1. RuntimeError from 1Password not caught in the group callback (medium)

cli.py:71 — The except ValueError block does not catch RuntimeError, which is what op_read raises when the op CLI is missing, times out, or returns an error. If a user passes --op-token-ref "op://vault/item/token" but the op binary isn't installed, the exception propagates as an unhandled traceback from the group callback — before any subcommand error handling runs.

Before this fix, errors surfaced inside each subcommand's try/except Exception → print_error() wrapper. Now they happen earlier with no handler in place.

Fix: catch RuntimeError separately and surface it as a ClickException:

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"] = login

Fix this →


2. No test for 1Password error path via CLI flag (minor)

tests/test_auth_op.py — There's no test covering what happens when --op-token-ref is passed but op_read raises RuntimeError (e.g., op not installed). With the new group-callback resolution, this path now produces an unhandled exception (issue #1 above). After fixing that, a test like this would close the gap:

@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.output

What was fixed from the previous review

  • Critical: --op-token-ref CLI flag now correctly resolves credentials in the cli() group callback (Option B), storing the final token/login in ctx.obj so all subcommands pick them up
  • Minor: test_op_token_ref_stored_in_ctx (which only ran --help and never verified resolution) replaced with test_op_token_ref_resolves_via_cli_flag, which actually invokes campaigns get --help and asserts op_read was called with the right ref

What looks good

  • op_read: shutil.which + list-form subprocess.run, no shell injection, TimeoutExpired caught and re-raised as RuntimeError
  • get_credentials() parameter signature matches the docstring (all 4 priority levels documented)
  • --op-token-ref / --op-login-ref have envvar= declarations so --help shows the env var names
  • The condition if token or login or op_token_ref or op_login_ref at cli.py:63 correctly handles the .env file case: load_dotenv() runs at import time (line 40), Click reads env vars via envvar= into the option params, so the condition is truthy when credentials come from .env too
  • Double get_credentials() call (group + create_client) is harmless: the second call gets the pre-resolved token and returns immediately without calling op_read again
  • Build simplification (setup.py → minimal shim, static version in pyproject.toml) is correct
  • Test coverage for op_read (success, not-found, error, timeout) and get_credentials (env fallback, priority) is solid

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

axisrow commented Mar 13, 2026

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Mar 13, 2026

Copy link
Copy Markdown

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


PR Review: Add 1Password auth and simplify build config

  • Gather context
  • Read auth.py, cli.py, and test files
  • Check if previous issues were addressed
  • Post final review

All issues from the previous review rounds have been addressed. This PR is ready to merge.


What was fixed in the latest round

  • Issue Add Claude Code GitHub Workflow #1 (medium): RuntimeError from 1Password is now caught in the cli() group callback and re-raised as click.ClickException (cli.py:71-72), giving users a clean error message instead of a raw traceback
  • Issue Add 1Password auth and simplify build config #2 (minor): test_op_token_ref_error_surfaces_cleanly (test_auth_op.py:121-131) covers this exact path — it patches op_read to raise RuntimeError, invokes the CLI with --op-token-ref, and asserts a non-zero exit code with the error message in output

Complete picture: what's in the PR

auth.py

  • op_read(): shutil.which + list-form subprocess.run, no shell injection possible; TimeoutExpired caught and re-raised as RuntimeError; non-zero returncode raises RuntimeError with stderr
  • get_credentials(): correct 4-level priority (CLI args → env vars → .env → 1Password); docstring matches implementation

cli.py

  • --op-token-ref / --op-login-ref options have envvar= declarations so --help shows env var names
  • Credentials resolved early in the group callback; RuntimeErrorclick.ClickException; ValueError (no token) deferred naturally to subcommands
  • All subcommands receive pre-resolved ctx.obj["token"] — no subcommand files needed to change

tests/test_auth_op.py — 9 tests covering:

  • op_read: success, CLI not found, CLI error, timeout
  • get_credentials: 1Password env fallback, env-over-1Password priority, login fallback, explicit ref param
  • CLI: --help shows options, flag resolves via op_read, error surfaces cleanly

Build simplification

  • setup.py reduced to a minimal shim; setuptools_scm removed; static version in pyproject.toml — correct modern practice

What looks good end-to-end

  • The double get_credentials() call (group callback + create_client()) is harmless: after resolution, the second call gets a non-empty token and returns immediately without calling op_read again
  • .env credentials work correctly: load_dotenv() at import time populates env vars, Click reads them via envvar= into option params, condition at cli.py:63 is truthy, credentials resolve normally
  • Credential priority matches the documented order and matches what users expect

Branch |

@axisrow axisrow merged commit 1d017ab into main Mar 13, 2026
1 check passed
@axisrow axisrow deleted the feature/1password-auth-and-improvements branch March 13, 2026 09:23
axisrow added a commit that referenced this pull request May 18, 2026
…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]>
axisrow added a commit that referenced this pull request May 19, 2026
…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]>
axisrow added a commit that referenced this pull request May 20, 2026
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]>
axisrow added a commit that referenced this pull request May 20, 2026
* 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]>
axisrow added a commit that referenced this pull request May 30, 2026
* 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]>
axisrow added a commit that referenced this pull request May 30, 2026
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant