fix(meta): close review findings on write client and budget validation

Task 4 review: Approved with no Critical findings, but three Important
and two hardening items. All five addressed:

- objects.build_campaign_payload: normalise a bare string
  special_ad_categories value to a single-element list instead of
  exploding it into characters via list() - the field controls
  regulated-advertising compliance and advanced is unvalidated input.
- objects.build_ad_set_payload: validate optimization_goal and
  billing_event against new allowlists before forwarding them, since
  billing_event determines how the ad account is charged and both come
  from client-controlled advanced.
- validation.validate_budget: reject a lifetime-only budget (Meta's
  create_ad_set only accepts daily_budget) so it fails at validation
  time instead of passing approval and only failing at sync.
- client.py: create_campaign/create_ad_set/create_ad now send the
  CREATE_STATUS constant rather than the caller's status object, so a
  str subclass with a lying __ne__ can no longer slip "ACTIVE" past the
  guard.
- client._post: refuse any POST to a campaign/adset/ad creation path
  with a non-PAUSED status before touching the transport, as defence in
  depth if a future caller bypasses the typed create_* guards.

10 new tests (8 in test_meta_writes.py, 2 in test_campaign_validation.py).

src/adclaw/campaigns/validation.py

@@ -78,11 +78,26 @@ def validate_budget(
                 ),
             )
 
-    if lifetime is not None and lifetime <= 0:
+    if lifetime is not None:
+        if lifetime <= 0:
+            issues.append(
+                ValidationIssue(
+                    field="budget.lifetime_budget",
+                    message="Lifetime budget must be greater than zero.",
+                ),
+            )
+        # Reached only when daily is None (the daily-and-lifetime-together
+        # case already returned above), i.e. this is a lifetime-only
+        # budget. create_ad_set only accepts daily_budget, so without this
+        # a lifetime-only campaign would pass validation and approval and
+        # only fail once Task 5's sync tries to create the Meta ad set.
         issues.append(
             ValidationIssue(
                 field="budget.lifetime_budget",
-                message="Lifetime budget must be greater than zero.",
+                message=(
+                    "Lifetime budgets are not supported yet. Set a daily "
+                    "budget instead."
+                ),
             ),
         )
 

src/adclaw/meta/client.py

@@ -52,6 +52,11 @@ CREATE_STATUS = "PAUSED"
 
 LIST_FIELDS = "id,name,status,effective_status,created_time"
 
+# Path suffixes that create a campaign/ad set/ad. _post refuses any of these
+# with a non-PAUSED status as defence in depth, in case a future caller
+# bypasses the typed create_* guards and calls _post directly.
+_PAUSED_ONLY_SUFFIXES = ("/campaigns", "/adsets", "/ads")
+
 Transport = Callable[[str, str, dict[str, Any]], Awaitable[dict[str, Any]]]
 
 
@@ -132,7 +137,22 @@ class MetaClient:
         Graph returns errors with HTTP 200, so both `_get` and `_post` must
         inspect the response body for an `"error"` key rather than trust the
         HTTP status code.
