fix(reddit): narrow pre-send classification; clarify manual-ad destination

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

@@ -9,7 +9,7 @@ tags:
   - reddit-ads
   - oauth2
   - go-package
-timestamp: "2026-07-13T23:55:00Z"
+timestamp: "2026-07-15T00:00:00Z"
 ---
 
 # internal/platform/reddit
@@ -42,4 +42,26 @@ retries once WITHOUT communities (keyword/geo-only) and emits a
 communities-skipped warning step, so an invalid subreddit never orphans the
 PAUSED campaign.
 
+Because create calls are mutating and paid, a FAILED create is classified by
+whether the request may have reached Reddit. `isPreSendDialError` reports a Do
+error as pre-send (request definitely NOT sent → clean not-created failure) ONLY
+for proofs that no bytes left the client: DNS resolution failure, and
+connection-refused/no-route/network-unreachable dial failures. NO TLS error is
+treated as pre-send (matching the merged Meta client): a TLS error is not a
+reliable pre-send proof for an arbitrary caller-supplied transport — a custom
+transport can enable renegotiation, and a wrapping/retrying `RoundTripper` can
+surface a `*tls.CertificateVerificationError` or `tls.RecordHeaderError` while
+reading a response after forwarding the POST — so both flow to the UNCONFIRMED
+path. Redirect following is still force-disabled on every client used, including
+one supplied via `WithHTTPClient` (`CheckRedirect` overridden to
+`http.ErrUseLastResponse` unconditionally on a shallow copy, so the caller's
+client is not mutated), which keeps 3xx handling well-defined. Everything not
+proven pre-send is treated as UNCONFIRMED (may have been applied): a 3xx on a
+MUTATING request (it reached a responder and may have committed before
+redirecting — a 3xx on a GET is not a create), a mid-flight/`Do`-time context
+error (the per-attempt timeout wraps the whole round trip, so it can fire after
+the POST reached Reddit), a read/decode failure on a 2xx body, and any 5xx are
+wrapped so callers report "may exist" and require verification before a manual
+retry.
+
 See [internal/platform/reddit](../../../internal/platform/reddit).

docs/knowledge/log.md

@@ -1,5 +1,25 @@
 # Log
 
