Skip to content

fix: remove keywords archive/unarchive, fix credential-leak in 18 tests#102

Merged
axisrow merged 1 commit into
mainfrom
fix/keywords-archive-removal-credential-leak
Apr 25, 2026
Merged

fix: remove keywords archive/unarchive, fix credential-leak in 18 tests#102
axisrow merged 1 commit into
mainfrom
fix/keywords-archive-removal-credential-leak

Conversation

@axisrow

@axisrow axisrow commented Apr 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • Remove keywords archive and keywords unarchive commands — Yandex Direct API Keywords service doesn't support these methods (only add, update, delete, suspend, resume, get)
  • Fix credential-leak in 18 tests across 5 files by adding @patch("direct_cli.auth.get_active_profile", return_value=None) so tests don't read real tokens from ~/.direct-cli/auth.json

Changes

Keywords archive/unarchive removal (11 files):

  • direct_cli/commands/keywords.py — removed archive() and unarchive() functions
  • direct_cli/smoke_matrix.py — removed from WRITE_SANDBOX
  • direct_cli/wsdl_coverage.py — removed from INTENTIONAL_EXTRA_METHODS
  • tests/test_api_coverage.py — removed from exclusions
  • tests/test_smoke_matrix.py — updated counts (122→120, 118→116, 77→75)
  • scripts/sandbox_write_live.py — removed handlers
  • scripts/test_dangerous_commands.sh, README.md, .github/copilot-instructions.md, tests/MANUAL_COVERAGE.md, tests/test_dry_run.py — updated docs

Credential-leak fix (5 files, 18 tests):

  • tests/test_auth_bw.py (7 tests)
  • tests/test_auth_op.py (6 tests)
  • tests/test_cli.py (1 test)
  • tests/test_comprehensive.py (3 tests)
  • tests/test_dry_run.py (1 test)

Test plan

  • pytest — 288 passed, 0 failed, 51 skipped
  • direct keywords archive → "No such command"
  • direct keywords unarchive → "No such command"
  • No remaining keywords.archive/keywords.unarchive references in codebase

Closes #101

🤖 Generated with Claude Code

…x credential-leak in 18 tests (#101)

Keywords service API only supports add/update/delete/suspend/resume/get.
The archive/unarchive methods don't exist, causing timeouts on every call.

Also fixes test isolation: 18 tests in 5 files leaked real tokens from
~/.direct-cli/auth.json because get_active_profile() wasn't mocked.

Closes #101

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@axisrow

axisrow commented Apr 25, 2026

Copy link
Copy Markdown
Owner Author

@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 Bot commented Apr 25, 2026

Copy link
Copy Markdown

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


PR Review

  • Read changed files and gather context
  • Review keywords archive/unarchive removal
  • Review credential-leak fix in tests
  • Post final review findings

Summary

The PR is correct and well-motivated. No critical bugs, security issues, or data-loss risks found. Two minor observations below.


Keywords archive/unarchive removal

Clean removal. All cross-references are consistent:

  • keywords.py — functions deleted
  • smoke_matrix.py, wsdl_coverage.py — entries removed
  • test_api_coverage.py, test_smoke_matrix.py, test_dry_run.py — counts/exclusions updated
  • sandbox_write_live.py — handlers removed

The README.md reference to archive/unarchive at line 384 is fine — it explicitly scopes those to campaigns/ads, not keywords.


Credential-leak fix

The fix is correct and addresses a real non-determinism bug. Without patching get_active_profile, a developer machine with an active profile in ~/.direct-cli/auth.json would cause tests like test_get_credentials_bw_fallback to skip the Bitwarden path entirely (because the profile supplies a real token), causing mock_bw_read.assert_called_once_with(...) to fail.

Decorator ordering is correct@patch applies bottom-up, so the new middle decorator maps correctly to the mock_profile argument position in all 13 affected test signatures.


Minor gap (non-blocking)

Two tests in tests/test_comprehensive.py still call get_active_profile() without patching it:

  • test_token_from_argument (line 161): calls auth.get_credentials(token="test_token", login="test_login") with no mocks at all.
  • test_argument_takes_priority_over_env (line 173): same pattern.

Since both pass a token directly, final_token is set before the profile branch (if selected_profile and not final_token:) can fire, so these tests won't fail even if an active profile exists. The auth file is still read from disk but the result is harmless. These are low-risk gaps, but worth patching for consistency with the rest of this PR's intent.

Fix this →


Overall: Approved. The removal is accurate (the API truly doesn't support these methods), the fix is correct and targeted, and the test count updates match the deletions. The two unpatched tests are a minor consistency gap, not a correctness issue.

@axisrow
axisrow merged commit affaee6 into main Apr 25, 2026
6 checks passed
@axisrow
axisrow deleted the fix/keywords-archive-removal-credential-leak branch April 25, 2026 11:56
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.

Fix credential-leak: 16 tests fail when ~/.direct-cli/auth.json has real token

1 participant