+
+        Defence in depth: a POST to a campaign/ad set/ad creation path with
+        a status other than PAUSED is refused here too, before any
+        transport call, even if a caller bypasses the typed create_*
+        guards. `update_object_status` posts to `/{object_id}`, which never
+        matches these suffixes, so Launch (the only path allowed to send
+        ACTIVE) is unaffected.
         """
+        if (
+            path.endswith(_PAUSED_ONLY_SUFFIXES)
+            and data.get("status") != CREATE_STATUS
+        ):
+            raise ValueError(
+                "Refusing to create a campaign/ad set/ad with a non-PAUSED "
+                "status. Use update_object_status to launch.",
+            )
         payload = dict(data)
         payload["access_token"] = self._token
         result = await self._transport("POST", f"{GRAPH_BASE_URL}{path}", payload)
@@ -201,7 +221,7 @@ class MetaClient:
             {
                 "name": name,
                 "objective": objective,
-                "status": status,
+                "status": CREATE_STATUS,
                 "special_ad_categories": json.dumps(special_ad_categories or []),
             },
         )
@@ -239,7 +259,7 @@ class MetaClient:
                 "targeting": json.dumps(targeting),
                 "optimization_goal": optimization_goal,
                 "billing_event": billing_event,
-                "status": status,
+                "status": CREATE_STATUS,
             },
         )
         return result["id"]
@@ -325,7 +345,7 @@ class MetaClient:
                 "name": name,
                 "adset_id": adset_id,
                 "creative": json.dumps({"creative_id": creative_id}),
-                "status": status,
+                "status": CREATE_STATUS,
             },
         )
         return result["id"]

src/adclaw/meta/objects.py

@@ -26,10 +26,19 @@ This is a deliberate, documented choice rather than an oversight:
   `targeting.geo_locations.countries`.
 - `spec.targeting["genders"]` -> Graph `targeting.genders`, if present.
 - `spec.advanced["special_ad_categories"]` -> campaign
-  `special_ad_categories` (defaults to an empty list).
+  `special_ad_categories` (defaults to an empty list). A bare string is
+  normalised to a single-element list rather than exploded into
+  characters, since `advanced` is unvalidated free-form JSON and this is
+  the compliance field for regulated advertising (housing, credit,
+  employment, ...).
 - `spec.advanced["optimization_goal"]` / `["billing_event"]` -> ad set
   fields of the same name, defaulting to `LEAD_GENERATION` /
-  `IMPRESSIONS` (MaskanX's default lead-gen objective).
+  `IMPRESSIONS` (MaskanX's default lead-gen objective). Both are checked
+  against an allowlist (`ALLOWED_OPTIMIZATION_GOALS` /
+  `ALLOWED_BILLING_EVENTS`) before being forwarded, since `advanced` is
+  client-controlled and `billing_event` in particular determines how the
+  ad account is charged; an unrecognised value raises `ValueError` naming
+  the offending value and the allowed set rather than reaching Meta.
 - `spec.advanced["message"]` / `["headline"]` / `["description"]` /
   `["link"]` -> ad creative `object_story_spec.link_data` fields of the
   same purpose (headline defaults to the campaign name if unset).
@@ -43,6 +52,18 @@ from ..campaigns.models import CampaignSpec
 DEFAULT_OPTIMIZATION_GOAL = "LEAD_GENERATION"
 DEFAULT_BILLING_EVENT = "IMPRESSIONS"
 
+# advanced.optimization_goal / advanced.billing_event are client-controlled
+# and billing_event determines how the ad account is charged, so both are
+# checked against an allowlist before being forwarded to a spending API.
+ALLOWED_OPTIMIZATION_GOALS = frozenset({
+    "LEAD_GENERATION", "LINK_CLICKS", "IMPRESSIONS", "REACH",
+    "OFFSITE_CONVERSIONS", "LANDING_PAGE_VIEWS", "THRUPLAY",
+    "POST_ENGAGEMENT", "QUALITY_CALL",
+})
+ALLOWED_BILLING_EVENTS = frozenset({
+    "IMPRESSIONS", "LINK_CLICKS", "THRUPLAY", "POST_ENGAGEMENT",
+})
+
 
 def build_campaign_payload(spec: CampaignSpec) -> dict[str, Any]:
     """Map a CampaignSpec onto `MetaClient.create_campaign` kwargs.
@@ -54,12 +75,18 @@ def build_campaign_payload(spec: CampaignSpec) -> dict[str, Any]:
         raise ValueError(
             "CampaignSpec.objective is required to create a Meta campaign.",
         )
+    # A bare string ("HOUSING") must become a single-element list, not be
+    # exploded into characters by list(); advanced is unvalidated JSON from
+    # the API and special_ad_categories is the compliance field for
+    # regulated advertising, so this cannot be left to Meta's opaque error.
+    raw_categories = spec.advanced.get("special_ad_categories") or []
+    categories = (
+        [raw_categories] if isinstance(raw_categories, str) else list(raw_categories)
+    )
     return {
         "name": spec.name,
         "objective": spec.objective,
-        "special_ad_categories": list(
-            spec.advanced.get("special_ad_categories") or [],
-        ),
+        "special_ad_categories": categories,
     }
 
 
@@ -90,15 +117,28 @@ def build_ad_set_payload(spec: CampaignSpec, campaign_id: str) -> dict[str, Any]
     if genders:
         targeting["genders"] = list(genders)
 
+    optimization_goal = spec.advanced.get(
+        "optimization_goal", DEFAULT_OPTIMIZATION_GOAL,
+    )
+    if optimization_goal not in ALLOWED_OPTIMIZATION_GOALS:
+        raise ValueError(
+            f"Unsupported optimization_goal {optimization_goal!r}. Allowed: "
+            f"{sorted(ALLOWED_OPTIMIZATION_GOALS)}",
+        )
+    billing_event = spec.advanced.get("billing_event", DEFAULT_BILLING_EVENT)
+    if billing_event not in ALLOWED_BILLING_EVENTS:
+        raise ValueError(
+            f"Unsupported billing_event {billing_event!r}. Allowed: "
+            f"{sorted(ALLOWED_BILLING_EVENTS)}",
+        )
+
     return {
         "campaign_id": campaign_id,
         "name": f"{spec.name} - Ad Set",
         "daily_budget": daily_budget,
         "targeting": targeting,
-        "optimization_goal": spec.advanced.get(
-            "optimization_goal", DEFAULT_OPTIMIZATION_GOAL,
-        ),
-        "billing_event": spec.advanced.get("billing_event", DEFAULT_BILLING_EVENT),
+        "optimization_goal": optimization_goal,
+        "billing_event": billing_event,
     }
 
 

tests/test_campaign_validation.py

@@ -30,6 +30,25 @@ def test_daily_and_lifetime_budget_together_is_rejected():
     assert [i.field for i in issues] == ["budget"]
 
 
