feat(shared-set): add attach/detach tools for CampaignSharedSet

[SullyGitHub]

Summary

Adds two write tools for managing CampaignSharedSet linkages — the missing piece between "create a shared negative list" and "actually have it apply to a campaign."

• attach_shared_set_to_campaigns(shared_set_id, campaign_ids) — creates CampaignSharedSet linkages so the campaigns inherit the shared set's criteria.
• detach_shared_set_from_campaigns(shared_set_id, campaign_ids) — removes the linkages. Uses the composite resource name {campaign_id}~{shared_set_id} directly so no lookup is needed.

Both follow the existing draft → confirm_and_apply pattern and are exposed via the FastMCP server.

Why

AdLoop already covers reads (get_negative_keyword_lists, get_negative_keyword_list_campaigns) and shared-criterion writes (add_to_negative_keyword_list, propose_negative_keyword_list), but there's no way to attach or detach a shared set from a campaign through the MCP layer.

The practical impact: when you build a new campaign and want it to inherit the existing shared negative list, the only options today are (a) bounce out to the Google Ads UI, or (b) drop into google-ads-python directly. Hit this myself recently — newly-built campaigns launched without inheriting 1800+ keywords of historical waste-blockers because the shared list wasn't attached. Hand-rolled the attach via raw SDK; this PR brings that into AdLoop.

Design

Validation lives in a small helper _normalize_shared_set_attachment_args shared by both attach and detach drafts:

• shared_set_id required, must be numeric.
• campaign_ids non-empty.
• Each campaign_id must be numeric (otherwise rejected with the offending value in the error).
• Input list deduplicated; whitespace trimmed.

Attach apply path uses CampaignSharedSetService.mutate_campaign_shared_sets with one create op per campaign, building paths via the existing service helpers.

Detach apply path constructs the composite resource name directly:

op.remove = f"customers/{cid}/campaignSharedSets/{campaign_id}~{shared_set_id}"

No GAQL lookup needed — the {campaign_id}~{shared_set_id} format is stable.

Tests

10 new unit tests in tests/test_ads_write.py:

• Validation (8 tests): missing/non-numeric shared_set_id, empty/non-numeric campaign_ids, dedup with whitespace, preview shape includes plan_id + correct entity_type/entity_id/changes.
• Apply (2 tests): _apply_attach_shared_set_to_campaigns produces one op per campaign with correct create.campaign and create.shared_set paths; _apply_detach_shared_set_from_campaigns uses the composite {campaign_id}~{shared_set_id} remove path.

uv run pytest — all 152 tests pass (142 baseline + 10 new).

Test plan

• uv run pytest — 152 pass
• Validation rejects non-numeric IDs
• Apply paths produce correct mutate operations against fakes
• Live-API smoke test attaching a real shared set to a paused test campaign (recommended before merge if a maintainer-side test account is available)

Related

Filed alongside #21 (RSA pinning). Both grew out of the same migration pain point of needing to hand-roll work that should fit cleanly inside AdLoop's existing safety-rail pattern.

[kLOsk]

Thanks — this fills a real gap between "create a shared list" and "actually have it apply to a campaign." The composite {campaign_id}~{shared_set_id} resource-name trick for detach is the right call and avoids a GAQL lookup. Validation helper sharing between attach/detach is clean, and the test coverage is solid.

One blocking change:

The PR description says detaching a non-attached set "will fail for that specific operation but does not affect the others; surfaced in the apply response." That's only true with partial_failure=True on the mutate request — and looking at both _apply_attach_shared_set_to_campaigns and _apply_detach_shared_set_from_campaigns, neither passes it. As written, the entire batch fails on the first non-existent linkage (or first duplicate attach attempt), which is the opposite of what the docstring promises.

Two acceptable fixes:

1. Set partial_failure=True on both mutate_campaign_shared_sets calls and surface per-op errors in the response (preferred — matches the docstring, more recoverable for the LLM, and consistent with how Google Ads partial-failure responses work).
2. Update the docstrings to say "all-or-nothing — call get_negative_keyword_list_campaigns first to verify attachments exist" if you'd rather keep the simple semantics.

Either is fine but the current state is a real bug.

Non-blocking follow-ups (we can handle):

• .cursor/rules/adloop.mdc — the "When user asks to add negative keywords" section currently only covers content (creating lists, adding keywords). It should also cover the attachment lifecycle: when to attach vs create new, when to detach. Without rules updates the LLM won't know these tools exist.
• Idempotency note: calling attach_shared_set_to_campaigns twice for the same campaign fails on the second call. Worth surfacing as a known limitation in the docstring.
• Detach safety: detaching a list with many keywords from a high-spend campaign is operationally significant. Consider showing the linkage's keyword count in the preview (one extra read), or at least a generic warning. Lower priority.

Once partial_failure=True is in (or the docstring is updated to match), this is good to merge.

[SullyGitHub]

Addressed the blocking change (option 1) — partial_failure=True now passes on both mutate_campaign_shared_sets calls, with per-op error mapping back to the originating campaign_id.

What changed:

• _apply_attach_shared_set_to_campaigns and _apply_detach_shared_set_from_campaigns now pass partial_failure=True.
• New helper _parse_partial_failure_per_op(client, partial_failure_error) deserializes partial_failure_error.details into {operation_index: error_message} using the standard client.get_type("GoogleAdsFailure") + FromString pattern. Returns empty dict on missing/unparseable input — callers still detect failures via empty result.resource_name and can fall back on partial_failure_error.message.
• Response shape — successful ops surface in resource_names (attach) / removed_resource_names (detach); failed ops surface under a new failed_campaigns key, each entry carrying campaign_id, operation_index, and error. partial_failure: True and partial_failure_message (the top-level Status message) only appear when at least one op failed, so the success-path response is unchanged from the original PR.
• Two new tests (test_apply_attach_shared_set_to_campaigns_surfaces_partial_failure, test_apply_detach_shared_set_from_campaigns_surfaces_partial_failure) cover the partial-failure path; existing tests updated to assert partial_failure=True is captured by the mutate mock and that the success path doesn't include the failure keys. Full suite: 196/196 passing locally.
• Docstrings updated to describe the new shape and call out the most common partial-failure scenarios (already-attached on attach, non-existent linkage on detach) — that picks up the "idempotency note" non-blocking follow-up while we're here.

Branch was rebased onto current main (clean rebase past PR #21 + PR #26) before the amend, so it's a single clean commit ready for re-review.

Happy to take the rules-doc update (.cursor/rules/adloop.mdc attachment lifecycle) as a separate PR if you'd prefer.