# ADR-0001 — v0.3.0 fix strategy

- Status: Accepted
- Date: 2026-05-26
- Scope: three bugs surfaced in v0.2.x + their twins, fixed together as one coherent release

## Context

Three bugs surfaced while exercising the MCP against the live Google Ads API v23:

1. **PAUSED-by-default invisibility.** Every newly created Responsive Search Ad ended up in `PAUSED` status, but the response payload returned `status: "APPLIED"` with no signal that the entity was not actually live. LLM agents consuming the MCP could not distinguish a live ad from a staged one without an extra GAQL round-trip, and silent staging led to days of zero impressions on freshly deployed ad groups.
2. **`dismiss_recommendation` 400 Bad Request.** The tool built a `{"dismissRecommendationOperation": …}` payload and routed it to `googleAds:mutate`. The v23 `MutateOperation.operation` oneof contains no `*RecommendationOperation` field; `apply` and `dismiss` are dedicated RPCs (`/recommendations:apply`, `/recommendations:dismiss`). `apply_recommendation` had the exact same shape and would have failed the same way under load.
3. **Cosmetic, non-actionable warning after successful apply.** When `require_dry_run=true` was set in the safety config and the user explicitly passed `dry_run=false`, `confirm_and_apply` returned `status: APPLIED` along with a warning string telling the caller to re-run with `dry_run=true`. The flag had no behavioural effect — it was security theatre.

Auditing the surrounding code revealed twin failure modes:

- `campaigns_write.rs` hardcoded `PAUSED` on three create paths.
- `pmax.rs` exposed a `start_paused` parameter that was never used in the payload.
- `ad_groups_write.rs` hardcoded `ENABLED` (the lone outlier).
- `requires_double_confirm` was dead code in `confirm.rs`.
- Tests covered the preview JSON shape but never asserted the content of `mutate_operations[0]`, the routing layer, or the HTTP request actually emitted. The same gap explained all three bugs.

## Decision process

The strategy was deliberated across eight independent expert lenses, each producing a recommendation per decision with explicit confidence and tradeoffs. The angles were:

| # | Lens | What it pushed for |
|---|------|--------------------|
| 1 | API contract design / semver pre-1.0 | Direct `0.3.0` minor bump, no two-step deprecation for a single power user |
| 2 | Rust idiomatic / type system | Discriminated enums (`AdStatus`, `PlanDispatch`), fail-fast `Result<T, E>`, no stringly-typed status |
| 3 | Google Ads API v23 ground truth | Dedicated RPC endpoints for apply/dismiss, whitelist of valid `MutateOperation` keys |
| 4 | Black-box integration testing | wiremock for endpoint routing assertions, no mocks of the system under test |
| 5 | Security & safety defaults | Fail-safe `PAUSED` default for a non-deterministic LLM caller, hard guards over warnings |
| 6 | DevX / agent UX (LLM as consumer) | Visibility of `status_after_apply` + actionable `next_action_hint` so the agent never needs UI |
| 7 | OSS maintenance / community | Conventional Commits with `!` for breaking, keepachangelog format, atomic commits per root cause |
| 8 | Risk management / production reliability | Pre-flight audit before enabling anything, blast-radius minimisation, test harness before refactor |

The five framing decisions and their resolution:

### D1 — Default status on ad/campaign create

**Resolved as: keep `PAUSED` default, expose the status param, add visibility + actionable hint.**

