@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

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

Claude PR Review

Focused on critical issues (bugs, security, logical errors, data loss, performance) per request. Verified the credential plumbing against direct_cli/auth.py and tests/conftest.py, the count derivations against direct_cli/smoke_matrix.py:237 (smoke_summary) and direct_cli/smoke_matrix.py:214 (_registered_cli_commands), and the probe-failure wrapping against the seven affected setUpClass callsites.

Verdict: No critical issues found. Ship it.

The five fixes are real and well-targeted. No bugs, security holes, logical errors, data-loss risks, or performance regressions surfaced.

Things I checked

High #2 — _prepend_credential_flags (tests/test_integration.py:60-86)

• Correctly handles the only multi-token leading case in this file (--sandbox then subcommand at line 630). Insertion order produces [--sandbox, --token X, --login Y, agencyclients, get, …], all three top-level group options, which Click resolves correctly.
• --token/--login are priority 1 in the chain (direct_cli/auth.py:get_credentials), so they correctly override an active developer profile — matches CLAUDE.md's inverted-credential contract.
• When _REAL_TOKEN is None the function returns the args untouched, but every class that calls invoke_get is gated by @skip_if_no_token, so the no-token path is unreachable. Fine.
• Token value is passed as a Python list argument to CliRunner.invoke (no shell), so no injection risk.

High #1 — _skip_class_on_probe_failure (tests/test_integration.py:88-100)

• except Exception correctly leaves KeyboardInterrupt/SystemExit alone.
• unittest.SkipTest raised from setUpClass is honoured by the runner and skips the whole class as intended.
• One trade-off worth noting (not blocking): the catch is broad enough that a probe refactor that raises AttributeError/TypeError would be masked as a "transient API outage" in CI rather than failing loudly. If a probe regression slips in, it'll show up as a chronic skip. Mentioning so the team is aware — not asking for changes.

Medium #5/#6 — dynamic count derivation (tests/test_smoke_matrix.py:60-110, tests/test_sandbox_write_audit.py:33-40)

• Walked the derivation against smoke_matrix.py:237-261 end-to-end:
  • total_cli_groups → len(cli.commands) ✓
  • total_cli_subcommands → test sums dict items, impl builds a set keyed on (group, command); since dict iteration produces no duplicate pairs, set-count == sum-count ✓
  • api_cli_subcommands → both exclude only "auth" ✓
  • wsdl_services, api_services_total, wsdl_operations → identical formulas on both sides ✓
• These are now tautological in the sense that bugs in smoke_summary could pass undetected, but the strict invariant lives in test_smoke_matrix_covers_every_cli_subcommand_once (line 49), which still gates the matrix↔CLI mapping. Net regression-detection is roughly equivalent: the old magic numbers caught any drift but with an opaque message; the new self-consistency check still catches the same class of bug via the coverage test, just with a usable failure message.
• test_sandbox_write_audit.py — assert len(rows) == expected_total still catches an audit script that under/over-counts vs the matrix. The check that mattered is preserved.

Medium #3 — dedup in tests/test_comprehensive.py

• Confirmed TestCLIHelp and test_campaigns_help were faithful duplicates of test_cli.py:TestCLI::test_cli_help and equivalents. Other classes in this file (TestCommandsRegistered, TestUtils, TestOutputFormatters, TestAuth, TestErrorHandling) cover surfaces not touched by test_cli.py. Safe deletion.

Checklist

• Read the diff (git diff origin/main...HEAD)
• Review tests/test_integration.py — credential plumbing & probe-failure skip
• Review tests/test_smoke_matrix.py — dynamic count derivation
• Review tests/test_sandbox_write_audit.py — dynamic count derivation
• Review tests/test_comprehensive.py — dedup
• Post review findings
  • Branch: test/integration-suite-cleanup

Rebased onto post-#401/#403 main (CHANGELOG conflicts resolved by appending Tests block under 0.3.14). No code changes vs. previous SHA. @claude review.

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

Claude PR Re-review (post-rebase)

• Confirm diff vs origin/main matches the previously-reviewed set of code changes
• Confirm CHANGELOG conflict was resolved cleanly (Tests block appended under 0.3.14)
• Re-validate the four critical-path checks from the prior review
• Post updated verdict

Verdict: No critical issues found. Rebase looks clean — ship it.

What changed since the prior review

Only CHANGELOG.md materially differs from the previously-reviewed SHA. The other four files (tests/test_integration.py, tests/test_comprehensive.py, tests/test_smoke_matrix.py, tests/test_sandbox_write_audit.py) carry the same code I checked before. Totals still +173 / −39 across 5 files.

CHANGELOG resolution check

The Tests block is appended under the existing 0.3.14 heading (CHANGELOG.md:41-65), after the Breaking changes / Added / Closes #360. sections introduced by #399/#400/#401/#403. Nothing from those PRs was overwritten:

• Breaking changes for --priority-goals (priority-goals: Value must be advertiser currency × 1,000,000 (wire-format bug) #387) — preserved at lines 5-17.
• Added entries for --sitelink-field-names, --fields/--search-field-names/--network-field-names on keywordbids, and the test_every_nested_fieldnames_param_has_cli_option regression test (Audit remaining typed field coverage after CalloutFieldNames gap #360/feat(api): expose separate *FieldNames params for sitelinks and keywordbids #403/Wave 2: expose remaining nested *FieldNames request parameters as CLI options #402) — preserved at lines 19-37.
• New Tests block — lines 41-63, with its own Closes #396. footer.
• 0.3.13 heading still at line 67, no off-by-one in the section boundary.

Single-version block, single Closes line per concern — matches the file's existing format.

Critical-path checks re-validated

All four findings from my prior review still hold against the rebased SHA:

1. _prepend_credential_flags + _invoke_env (tests/test_integration.py:60-87): credentials are passed as both --token/--login CLI flags (priority 1) and env vars — belt-and-suspenders. The --sandbox insertion-order branch is intact (lines 80-83); --sandbox stays attached to the top-level group and credential flags slot in before the subcommand. Token is a list arg to CliRunner.invoke, no shell, no injection risk.
2. _skip_class_on_probe_failure (tests/test_integration.py:89-102): unchanged. Broad-Exception catch still leaves KeyboardInterrupt/SystemExit alone. Skip is honoured by setUpClass. Same caveat as before — a probe refactor raising AttributeError/TypeError would be masked as a "transient outage" and shown as a chronic skip; not a blocker.
3. Dynamic count derivation (tests/test_smoke_matrix.py, tests/test_sandbox_write_audit.py): formulas still match smoke_matrix.py:smoke_summary exactly; the strict invariant in test_smoke_matrix_covers_every_cli_subcommand_once still gates the matrix↔CLI mapping. No regression in detection coverage; just better failure messages.
4. test_comprehensive.py dedup: TestCLIHelp removed, surviving classes (TestCommandsRegistered, TestUtils, TestOutputFormatters, TestAuth, TestErrorHandling) still cover surfaces not touched by test_cli.py.

Nothing in the rebase resurrected an issue; the audit-fix story is intact.
• Branch: test/integration-suite-cleanup