From d203839ac60dddab6a722ce1929fdd558d318b3a Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Tue, 14 Jul 2026 09:33:11 -0700 Subject: [PATCH 01/15] fix(reddit): narrow pre-send classification; clarify manual-ad destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the merged Reddit Ads client (#21), addressing two Copilot items that landed after David's approval: - client.go: isPreSendDialError now also classifies TLS handshake / certificate failures and context cancellation/deadline as pre-send (request NOT sent), so they are not wrapped as an ambiguous "may exist" transportError. httpClient.Do returning an error does not prove bytes reached Reddit — only a failure after a connection is established and bytes were sent (mid-flight timeout, unexpected EOF) is genuinely ambiguous. - client.go: the manual-ad instruction steps now clarify that the shown click URL carries only the generated utm_* params (the caller's pre-existing query is omitted to avoid persisting a secret), and instruct the operator to append them to their own registration URL — so the manual destination matches an automated ad's. - client_test.go: TestIsPreSendDialError covers DNS/refused/TLS/ctx (pre-send) vs unexpected-EOF (ambiguous). Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./internal/platform/reddit. Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 46 ++++++++++++++++++++----- internal/platform/reddit/client_test.go | 33 ++++++++++++++++++ 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index dc1dca8a..2242bb1a 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -14,6 +14,7 @@ package reddit import ( "bytes" "context" + "crypto/tls" "encoding/base64" "encoding/json" "errors" @@ -617,11 +618,20 @@ func createOutcomeAmbiguous(err error) bool { } // 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: +// - DNS resolution failure; +// - connection refused / no route / network unreachable (never connected); +// - TLS handshake / certificate errors (the secure channel was never +// established, so no request body was sent); +// - context cancellation/deadline that fired before or during connection setup +// (the caller aborted; treated as not-sent — the campaign path additionally +// checks ctx.Err() first, so a genuine caller-cancel is handled there). +// +// 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) { @@ -630,6 +640,22 @@ func isPreSendDialError(err error) bool { if errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH) { return true } + // TLS handshake / certificate verification failures: the secure channel was + // never established, so no request bytes were sent. + var certErr *tls.CertificateVerificationError + if errors.As(err, &certErr) { + return true + } + var recordErr tls.RecordHeaderError + if errors.As(err, &recordErr) { + return true + } + // Context cancellation/deadline surfacing from Do (e.g. cancelled between token + // refresh and the send, or during connection setup): the request did not + // complete a send that could have been applied. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return true + } return false } @@ -1425,10 +1451,14 @@ 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 add these utm_* parameters, so the destination matches + // what an automated ad would use. + steps = append(steps, fmt.Sprintf("%d ad variant(s) ready -- create ads in Reddit Ads Manager with these headlines (append the shown utm_* params to your registration URL, keeping its existing query):", 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 { diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 22836d0c..1f2dfe7b 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -5,8 +5,10 @@ package reddit import ( "context" + "crypto/tls" "encoding/json" "errors" + "fmt" "io" "net" "net/http" @@ -16,6 +18,7 @@ import ( "strings" "sync" "sync/atomic" + "syscall" "testing" "time" ) @@ -908,6 +911,36 @@ func TestCreateCampaign_ConnRefusedNotUnconfirmed(t *testing.T) { } } +// TestIsPreSendDialError classifies which Do errors mean the request was NOT sent +// (so a create is NOT ambiguous): DNS, connection-refused/no-route, TLS handshake/ +// certificate failures, and context cancellation/deadline. A generic post-connect +// failure (e.g. unexpected EOF) stays ambiguous (not pre-send). +func TestIsPreSendDialError(t *testing.T) { + preSend := []error{ + &net.DNSError{Err: "no such host", Name: "x"}, + syscall.ECONNREFUSED, + syscall.EHOSTUNREACH, + &tls.CertificateVerificationError{}, + context.Canceled, + context.DeadlineExceeded, + fmt.Errorf("wrapped: %w", context.Canceled), + } + for _, e := range preSend { + if !isPreSendDialError(e) { + t.Errorf("isPreSendDialError(%v) = false, want true (pre-send / not sent)", e) + } + } + notPreSend := []error{ + io.ErrUnexpectedEOF, // mid-flight after a connection was established + errors.New("some other error"), + } + for _, e := range notPreSend { + if isPreSendDialError(e) { + t.Errorf("isPreSendDialError(%v) = true, want false (ambiguous / post-connect)", e) + } + } +} + // TestCreateCampaign_2xxUndecodableIsUnconfirmed verifies a campaign create that // returns 2xx with an undecodable body is treated as UNCONFIRMED (the mutation // likely succeeded), returning a partial with the campaign name. From 3be35a7da6b229fca3de3422223fe320690372a3 Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Tue, 14 Jul 2026 10:05:17 -0700 Subject: [PATCH 02/15] fix(review): treat context errors during create as ambiguous, not pre-send isPreSendDialError classified every context.Canceled/DeadlineExceeded from Do as pre-send, but the per-attempt timeout wraps the whole round trip, so a ctx error can fire after the POST reached Reddit. Reporting that as definitely-failed risks a caller double-creating. Route ctx errors to the ambiguous (UNCONFIRMED) path instead, keeping only DNS/dial/TLS-handshake failures as provably pre-send. Add tests for the TLS RecordHeaderError branch and the manual-variant creation path. Addresses Copilot/Cursor review on PR #27. Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 47 +++--- internal/platform/reddit/client_test.go | 192 +++++++++++++++++++++++- 2 files changed, 211 insertions(+), 28 deletions(-) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 2242bb1a..e6393a33 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -599,15 +599,21 @@ 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. // // 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". +// body encode/build, a pre-connect dial/TLS-handshake 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) { @@ -620,14 +626,22 @@ func createOutcomeAmbiguous(err error) bool { // isPreSendDialError reports whether a httpClient.Do error clearly happened // 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: -// - DNS resolution failure; +// 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); // - TLS handshake / certificate errors (the secure channel was never -// established, so no request body was sent); -// - context cancellation/deadline that fired before or during connection setup -// (the caller aborted; treated as not-sent — the campaign path additionally -// checks ctx.Err() first, so a genuine caller-cancel is handled there). +// established, so no request body was sent). +// +// 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 @@ -647,16 +661,7 @@ func isPreSendDialError(err error) bool { return true } var recordErr tls.RecordHeaderError - if errors.As(err, &recordErr) { - return true - } - // Context cancellation/deadline surfacing from Do (e.g. cancelled between token - // refresh and the send, or during connection setup): the request did not - // complete a send that could have been applied. - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return true - } - return false + return errors.As(err, &recordErr) } // request performs an authenticated Reddit Ads API call, sanitizing the path diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 1f2dfe7b..ecf9783c 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -558,6 +558,110 @@ func TestCreateCampaign_HappyPath(t *testing.T) { } } +// TestCreateCampaign_ManualVariantsNoPostURL exercises the manual-variant branch: +// Variants supplied WITHOUT a PostURL. In that case the client does NOT create +// ads (there is no post to promote); instead it emits one "ready" instruction per +// variant carrying the headline and a DISPLAY click URL. The registration URL +// carries a secret in its query, so this also confirms the secret is stripped: +// displayRedditUTMURL discards the caller's original query and keeps ONLY the +// generated utm_* params, so the secret never lands in the returned steps. +func TestCreateCampaign_ManualVariantsNoPostURL(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": "tok", "expires_in": 3600}) + })) + defer tokenSrv.Close() + + var mu sync.Mutex + var paths []string + handler := http.NewServeMux() + handler.HandleFunc("/api/v3/", func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + paths = append(paths, r.Method+" "+r.URL.Path) + mu.Unlock() + path := r.URL.Path + switch { + case strings.HasSuffix(path, "/ad_accounts/t2_test") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "t2_test"}}) + case strings.HasSuffix(path, "/campaigns") && r.Method == http.MethodPost: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "camp_1"}}) + case strings.HasSuffix(path, "/ad_groups") && r.Method == http.MethodPost: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "ag_1"}}) + default: + http.Error(w, "unexpected", http.StatusNotFound) + } + }) + apiSrv := httptest.NewServer(handler) + defer apiSrv.Close() + + c := NewClient(testCreds, testAccount, WithBaseURL(apiSrv.URL+"/api/v3"), WithTokenURL(tokenSrv.URL), WithNowFunc(fixedRedditClock())) + + // Compose the secret-bearing registration URL at runtime so no credential-shaped + // literal appears in the test source (avoids tripping secretlint/gitleaks). + secret := "s3cr" + "et-token-" + "9f8a7b" + in := CampaignInput{ + EventName: "Open Source Summit", + Project: "tlf", + EventSlug: "oss-2026", + RegistrationURL: "https://events.linuxfoundation.org/oss/?token=" + secret, + BudgetUSD: 500, + StartDate: "2026-08-01", + EndDate: "2026-08-31", + GeoTargets: []string{"us"}, + Keywords: []string{"kubernetes"}, + Objective: "traffic", + Variants: []AdVariant{ + {Headline: "Join us at OSS", Body: "The premier open source event"}, + {Headline: "Register for OSS 2026", Body: "Talks, workshops, and more"}, + }, + // No PostURL -> manual-variant branch. + } + + res, err := c.CreateCampaign(context.Background(), in) + if err != nil { + t.Fatalf("CreateCampaign: %v", err) + } + + // No ad is created in the manual-variant path (no post to promote), and no + // AdID/AdCount is set. + mu.Lock() + for _, p := range paths { + if strings.HasSuffix(p, "/ads") { + t.Errorf("manual-variant path must NOT create ads, but saw %q", p) + } + } + mu.Unlock() + if res.AdCount != 0 || res.AdID != "" { + t.Errorf("manual-variant path: AdCount=%d AdID=%q, want 0 and empty", res.AdCount, res.AdID) + } + + allSteps := strings.Join(res.Steps, "\n") + + // The secret from the registration URL must never appear in the returned steps. + if strings.Contains(allSteps, secret) { + t.Errorf("registration-URL secret leaked into steps:\n%s", allSteps) + } + + // There must be one variant instruction per supplied variant, each carrying its + // headline and the sanitized utm_* display URL. + if !strings.Contains(allSteps, "2 ad variant(s) ready") { + t.Errorf("expected a '2 ad variant(s) ready' summary step, got:\n%s", allSteps) + } + for i, v := range in.Variants { + if !strings.Contains(allSteps, v.Headline) { + t.Errorf("variant %d headline %q missing from steps:\n%s", i+1, v.Headline, allSteps) + } + // Each variant's display URL carries a distinct utm_content=variant-N and the + // generated utm params (proof the sanitized destination was built per variant). + wantContent := fmt.Sprintf("utm_content=variant-%d", i+1) + if !strings.Contains(allSteps, wantContent) { + t.Errorf("variant %d: expected %q in steps:\n%s", i+1, wantContent, allSteps) + } + } + if !strings.Contains(allSteps, "utm_source=reddit") { + t.Errorf("expected generated utm_source=reddit in variant display URLs:\n%s", allSteps) + } +} + // TestCreateCampaign_CommunityFallback verifies the retry-without-communities // path when the ad-group create returns "invalid communities". func TestCreateCampaign_CommunityFallback(t *testing.T) { @@ -911,19 +1015,27 @@ func TestCreateCampaign_ConnRefusedNotUnconfirmed(t *testing.T) { } } -// TestIsPreSendDialError classifies which Do errors mean the request was NOT sent -// (so a create is NOT ambiguous): DNS, connection-refused/no-route, TLS handshake/ -// certificate failures, and context cancellation/deadline. A generic post-connect -// failure (e.g. unexpected EOF) stays ambiguous (not pre-send). +// TestIsPreSendDialError classifies which Do errors PROVE the request was NOT +// sent (so a create is NOT ambiguous): DNS, connection-refused/no-route, and TLS +// handshake/certificate failures — in every one of these the connection or secure +// channel was never established, so no request bytes could have reached Reddit. +// +// A context cancellation/deadline is deliberately NOT pre-send: the per-attempt +// context wraps the whole round trip, so a ctx error from Do can fire AFTER the +// POST body reached Reddit. Treating it as pre-send (definitely-failed) would risk +// a caller double-creating, so ctx errors must classify as NOT pre-send (they are +// wrapped as transportError and reported UNCONFIRMED instead). A generic +// post-connect failure (e.g. unexpected EOF) likewise stays ambiguous (not +// pre-send). func TestIsPreSendDialError(t *testing.T) { preSend := []error{ &net.DNSError{Err: "no such host", Name: "x"}, syscall.ECONNREFUSED, syscall.EHOSTUNREACH, &tls.CertificateVerificationError{}, - context.Canceled, - context.DeadlineExceeded, - fmt.Errorf("wrapped: %w", context.Canceled), + // TLS record-header failure (e.g. talking TLS to a plaintext/misconfigured + // endpoint): the handshake never completed, so nothing was sent. + tls.RecordHeaderError{Msg: "bad record"}, } for _, e := range preSend { if !isPreSendDialError(e) { @@ -933,6 +1045,12 @@ func TestIsPreSendDialError(t *testing.T) { notPreSend := []error{ io.ErrUnexpectedEOF, // mid-flight after a connection was established errors.New("some other error"), + // Context errors are NOT pre-send: the per-attempt ctx wraps the whole round + // trip, so a ctx error can surface after the POST reached Reddit. They must be + // treated as ambiguous (UNCONFIRMED), never definitely-failed. + context.Canceled, + context.DeadlineExceeded, + fmt.Errorf("wrapped: %w", context.Canceled), } for _, e := range notPreSend { if isPreSendDialError(e) { @@ -1087,6 +1205,66 @@ func TestCreateCampaign_CtxCancelDuringVerifyIsFatal(t *testing.T) { } } +// TestCreateCampaign_CtxCancelDuringCampaignPostIsUnconfirmed verifies the +// corrected semantics: a caller context cancellation that fires while the +// campaign POST is IN FLIGHT (after the request has gone out but before the +// response is read) must be reported as UNCONFIRMED — the campaign MAY have been +// created — NOT as a clean pre-send FAILED. The per-attempt context wraps the +// whole round trip, so the ctx error surfaces from Do and is wrapped as a +// transportError, which createOutcomeAmbiguous treats as ambiguous. Reporting it +// as definitely-failed would let a caller retry and double-create. +func TestCreateCampaign_CtxCancelDuringCampaignPostIsUnconfirmed(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": "tok", "expires_in": 3600}) + })) + defer tokenSrv.Close() + + ctx, cancel := context.WithCancel(context.Background()) + // serverDone unblocks the POST handler unconditionally at test end, so the + // handler goroutine can never leak if the client-side connection teardown + // doesn't cancel the server request context promptly (which would otherwise + // hang the deferred Server.Close on -race). + serverDone := make(chan struct{}) + handler := http.NewServeMux() + handler.HandleFunc("/api/v3/", func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/ad_accounts/t2_test") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"id": "t2_test"}}) + case strings.HasSuffix(r.URL.Path, "/campaigns") && r.Method == http.MethodPost: + // The POST body has already reached the server (Reddit may have created + // the campaign). Cancel the caller ctx now and never send a response, so + // the client's Do fails with a ctx error AFTER the request went out. Block + // until either the server request ctx is cancelled (client tore down the + // connection) or the test signals cleanup. + cancel() + select { + case <-r.Context().Done(): + case <-serverDone: + } + default: + http.Error(w, "unexpected", http.StatusNotFound) + } + }) + apiSrv := httptest.NewServer(handler) + defer apiSrv.Close() + defer close(serverDone) // runs before apiSrv.Close() (LIFO), releasing the handler + + c := NewClient(testCreds, testAccount, WithBaseURL(apiSrv.URL+"/api/v3"), WithTokenURL(tokenSrv.URL), WithNowFunc(fixedRedditClock())) + res, err := c.CreateCampaign(ctx, validRedditInput()) + if err == nil { + t.Fatal("expected an error on ctx cancellation during the campaign POST") + } + if res == nil || res.CampaignName == "" { + t.Fatalf("in-flight cancel must return a partial UNCONFIRMED result with the campaign name, got %+v", res) + } + if !strings.Contains(err.Error(), "UNCONFIRMED") { + t.Errorf("an in-flight campaign-POST cancel must be UNCONFIRMED (may exist), got %v", err) + } + if strings.Contains(err.Error(), "aborted before completion") { + t.Errorf("an in-flight cancel must NOT be reported as a clean pre-POST abort, got %v", err) + } +} + // TestCreateCampaign_NoSubredditsNoSkipWarning verifies FINDING 3: a normal // keyword/geo-only campaign (no subreddits supplied) must NOT be reported as // having skipped communities that need manual action. From d5a0891360c0fb73240f96e963f85b00fec63bd1 Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 09:46:20 -0700 Subject: [PATCH 03/15] fix(review): disable redirects so TLS errors prove pre-send (PR #27) Address David's [blocking] and Copilot review: - disable redirect following (CheckRedirect -> ErrUseLastResponse) so a TLS certificate/record error genuinely proves the original POST was unsent; otherwise a POST could be received then redirected to a TLS-broken target and misclassified as not-created, duplicating a paid resource on retry - reword the operator UTM instruction to set/replace the utm_* params (matching buildRedditUTMURL's Query.Set), preserving only other query params - document the outcome-classification fix in the reddit concept doc and log Signed-off-by: Misha Rautela --- .../code/internal-platform-reddit.md | 19 +++++- docs/knowledge/log.md | 15 ++++ internal/platform/reddit/client.go | 44 +++++++++--- internal/platform/reddit/client_test.go | 68 +++++++++++++++++++ 4 files changed, 137 insertions(+), 9 deletions(-) diff --git a/docs/knowledge/code/internal-platform-reddit.md b/docs/knowledge/code/internal-platform-reddit.md index 7ed0153a..726e7e94 100644 --- a/docs/knowledge/code/internal-platform-reddit.md +++ b/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,21 @@ 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, +connection-refused/no-route, and TLS handshake/certificate errors. The TLS +branches are sound ONLY because the built-in `*http.Client` disables redirect +following (`CheckRedirect` returns `http.ErrUseLastResponse`); otherwise a POST +could be received by Reddit and then redirected to a TLS-broken target, whose +cert error would be misread as pre-send and let a retry duplicate a paid +resource. With redirects disabled, a disallowed 3xx surfaces as a non-2xx +`apiError` (< 500), i.e. a clean not-created failure — Reddit never legitimately +3xx-redirects these calls. Everything else is treated as UNCONFIRMED (may have +been applied): 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). diff --git a/docs/knowledge/log.md b/docs/knowledge/log.md index ff311548..e0d5b203 100644 --- a/docs/knowledge/log.md +++ b/docs/knowledge/log.md @@ -1,5 +1,20 @@ # Log +## 2026-07-15 + +**Update** — Hardened the Reddit Ads client's ambiguous-outcome classification +(PR #27): the built-in `*http.Client` now disables redirect following +(`CheckRedirect` returns `http.ErrUseLastResponse`), so a TLS +certificate/record error can only come from the ORIGINAL request's handshake. +This makes `isPreSendDialError`'s TLS branches sound — otherwise a mutating POST +could be received then redirected to a TLS-broken target and misclassified as +pre-send, duplicating a paid resource on retry. A disallowed 3xx now surfaces as +a non-2xx `apiError` (< 500), i.e. a clean not-created failure. Documented that +`isPreSendDialError` proves pre-send only for DNS/dial/TLS-with-redirects- +disabled, while context errors and 5xx/mid-flight transport failures stay +UNCONFIRMED. Reworded the manual-fallback UTM step to SET/REPLACE the utm_* +params (matching `buildRedditUTMURL`'s `url.Values.Set`). + ## 2026-07-13 **Creation** — Added OKF concept doc for internal/platform/meta (Meta Ads Graph diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index e6393a33..6fdcb049 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -261,11 +261,28 @@ type tokenRefresh struct { // NewClient builds a Client from injected credentials and account config. 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, + // Do NOT 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 ErrUseLastResponse stops following and + // hands the 3xx response back to request(), where a non-2xx status is + // surfaced as an error (see the StatusCode check in request()). This makes + // isPreSendDialError's TLS branches sound: a cert/record error now proves + // the ORIGINAL request's transport failed pre-send. A caller that supplies + // its own client via WithHTTPClient is responsible for its own redirect + // policy; this default only applies to the built-in client. + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + }, + }, now: time.Now, retryBaseDelay: retryBaseDelay, } @@ -632,6 +649,14 @@ func createOutcomeAmbiguous(err error) bool { // - TLS handshake / certificate errors (the secure channel was never // established, so no request body was sent). // +// The TLS branches are sound ONLY because the built-in client disables redirect +// following (CheckRedirect returns http.ErrUseLastResponse in NewClient). Were +// redirects followed, a mutating POST could be RECEIVED by Reddit, then +// redirected to a TLS-broken target; the resulting cert/record error would match +// here and be misreported as pre-send, letting a retry duplicate a paid resource. +// With redirects disabled, a cert/record error can only come from the ORIGINAL +// request's handshake, so it genuinely proves no request body was sent. +// // 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 @@ -1460,9 +1485,12 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig // 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 add these utm_* parameters, so the destination matches - // what an automated ad would use. - steps = append(steps, fmt.Sprintf("%d ad variant(s) ready -- create ads in Reddit Ads Manager with these headlines (append the shown utm_* params to your registration URL, keeping its existing query):", variantCount)) + // 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++ { steps = append(steps, fmt.Sprintf(" Variant %d: %q -> %s", i+1, in.Variants[i].Headline, displayRedditUTMURL(in, i))) } diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index ecf9783c..053a1f5b 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -3609,3 +3609,71 @@ func TestRedactURL(t *testing.T) { } } } + +// TestBuiltinClientDoesNotFollowRedirectsOnPOST verifies the built-in +// *http.Client disables redirect following: a mutating POST that receives a 3xx +// must NOT be followed to the redirect target (which could be TLS-broken and let +// isPreSendDialError misclassify a possibly-received POST as pre-send). The 3xx +// must instead surface as a non-2xx error, and the redirect target handler must +// never be hit. +func TestBuiltinClientDoesNotFollowRedirectsOnPOST(t *testing.T) { + var targetHit atomic.Bool + // target is where the redirect would send the request if it were followed. + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + targetHit.Store(true) + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + + apiSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + http.Error(w, "unexpected", http.StatusNotFound) + return + } + // Redirect the mutating POST elsewhere. A redirect-following client would + // re-issue against target; the built-in client must not. + http.Redirect(w, r, target.URL, http.StatusFound) + })) + defer apiSrv.Close() + + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{"access_token": "tok", "expires_in": 3600}) + })) + defer tokenSrv.Close() + + // Uses the DEFAULT built-in client (no WithHTTPClient), so CheckRedirect applies. + c := NewClient(testCreds, testAccount, WithBaseURL(apiSrv.URL+"/api/v3"), WithTokenURL(tokenSrv.URL), WithNowFunc(fixedRedditClock())) + + _, err := c.request(context.Background(), http.MethodPost, "/campaigns", map[string]any{"x": 1}) + if err == nil { + t.Fatal("a 3xx redirect on a POST must surface as an error, got nil") + } + // The 3xx must be reported as a non-2xx apiError, not a false success, and + // not as an ambiguous transportError (3xx < 500 → clean not-created failure). + var ae *apiError + if !errors.As(err, &ae) { + t.Fatalf("err = %v (%T), want *apiError", err, err) + } + if ae.StatusCode != http.StatusFound { + t.Errorf("apiError.StatusCode = %d, want %d (302 Found)", ae.StatusCode, http.StatusFound) + } + if createOutcomeAmbiguous(err) { + t.Errorf("a 3xx (< 500) must NOT be classified as ambiguous, got ambiguous for %v", err) + } + if targetHit.Load() { + t.Error("redirect target was hit: the built-in client followed a redirect on a mutating POST") + } +} + +// TestBuiltinClientCheckRedirectReturnsErrUseLastResponse pins the redirect +// policy directly: the built-in client's CheckRedirect must return +// http.ErrUseLastResponse so a 3xx is handed back as-is rather than followed. +func TestBuiltinClientCheckRedirectReturnsErrUseLastResponse(t *testing.T) { + c := NewClient(testCreds, testAccount) + if c.httpClient.CheckRedirect == nil { + t.Fatal("built-in client must set CheckRedirect to disable redirect following") + } + if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, http.ErrUseLastResponse) { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse", got) + } +} From 9e06ad7c07ee5c469d20f1e5c3d15108e826c97a Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 10:05:07 -0700 Subject: [PATCH 04/15] fix(review): tighten redirect/TLS outcome classification (PR #27) Address Copilot follow-ups on the redirect fix: - drop tls.RecordHeaderError from isPreSendDialError: it can surface after version negotiation while reading a response, so it doesn't prove pre-send; it now flows to the UNCONFIRMED path. CertificateVerificationError stays (handshake-time, provably pre-send) - enforce the no-follow redirect policy on a caller-supplied http.Client too, via a shallow copy so the caller's client is never mutated; an explicit caller CheckRedirect is respected - classify a 3xx response to a mutating request as UNCONFIRMED, since the request reached a responder and a resource may have been committed before the redirect - assert the new operator set/replace UTM instruction text explicitly Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 126 +++++++++++++++++------- internal/platform/reddit/client_test.go | 94 ++++++++++++++++-- 2 files changed, 176 insertions(+), 44 deletions(-) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 6fdcb049..5d3be7a4 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -259,6 +259,22 @@ 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 +// ErrUseLastResponse stops following and hands the 3xx response back to request(), +// where a non-2xx status is surfaced as an error (see the StatusCode check in +// request()). This makes isPreSendDialError's CertificateVerificationError branch +// sound: a cert error then proves the ORIGINAL request's handshake failed +// pre-send. 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 +} + func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client { c := &Client{ creds: creds, @@ -266,22 +282,8 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client baseURL: redditAdsBaseURL, tokenURL: redditTokenURL, httpClient: &http.Client{ - Timeout: redditRequestTimeout, - // Do NOT 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 ErrUseLastResponse stops following and - // hands the 3xx response back to request(), where a non-2xx status is - // surfaced as an error (see the StatusCode check in request()). This makes - // isPreSendDialError's TLS branches sound: a cert/record error now proves - // the ORIGINAL request's transport failed pre-send. A caller that supplies - // its own client via WithHTTPClient is responsible for its own redirect - // policy; this default only applies to the built-in client. - CheckRedirect: func(_ *http.Request, _ []*http.Request) error { - return http.ErrUseLastResponse - }, + Timeout: redditRequestTimeout, + CheckRedirect: noFollow, }, now: time.Now, retryBaseDelay: retryBaseDelay, @@ -289,6 +291,19 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client for _, opt := range opts { opt(c) } + // Enforce the no-follow redirect policy on whatever client ended up on + // c.httpClient — INCLUDING one supplied via WithHTTPClient, which replaces the + // default above. A supplied client that follows redirects would let the original + // mutating POST be committed and then a TLS failure at the redirect target be + // misclassified as pre-send. A supplied client that doesn't set CheckRedirect + // gets no-follow enforced via a SHALLOW COPY (so the caller's *http.Client is + // never mutated — it may be reused elsewhere); a supplied client with its own + // CheckRedirect is respected and left untouched. + if c.httpClient != nil && c.httpClient.CheckRedirect == nil { + hc := *c.httpClient + hc.CheckRedirect = noFollow + c.httpClient = &hc + } return c } @@ -625,19 +640,44 @@ func (e *transportError) Unwrap() error { return e.Err } // 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/TLS-handshake 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". +// 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 @@ -646,16 +686,30 @@ func createOutcomeAmbiguous(err error) bool { // 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); -// - TLS handshake / certificate errors (the secure channel was never +// - TLS certificate verification failure (the secure channel was never // established, so no request body was sent). // -// The TLS branches are sound ONLY because the built-in client disables redirect -// following (CheckRedirect returns http.ErrUseLastResponse in NewClient). Were -// redirects followed, a mutating POST could be RECEIVED by Reddit, then -// redirected to a TLS-broken target; the resulting cert/record error would match -// here and be misreported as pre-send, letting a retry duplicate a paid resource. -// With redirects disabled, a cert/record error can only come from the ORIGINAL -// request's handshake, so it genuinely proves no request body was sent. +// Note: tls.RecordHeaderError is deliberately NOT treated as pre-send. Unlike a +// cert verification failure, a RecordHeaderError does not prove a handshake-time +// failure: crypto/tls can also return it AFTER a version has been negotiated, +// when a later record carries an invalid header/version — including while READING +// THE RESPONSE after this POST was already sent. So even with redirects disabled, +// a RecordHeaderError does not prove pre-send; classifying it as pre-send could +// report a possibly-created paid resource as definitely absent, duplicating it on +// retry. It therefore falls through to the transportError wrapping in request(), +// which createOutcomeAmbiguous treats as ambiguous (UNCONFIRMED) — the safe +// classification. +// +// The CertificateVerificationError branch is sound ONLY because the built-in +// client disables redirect following (CheckRedirect returns +// http.ErrUseLastResponse in NewClient) — a policy also enforced on a +// caller-supplied client via a shallow copy in NewClient. Were redirects +// followed, a mutating POST could be RECEIVED by Reddit, then redirected to a +// TLS-broken target; the resulting cert error would match here and be misreported +// as pre-send, letting a retry duplicate a paid resource. With redirects +// disabled, a cert verification failure is a handshake-time event that can only +// come from the ORIGINAL request's handshake, so it genuinely proves no request +// body was sent. // // A context cancellation/deadline is deliberately NOT treated as pre-send here: // the per-attempt attemptCtx wraps the ENTIRE round trip (send + response read), @@ -679,14 +733,12 @@ func isPreSendDialError(err error) bool { if errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.EHOSTUNREACH) || errors.Is(err, syscall.ENETUNREACH) { return true } - // TLS handshake / certificate verification failures: the secure channel was - // never established, so no request bytes were sent. + // TLS certificate verification failure: a handshake-time event, so the secure + // channel was never established and no request bytes were sent. (RecordHeaderError + // is intentionally excluded — see the doc comment: it can surface post-negotiation + // while reading a response, so it does not prove pre-send.) var certErr *tls.CertificateVerificationError - if errors.As(err, &certErr) { - return true - } - var recordErr tls.RecordHeaderError - return errors.As(err, &recordErr) + return errors.As(err, &certErr) } // request performs an authenticated Reddit Ads API call, sanitizing the path diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 053a1f5b..27bf0b99 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -646,6 +646,20 @@ func TestCreateCampaign_ManualVariantsNoPostURL(t *testing.T) { if !strings.Contains(allSteps, "2 ad variant(s) ready") { t.Errorf("expected a '2 ad variant(s) ready' summary step, got:\n%s", allSteps) } + // The operator instruction must carry the NEW set/replace semantics explicitly: + // the operator SETS/REPLACES the shown utm_* params on their registration URL, + // keeping only its OTHER, non-utm_* query params. Assert both distinctive + // phrases so this operator-critical guidance can't regress to the old "append ... + // keeping its existing query" wording (which the summary-count check alone would + // still pass). + for _, want := range []string{ + "set/replace the shown utm_* params", + "keeping only its other, non-utm_* query params", + } { + if !strings.Contains(allSteps, want) { + t.Errorf("operator instruction missing new phrase %q; got:\n%s", want, allSteps) + } + } for i, v := range in.Variants { if !strings.Contains(allSteps, v.Headline) { t.Errorf("variant %d headline %q missing from steps:\n%s", i+1, v.Headline, allSteps) @@ -1033,9 +1047,6 @@ func TestIsPreSendDialError(t *testing.T) { syscall.ECONNREFUSED, syscall.EHOSTUNREACH, &tls.CertificateVerificationError{}, - // TLS record-header failure (e.g. talking TLS to a plaintext/misconfigured - // endpoint): the handshake never completed, so nothing was sent. - tls.RecordHeaderError{Msg: "bad record"}, } for _, e := range preSend { if !isPreSendDialError(e) { @@ -1045,6 +1056,11 @@ func TestIsPreSendDialError(t *testing.T) { notPreSend := []error{ io.ErrUnexpectedEOF, // mid-flight after a connection was established errors.New("some other error"), + // TLS record-header failure is NOT pre-send: crypto/tls can return it AFTER a + // version has been negotiated, when a later record has an invalid header — + // including while READING THE RESPONSE after the POST was sent. So it doesn't + // prove pre-send and must be treated as ambiguous (UNCONFIRMED). + tls.RecordHeaderError{Msg: "bad record"}, // Context errors are NOT pre-send: the per-attempt ctx wraps the whole round // trip, so a ctx error can surface after the POST reached Reddit. They must be // treated as ambiguous (UNCONFIRMED), never definitely-failed. @@ -3648,8 +3664,7 @@ func TestBuiltinClientDoesNotFollowRedirectsOnPOST(t *testing.T) { if err == nil { t.Fatal("a 3xx redirect on a POST must surface as an error, got nil") } - // The 3xx must be reported as a non-2xx apiError, not a false success, and - // not as an ambiguous transportError (3xx < 500 → clean not-created failure). + // The 3xx must be reported as a non-2xx apiError, not a false success. var ae *apiError if !errors.As(err, &ae) { t.Fatalf("err = %v (%T), want *apiError", err, err) @@ -3657,8 +3672,11 @@ func TestBuiltinClientDoesNotFollowRedirectsOnPOST(t *testing.T) { if ae.StatusCode != http.StatusFound { t.Errorf("apiError.StatusCode = %d, want %d (302 Found)", ae.StatusCode, http.StatusFound) } - if createOutcomeAmbiguous(err) { - t.Errorf("a 3xx (< 500) must NOT be classified as ambiguous, got ambiguous for %v", err) + // A 302 on a MUTATING request must be UNCONFIRMED (ambiguous): receiving a 3xx + // proves the POST reached a responder, but does not prove no resource was + // committed before the redirect — so the caller must NOT retry into a duplicate. + if !createOutcomeAmbiguous(err) { + t.Errorf("a 3xx on a mutating POST must be classified as ambiguous (UNCONFIRMED), got not-ambiguous for %v", err) } if targetHit.Load() { t.Error("redirect target was hit: the built-in client followed a redirect on a mutating POST") @@ -3677,3 +3695,65 @@ func TestBuiltinClientCheckRedirectReturnsErrUseLastResponse(t *testing.T) { t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse", got) } } + +// TestWithHTTPClientEnforcesNoFollowWithoutMutatingCaller verifies that a +// caller-supplied *http.Client WITHOUT a CheckRedirect gets the no-follow policy +// enforced via a shallow copy on c.httpClient, while the caller's original client +// is provably NOT mutated (its CheckRedirect stays nil). +func TestWithHTTPClientEnforcesNoFollowWithoutMutatingCaller(t *testing.T) { + supplied := &http.Client{Timeout: 5 * time.Second} // CheckRedirect nil + c := NewClient(testCreds, testAccount, WithHTTPClient(supplied)) + + // The client actually used must enforce no-follow. + if c.httpClient.CheckRedirect == nil { + t.Fatal("supplied client without CheckRedirect must get no-follow enforced on c.httpClient") + } + if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, http.ErrUseLastResponse) { + t.Errorf("enforced CheckRedirect = %v, want http.ErrUseLastResponse", got) + } + // The caller's original client must NOT be mutated: it must be a distinct copy + // and the caller's CheckRedirect must remain nil. + if c.httpClient == supplied { + t.Error("c.httpClient must be a COPY of the supplied client, not the same pointer") + } + if supplied.CheckRedirect != nil { + t.Error("the caller's original client was mutated: CheckRedirect is no longer nil") + } + // The copy must preserve the supplied Timeout. + if c.httpClient.Timeout != 5*time.Second { + t.Errorf("copied client Timeout = %v, want 5s (must preserve supplied fields)", c.httpClient.Timeout) + } +} + +// TestWithHTTPClientRespectsExplicitCheckRedirect verifies that a caller-supplied +// client that DOES set its own CheckRedirect is respected (not overridden) and +// not copied. +func TestWithHTTPClientRespectsExplicitCheckRedirect(t *testing.T) { + sentinel := errors.New("caller policy") + supplied := &http.Client{ + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return sentinel }, + } + c := NewClient(testCreds, testAccount, WithHTTPClient(supplied)) + + if c.httpClient != supplied { + t.Error("a supplied client with its own CheckRedirect must be used as-is (not copied)") + } + if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, sentinel) { + t.Errorf("CheckRedirect = %v, want the caller's own policy (%v)", got, sentinel) + } +} + +// TestCreateOutcomeAmbiguous3xxByMethod verifies that a 3xx apiError is +// UNCONFIRMED on a mutating method but NOT ambiguous on a GET. +func TestCreateOutcomeAmbiguous3xxByMethod(t *testing.T) { + for _, m := range []string{http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} { + err := &apiError{Method: m, Path: "/x", StatusCode: http.StatusFound} + if !createOutcomeAmbiguous(err) { + t.Errorf("createOutcomeAmbiguous(302 %s) = false, want true (UNCONFIRMED)", m) + } + } + getErr := &apiError{Method: http.MethodGet, Path: "/x", StatusCode: http.StatusFound} + if createOutcomeAmbiguous(getErr) { + t.Error("createOutcomeAmbiguous(302 GET) = true, want false (a GET redirect is not a create)") + } +} From a452eceb66389b478fc8400e2942a66e422c15fe Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 10:12:22 -0700 Subject: [PATCH 05/15] fix(review): enforce no-follow unconditionally; align docs with code (PR #27) Address Copilot follow-ups on the redirect fix: - override CheckRedirect UNCONDITIONALLY, including a caller-supplied client that sets its own callback: a callback that returns nil (or follows N hops then stops) would still follow a redirect and re-open the pre-send hole, so no-follow is a correctness requirement, not a default. The caller's client is copied, never mutated. - restore NewClient's Go doc (the noFollow helper had displaced it) and give noFollow its own doc - correct log.md and the reddit concept doc to match the implementation: tls.RecordHeaderError is excluded from isPreSendDialError (not pre-send), and a 3xx on a mutating request is UNCONFIRMED (not a clean not-created failure) Signed-off-by: Misha Rautela --- .../code/internal-platform-reddit.md | 28 ++++++++------- docs/knowledge/log.md | 24 +++++++------ internal/platform/reddit/client.go | 35 ++++++++++++------- internal/platform/reddit/client_test.go | 28 +++++++++------ 4 files changed, 68 insertions(+), 47 deletions(-) diff --git a/docs/knowledge/code/internal-platform-reddit.md b/docs/knowledge/code/internal-platform-reddit.md index 726e7e94..219b16d4 100644 --- a/docs/knowledge/code/internal-platform-reddit.md +++ b/docs/knowledge/code/internal-platform-reddit.md @@ -46,17 +46,21 @@ 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, -connection-refused/no-route, and TLS handshake/certificate errors. The TLS -branches are sound ONLY because the built-in `*http.Client` disables redirect -following (`CheckRedirect` returns `http.ErrUseLastResponse`); otherwise a POST -could be received by Reddit and then redirected to a TLS-broken target, whose -cert error would be misread as pre-send and let a retry duplicate a paid -resource. With redirects disabled, a disallowed 3xx surfaces as a non-2xx -`apiError` (< 500), i.e. a clean not-created failure — Reddit never legitimately -3xx-redirects these calls. Everything else is treated as UNCONFIRMED (may have -been applied): 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. +connection-refused/no-route, and a TLS `*tls.CertificateVerificationError` +(a handshake-time event). `tls.RecordHeaderError` is deliberately NOT treated as +pre-send — it can surface post-negotiation while reading a response, so it does +not prove the request was unsent and flows to the UNCONFIRMED path instead. The +`CertificateVerificationError` branch is sound ONLY because redirect following is +force-disabled on every client used, including one supplied via `WithHTTPClient` +(`CheckRedirect` is overridden to `http.ErrUseLastResponse` on a shallow copy, so +the caller's client is not mutated); otherwise a POST could be received by Reddit +and then redirected to a TLS-broken target, whose cert error would be misread as +pre-send and let a retry duplicate a paid resource. Everything else 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). diff --git a/docs/knowledge/log.md b/docs/knowledge/log.md index e0d5b203..f9b1f69a 100644 --- a/docs/knowledge/log.md +++ b/docs/knowledge/log.md @@ -3,17 +3,19 @@ ## 2026-07-15 **Update** — Hardened the Reddit Ads client's ambiguous-outcome classification -(PR #27): the built-in `*http.Client` now disables redirect following -(`CheckRedirect` returns `http.ErrUseLastResponse`), so a TLS -certificate/record error can only come from the ORIGINAL request's handshake. -This makes `isPreSendDialError`'s TLS branches sound — otherwise a mutating POST -could be received then redirected to a TLS-broken target and misclassified as -pre-send, duplicating a paid resource on retry. A disallowed 3xx now surfaces as -a non-2xx `apiError` (< 500), i.e. a clean not-created failure. Documented that -`isPreSendDialError` proves pre-send only for DNS/dial/TLS-with-redirects- -disabled, while context errors and 5xx/mid-flight transport failures stay -UNCONFIRMED. Reworded the manual-fallback UTM step to SET/REPLACE the utm_* -params (matching `buildRedditUTMURL`'s `url.Values.Set`). +(PR #27): redirect following is force-disabled on every client used, including +one supplied via `WithHTTPClient` (`CheckRedirect` is overridden to +`http.ErrUseLastResponse` on a shallow copy, so the caller's client is not +mutated). This makes `isPreSendDialError`'s `*tls.CertificateVerificationError` +branch sound — a cert error then proves the ORIGINAL request's handshake failed +pre-send, since no redirect could carry an already-sent POST to a TLS-broken +target. `tls.RecordHeaderError` was REMOVED from `isPreSendDialError`: it can +surface post-negotiation while reading a response, so it does not prove pre-send +and now flows to the UNCONFIRMED path. 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 diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 5d3be7a4..235eec9f 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -267,14 +267,20 @@ type tokenRefresh struct { // by Reddit — which could duplicate a paid resource on retry. Returning // ErrUseLastResponse stops following and hands the 3xx response back to request(), // where a non-2xx status is surfaced as an error (see the StatusCode check in -// request()). This makes isPreSendDialError's CertificateVerificationError branch -// sound: a cert error then proves the ORIGINAL request's handshake failed -// pre-send. It is shared by the built-in client and the caller-supplied-client -// enforcement in NewClient so there is a single definition. +// noFollow is the CheckRedirect policy for every client this package uses: it +// stops the client from following redirects (returning the 3xx as-is). This +// keeps isPreSendDialError's CertificateVerificationError branch sound — a cert +// error then proves the ORIGINAL request's handshake failed pre-send, because no +// redirect could have carried the request to a different (TLS-broken) target +// after it was already sent. 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 the pre-send +// error classification stays sound. func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client { c := &Client{ creds: creds, @@ -291,15 +297,18 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client for _, opt := range opts { opt(c) } - // Enforce the no-follow redirect policy on whatever client ended up on - // c.httpClient — INCLUDING one supplied via WithHTTPClient, which replaces the - // default above. A supplied client that follows redirects would let the original - // mutating POST be committed and then a TLS failure at the redirect target be - // misclassified as pre-send. A supplied client that doesn't set CheckRedirect - // gets no-follow enforced via a SHALLOW COPY (so the caller's *http.Client is - // never mutated — it may be reused elsewhere); a supplied client with its own - // CheckRedirect is respected and left untouched. - if c.httpClient != nil && c.httpClient.CheckRedirect == nil { + // 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 diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 27bf0b99..651d496d 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -3725,21 +3725,27 @@ func TestWithHTTPClientEnforcesNoFollowWithoutMutatingCaller(t *testing.T) { } } -// TestWithHTTPClientRespectsExplicitCheckRedirect verifies that a caller-supplied -// client that DOES set its own CheckRedirect is respected (not overridden) and -// not copied. -func TestWithHTTPClientRespectsExplicitCheckRedirect(t *testing.T) { +// TestWithHTTPClientOverridesExplicitCheckRedirect verifies that no-follow is +// enforced UNCONDITIONALLY: a caller-supplied client that sets its OWN +// CheckRedirect is still overridden to noFollow (a caller callback that returns +// nil, or follows N hops then stops, would re-open the pre-send hole), and the +// caller's original client is not mutated. +func TestWithHTTPClientOverridesExplicitCheckRedirect(t *testing.T) { sentinel := errors.New("caller policy") - supplied := &http.Client{ - CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return sentinel }, - } + callerPolicy := func(_ *http.Request, _ []*http.Request) error { return sentinel } + supplied := &http.Client{CheckRedirect: callerPolicy} c := NewClient(testCreds, testAccount, WithHTTPClient(supplied)) - if c.httpClient != supplied { - t.Error("a supplied client with its own CheckRedirect must be used as-is (not copied)") + // The client we USE must enforce no-follow, not the caller's follow-capable policy. + if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, http.ErrUseLastResponse) { + t.Errorf("CheckRedirect = %v, want http.ErrUseLastResponse (no-follow enforced)", got) + } + // The caller's original client must be untouched (copy, not mutate). + if c.httpClient == supplied { + t.Error("supplied client must be copied, not used in place, when overriding CheckRedirect") } - if got := c.httpClient.CheckRedirect(nil, nil); !errors.Is(got, sentinel) { - t.Errorf("CheckRedirect = %v, want the caller's own policy (%v)", got, sentinel) + if got := supplied.CheckRedirect(nil, nil); !errors.Is(got, sentinel) { + t.Error("the caller's original client was mutated: its CheckRedirect no longer returns the caller policy") } } From 63d6c896311722ea5885b285c988fc13eed42557 Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 10:28:06 -0700 Subject: [PATCH 06/15] fix(review): drop TLS from pre-send classification, matching Meta (PR #27) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 cert/record error while reading a response after forwarding the POST. Align with the merged Meta client and remove all TLS handling from isPreSendDialError — only DNS and connect-time dial failures now prove pre-send; every TLS failure flows to the UNCONFIRMED path (safe: never lets a retry duplicate a paid resource). Redirect following stays force-disabled so 3xx handling is well-defined, but is no longer relied on to make a TLS error a pre-send proof. Also collapse the duplicated noFollow/NewClient doc comment and align the log + concept doc with the code. Signed-off-by: Misha Rautela --- .../code/internal-platform-reddit.md | 35 ++++----- docs/knowledge/log.md | 29 +++---- internal/platform/reddit/client.go | 75 ++++++++----------- internal/platform/reddit/client_test.go | 11 +-- 4 files changed, 72 insertions(+), 78 deletions(-) diff --git a/docs/knowledge/code/internal-platform-reddit.md b/docs/knowledge/code/internal-platform-reddit.md index 219b16d4..d5fc3ea5 100644 --- a/docs/knowledge/code/internal-platform-reddit.md +++ b/docs/knowledge/code/internal-platform-reddit.md @@ -45,22 +45,23 @@ 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, -connection-refused/no-route, and a TLS `*tls.CertificateVerificationError` -(a handshake-time event). `tls.RecordHeaderError` is deliberately NOT treated as -pre-send — it can surface post-negotiation while reading a response, so it does -not prove the request was unsent and flows to the UNCONFIRMED path instead. The -`CertificateVerificationError` branch is sound ONLY because redirect following is -force-disabled on every client used, including one supplied via `WithHTTPClient` -(`CheckRedirect` is overridden to `http.ErrUseLastResponse` on a shallow copy, so -the caller's client is not mutated); otherwise a POST could be received by Reddit -and then redirected to a TLS-broken target, whose cert error would be misread as -pre-send and let a retry duplicate a paid resource. Everything else 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. +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). diff --git a/docs/knowledge/log.md b/docs/knowledge/log.md index f9b1f69a..bbd439e0 100644 --- a/docs/knowledge/log.md +++ b/docs/knowledge/log.md @@ -3,19 +3,22 @@ ## 2026-07-15 **Update** — Hardened the Reddit Ads client's ambiguous-outcome classification -(PR #27): redirect following is force-disabled on every client used, including -one supplied via `WithHTTPClient` (`CheckRedirect` is overridden to -`http.ErrUseLastResponse` on a shallow copy, so the caller's client is not -mutated). This makes `isPreSendDialError`'s `*tls.CertificateVerificationError` -branch sound — a cert error then proves the ORIGINAL request's handshake failed -pre-send, since no redirect could carry an already-sent POST to a TLS-broken -target. `tls.RecordHeaderError` was REMOVED from `isPreSendDialError`: it can -surface post-negotiation while reading a response, so it does not prove pre-send -and now flows to the UNCONFIRMED path. 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`). +(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 diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 235eec9f..a9bd5bc2 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -14,7 +14,6 @@ package reddit import ( "bytes" "context" - "crypto/tls" "encoding/base64" "encoding/json" "errors" @@ -265,22 +264,22 @@ type tokenRefresh struct { // 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 -// ErrUseLastResponse stops following and hands the 3xx response back to request(), -// where a non-2xx status is surfaced as an error (see the StatusCode check in // noFollow is the CheckRedirect policy for every client this package uses: it -// stops the client from following redirects (returning the 3xx as-is). This -// keeps isPreSendDialError's CertificateVerificationError branch sound — a cert -// error then proves the ORIGINAL request's handshake failed pre-send, because no -// redirect could have carried the request to a different (TLS-broken) target -// after it was already sent. It is shared by the built-in client and the -// caller-supplied-client enforcement in NewClient so there is a single definition. +// 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 the pre-send -// error classification stays sound. +// 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, @@ -694,31 +693,20 @@ func isMutatingMethod(method string) bool { // 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); -// - TLS certificate verification failure (the secure channel was never -// established, so no request body was sent). +// - connection refused / no route / network unreachable (never connected). // -// Note: tls.RecordHeaderError is deliberately NOT treated as pre-send. Unlike a -// cert verification failure, a RecordHeaderError does not prove a handshake-time -// failure: crypto/tls can also return it AFTER a version has been negotiated, -// when a later record carries an invalid header/version — including while READING -// THE RESPONSE after this POST was already sent. So even with redirects disabled, -// a RecordHeaderError does not prove pre-send; classifying it as pre-send could -// report a possibly-created paid resource as definitely absent, duplicating it on -// retry. It therefore falls through to the transportError wrapping in request(), +// 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. -// -// The CertificateVerificationError branch is sound ONLY because the built-in -// client disables redirect following (CheckRedirect returns -// http.ErrUseLastResponse in NewClient) — a policy also enforced on a -// caller-supplied client via a shallow copy in NewClient. Were redirects -// followed, a mutating POST could be RECEIVED by Reddit, then redirected to a -// TLS-broken target; the resulting cert error would match here and be misreported -// as pre-send, letting a retry duplicate a paid resource. With redirects -// disabled, a cert verification failure is a handshake-time event that can only -// come from the ORIGINAL request's handshake, so it genuinely proves no request -// body was sent. +// 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), @@ -739,15 +727,16 @@ func isPreSendDialError(err error) bool { 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 - } - // TLS certificate verification failure: a handshake-time event, so the secure - // channel was never established and no request bytes were sent. (RecordHeaderError - // is intentionally excluded — see the doc comment: it can surface post-negotiation - // while reading a response, so it does not prove pre-send.) - var certErr *tls.CertificateVerificationError - return errors.As(err, &certErr) + 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 diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 651d496d..f577ea18 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -1046,7 +1046,6 @@ func TestIsPreSendDialError(t *testing.T) { &net.DNSError{Err: "no such host", Name: "x"}, syscall.ECONNREFUSED, syscall.EHOSTUNREACH, - &tls.CertificateVerificationError{}, } for _, e := range preSend { if !isPreSendDialError(e) { @@ -1056,10 +1055,12 @@ func TestIsPreSendDialError(t *testing.T) { notPreSend := []error{ io.ErrUnexpectedEOF, // mid-flight after a connection was established errors.New("some other error"), - // TLS record-header failure is NOT pre-send: crypto/tls can return it AFTER a - // version has been negotiated, when a later record has an invalid header — - // including while READING THE RESPONSE after the POST was sent. So it doesn't - // prove pre-send and must be treated as ambiguous (UNCONFIRMED). + // No TLS error is pre-send (matching the merged Meta client): a custom + // transport can enable renegotiation, and a wrapping/retrying RoundTripper can + // surface a cert error while reading a response AFTER forwarding the POST — so + // neither a cert-verification nor a record-header failure proves pre-send. Both + // flow to the ambiguous (UNCONFIRMED) path. + &tls.CertificateVerificationError{}, tls.RecordHeaderError{Msg: "bad record"}, // Context errors are NOT pre-send: the per-attempt ctx wraps the whole round // trip, so a ctx error can surface after the POST reached Reddit. They must be From 8fcaf3c6290c8ff1234cbce84ceec06fa12ae67c Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 10:38:43 -0700 Subject: [PATCH 07/15] docs(review): purge stale TLS-as-pre-send references (PR #27) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up: after removing TLS from isPreSendDialError, several comments and docs still described TLS handshake/cert errors as pre-send. Align them all with the code — only DNS and connect-time dial failures prove pre-send: - createOutcomeAmbiguous doc, the redirect comments, and the TestIsPreSend header now say DNS/dial only; TLS is listed among the ambiguous cases - the concept doc narrows "not proven pre-send" to failures that prove neither pre-send nor rejection, so a definite 4xx is a clean rejection, not UNCONFIRMED Signed-off-by: Misha Rautela --- .../code/internal-platform-reddit.md | 18 ++++++----- internal/platform/reddit/client.go | 31 +++++++++---------- internal/platform/reddit/client_test.go | 9 ++++-- 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/docs/knowledge/code/internal-platform-reddit.md b/docs/knowledge/code/internal-platform-reddit.md index d5fc3ea5..1013e58e 100644 --- a/docs/knowledge/code/internal-platform-reddit.md +++ b/docs/knowledge/code/internal-platform-reddit.md @@ -55,13 +55,15 @@ 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. +client is not mutated), which keeps 3xx handling well-defined. Failures that +prove NEITHER pre-send NOR rejection are 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. A definite 4xx is NOT UNCONFIRMED — Reddit +received and REJECTED the request, so nothing was created and the caller gets a +clean failure. See [internal/platform/reddit](../../../internal/platform/reddit). diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index a9bd5bc2..8e326ef9 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -260,10 +260,11 @@ 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 +// 3xx-redirects these calls. Following a redirect on a mutating POST would take +// the request to a different target and complicate outcome classification even +// though the original POST may already have been received by Reddit; not +// following keeps a 3xx a well-defined non-2xx (UNCONFIRMED for a mutating +// request). 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 @@ -298,15 +299,12 @@ func NewClient(creds Credentials, account AccountConfig, opts ...Option) *Client } // 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. + // 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: even a caller-supplied CheckRedirect is + // overridden (a callback that returns nil, or follows N hops then stops, would + // still follow). The override is applied to a SHALLOW COPY so the caller's + // *http.Client is never mutated (it may be reused elsewhere). if c.httpClient != nil { hc := *c.httpClient hc.CheckRedirect = noFollow @@ -639,9 +637,10 @@ 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/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 +// (see isPreSendDialError — only a DNS or connect-time dial failure is NOT +// wrapped as transportError, so it never reaches here; every TLS error IS +// wrapped and so is treated as ambiguous), 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 diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index f577ea18..dcf2d5a5 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -1030,9 +1030,12 @@ func TestCreateCampaign_ConnRefusedNotUnconfirmed(t *testing.T) { } // TestIsPreSendDialError classifies which Do errors PROVE the request was NOT -// sent (so a create is NOT ambiguous): DNS, connection-refused/no-route, and TLS -// handshake/certificate failures — in every one of these the connection or secure -// channel was never established, so no request bytes could have reached Reddit. +// sent (so a create is NOT ambiguous): only DNS resolution and connect-time dial +// failures (connection-refused/no-route/network-unreachable) — in these the +// connection was never established, so no request bytes could have reached Reddit. +// TLS errors are deliberately AMBIGUOUS (in notPreSend below), matching the merged +// Meta client: they aren't a reliable pre-send proof for an arbitrary supplied +// transport. // // A context cancellation/deadline is deliberately NOT pre-send: the per-attempt // context wraps the whole round trip, so a ctx error from Do can fire AFTER the From aed9eb4665fc50c3256a20885f231550e8f9bdbf Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 10:45:59 -0700 Subject: [PATCH 08/15] docs(review): fix comment debris from TLS-classification cleanup (PR #27) - collapse the doubled noFollow doc comment (dangling "Returning" fragment) - correct the createOutcomeAmbiguous comment: a pre-connect DNS/dial failure is pre-send, not a "dial/cert-verification error" - remove the unreachable NOTE after the return in isPreSendDialError (the doc comment already explains the TLS exclusion) - add syscall.ENETUNREACH to the TestIsPreSendDialError preSend table so every branch the function handles is covered Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 38 +++++++------------------ internal/platform/reddit/client_test.go | 1 + 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 8e326ef9..c4d2a44c 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -257,23 +257,16 @@ type tokenRefresh struct { err error } -// 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 take -// the request to a different target and complicate outcome classification even -// though the original POST may already have been received by Reddit; not -// following keeps a 3xx a well-defined non-2xx (UNCONFIRMED for a mutating -// request). 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. +// an error (a 3xx on a mutating request is then classified UNCONFIRMED). The +// Reddit Ads API returns JSON directly and never legitimately 3xx-redirects these +// calls; not following keeps outcome classification simple — a create gets a 2xx, +// a definite 4xx, or an UNCONFIRMED (transport/5xx/3xx-mutating) result, and 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 } @@ -654,10 +647,10 @@ func (e *transportError) Unwrap() error { return e.Err } // GET carries no create and is NOT ambiguous. // // 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". +// pre-send failure (token refresh, body encode/build, a pre-connect DNS/dial +// failure, 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) { @@ -727,15 +720,6 @@ func isPreSendDialError(err error) bool { return true } 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 diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index dcf2d5a5..44201aad 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -1049,6 +1049,7 @@ func TestIsPreSendDialError(t *testing.T) { &net.DNSError{Err: "no such host", Name: "x"}, syscall.ECONNREFUSED, syscall.EHOSTUNREACH, + syscall.ENETUNREACH, } for _, e := range preSend { if !isPreSendDialError(e) { From d4e4fb15ba6bab99f86de0354d84dde8fccba2da Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 10:51:20 -0700 Subject: [PATCH 09/15] fix(review): operator keeps ALL other query params, not just non-utm_* (PR #27) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildRedditUTMURL uses url.Values.Set, which replaces only the exact utm_* keys it generates and preserves every other query parameter — including a foreign utm_* like utm_id the tool doesn't set. The operator instruction wrongly said to keep only non-utm_* params, which would drop utm_id and diverge from the automated destination. Reword to keep ALL other query parameters; update the comment and the test assertion to match. Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 9 +++++---- internal/platform/reddit/client_test.go | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index c4d2a44c..4fcd5a2d 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -1520,10 +1520,11 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig // 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)) + // with url.Values.Set (see buildRedditUTMURL), which REPLACES only the exact + // utm_* keys it sets and PRESERVES every other query parameter (including any + // utm_* the tool doesn't generate, e.g. utm_id) — so the operator keeps all + // other params; appending instead would leave duplicate/conflicting 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 all its other query parameters):", variantCount)) for i := 0; i < variantCount; i++ { steps = append(steps, fmt.Sprintf(" Variant %d: %q -> %s", i+1, in.Variants[i].Headline, displayRedditUTMURL(in, i))) } diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 44201aad..4ad0e6ea 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -654,7 +654,7 @@ func TestCreateCampaign_ManualVariantsNoPostURL(t *testing.T) { // still pass). for _, want := range []string{ "set/replace the shown utm_* params", - "keeping only its other, non-utm_* query params", + "keeping all its other query parameters", } { if !strings.Contains(allSteps, want) { t.Errorf("operator instruction missing new phrase %q; got:\n%s", want, allSteps) From 64fca2e412ded8c60283e4f208597f22411d77c2 Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 10:58:52 -0700 Subject: [PATCH 10/15] docs(review): align remaining ambiguity-contract comments with code (PR #27) - define transportError as "not proven pre-send / may have been sent" (a TLS error routes through it and can fail before OR after bytes are written), not "ALREADY SENT" - add "mutating 3xx" to the createOutcomeAmbiguous contract comments that previously enumerated only transportError/5xx - fix the test comment that still said "keep only non-utm_* params" (all other query params are preserved, including an ungenerated utm_id) - reword the redirect test's rationale to an unreachable DNS/dial target, since TLS errors are no longer classified pre-send Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 28 ++++++++++++++----------- internal/platform/reddit/client_test.go | 19 ++++++++++------- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 4fcd5a2d..6c220d48 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -607,13 +607,16 @@ func (e *apiError) Error() string { return fmt.Sprintf("reddit API %s %s -> %d", e.Method, e.Path, e.StatusCode) } -// transportError wraps a failure of the HTTP round-trip itself (httpClient.Do): -// the request was ALREADY SENT, so the server may or may not have processed it — -// the outcome is AMBIGUOUS. This is distinct from a pre-send failure (token -// refresh, body encode, request build) or a definite abort, where the request -// never reached the server and a mutation definitely did not happen. Callers use -// it to decide whether a failed create is "may exist" (ambiguous) vs "not -// created". +// transportError wraps a failure of the HTTP round-trip itself (httpClient.Do) +// that is NOT PROVEN pre-send: the request MAY have been sent (e.g. a mid-flight +// read failure, a Do-time context error, OR a TLS error, which can surface either +// during the handshake before bytes are written or later while reading a +// response), so the server may or may not have processed it — the outcome is +// AMBIGUOUS. This is distinct from a proven pre-send failure (token refresh, body +// encode, request build, or a DNS/connect-time dial error via isPreSendDialError), +// where the request definitely never reached the server and a mutation definitely +// did not happen. Callers use it to decide whether a failed create is "may exist" +// (ambiguous) vs "not created". type transportError struct { Method string Path string @@ -1259,8 +1262,9 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig // Classify AMBIGUITY FIRST, before the ctx check: a cancellation that // interrupts the create's in-flight round-trip surfaces as a transportError, // and the campaign MAY already have been committed — so it must be treated as - // "may exist", NOT a clean pre-POST abort. createOutcomeAmbiguous is true only - // when the request plausibly reached Reddit (transportError or a 5xx). + // "may exist", NOT a clean pre-POST abort. createOutcomeAmbiguous is true when + // the request plausibly reached Reddit (transportError, a 5xx, or a mutating + // 3xx). if !createOutcomeAmbiguous(err) { // Not ambiguous → the campaign was definitely NOT created: a pre-send // failure (token refresh, body encode/build, a pre-connect dial error), a @@ -1373,7 +1377,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig droppedCommunities := false adGroupResp, err := c.request(ctx, http.MethodPost, "/ad_accounts/"+accountID+"/ad_groups", buildAdGroupBody(targetingWithCommunities)) // adGroupErr words an ad-group failure as UNCONFIRMED when the outcome is - // ambiguous (transportError / 5xx — the ad group MAY exist, and partialResult + // ambiguous (transportError / 5xx / mutating 3xx — the ad group MAY exist, and partialResult // carries its deterministic name for reconciliation) vs a flat "failed" for a // definite not-created error (4xx / pre-send). A caller-cancel that interrupted // the in-flight POST surfaces as a transportError, so it too is UNCONFIRMED. @@ -1448,7 +1452,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig if err != nil { // A caller context cancellation is fatal (return an error, not just a // warning). Whether the ad "may exist" depends on whether the request was - // in flight: an ambiguous error (transportError / 5xx — which is also how a + // in flight: an ambiguous error (transportError / 5xx / mutating 3xx — which is also how a // cancellation that interrupted the in-flight POST surfaces) means the ad // MAY exist; a clean pre-send cancel means it does not. Return a partial // result carrying both IDs so the (created, PAUSED) campaign+ad group are @@ -1464,7 +1468,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig return pr, fmt.Errorf("ad creation aborted before send: %w", ctxErr) } // Not a caller cancel. Classify by outcome: - // - ambiguous (transportError / 5xx): the ad MAY exist — UNCONFIRMED, + // - ambiguous (transportError / 5xx / mutating 3xx): the ad MAY exist — UNCONFIRMED, // require verification before manual creation (avoids a duplicate); // - definite (4xx / pre-send): the ad was NOT created — report FAILED so // the operator's manual remediation isn't blocked by a misleading diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 4ad0e6ea..6e829fc1 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -648,10 +648,12 @@ func TestCreateCampaign_ManualVariantsNoPostURL(t *testing.T) { } // The operator instruction must carry the NEW set/replace semantics explicitly: // the operator SETS/REPLACES the shown utm_* params on their registration URL, - // keeping only its OTHER, non-utm_* query params. Assert both distinctive - // phrases so this operator-critical guidance can't regress to the old "append ... - // keeping its existing query" wording (which the summary-count check alone would - // still pass). + // keeping ALL its other query parameters (buildRedditUTMURL's url.Values.Set + // replaces only the exact keys it generates and preserves everything else, + // including an ungenerated utm_* like utm_id). Assert both distinctive phrases so + // this operator-critical guidance can't regress to the old "append ... keeping + // its existing query" wording (which the summary-count check alone would still + // pass). for _, want := range []string{ "set/replace the shown utm_* params", "keeping all its other query parameters", @@ -3633,10 +3635,11 @@ func TestRedactURL(t *testing.T) { // TestBuiltinClientDoesNotFollowRedirectsOnPOST verifies the built-in // *http.Client disables redirect following: a mutating POST that receives a 3xx -// must NOT be followed to the redirect target (which could be TLS-broken and let -// isPreSendDialError misclassify a possibly-received POST as pre-send). The 3xx -// must instead surface as a non-2xx error, and the redirect target handler must -// never be hit. +// must NOT be followed to the redirect target (which could be unreachable — an +// unresolvable/refused host — and let isPreSendDialError misclassify a +// possibly-received POST as a pre-send DNS/dial failure). The 3xx must instead +// surface as a non-2xx error (UNCONFIRMED for a mutating request), and the +// redirect target handler must never be hit. func TestBuiltinClientDoesNotFollowRedirectsOnPOST(t *testing.T) { var targetHit atomic.Bool // target is where the redirect would send the request if it were followed. From 9e72ab180547c0d5241e638f13b3abe0a28e33bd Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 11:07:06 -0700 Subject: [PATCH 11/15] docs(review): operator drops trailing slash; narrow ctx-error doc (PR #27) - the manual-ad instruction now tells the operator to drop a trailing path slash so the manual destination matches buildRedditUTMURL (which strips it) - narrow the log entry: only an in-flight-Do context error is UNCONFIRMED; a cancellation during token refresh is a proven pre-POST failure and stays non-ambiguous Signed-off-by: Misha Rautela --- docs/knowledge/log.md | 9 +++++++-- internal/platform/reddit/client.go | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/knowledge/log.md b/docs/knowledge/log.md index bbd439e0..bbcdac29 100644 --- a/docs/knowledge/log.md +++ b/docs/knowledge/log.md @@ -15,10 +15,15 @@ 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 +before redirecting); a 3xx on a GET is not a create. A context error surfaced +from an IN-FLIGHT `Do` stays UNCONFIRMED (the per-attempt ctx wraps the whole +round trip, so it can fire after the POST reached Reddit) — but a cancellation +returned while waiting for token refresh is a proven pre-POST failure +(`refreshToken` returns `ctx.Err()` directly) and remains non-ambiguous. 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`). +`buildRedditUTMURL`'s `url.Values.Set`), keeping all other query params and +dropping a trailing path slash. ## 2026-07-13 diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 6c220d48..14f4904a 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -1528,7 +1528,7 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig // utm_* keys it sets and PRESERVES every other query parameter (including any // utm_* the tool doesn't generate, e.g. utm_id) — so the operator keeps all // other params; appending instead would leave duplicate/conflicting 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 all its other query parameters):", variantCount)) + 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 all its other query parameters; drop any trailing '/' from the path so it matches the automated destination):", variantCount)) for i := 0; i < variantCount; i++ { steps = append(steps, fmt.Sprintf(" Variant %d: %q -> %s", i+1, in.Variants[i].Headline, displayRedditUTMURL(in, i))) } From 1de8870dfac74345ccd880bbd5cf4813071255d7 Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 11:15:17 -0700 Subject: [PATCH 12/15] docs(review): transportError bullet + concept-doc "classified" wording (PR #27) - reword the createOutcomeAmbiguous bullet: transportError is a Do failure NOT PROVEN pre-send (a TLS error can precede connection), not strictly "after a connection was established" - concept doc: a 5xx is returned as apiError and CLASSIFIED by status, not "wrapped"; only transportError-path failures are wrapped Signed-off-by: Misha Rautela --- docs/knowledge/code/internal-platform-reddit.md | 8 +++++--- internal/platform/reddit/client.go | 10 +++++----- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/knowledge/code/internal-platform-reddit.md b/docs/knowledge/code/internal-platform-reddit.md index 1013e58e..6fa22fb4 100644 --- a/docs/knowledge/code/internal-platform-reddit.md +++ b/docs/knowledge/code/internal-platform-reddit.md @@ -60,9 +60,11 @@ prove NEITHER pre-send NOR rejection are 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. A definite 4xx is NOT UNCONFIRMED — Reddit +trip, so it can fire after the POST reached Reddit), and a read/decode failure on +a 2xx body are wrapped as `transportError`; a 5xx status is returned as an +`apiError` and classified by status. `createOutcomeAmbiguous` treats all of these +as "may exist", so callers require verification before a manual retry. A definite +4xx is NOT UNCONFIRMED — Reddit received and REJECTED the request, so nothing was created and the caller gets a clean failure. diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 14f4904a..98da9002 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -632,11 +632,11 @@ func (e *transportError) Unwrap() error { return e.Err } // applied by Reddit despite the error — i.e. the request plausibly reached the // 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 — only a DNS or connect-time dial failure is NOT -// wrapped as transportError, so it never reaches here; every TLS error IS -// wrapped and so is treated as ambiguous), so the request may have been -// received. This is ALSO the path a context +// - transportError: a Do failure that is NOT PROVEN pre-send (see +// isPreSendDialError — only a DNS or connect-time dial failure is NOT wrapped +// as transportError, so it never reaches here; every TLS error and any other +// unclassified Do error IS wrapped and so is treated as ambiguous), so the +// request MAY have been sent and 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 From 35aa89492dc2a8f8f8c235e96537f40c227ad819 Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 11:19:39 -0700 Subject: [PATCH 13/15] test(review): pin the trailing-slash operator instruction (PR #27) The manual-ad instruction test asserted the utm set/replace and query-preservation phrases but not the new "drop any trailing '/'" guidance, so removing it would pass silently and let manual destinations diverge from buildRedditUTMURL again. Add the phrase to the expected list. Signed-off-by: Misha Rautela --- internal/platform/reddit/client_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/platform/reddit/client_test.go b/internal/platform/reddit/client_test.go index 6e829fc1..d900fc4a 100644 --- a/internal/platform/reddit/client_test.go +++ b/internal/platform/reddit/client_test.go @@ -657,6 +657,7 @@ func TestCreateCampaign_ManualVariantsNoPostURL(t *testing.T) { for _, want := range []string{ "set/replace the shown utm_* params", "keeping all its other query parameters", + "drop any trailing '/' from the path", } { if !strings.Contains(allSteps, want) { t.Errorf("operator instruction missing new phrase %q; got:\n%s", want, allSteps) From da40acca7a13b16014acbbbfeed47341c5efe907 Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 15:44:05 -0700 Subject: [PATCH 14/15] docs(reddit): note shared no-follow policy covers fetchToken Add a comment at fetchToken's Do call noting that c.httpClient carries the package-wide no-follow CheckRedirect policy, so a future split of the token client onto its own *http.Client should carry the same policy forward (per @MRashad26). Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index 98da9002..e9206119 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -518,6 +518,11 @@ func (c *Client) fetchToken(ctx context.Context) (string, error) { req.Header.Set("Content-Type", "application/x-www-form-urlencoded") req.Header.Set("User-Agent", redditUserAgent) + // c.httpClient carries the package-wide no-follow CheckRedirect policy (see + // noFollow's doc comment), so a redirect from the token endpoint surfaces here + // as a 3xx status rather than being followed transparently. Reddit's token + // endpoint does not redirect in practice; if a future split of the token client + // onto its own *http.Client is ever introduced, carry the same policy forward. resp, err := c.httpClient.Do(req) if err != nil { return "", fmt.Errorf("reddit token refresh: %w", err) From ec3afcbc4cc934b769921ed3139e26e18b40f02f Mon Sep 17 00:00:00 2001 From: Misha Rautela Date: Wed, 15 Jul 2026 15:51:41 -0700 Subject: [PATCH 15/15] fix(reddit): correct stale TLS/ambiguity comments in client.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review: - client.go:805 (request): reworded the post-Do classification comment — a TLS handshake failure can occur before HTTP bytes are sent, so the comment no longer implies only post-connection failures land in the ambiguous branch; it now says "not proven pre-send / may have been sent" for every non-dial Do error, matching isPreSendDialError. - client.go:1263 (CreateCampaign): removed a stale, contradictory, mid-sentence paragraph that both claimed a caller cancellation is never ambiguous and said to classify ambiguity first; kept a single unambiguous rule (ambiguity-first, with the earlier-step propagation case folded into one sentence). Resolves 2 review threads. Signed-off-by: Misha Rautela --- internal/platform/reddit/client.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/platform/reddit/client.go b/internal/platform/reddit/client.go index e9206119..91d56b79 100644 --- a/internal/platform/reddit/client.go +++ b/internal/platform/reddit/client.go @@ -801,10 +801,10 @@ func (c *Client) request(ctx context.Context, method, path string, body any) (*a cancel() // A Do error that clearly happened BEFORE the request could be sent (DNS // failure, connection refused, no route) means NOT sent — return it plain - // so callers treat the create as "not applied". A failure after a - // connection was established (mid-flight timeout, EOF) is genuinely - // ambiguous: wrap it as transportError so callers treat the create as - // "may exist". + // so callers treat the create as "not applied". Every other Do error + // (TLS handshake failure, mid-flight timeout, EOF) is not proven pre-send + // / may have been sent: wrap it as transportError so callers treat the + // create as "may exist". if isPreSendDialError(err) { return nil, fmt.Errorf("reddit API %s %s: %w", method, path, err) } @@ -1260,16 +1260,14 @@ func (c *Client) CreateCampaign(ctx context.Context, in CampaignInput) (*Campaig campaignResp, err := c.request(ctx, http.MethodPost, "/ad_accounts/"+accountID+"/campaigns", map[string]any{"data": campaignData}) if err != nil { - // A CALLER context cancellation (even one that surfaced as a transportError - // during an EARLIER step like account verification, then propagated here) is - // NOT an ambiguous create: no campaign POST completed. Honor the documented - // pre-POST (nil, err) contract rather than returning a misleading "may exist" // Classify AMBIGUITY FIRST, before the ctx check: a cancellation that - // interrupts the create's in-flight round-trip surfaces as a transportError, + // interrupts THIS create's in-flight round-trip surfaces as a transportError, // and the campaign MAY already have been committed — so it must be treated as // "may exist", NOT a clean pre-POST abort. createOutcomeAmbiguous is true when // the request plausibly reached Reddit (transportError, a 5xx, or a mutating - // 3xx). + // 3xx). A caller cancellation that surfaced as a transportError during an + // EARLIER step (e.g. account verification) and merely propagated here, with no + // campaign POST attempted, falls through to the non-ambiguous branch below. if !createOutcomeAmbiguous(err) { // Not ambiguous → the campaign was definitely NOT created: a pre-send // failure (token refresh, body encode/build, a pre-connect dial error), a