The two-step `draft → confirm_and_apply(dry_run=false)` is already the human-in-the-loop. The argument for flipping the default to `ENABLED` (alignment with Google's own SDK samples) lost to the safety case: the caller is a non-deterministic LLM, the blast radius is financial, and the cost of "one extra `enable_entity` call" is zero compared to the cost of a wrongly-live campaign. What the bug was really about was *invisibility*, not the default itself — fix the invisibility, keep the safe default.

### D2 — Bug 2 fix scope

**Resolved as: dedicated RPCs + client-side whitelist (defence in depth), no broader refactor.**

A full `PlanExecution` enum refactor across `safety/preview.rs` was tempting from a pure Rust-idiomatic standpoint, but its surface area was too large for a release lacking a test harness. The patch routes the two affected ops to their dedicated endpoints, and the whitelist catches future regressions of the same shape with negligible code cost. A `PlanDispatch` discriminator was added to `ChangePlan`, which is enough to keep the option for the larger refactor open without committing to it.

### D3 — Bug 3 fix shape

**Resolved as: hard guard that returns `Err` before any HTTP, not a warning.**

`require_dry_run` either means something or it does not. As a warning emitted *after* the live call already happened, it meant nothing. Turning it into a hard guard makes the safety config name match its behaviour. Opt-in escape hatches (`bypass_require_dry_run`, `confirmed_twice`) preserve usability for legitimate cases. The cosmetic warning is gone. The same treatment was applied to `requires_double_confirm`, which was dead code.

### D4 — Versioning

**Resolved as: `0.3.0` direct, no `0.2.2` deprecation step.**

In semver pre-1.0 the minor bump is the canonical breaking signal, and the audience does not justify a two-release deprecation dance. A keepachangelog `CHANGELOG.md` plus a "Migrating from v0.2.x" section in the README carries the load.

### D5 — Scope of twin bug fixes

**Resolved as: every twin in the same PR. Nothing deferred.**

Twin bugs deferred to "later issues" are not in fact fixed later — they resurface as separate incidents. The cost of including the twins was bounded (every change is mechanical once the foundations are in place) while the cost of leaving them was an open invitation to repeat the same incident on a different surface.

## Tradeoffs accepted

- **Surface area of v0.3.0 is large.** Twelve source files touched, five atomic commits, breaking changes on three semantic boundaries (status default consistency, hard guards, recommendations routing). Mitigated by atomic commits per root cause for bisectability and by exhaustive black-box tests.
- **The `PlanExecution` enum refactor was deferred.** A `PlanDispatch` field on `ChangePlan` plus the routing switch in `confirm.rs` solved the immediate problem without the larger refactor's regression risk. The next release that needs to add a third non-mutate RPC will be the right moment to revisit.
- **`PAUSED` default has friction for some agents.** A naïve "create one ad, expect it live" workflow now needs a second tool call. Mitigated by the `next_action_hint` field carrying the exact `enable_entity` invocation, so the friction is purely mechanical, never investigative.

## Tests

Every acceptance criterion is covered by a black-box integration test using wiremock as a fake Google Ads server:

- Routing tests assert both the expected endpoint hit and the wrong endpoint NOT hit, so a regression of the original 400 shape cannot pass silently.
- Hard-guard tests assert zero HTTP calls when the guard fires.
- Whitelist tests cover known-good keys, known-bad keys (the operation names the bugs used), and typos.
- Status visibility tests assert the full response shape: `status_after_apply`, `next_action_hint.tool`, `next_action_hint.params`, `next_action_hint.description`.

`wiremock` is treated as a fake of an external dependency, not a mock of the system under test, and is explicitly allowed by the testing convention.

## What would change this decision

- A second tracked consumer of the MCP appearing on `crates.io` would push D4 toward a two-step deprecation path.
- A measured workflow where the default `PAUSED` friction outweighs the safety benefit (e.g., a strict CI pipeline that always applies through `confirm_and_apply`) would push D1 toward `ENABLED` default with explicit opt-in to `PAUSED`.
- A third non-mutate RPC needed by the MCP would justify completing the `PlanExecution` refactor deferred in D2.

## Consequences

- Five atomic commits on `fix/v0.3.0-rsa-paused-visibility-and-recommendations-routing`.
- 251 tests passing (226 lib + 25 new wiremock integration), `clippy -D warnings` and `fmt --check` clean.
- One breaking-change PR open against `main`, ready to merge as `v0.3.0`.
