fix(reddit): narrow pre-send classification; clarify manual-ad destination#27
Open
mrautela365 wants to merge 15 commits into
Open
fix(reddit): narrow pre-send classification; clarify manual-ad destination#27mrautela365 wants to merge 15 commits into
mrautela365 wants to merge 15 commits into
Conversation
…ation 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 <mrautela@linuxfoundation.org>
PR SummaryMedium Risk Overview
Redirect following is disabled everywhere: new Manual variant steps tell operators to set/replace the shown Docs/knowledge updated; tests cover pre-send vs ambiguous errors, in-flight ctx cancel → UNCONFIRMED, redirect policy, 3xx-by-method, and manual-variant wording/secret stripping. Reviewed by Cursor Bugbot for commit e587090. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a36b8a. Configure here.
There was a problem hiding this comment.
Pull request overview
Refines Reddit Ads failure classification and manual-ad destination guidance.
Changes:
- Classifies TLS and context errors as pre-send failures.
- Clarifies manual UTM instructions.
- Adds classifier tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
internal/platform/reddit/client.go |
Updates error classification and manual-ad steps. |
internal/platform/reddit/client_test.go |
Adds pre-send classification tests. |
…-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 <mrautela@linuxfoundation.org>
dealako
requested changes
Jul 14, 2026
dealako
left a comment
dealako
left a comment
Contributor
There was a problem hiding this comment.
Hi @mrautela365 👋 — nice follow-up. The context-cancellation correction is exactly right: because the per-attempt context.WithTimeout wraps the whole round trip, keeping Do-time ctx errors ambiguous (UNCONFIRMED) instead of pre-send FAILED is the safe call, and the new TestCreateCampaign_CtxCancelDuringCampaignPostIsUnconfirmed (clean under -race, no goroutine leak — the LIFO close(serverDone) before apiSrv.Close() is well done) pins it down. Prior-round Copilot/Cursor items on the ctx branch are resolved in 87447ae.
The TLS branch, however, reintroduces the same misclassification risk on the other side of the round trip, which is why I'm requesting changes.
Revision tracking
- ✅ Resolved — ctx errors no longer classified pre-send (Cursor High / Copilot); test table flipped; regression test added. (87447ae)
- ✅ Resolved —
tls.RecordHeaderErrornow covered inTestIsPreSendDialError. (87447ae) - ✅ Resolved — manual-variant path now has
TestCreateCampaign_ManualVariantsNoPostURLproving the registration-URL secret is stripped. (87447ae) I independently confirmeddisplayRedditUTMURLrebuildsRawQueryfrom the allowlist (client.go:1705) and nils userinfo/fragment — secret stripping is correct. - 🔴 Still open — TLS pre-send classification is unsound while redirects are followed (Copilot,
client.go:664). See inline.
Issue count
- 🔴 Blocking: 1 — TLS/dial pre-send classification defeated by default redirect-following → double-create risk on a paid resource.
- 🟡 Minor: 3 —
tls.RecordHeaderErroris not exclusively pre-handshake; manual-variant "append" guidance diverges from the automatedQuery.Setsemantics; PR description states ctx errors are pre-send, contradicting the implementation. - ⚪ Nit: 1 — knowledge-base update convention not followed for a notable outcome-classification fix.
Final decision: 🔴 Needs changes before approval
The redirect/pre-send interaction is the crux — it's cheap to make sound (CheckRedirect = http.ErrUseLastResponse) and directly protects the idempotency guarantee this whole change exists to uphold. Everything else is minor/nit.
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 <mrautela@linuxfoundation.org>
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 <mrautela@linuxfoundation.org>
…(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 <mrautela@linuxfoundation.org>
…27) 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 <mrautela@linuxfoundation.org>
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 <mrautela@linuxfoundation.org>
…* (PR #27) 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 <mrautela@linuxfoundation.org>
#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 <mrautela@linuxfoundation.org>
…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 <mrautela@linuxfoundation.org>
…g (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 <mrautela@linuxfoundation.org>
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 <mrautela@linuxfoundation.org>
MRashad26
reviewed
Jul 15, 2026
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 <mrautela@linuxfoundation.org>
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 <mrautela@linuxfoundation.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #21 (merged), addressing the two Copilot items that landed after the approval.
Changes
isPreSendDialErrorclassifies ONLY DNS resolution and connect-time dial failures (connection-refused/no-route/network-unreachable) as pre-send (request NOT sent). No TLS error is treated as pre-send (matching the merged Meta client): a TLS error isn't 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 all TLS failures stay AMBIGUOUS (UNCONFIRMED) viatransportError. Context cancellation/deadline from an in-flightDoare likewise NOT pre-send — the per-attempt timeout wraps the whole round trip, so a ctx error can fire after the POST reached Reddit. Redirect following is force-disabled on every client used (CheckRedirect→http.ErrUseLastResponse, unconditionally, on a non-mutating shallow copy of a supplied client), which keeps 3xx handling well-defined; a 3xx on a mutating request is UNCONFIRMED.utm_*params (the caller's pre-existing query is omitted to avoid persisting a secret) and instruct the operator to set/replace them on their own registration URL (matchingbuildRedditUTMURL'surl.Values.Set, which replaces only the exactutm_*keys it generates and preserves EVERY other query parameter — including an ungeneratedutm_*likeutm_id; the operator is also told to drop a trailing path slash) — so the manual destination matches an automated ad's.TestIsPreSendDialError(DNS/refused → pre-send; TLS cert/record errors, unexpected-EOF, and Do-time ctx error → ambiguous) and redirect-policy tests (a 3xx on a POST is not followed and surfaces as UNCONFIRMED; a supplied client's CheckRedirect is overridden without mutating the caller's client).Verified:
gofmt,go build,go vet,golangci-lint(0 issues),go test -race ./internal/platform/reddit.🤖 Generated with Claude Code