fix(platform): disable redirect following on meta and x/twitter clients

docs/knowledge/code/internal-platform-meta.md

@@ -89,5 +89,16 @@ past start date is refused, with a same-day ad-set `start_time` nudged to
 now+buffer. `doRequest` retries HTTP 429 and Graph rate-limit envelope codes
 (4/17/32/341/613/80004) with bounded backoff, draining the body before close, and a
 truncated response body is surfaced rather than reported as a false success.
+Redirect following is force-disabled (a shared `noFollow` `CheckRedirect` policy).
+For a `WithHTTPClient`-supplied client, `NewClient` builds a FRESH `*http.Client`
+carrying the caller's reusable exported fields (`Transport`, `Jar`, `Timeout`) with
+`CheckRedirect: noFollow`, rather than value-copying the caller's client (an
+`http.Client` must not be copied after first use — a copy duplicates its internal
+mutex while sharing the request-cancellation map). So a 3xx on a mutating POST is
+surfaced rather than followed to a different target; `createOutcomeAmbiguous`
+classifies a mutating 3xx (gated on the method) and any 5xx/transport failure as
+UNCONFIRMED, so a create that may have committed is not blind-retried into a
+duplicate. The status is preserved even when the response body is unreadable or
+oversized, so an ambiguous outcome is never downgraded to a definite failure.
 
 See [internal/platform/meta](../../../internal/platform/meta).

docs/knowledge/code/internal-platform-twitter.md

@@ -47,6 +47,12 @@ toward the 1-req/sec limit; it does NOT enforce the account-wide write limit
 across concurrent dispatches or replicas (that needs cross-replica coordination,
 tracked in LFXV2-2665), so operators must not rely on this stateless client for
 cross-dispatch rate limiting. When the account limit is hit anyway, 429s are
-retried with backoff bounded by `Retry-After` / `X-Rate-Limit-Reset`.
+retried with backoff bounded by `Retry-After` / `X-Rate-Limit-Reset`. Redirect
+following is force-disabled (a shared `noFollow` `CheckRedirect` policy on the
+default and any `WithHTTPClient`-supplied client, via a shallow copy) so a 3xx is
+surfaced rather than followed — important with OAuth 1.0a, where a followed
+redirect would resend a request signed for the original URL to a different one.
+(Typing the non-2xx error and classifying a mutating 3xx as UNCONFIRMED, as the
+meta/reddit clients do, is tracked as a follow-up — LFXV2-2642.)
 
 See [internal/platform/twitter](../../../internal/platform/twitter).

docs/knowledge/log.md

@@ -1,5 +1,58 @@
 # Log
 
