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

[mrautela365]

Problem

The Meta and X/Twitter Ads clients built their *http.Client (and accepted WithHTTPClient clients) with no CheckRedirect, so the Go stdlib follows redirects by default. On a mutating POST, the client could follow a 3xx after the create was already committed upstream and misclassify the outcome — the same duplicate-create risk already fixed on the reddit (#27), linkedin (#22), and google-ads (#29) clients. For X/Twitter it's worse: a followed redirect resends an OAuth 1.0a request signed for the original URL to a different target.

This closes the last two clients that lacked the guard.

Changes

• meta/client.go, twitter/client.go — add a shared noFollow policy (http.ErrUseLastResponse) on the default client, and enforce it unconditionally after options are applied (including on a WithHTTPClient-supplied client) via a shallow copy so the caller's client is not mutated. Mirrors the reddit client's NewClient enforcement.
• Regression tests for each: default client has no-follow, and an injected client's CheckRedirect is overridden without mutating the caller's client.

Verification

go build, go test -race, golangci-lint all clean; existing tests unaffected (none relied on redirect-following). No new dependencies.

LFXV2-2641

[cursor]

PR Summary

Medium Risk
Changes sit on paid campaign creation paths and error classification that gates blind retries; incorrect ambiguity handling could still duplicate resources, though the intent is to reduce that risk.

Overview
Meta and X/Twitter Ads clients now disable HTTP redirect following (noFollow / http.ErrUseLastResponse) on the default and WithHTTPClient clients, aligning with reddit/linkedin/googleads so mutating POSTs are not followed after a possible commit (OAuth 1.0a on X makes followed redirects especially unsafe).

On Meta, the change goes further than no-follow alone: NewClient builds a fresh *http.Client (Transport/Jar/Timeout only) instead of value-copying the caller’s client; createOutcomeAmbiguous treats mutating 3xx like 5xx/transport failures; doRequest keeps HTTP status on non-2xx oversized or unreadable bodies; ad-set create uses the same UNCONFIRMED path for ambiguous errors and for 2xx with no id; campaign UNCONFIRMED messaging mentions unfollowed redirects. Regression tests cover no-follow (including sentinel override), status preservation, and ambiguity cases.

Twitter gets no-follow plus a shallow-copy override and tests; typed 3xx UNCONFIRMED there remains a follow-up (LFXV2-2642). Concept docs and the knowledge log are updated.

^{Reviewed by Cursor Bugbot for commit e506639. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

Pull request overview

Disables automatic redirects in Meta and X/Twitter Ads clients to reduce duplicate-create risk.

Changes:

• Adds enforced no-follow redirect policies.
• Preserves caller-supplied HTTP clients via shallow copies.
• Adds policy tests and a knowledge-log entry.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/platform/meta/client.go	Enforces no-follow redirects.
internal/platform/meta/client_test.go	Tests Meta redirect configuration.
internal/platform/twitter/client.go	Enforces no-follow redirects.
internal/platform/twitter/client_test.go	Tests X redirect configuration.
docs/knowledge/log.md	Records the redirect hardening.

[mrautela365]

Review feedback addressed (round 2)

Commit: e8042d9

Changes made

• meta/client.go — classify a mutating 3xx as ambiguous in createOutcomeAmbiguous (a Meta create that committed before redirecting is now UNCONFIRMED, not a definite failure → no duplicate retry). Added a table-driven test. (per copilot)
• docs/knowledge — documented the no-follow policy (+ Meta's 3xx behavior) in the meta and twitter concept docs. (per copilot)

Deferred to follow-up

• twitter/client.go 3xx classification — X has no typed error / createOutcomeAmbiguous helper at all (bare fmt.Errorf for non-2xx), so typing the error + adding ambiguity classification is a separate design change tracked as LFXV2-2642. The no-follow guard here is still a strict improvement. Thread left open to track it.

Verification

build / go test -race / golangci-lint clean.

Threads resolved

2 of 3 (twitter 3xx-classification deferred to LFXV2-2642).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/platform/meta/client.go:682

• Not every surfaced 3xx reaches this classifier. doRequest returns a plain fmt.Errorf when a non-2xx body is truncated (client.go:761-770) or exceeds the size cap (client.go:735-738), discarding the known status. A mutating POST that committed and then returned such a 3xx therefore makes createOutcomeAmbiguous return false and can be blind-retried. Preserve the status/method in a typed error for these paths and cover the POST→truncated/oversized-3xx flow.

	var ae *APIError
	// A mutating 3xx (redirect, not followed) or 5xx may follow a committed create.
	return errors.As(err, &ae) && ((ae.StatusCode >= 300 && ae.StatusCode < 400) || ae.StatusCode >= 500)

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/platform/meta/client.go:682

• The new 3xx classification is bypassed when the redirect body is truncated or exceeds the response cap. doRequest returns a plain fmt.Errorf from those body-failure paths before constructing APIError, so this predicate returns false even though the mutating POST reached a responder and may have committed. Preserve the status/method for 3xx body failures (for example via APIError or an ambiguous wrapper), and cover a POST→3xx response with a body read failure.

	return errors.As(err, &ae) && ((ae.StatusCode >= 300 && ae.StatusCode < 400) || ae.StatusCode >= 500)

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 377c750. Configure here.}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[dealako]

[minor] Fresh-client rebuild is justified by a factually incorrect claim

Issue: The comment states an http.Client "must not be copied after first use (a value copy duplicates its internal mutex while sharing the request-cancellation map)". http.Client has no internal mutex and no request-cancellation map — its only fields are Transport, CheckRedirect, Jar, Timeout. The lock/connection state lives on http.Transport, which is shared by pointer in both the value-copy and fresh-client approaches.

Proof: go doc net/http.Client shows the four exported fields and nothing else; go vet ./internal/platform/twitter/ (which value-copies hc := *c.httpClient) passes clean — the copylocks analyzer would flag any struct embedding a sync.Locker.

Why it matters: The rebuild itself is harmless, but the rationale is the stated basis for diverging Meta from the reddit/twitter value-copy pattern this PR claims to "mirror," and the same incorrect claim is propagated into docs/knowledge/code/internal-platform-meta.md:96-97 and docs/knowledge/log.md:18-19. AGENTS.md asks the KB to stay accurate, so the myth shouldn't be enshrined there.

Fix: Either keep the fresh-client build but correct the reason (e.g. "build a fresh client so we set our own CheckRedirect without mutating the caller's client; carry over the reusable Transport/Jar/Timeout"), or revert to the value-copy that matches reddit/twitter. Update the two KB docs to match.

[dealako]

Review — @mrautela365

Hi @mrautela365 👋 — thorough, well-tested piece of hardening. This closes the last two clients (Meta + X/Twitter) that lacked a CheckRedirect guard, and the OAuth-1.0a angle for X is a genuinely good catch: with noFollow returning http.ErrUseLastResponse, Do returns the 3xx without issuing a second request, so a request signed for the original URL is never replayed against a redirect target. The Meta ambiguity work (method-gated 3xx, status preservation on unreadable/oversized bodies, ad-set 5xx + 2xx-no-id → UNCONFIRMED) is careful and each branch is backed by a real, reachable test. go build, go vet, and go test -race on both packages are clean, and the branch is mergeable.

I traced the correctness-critical paths independently and ran parallel security + code-quality passes. All prior Copilot/Cursor threads are addressed. The one substantive item is a documentation-accuracy problem, not a behavioral one.

Issue count

• 🔴 Blocking: 0
• 🟡 Minor: 1 — the fresh-client rebuild in meta/client.go is justified by a factually incorrect claim (http.Client "duplicates its internal mutex while sharing the request-cancellation map"). It has no mutex or cancellation map — those are on http.Transport, shared by pointer either way. The same wrong claim is now in the KB concept doc and log.md. go vet copylocks passes on the value-copy, confirming it's safe. (inline)
• ⚪ Nit: 1 — the fresh-client rebuild enumerates Transport/CheckRedirect/Jar/Timeout; a value-copy (reddit/twitter) would carry any future http.Client field automatically. Trivial robustness trade-off, take-it-or-leave-it.
• ❔ Question: 1 — Meta was changed to build a fresh client while X/Twitter and reddit stay on value-copy (X deferred to #31). Since the value-copy is provably safe, deferring is fine — but the split makes a safe pattern look like a bug fix. Worth reconciling on one enforcement pattern across the three clients (already tracked, so just confirming it's intentional).

Data privacy / security

No exposure introduced. Auth travels in headers, never URLs; error types surface method/path/status, not the signed request URL or tokens; body reads stay bounded by maxResponseBody; test fixtures are obviously synthetic (tok-abc, ck/cs/at/ats, events.example.org).

Decision

✅ Approved with minor comments — the one minor and the parity question are documentation/consistency, not correctness. Nothing blocks merge; addressing the inaccurate http.Client rationale (in code + the two KB docs) before or right after merge would keep the knowledge base trustworthy.