+def test_lifetime_only_budget_is_rejected_as_unsupported():
+    # create_ad_set only accepts daily_budget (objects.build_ad_set_payload
+    # raises ValueError for a lifetime-only spec), so a lifetime-only
+    # budget must be caught here rather than passing validation/approval
+    # and only failing once sync tries to create the Meta ad set.
+    issues = validate_budget({"lifetime_budget": 50000}, min_daily_budget=9709)
+    assert [i.field for i in issues] == ["budget.lifetime_budget"]
+    assert "not supported yet" in issues[0].message
+
+
+def test_lifetime_only_negative_budget_reports_both_issues():
+    issues = validate_budget({"lifetime_budget": -5}, min_daily_budget=9709)
+    fields = [i.field for i in issues]
+    assert fields == ["budget.lifetime_budget", "budget.lifetime_budget"]
+    messages = " ".join(i.message for i in issues)
+    assert "greater than zero" in messages
+    assert "not supported yet" in messages
+
+
 def test_auto_pause_above_daily_budget_is_rejected():
     issues = validate_guardrails(
         {"daily_budget": 10000},

tests/test_meta_writes.py

@@ -289,6 +289,38 @@ async def test_update_object_status_returns_none():
     assert result is None
 
 
+# ---------------------------------------------------------------------------
+# _post defence in depth - reachable directly, still refuses non-PAUSED
+# creates on campaign/adset/ad paths, before any transport call
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_post_refuses_non_paused_status_on_campaign_create_path():
+    transport = FakeTransport([{"id": "c1"}])
+    client = MetaClient(access_token="tok", transport=transport)
+
+    with pytest.raises(ValueError):
+        await client._post(
+            "/act_1/campaigns",
+            {"name": "Q3", "objective": "OUTCOME_LEADS", "status": "ACTIVE"},
+        )
+    assert transport.calls == []
+
+
+@pytest.mark.asyncio
+async def test_post_still_allows_update_object_status_to_send_active():
+    """update_object_status posts to /{object_id}, which never matches the
+    campaign/adset/ad creation suffixes, so the _post guard above must not
+    block Launch."""
+    transport = FakeTransport([{"success": True}])
+    client = MetaClient(access_token="tok", transport=transport)
+
+    await client.update_object_status("c1", "ACTIVE")
+
+    assert transport.calls[0]["params"]["status"] == "ACTIVE"
+
+
 # ---------------------------------------------------------------------------
 # list_campaigns / list_ad_sets / list_ads
 # ---------------------------------------------------------------------------
@@ -383,6 +415,25 @@ def test_build_campaign_payload_requires_objective():
         build_campaign_payload(spec)
 
 
+def test_build_campaign_payload_normalises_bare_string_special_ad_category():
+    # advanced is unvalidated free-form JSON, so a bare string is a
+    # plausible client input for this compliance field. It must become a
+    # single-element list, not be exploded into characters by list().
+    spec = _spec(advanced={"special_ad_categories": "HOUSING"})
+
+    payload = build_campaign_payload(spec)
+
+    assert payload["special_ad_categories"] == ["HOUSING"]
+
+
+def test_build_campaign_payload_passes_through_a_list_unchanged():
+    spec = _spec(advanced={"special_ad_categories": ["HOUSING", "EMPLOYMENT"]})
+
+    payload = build_campaign_payload(spec)
+
+    assert payload["special_ad_categories"] == ["HOUSING", "EMPLOYMENT"]
+
+
 def test_build_ad_set_payload_maps_targeting_and_budget():
     spec = _spec()
 
@@ -420,6 +471,40 @@ def test_build_ad_set_payload_requires_daily_budget():
         build_ad_set_payload(spec, campaign_id="c1")
 
 
+def test_build_ad_set_payload_accepts_allowlisted_optimization_goal():
+    spec = _spec(advanced={"optimization_goal": "LINK_CLICKS"})
+
+    payload = build_ad_set_payload(spec, campaign_id="c1")
+
+    assert payload["optimization_goal"] == "LINK_CLICKS"
+
+
+def test_build_ad_set_payload_rejects_unknown_optimization_goal():
+    spec = _spec(advanced={"optimization_goal": "SOMETHING_MADE_UP"})
+
+    with pytest.raises(ValueError):
+        build_ad_set_payload(spec, campaign_id="c1")
+
+
+def test_build_ad_set_payload_accepts_allowlisted_billing_event():
+    spec = _spec(advanced={"billing_event": "LINK_CLICKS"})
+
+    payload = build_ad_set_payload(spec, campaign_id="c1")
+
+    assert payload["billing_event"] == "LINK_CLICKS"
+
+
+def test_build_ad_set_payload_rejects_unknown_billing_event():
+    # billing_event determines how the ad account is charged, so an
+    # unrecognised value from client-controlled `advanced` must never reach
+    # Meta - a spending-relevant field with no allowlist is the finding
+    # this guards against.
+    spec = _spec(advanced={"billing_event": "SOMETHING_MADE_UP"})
+
+    with pytest.raises(ValueError):
+        build_ad_set_payload(spec, campaign_id="c1")
+
+
 def test_build_ad_set_payload_targeting_can_produce_json_dumpable_dict():
     # This is what create_ad_set will json.dumps() before sending - make
     # sure the mapping never emits anything that would break that step.