+## 2026-07-20
+
+**Update** — Extended the Meta ad-set ambiguity to the 2xx-no-id case (LFXV2-2641,
+PR #30 review by Copilot). The ad-set create's error path already routed through
+`createOutcomeAmbiguous`, but a 2xx response with an empty `id` fell through to a
+definite "returned no ad set ID" — the same duplicate-create risk as the campaign
+and twitter no-id paths. Now surfaces UNCONFIRMED (verify before retrying). Test
+added. Also fixed a CI `check-fmt` failure (gofmt comment alignment in the meta
+test).
+
+## 2026-07-19
+
+**Update** — Fixed an http.Client copy-after-use in the Meta client's no-follow
+enforcement (LFXV2-2641, PR #30 review by Copilot). `NewClient` value-copied a
+`WithHTTPClient`-supplied client (`hc := *c.httpClient`) to override CheckRedirect
+— but an `http.Client` must not be copied after first use (the copy duplicates its
+internal mutex while sharing the request-cancellation map, so concurrent use of
+the caller's client and the copy can race). Now builds a FRESH `*http.Client`
+carrying only the exported reusable fields (Transport, Jar, Timeout) with
+`CheckRedirect: noFollow`. The no-follow test asserts Transport/Timeout are
+preserved and the fresh client is a distinct pointer. Also made the campaign
+UNCONFIRMED step reason-neutral ("ambiguous response — timeout, server error, or
+an unfollowed redirect") since a 3xx now routes there too. NOTE: the reddit client
+(merged) has the same value-copy pattern — follow-up tracked to apply the same
+fresh-client fix there. The twitter client gets the same fix on PR #31.
+
+**Update** — Closed two more Meta ambiguity gaps (LFXV2-2641, PR #30 review by
+Copilot). (1) `doRequest` returned a plain error when a NON-2xx response body
+failed to read, stripping the HTTP status — so a mutating 3xx/5xx with an
+unreadable body (the create may have committed) was mis-seen as a definite failure
+by `createOutcomeAmbiguous` (which keys on the `*APIError` status). It now returns
+an `*APIError` preserving the status on a non-2xx read failure (2xx read failures
+stay `transportError`). (2) The ad-set create returned its error directly without
+the ambiguity check the campaign and ad/creative creates use, so a surfaced 3xx/5xx
+read as a definite "ad set creation failed" — risking a duplicate ad set on retry.
+It now routes through `createOutcomeAmbiguous`: ambiguous → UNCONFIRMED (verify
+before retrying), definite 4xx → "failed". Tests added for both. (3) The same
+status-stripping existed in the OVERSIZED-body branch (>1 MiB), which returned a
+plain error before recording the status — a mutating 3xx/5xx over the cap was still
+mis-classified as a definite failure. Now the oversized-body branch preserves the
+status the same way (2xx → transportError, non-2xx → *APIError), with a regression
+test. Updated the meta concept doc to describe the fresh-client + status-preservation.
+
+**Update** — Gated the Meta client's 3xx create-outcome ambiguity on a mutating
+method (LFXV2-2641, PR #30 review by Cursor Bugbot). `createOutcomeAmbiguous`
+treated EVERY 3xx as UNCONFIRMED without checking the method, diverging from the
+reddit client (which gates 3xx on `isMutatingMethod`) despite claiming to mirror
+it. All call sites pass POST today so behavior was unchanged, but the helper's
+contract was wrong for any future GET caller — a GET redirect is not a create.
+Added `isMutatingMethod` to the meta client and gated the 3xx branch (5xx and
+transport errors stay ambiguous regardless of method); extended the ambiguity test
+with GET/POST/DELETE method cases. Now genuinely identical to reddit.
+
 ## 2026-07-18
 
 **Creation** — Added the `internal/platform/googleads` Go package (GA-1 scaffold,
@@ -13,6 +66,23 @@ campaign.start_date_time/end_date_time. Concept doc + code index updated. Campai
 creation (:mutate), metrics/keywords/audience, and keyword actions follow in
 GA-2..GA-5.
 
+**Update** — Also strengthened the no-follow regression tests (meta + twitter):
+they injected a nil-`CheckRedirect` client, which couldn't prove the override is
+UNCONDITIONAL (a "fill only nil callbacks" impl would pass). Now they inject a
+caller client carrying a SENTINEL `CheckRedirect` and assert the client the code
+uses returns `http.ErrUseLastResponse` despite it, while the caller's original
+still returns the sentinel (shallow copy, not mutation). (PR #30 review by Copilot.)
+
+**Update** — Disabled HTTP redirect following on the Meta and X/Twitter Ads
+clients (LFXV2-2641), closing a duplicate-create gap: both built their
+`*http.Client` (and accepted `WithHTTPClient` clients) with no `CheckRedirect`, so
+the stdlib could follow a 3xx on a mutating POST after the create was committed and
+muddy outcome classification (for X, a followed redirect also resends an OAuth-1.0a
+request signed for the original URL). Added a shared `noFollow`
+(`http.ErrUseLastResponse`) policy set on the default client and enforced
+unconditionally after options via a shallow copy (so a caller's client isn't
+mutated) — matching the reddit/linkedin/googleads clients. Regression tests added.
+
 ## 2026-07-15
 
 **Update** — Hardened the Reddit Ads client's ambiguous-outcome classification

internal/platform/meta/client.go

@@ -298,7 +298,21 @@ type Client struct {
 // Option customizes a Client.
 type Option func(*Client)
 
-// WithHTTPClient overrides the HTTP client (useful for tests / timeouts).
+// noFollow is the CheckRedirect policy for every client this package uses: it
+// returns http.ErrUseLastResponse so the client does NOT follow redirects and
+// hands the 3xx response back to the request layer, where a non-2xx status is
+// surfaced as an error. The Graph API returns JSON directly and never legitimately
+// 3xx-redirects these calls; not following keeps outcome classification sound — a
+// redirect can't carry an already-sent mutating POST to a different target and be
+// misclassified. It is shared by the built-in client and the caller-supplied-
+// client enforcement in NewClient. Mirrors the reddit/linkedin/googleads clients.
+func noFollow(_ *http.Request, _ []*http.Request) error {
+	return http.ErrUseLastResponse
+}
+
+// WithHTTPClient overrides the HTTP client (useful for tests / timeouts). Redirect
+// following is force-disabled on whatever client ends up in use (see NewClient),
+// so an injected client cannot reintroduce redirect following.
 func WithHTTPClient(h *http.Client) Option {
 	return func(c *Client) {
 		// Ignore a nil client so the safe default installed by NewClient isn't
@@ -355,7 +369,7 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client
 	c := &Client{
 		creds:          creds,
 		account:        account,
-		httpClient:     &http.Client{Timeout: DefaultRequestTimeout},
+		httpClient:     &http.Client{Timeout: DefaultRequestTimeout, CheckRedirect: noFollow},
 		baseURL:        DefaultBaseURL,
 		adsManagerURL:  DefaultAdsManagerURL,
 		timeNow:        time.Now,
@@ -364,6 +378,27 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client
 	for _, o := range opts {
 		o(c)
 	}
+	// Enforce the no-follow redirect policy UNCONDITIONALLY on whatever client ended
+	// up on c.httpClient — INCLUDING one supplied via WithHTTPClient, which replaces
+	// the default above. Following a redirect would carry an already-sent mutating
+	// POST to a different target and muddy outcome classification, so no-follow is a
+	// correctness requirement, not a default.
+	//
+	// Build a FRESH *http.Client rather than value-copying the caller's: an
+	// http.Client must not be copied after first use (a value copy duplicates its
+	// internal mutex while sharing the request-cancellation map, so concurrent use
+	// of the caller's client and our copy can race). We carry over only the exported,
+	// reusable fields (Transport, Jar, Timeout) — the shareable connection pool /
+	// cookie jar / deadline — and set our own CheckRedirect. The caller's client is
+	// never mutated and is safe to keep using elsewhere.
+	if c.httpClient != nil {
+		c.httpClient = &http.Client{
+			Transport:     c.httpClient.Transport,
+			CheckRedirect: noFollow,
+			Jar:           c.httpClient.Jar,
+			Timeout:       c.httpClient.Timeout,
+		}
+	}
 	return c
 }
 
@@ -638,6 +673,10 @@ func isPreSendDialError(err error) bool {
 //     reaches here), so the request may have been received;
 //   - *APIError with a 5xx status: Meta received it and may have committed the
 //     mutation before erroring.
+//   - *APIError with a 3xx status: redirect following is force-disabled (see
+//     noFollow), so a 3xx is surfaced here rather than followed. A 3xx on a
+//     mutating request is NOT a definite rejection — Meta may have committed the
+//     create and then returned a redirect — so it is ambiguous like a 5xx.
 //
 // A definite 4xx (Meta rejected it), or any pre-send failure (token missing,
 // body encode/build, a pre-connect dial error), means NOT applied → returns
@@ -649,7 +688,30 @@ func createOutcomeAmbiguous(err error) bool {
 		return true
 	}
 	var ae *APIError
-	return errors.As(err, &ae) && ae.StatusCode >= 500
+	if !errors.As(err, &ae) {
+		return false
+	}
+	// A 5xx may follow a committed create.
+	if ae.StatusCode >= 500 {
+		return true
+	}
+	// A 3xx on a MUTATING request reached a responder and may have committed a
+	// resource before redirecting — UNCONFIRMED. A 3xx on a GET is not a create, so
+	// it stays non-ambiguous. Gating on the method (rather than treating every 3xx
+	// as ambiguous) keeps this helper's contract correct for any caller, not just
+	// the create path — and makes it genuinely identical to the reddit client.
+	return ae.StatusCode >= 300 && ae.StatusCode < 400 && isMutatingMethod(ae.Method)
+}
+
+// isMutatingMethod reports whether an HTTP method can create/modify server state,
+// so a 3xx on it may hide a committed mutation. Mirrors the reddit client.
+func isMutatingMethod(method string) bool {
+	switch method {
+	case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete:
+		return true
+	default:
+		return false
+	}
 }
 
 // doRequest performs a Graph API call and decodes the JSON body into out.
@@ -702,14 +764,27 @@ func (c *Client) doRequest(ctx context.Context, method, path string, body map[st
 		// returns EOF (not an error) at the limit, so an oversized body would
 		// otherwise be silently truncated and mis-parsed as a valid short response.
 		raw, readErr := io.ReadAll(io.LimitReader(resp.Body, maxResponseBody+1))
-		if readErr == nil && int64(len(raw)) > maxResponseBody {
-			_ = resp.Body.Close()
-			return fmt.Errorf("meta API %s %s: response exceeds %d bytes", method, path, maxResponseBody)
-		}
 		retryAfter := c.parseRetryAfter(resp)
 		status := resp.StatusCode
 		_ = resp.Body.Close()
 
+		if readErr == nil && int64(len(raw)) > maxResponseBody {
+			// Oversized body: we can't trust the payload, but the STATUS must still be
+			// preserved for the same reason as a read failure below — a mutating 3xx/5xx
+			// (or a 2xx) may reflect a committed create, and stripping the status would
+			// mis-classify it as a definite failure and invite a duplicate on retry. A
+			// 2xx is ambiguous (transportError); a non-2xx carries its status via
+			// *APIError. (An oversized error/redirect body is anomalous, but we classify
+			// on status, not payload.)
+			if status >= 200 && status < 300 {
+				return &transportError{Method: method, Path: path, Err: fmt.Errorf("response exceeds %d bytes", maxResponseBody)}
+			}
+			return &APIError{
+				StatusCode: status, Method: method, Path: path,
+				Message: fmt.Sprintf("response exceeds %d bytes", maxResponseBody),
+			}
+		}
+
 		// Meta reports throttling either as HTTP 429 or, commonly, as HTTP 400 with
 		// a Graph error envelope whose code is a known rate-limit code. Treat both
 		// as retryable with the same bounded backoff. The envelope is only consumed
@@ -731,13 +806,23 @@ func (c *Client) doRequest(ctx context.Context, method, path string, body map[st
 		if readErr != nil && (!throttled || attempt >= retryMax) {
 			// A read failure on a 2xx is AMBIGUOUS: Meta committed the mutation but we
 			// couldn't read the result — wrap it as transportError so a create is
-			// treated as "may exist". A read failure on a non-2xx isn't a committed
-			// mutation, so return it plain. Mirrors the reddit client wrapping 2xx
-			// read/decode failures as transportError.
+			// treated as "may exist". Mirrors the reddit client wrapping 2xx read/decode
+			// failures as transportError.
 			if status >= 200 && status < 300 {
 				return &transportError{Method: method, Path: path, Err: fmt.Errorf("read response body: %w", readErr)}
 			}
-			return fmt.Errorf("meta API %s %s: read response body: %w", method, path, readErr)
+			// A read failure on a NON-2xx still must preserve the HTTP status: a
+			// mutating 3xx (redirect, not followed) or 5xx may have committed the create
+			// before the unreadable body, and createOutcomeAmbiguous classifies on the
+			// *APIError status. Returning a plain error here would strip the status and
+			// silently turn an ambiguous create into a definite "failed" — the exact
+			// duplicate-on-retry risk the no-follow + ambiguity handling exists to close.
+			// The body couldn't be read, so no Graph envelope diagnostics are available;
+			// carry the status/method/path and note the read failure in the message.
+			return &APIError{
+				StatusCode: status, Method: method, Path: path,
+				Message: fmt.Sprintf("read response body: %v", readErr),
+			}
 		}
 
 		if throttled && attempt < retryMax {
@@ -1773,7 +1858,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig
 		// (retry-safe idempotency is tracked in LFXV2-2665). Mirrors the reddit
 		// client's createOutcomeAmbiguous handling.
 		if createOutcomeAmbiguous(err) {
-			steps = append(steps, "Campaign creation outcome is UNCONFIRMED (timeout or server error); a PAUSED campaign may exist — verify by name in Meta Ads Manager")
+			steps = append(steps, "Campaign creation outcome is UNCONFIRMED (ambiguous response — timeout, server error, or an unfollowed redirect); a PAUSED campaign may exist — verify by name in Meta Ads Manager")
 			return &CampaignResult{
 				Platform:     "meta-ads",
 				CampaignName: campaignName,
@@ -1861,11 +1946,25 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig
 		// The campaign was already created (PAUSED). Return a partial result carrying
 		// its id so the caller can identify/clean up the orphan without parsing the
 		// error string; auto-deleting here would race a retry that reuses it.
+		//
+		// An AMBIGUOUS ad-set failure (transport/timeout, a mutating 3xx now surfaced
+		// because redirects aren't followed, or a 5xx) can occur AFTER Meta committed
+		// the ad set — a definite "failed" instruction would let a retry create a
+		// DUPLICATE ad set. Word it UNCONFIRMED (verify before retrying) in that case;
+		// a clear 4xx rejection means nothing was created, so keep the plain "failed"
+		// wording. Mirrors the campaign and ad/creative create paths.
+		if createOutcomeAmbiguous(err) {
+			return partialResult(), fmt.Errorf("meta ad set creation UNCONFIRMED (campaign %s created, PAUSED; an ad set may exist — verify in Meta Ads Manager before retrying): %w", campaignID, err)
+		}
 		return partialResult(), fmt.Errorf("meta ad set creation failed (campaign %s created, PAUSED): %w", campaignID, err)
 	}
 	adSetID = adSetResp.ID
 	if adSetID == "" {
-		return partialResult(), fmt.Errorf("meta ad set creation succeeded but returned no ad set ID (campaign %s created, PAUSED)", campaignID)
+		// A 2xx with no id is a malformed SUCCESS: Meta may have created the ad set
+		// but didn't return a usable id. UNCONFIRMED (verify before retrying), NOT a
+		// clean failure — a blind retry could duplicate an ad set Meta already made.
+		// Mirrors the campaign/ad no-id and the ad-set error-path handling.
+		return partialResult(), fmt.Errorf("meta ad set creation UNCONFIRMED (campaign %s created, PAUSED; Meta returned a 2xx with no ad set ID — an ad set may exist; verify in Meta Ads Manager before retrying)", campaignID)
 	}
 	budgetLabel := "daily"
 	if in.LifetimeBudget {