+## 2026-07-15
+
+**Update** — Hardened the Reddit Ads client's ambiguous-outcome classification
+(PR #27): `isPreSendDialError` now proves pre-send ONLY for DNS resolution and
+connect-time dial failures (ECONNREFUSED/EHOSTUNREACH/ENETUNREACH). NO TLS error
+is treated as pre-send, matching the merged Meta client — a TLS error is not a
+reliable pre-send proof for an arbitrary caller-supplied transport (renegotiation,
+or a wrapping RoundTripper surfacing a cert/record error while reading a response
+after forwarding the POST), so both `*tls.CertificateVerificationError` and
+`tls.RecordHeaderError` flow to the UNCONFIRMED path — the safe classification.
+Redirect following is still force-disabled on every client used, including one
+supplied via `WithHTTPClient` (`CheckRedirect` overridden to
+`http.ErrUseLastResponse` UNCONDITIONALLY on a shallow copy, so the caller's
+client is not mutated), which keeps 3xx handling well-defined. A 3xx on a MUTATING
+request is classified UNCONFIRMED (it reached a responder and may have committed
+before redirecting); a 3xx on a GET is not a create. Context errors and
+5xx/mid-flight transport failures also stay UNCONFIRMED. Reworded the
+manual-fallback UTM step to SET/REPLACE the utm_* params (matching
+`buildRedditUTMURL`'s `url.Values.Set`).
+
 ## 2026-07-13
 
 **Update** — Added OKF-recommended `tags` and `timestamp` frontmatter to the

internal/platform/reddit/client.go

@@ -258,19 +258,60 @@ type tokenRefresh struct {
 }
 
 // NewClient builds a Client from injected credentials and account config.
+// noFollow is the redirect policy for the Reddit Ads client: never follow
+// redirects. The Reddit Ads API returns JSON directly and never legitimately
+// 3xx-redirects these calls. Following a redirect on a mutating POST would let a
+// TLS/transport error at the redirect TARGET be misclassified as pre-send by
+// isPreSendDialError, even though the original POST may already have been received
+// by Reddit — which could duplicate a paid resource on retry. Returning
+// 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 request(), where a non-2xx status is surfaced as
+// an error (a 3xx on a mutating request is then classified UNCONFIRMED). Not
+// following redirects keeps the outcome classification simple: a create either
+// gets a 2xx, a definite 4xx, or an UNCONFIRMED (transport/5xx/3xx-mutating)
+// result — a redirect can't carry an already-sent POST to a different target. It
+// is shared by the built-in client and the caller-supplied-client enforcement in
+// NewClient so there is a single definition.
+func noFollow(_ *http.Request, _ []*http.Request) error {
+	return http.ErrUseLastResponse
+}
+
+// NewClient builds a Reddit Ads client. Redirect following is force-disabled on
+// whatever *http.Client is used (see the enforcement below) so 3xx responses are
+// classified by request() rather than transparently followed.
 func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client {
 	c := &Client{
-		creds:          creds,
-		account:        account,
-		baseURL:        redditAdsBaseURL,
-		tokenURL:       redditTokenURL,
-		httpClient:     &http.Client{Timeout: redditRequestTimeout},
+		creds:    creds,
+		account:  account,
+		baseURL:  redditAdsBaseURL,
+		tokenURL: redditTokenURL,
+		httpClient: &http.Client{
+			Timeout:       redditRequestTimeout,
+			CheckRedirect: noFollow,
+		},
 		now:            time.Now,
 		retryBaseDelay: retryBaseDelay,
 	}
 	for _, opt := range opts {
 		opt(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 let the original mutating POST be
+	// committed and then a TLS failure at the redirect target be misclassified as
+	// pre-send, so the invariant is a correctness requirement, not a default: even a
+	// caller-supplied CheckRedirect is overridden (a callback that returns nil, or
+	// follows N hops then stops, would still follow — re-opening the hole). The
+	// override is applied to a SHALLOW COPY so the caller's *http.Client is never
+	// mutated (it may be reused elsewhere). Skip the copy only if noFollow is already
+	// in force (the built-in client), which can't be detected by value, so compare by
+	// behavior: always copy unless the client is our own default.
+	if c.httpClient != nil {
+		hc := *c.httpClient
+		hc.CheckRedirect = noFollow
+		c.httpClient = &hc
+	}
 	return c
 }
 
@@ -598,39 +639,104 @@ func (e *transportError) Unwrap() error { return e.Err }
 // server and its outcome is unknowable. It is the single source of truth shared
 // by the campaign, ad-group, and ad create paths so they classify identically:
 //   - transportError: the round-trip failed AFTER a connection was established
-//     (see isPreSendDialError — a DNS/dial/connection-refused failure is NOT
-//     wrapped as transportError, so it never reaches here), so the request may
-//     have been received;
+//     (see isPreSendDialError — a DNS/dial/connection-refused/TLS-handshake
+//     failure is NOT wrapped as transportError, so it never reaches here), so the
+//     request may have been received. This is ALSO the path a context
+//     cancellation/deadline from the in-flight Do takes: the per-attempt timeout
+//     wraps the whole round trip, so a ctx error can fire after the POST reached
+//     Reddit, and request() wraps it as transportError so it is treated as
+//     ambiguous (UNCONFIRMED), never definitely-failed;
 //   - apiError with a 5xx status: Reddit received it and may have committed the
 //     mutation before erroring.
+//   - apiError with a 3xx status on a MUTATING method: redirects are disabled, so
+//     a 3xx surfaces as an apiError (see NewClient's noFollow policy). Receiving a
+//     3xx proves the mutating request REACHED a responder, but does NOT prove no
+//     resource was committed before the redirect, so it is UNCONFIRMED. A 3xx on a
+//     GET carries no create and is NOT ambiguous.
 //
-// A definite 4xx (Reddit rejected it), or any pre-send failure (token refresh,
-// body encode/build, a pre-connect dial error), means NOT applied → returns false
-// so the caller returns a clean (nil, err) / "failed" rather than "may exist".
+// A definite 4xx (Reddit rejected it), a 3xx on a non-mutating method, or any
+// pre-send failure (token refresh, body encode/build, a pre-connect
+// dial/cert-verification error, or a caller-cancel that surfaces raw BEFORE the
+// POST — e.g. from refreshToken), means NOT applied → returns false so the caller
+// returns a clean (nil, err) / "failed" rather than "may exist".
 func createOutcomeAmbiguous(err error) bool {
 	var te *transportError
 	if errors.As(err, &te) {
 		return true
 	}
 	var ae *apiError
-	return errors.As(err, &ae) && ae.StatusCode >= 500
+	if !errors.As(err, &ae) {
+		return false
+	}
+	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.
+	return ae.StatusCode >= 300 && ae.StatusCode < 400 && isMutatingMethod(ae.Method)
+}
+
+// isMutatingMethod reports whether an HTTP method can create/modify a resource
+// (POST/PUT/PATCH/DELETE). Used to decide whether a 3xx response is a possible
+// (UNCONFIRMED) create vs a harmless GET redirect.
+func isMutatingMethod(method string) bool {
+	switch method {
+	case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete:
+		return true
+	default:
+		return false
+	}
 }
 
 // isPreSendDialError reports whether a httpClient.Do error clearly happened
-// BEFORE any request bytes could have reached the server (DNS resolution failure,
-// connection refused, or no route/network unreachable). Such a failure means the
-// request was NOT sent, so it must NOT be treated as an ambiguous "may exist"
-// transportError. A failure AFTER a connection is established (mid-flight timeout,
-// unexpected EOF) is genuinely ambiguous and IS wrapped as transportError.
+// BEFORE any request bytes could have reached the server, so the request was NOT
+// sent and must NOT be treated as an ambiguous "may exist" transportError. It
+// covers ONLY failures that PROVE no request body was transmitted:
+//   - DNS resolution failure (the host never resolved);
+//   - connection refused / no route / network unreachable (never connected).
+//
+// No TLS error is treated as pre-send, matching the merged Meta client
+// (internal/platform/meta). A TLS error is not a reliable pre-send proof for an
+// arbitrary caller-supplied transport: a custom transport can enable TLS
+// renegotiation, and a wrapping/retrying RoundTripper can surface a
+// CertificateVerificationError (or a RecordHeaderError) while READING THE RESPONSE
+// after forwarding the POST. Treating either as pre-send could report a
+// possibly-created paid resource as definitely absent, duplicating it on retry.
+// So all TLS failures fall through to the transportError wrapping in request(),
+// which createOutcomeAmbiguous treats as ambiguous (UNCONFIRMED) — the safe
+// classification. (Redirect following is still force-disabled on every client in
+// NewClient, which keeps 3xx handling well-defined; it is no longer relied on to
+// make any TLS error a pre-send proof.)
+//
+// A context cancellation/deadline is deliberately NOT treated as pre-send here:
+// the per-attempt attemptCtx wraps the ENTIRE round trip (send + response read),
+// so a context.Canceled/DeadlineExceeded surfacing from Do can fire AFTER the
+// POST body already reached Reddit (Reddit may have created the resource; we
+// just never read the response). Classifying that as pre-send would let a caller
+// treat a possibly-created campaign as definitely-failed and retry, risking a
+// double-create. Such ctx errors therefore fall through to the transportError
+// wrapping in request(), which createOutcomeAmbiguous treats as ambiguous
+// (UNCONFIRMED) — never FAILED. A genuine caller-cancel before any POST is still
+// handled precisely by the ctx.Err() checks in the create path.
+//
+// A failure AFTER a connection is established and bytes were sent (mid-flight
+// timeout, unexpected EOF on the response) is genuinely ambiguous and IS wrapped
+// as transportError.
 func isPreSendDialError(err error) bool {
 	var dnsErr *net.DNSError
 	if errors.As(err, &dnsErr) {
 		return true
 	}
-	if errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH) {
-		return true
-	}
-	return false
+	return errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH)
+	// NOTE: no TLS error is treated as pre-send, matching the merged Meta client
+	// (internal/platform/meta). A TLS error is not a reliable pre-send proof for an
+	// arbitrary caller-supplied transport: a custom transport can enable TLS
+	// renegotiation, and a wrapping/retrying RoundTripper can surface a
+	// CertificateVerificationError while reading a response AFTER forwarding the
+	// POST. So all TLS failures flow to the UNCONFIRMED (transportError) path — the
+	// safe classification, since UNCONFIRMED never lets a retry duplicate a paid
+	// resource. Only DNS resolution and connect-time dial failures (above) prove no
+	// bytes were sent.
 }
 
 // request performs an authenticated Reddit Ads API call, sanitizing the path
@@ -1425,10 +1531,17 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig
 	} else {
 		variantCount := len(in.Variants)
 		if variantCount > 0 {
-			steps = append(steps, fmt.Sprintf("%d ad variant(s) ready -- create ads in Reddit Ads Manager with these headlines:", variantCount))
+			// The click URLs below show ONLY the generated utm_* parameters on the
+			// destination — any pre-existing query on the registration URL is omitted
+			// here to avoid persisting a secret in the returned steps. When building the
+			// ads manually, use YOUR registration URL (with its own query params intact)
+			// as the base and SET/REPLACE these utm_* parameters, so the destination
+			// matches what an automated ad would use. The automated click_url is built
+			// with url.Values.Set (see buildRedditUTMURL), which REPLACES any existing
+			// utm_* key while preserving the other (non-utm_*) query params — appending
+			// instead would leave duplicate/conflicting utm values.
+			steps = append(steps, fmt.Sprintf("%d ad variant(s) ready -- create ads in Reddit Ads Manager with these headlines (set/replace the shown utm_* params on your registration URL, keeping only its other, non-utm_* query params):", variantCount))
 			for i := 0; i < variantCount; i++ {
-				// These are manual-action instructions returned to the caller; show the
-				// sanitized click URL (utm_* only) so no pre-existing secret leaks.
 				steps = append(steps, fmt.Sprintf("  Variant %d: %q -> %s", i+1, in.Variants[i].Headline, displayRedditUTMURL(in, i)))
 			}
 		} else {