fix(platform): type x/twitter errors and classify create outcomes#31
Conversation
PR SummaryMedium Risk Overview Error model — Non-2xx returns Create path — HTTP client — Knowledge docs and broad regression tests cover no-follow, ambiguity matrix, URL/body leak cases, and full Reviewed by Cursor Bugbot for commit 5ee8caa. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Adds typed X/Twitter Ads errors and ambiguous create-outcome handling while disabling redirects for X and Meta clients.
Changes:
- Introduces typed API and transport errors.
- Classifies uncertain promoted-tweet creation outcomes.
- Enforces no-follow redirect policies and adds tests/documentation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/platform/twitter/client.go |
Adds typed errors and outcome classification. |
internal/platform/twitter/client_test.go |
Tests errors, redirects, and create outcomes. |
internal/platform/meta/client.go |
Disables redirects and classifies 3xx outcomes. |
internal/platform/meta/client_test.go |
Tests Meta redirect and ambiguity behavior. |
docs/knowledge/log.md |
Records both behavioral changes. |
docs/knowledge/code/internal-platform-twitter.md |
Documents X client behavior. |
docs/knowledge/code/internal-platform-meta.md |
Documents Meta redirect handling. |
Address PR #31 review feedback from copilot[bot]: - internal/platform/twitter/client.go: apiError.Error() no longer renders ErrorCodes. The code field comes from an untrusted response body and nothing bounds it to a short enum token, so joining it into Error() re-opened the body-leak channel into persisted PromotedTweetWarning/Steps that this PR exists to close. Codes are now retained ONLY for internal classification (hasErrorCode); Error() renders method/path/status only, mirroring reddit. - internal/platform/twitter/client.go: parseErrorCodes now drops over-long code values (>128 chars) and caps retention at 16, so even the internally-held codes can't be inflated by a hostile body. - internal/platform/twitter/client_test.go: added TestApiError_ErrorNeverSurfacesCode (sensitive text placed in errors[].code must not appear in Error()) and TestParseErrorCodes_BoundsUntrustedBody. - docs/knowledge: corrected the twitter concept + added a log entry. Resolves 1 review thread. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review Feedback AddressedCommit: 02e7e25 Changes Made
Build, Threads Resolved1 of 1 unresolved thread addressed in this iteration. |
Address PR #31 review feedback from copilot[bot]: - internal/platform/twitter/client.go: isDuplicatePromotedTweetErr now requires a definite 4xx status. The duplicate branch runs before createOutcomeAmbiguous, so a mutating 3xx/5xx carrying DUPLICATE_PROMOTABLE_ENTITY was being reported as a known duplicate instead of the required UNCONFIRMED outcome — dropping the ambiguity for exactly the case that must stay ambiguous (the create may have committed on a 5xx). Now a 3xx/5xx falls through to createOutcomeAmbiguous. - internal/platform/twitter/client.go: reworded the ambiguous-create warning from "the create request reached X" to "may have reached X" — a transportError is only plausibly sent (mid-flight timeout/EOF), so "reached" overstated it. - internal/platform/twitter/client_test.go: added TestPromotedTweetDuplicateCodeOn5xxIsUnconfirmed (a 503 carrying the duplicate code must be UNCONFIRMED, not a known duplicate). - docs/knowledge/log.md: corrected the classifier description — it is status/type-based and NOT method-gated; GET failures never reach it because it is invoked only on the mutating create path (caller-scoped), rather than being "classified clean". Resolves 3 review threads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review Feedback AddressedCommit: e9e43e7 Changes Made
Build, Threads Resolved3 of 3 unresolved threads addressed in this iteration. |
Address PR #31 review feedback from copilot[bot]: - internal/platform/twitter/client.go: a promoted_tweets POST returning a 2xx with no data.id was warning "add it manually" — but a 2xx means the POST succeeded and X MAY have created the association, so a manual re-add invites the duplicate this classification exists to prevent. The missing-ID case is now surfaced as UNCONFIRMED with verify-before-retry wording, matching the ambiguous-error branch, instead of a clean "add manually" failure. - internal/platform/twitter/client_test.go: updated TestPromotedTweetMissingIDWarns to assert the result is UNCONFIRMED and does NOT tell the operator to add manually. - docs/knowledge/log.md: log entry for the 2xx-edge refinement. Resolves 1 review thread. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review Feedback AddressedCommit: e96f49c Changes Made
Build, Threads Resolved1 of 1 unresolved thread addressed in this iteration. |
Address PR #31 review feedback from copilot[bot]: - internal/platform/twitter/client.go: apiError.Error() no longer renders ErrorCodes. The code field comes from an untrusted response body and nothing bounds it to a short enum token, so joining it into Error() re-opened the body-leak channel into persisted PromotedTweetWarning/Steps that this PR exists to close. Codes are now retained ONLY for internal classification (hasErrorCode); Error() renders method/path/status only, mirroring reddit. - internal/platform/twitter/client.go: parseErrorCodes now drops over-long code values (>128 chars) and caps retention at 16, so even the internally-held codes can't be inflated by a hostile body. - internal/platform/twitter/client_test.go: added TestApiError_ErrorNeverSurfacesCode (sensitive text placed in errors[].code must not appear in Error()) and TestParseErrorCodes_BoundsUntrustedBody. - docs/knowledge: corrected the twitter concept + added a log entry. Resolves 1 review thread. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Address PR #31 review feedback from copilot[bot]: - internal/platform/twitter/client.go: isDuplicatePromotedTweetErr now requires a definite 4xx status. The duplicate branch runs before createOutcomeAmbiguous, so a mutating 3xx/5xx carrying DUPLICATE_PROMOTABLE_ENTITY was being reported as a known duplicate instead of the required UNCONFIRMED outcome — dropping the ambiguity for exactly the case that must stay ambiguous (the create may have committed on a 5xx). Now a 3xx/5xx falls through to createOutcomeAmbiguous. - internal/platform/twitter/client.go: reworded the ambiguous-create warning from "the create request reached X" to "may have reached X" — a transportError is only plausibly sent (mid-flight timeout/EOF), so "reached" overstated it. - internal/platform/twitter/client_test.go: added TestPromotedTweetDuplicateCodeOn5xxIsUnconfirmed (a 503 carrying the duplicate code must be UNCONFIRMED, not a known duplicate). - docs/knowledge/log.md: corrected the classifier description — it is status/type-based and NOT method-gated; GET failures never reach it because it is invoked only on the mutating create path (caller-scoped), rather than being "classified clean". Resolves 3 review threads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Address PR #31 review feedback from copilot[bot]: - internal/platform/twitter/client.go: a promoted_tweets POST returning a 2xx with no data.id was warning "add it manually" — but a 2xx means the POST succeeded and X MAY have created the association, so a manual re-add invites the duplicate this classification exists to prevent. The missing-ID case is now surfaced as UNCONFIRMED with verify-before-retry wording, matching the ambiguous-error branch, instead of a clean "add manually" failure. - internal/platform/twitter/client_test.go: updated TestPromotedTweetMissingIDWarns to assert the result is UNCONFIRMED and does NOT tell the operator to add manually. - docs/knowledge/log.md: log entry for the 2xx-edge refinement. Resolves 1 review thread. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
e96f49c to
a0b7381
Compare
…ment Mirror the meta fix (PR #30): NewClient value-copied a WithHTTPClient-supplied *http.Client to override CheckRedirect, but an http.Client must not be copied after first use (the copy duplicates its internal mutex while sharing the request- cancellation map, so concurrent use of the caller's client and the copy can race). - internal/platform/twitter/client.go: build a fresh *http.Client carrying only the exported reusable fields (Transport, Jar, Timeout) with CheckRedirect: noFollow. The caller's client is neither copied nor mutated. - internal/platform/twitter/client_test.go: the no-follow test now asserts the fresh client preserves the caller's Transport/Timeout and is a distinct pointer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…FIRMED Address PR #31 review feedback from cursor, copilot[bot]: - internal/platform/twitter/client.go: the line-item POST returned a definite "line item creation failed" on any error and "returned no line item ID" on a 2xx-no-id — even for a 5xx/mutating-3xx/transport failure or malformed 2xx where X may have committed the line item, inviting a blind-retry duplicate. Both now surface UNCONFIRMED (verify before retrying) when ambiguous; a definite 4xx/pre- send error still reads "failed". Mirrors the campaign/promoted-tweet/ad-set paths. - internal/platform/twitter/client.go: updated the PromotedTweetWarning field contract — it told consumers the tweet "may need to be added manually", which for an UNCONFIRMED outcome is the duplicate risk this prevents; now it requires verifying before adding or retrying. (per copilot[bot]) - docs/knowledge: corrected the twitter concept doc's "shallow copy" to the fresh- client construction; added a log entry. - internal/platform/twitter/client_test.go: added line-item ambiguous-5xx and 2xx-no-id UNCONFIRMED tests. Resolves 3 review threads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Address PR #31 review feedback from cursor, copilot[bot]: The initial campaign POST — the last uncovered create step — returned a bare (nil, err) on an ambiguous 3xx/5xx/transport failure and a plain error on a 2xx-with-no-id, discarding the deterministic campaign name. X may have committed the PAUSED campaign, so a caller got no reconcile signal and could retry into a duplicate (the same gap already closed for line items and promoted tweets). - internal/platform/twitter/client.go: both cases now return a name-carrying partial result + UNCONFIRMED wording (verify before retrying); a definite 4xx/pre-send error still returns plain (nil, err). Mirrors the meta/reddit name-only partial for the first create step. The whole twitter flow now classifies every create outcome consistently. - internal/platform/twitter/client_test.go: added campaign ambiguous-5xx and 2xx-no-id UNCONFIRMED tests. Resolves 2 review threads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
bff89c2 to
1ac7c39
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/platform/twitter/client.go:548
- This contract still says the predicate is not method-gated, but the implementation below and its new tests explicitly gate 3xx responses on
isMutatingMethod(matching Meta/Reddit). The PR description also repeats the opposite claim. Update this comment and the PR description to document the intended method-gated 3xx behavior; otherwise future callers cannot rely on the stated contract.
// Mirrors the meta/reddit clients: a transportError is always ambiguous, and an
// *apiError is ambiguous on a 3xx or 5xx status — the predicate is NOT gated on
// the HTTP method (unlike an earlier draft). Callers already invoke this only on
// mutating (create) failures, so a method gate added nothing but a silent
// divergence from the siblings; a GET-5xx is a caller-scoping concern, not this
internal/platform/twitter/client.go:426
- The Meta client does not share this no-body rendering guarantee:
internal/platform/meta/client.go:604-627includes the body-derivedMessage,Type, andFBTraceIDinAPIError.Error(). Referencing Meta here misdocuments the security behavior; Reddit, LinkedIn, and Google Ads are the matching implementations.
// caller / logged. Report only method, path, and status. Mirrors the
// reddit/meta/googleads clients' apiError.
dealako
left a comment
There was a problem hiding this comment.
Hi @mrautela365 👋 — thanks for the thorough work here, and especially for the transparent commit-by-commit trail showing how each bot finding was run to ground.
Overall impression
This is a high-quality, tightly-scoped change that achieves genuine behavioral parity with the reddit/meta/google-ads clients — not just type parity. The two goals (stop echoing the response body into persisted Steps/warnings, and classify ambiguous mutating failures as UNCONFIRMED so a committed-then-failed create isn't blind-retried into a duplicate) are both met and, importantly, tested: apiError.Error() and parseErrorCodes never surface body-derived text, the retained codes are bounded (16 entries × 128 chars), and createOutcomeAmbiguous is wired into all three create steps (campaign, line-item, promoted-tweet) including each 2xx-with-no-id branch. isDuplicatePromotedTweetErr is correctly 4xx-gated and evaluated before the ambiguity check. Build/vet clean locally.
The prior Copilot and Cursor findings are all addressed — I traced each one to its resolving commit and agree with the resolutions. My independent pass (plus a parallel security + code-quality sweep) surfaced only one documentation defect worth fixing and one defense-in-depth nit.
Reconciliation with existing bot comments
- Copilot / Cursor — body-leak, duplicate-on-any-status, 2xx-no-id, method-gate, line-item/campaign ambiguity, field-doc contract → all verified resolved in the referenced commits (
02e7e25,e9e43e7,e96f49c,7f6e295,79f994b,bff89c2). Agree with the fixes; no re-litigation needed.
Structured issue count
- 🔴 Blocking: 0
- 🟡 Minor: 1 — a stale header comment on
createOutcomeAmbiguousnow contradicts the code (and misstates sibling behavior). - ⚪ Nit: 1 —
transportError.Error()folds the wrappedDo/decode error (request URL / a JSON-syntax byte) into persistedSteps; benign for this client today, defense-in-depth only. - ❔ Question: 0
Final decision
✅ Approved with minor comments — the one minor item is a comment/documentation fix, not a code-behavior bug, so it need not block. Please fix it before merge (or ack) since it directly touches this PR's headline parity claim.
dealako
left a comment
There was a problem hiding this comment.
✅ Approved with minor comments. Behavioral parity is achieved and well-tested; all prior bot findings are resolved. One minor doc-comment fix (the stale createOutcomeAmbiguous header) and one optional defense-in-depth nit are noted inline — neither blocks merge. Please fix or ack the doc comment since it touches the parity claim. Nice work.
The merge-base changed after approval.
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 1396005. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/platform/twitter/client.go:549
- This comment says the predicate is not method-gated, but the implementation at line 568 and its tests explicitly gate 3xx responses on a mutating method. Because this classifier controls duplicate-create handling, the contradictory contract can prompt a future “fix” that changes the intended behavior. State that transport/5xx cases are method-independent while 3xx requires a mutating method.
// Mirrors the meta/reddit clients: a transportError is always ambiguous, and an
// *apiError is ambiguous on a 3xx or 5xx status — the predicate is NOT gated on
// the HTTP method (unlike an earlier draft). Callers already invoke this only on
// mutating (create) failures, so a method gate added nothing but a silent
// divergence from the siblings; a GET-5xx is a caller-scoping concern, not this
// predicate's.
…eps (PR #31) Address review (cursor Medium, dealako, copilot): - internal/platform/twitter/client.go: transportError.Error() rendered %v of the wrapped httpClient.Do error — usually a *url.Error embedding the full request URL (which can carry request material / a destination's secret query) — and that string is copied into PromotedTweetWarning and persisted Steps. Added safeTransportCause: it unwraps a *url.Error to its underlying cause (timeout/EOF/reset), which carries no URL, and Error() renders method/path + that. - corrected the stale createOutcomeAmbiguous header comment that still claimed the predicate is "NOT gated on the HTTP method" after the 3xx gate was re-added. - documented CreateCampaign's non-standard (non-nil result, non-nil error) contract so callers inspect the result on error to reconcile, not discard it. Test: TestTransportError_DoesNotLeakURL. (reddit/meta share the same transportError render; a follow-up will apply the same URL suppression there.) Resolves 4 review threads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
internal/platform/twitter/client.go:1161
- This “only” is inaccurate: a definite 4xx campaign-create rejection also returns
(nil, err)at line 1352, as do pre-create lookup failures. Describe the contract in terms of whether a campaign may have been created so callers do not infer that every nil result failed before an API response.
// only a definite pre-send/validation failure returns (nil, err).
internal/platform/twitter/client.go:589
- The current PR description explicitly says
createOutcomeAmbiguousis “not method-gated,” but this line intentionally gates 3xx responses by method (matching the tests and sibling clients). Since the prior reply confirms the gated behavior is intended, update the PR description to remove the contradictory acceptance claim.
return ae.StatusCode >= 300 && ae.StatusCode < 400 && isMutatingMethod(ae.Method)
Address review (copilot): safeTransportCause unwrapped only the OUTERMOST *url.Error. http.Client.Do wraps a RoundTripper's error in its own *url.Error, and an injected transport can itself return a *url.Error, so a single unwrap can leave an inner *url.Error whose .Error() still embeds the request URL — re-leaking query secrets into PromotedTweetWarning/Steps. Now loops until the cause is no longer a *url.Error, then renders that URL-free cause. Test extended to a nested *url.Error. Resolves 1 review thread. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/platform/twitter/client.go:538
- Peeling
*url.Errorwrappers does not guarantee that the remaining error text is URL-free. A supported customRoundTrippercan return an ordinary error containingreq.URL;http.Client.Dowraps it in*url.Error, this loop removes that wrapper, anderr.Error()then persists the full URL. Use a fixed or allowlisted description for the final cause instead of arbitrary error text, and cover a non-url.Errorinner cause containing a secret URL.
return err.Error()
dealako
left a comment
There was a problem hiding this comment.
Hi @mrautela365 👋 — follow-up pass after your last two pushes. First: thank you for closing out the two items from my previous review so cleanly, and for the continued commit-by-commit trail. I re-ran a full independent sweep (build + go vet + -race tests on twitter/meta all green locally) plus parallel security and code-quality subagents on the updated diff.
✅ Prior feedback — both resolved
- ✅ Minor (stale
createOutcomeAmbiguousheader) → resolved in1e301dc. The contract now correctly reads "5xx regardless of method, 3xx ONLY for a mutating method" and matches theisMutatingMethod-gated code atclient.go:600. No more contradiction with the tests or the sibling clients. - ✅ Nit (
transportError.Error()folding the wrappedDo/decode error intoSteps) → resolved in1e301dc+97d64bc. The newsafeTransportCausepeels every nested*url.Errorlayer before rendering, andTestTransportError_DoesNotLeakURLproves a doubly-wrapped signed URL never reaches the persisted string. Nicely done — you went further than the original nit asked.
👏 Nice work in this revision
- The nested-
*url.Errorloop insafeTransportCauseis exactly right —http.Client.Dowraps aRoundTripper's own*url.Errorin another, so a single unwrap would have left the inner one leaking. The regression test nails that specific case. - The
CreateCampaignheader now documents the non-standard(partial, err)reconcile contract explicitly — a real improvement for callers.
🔎 New findings this pass
My independent sweep surfaced one in-scope minor that undercuts this PR's headline no-URL-leak invariant, plus a doc-accuracy minor and two nits.
🟡 Minor 1 — the pre-send dial branch still leaks the full request URL (the one branch safeTransportCause doesn't cover)
client.go:687 returns the raw Do error via %w: fmt.Errorf("x ads api %s %s: %w", method, path, err). For a connection-refused / DNS / no-route failure, err is a *url.Error whose .Error() embeds the full request URL. Because X Ads folds create params into the query string (unlike meta/reddit, which use a JSON body), that URL carries funding_instrument_id (a financial-account identifier not otherwise surfaced in Steps), plus name/budget/line_item_id/tweet_ids. A pre-send error is non-ambiguous, so it lands in the definite-failure branch at :1519–1524, which writes err.Error() into both PromotedTweetWarning and Steps (and the campaign path returns it via (nil, err) at :1363). This is the exact persisted sink the typed-error rework set out to protect — the transport path is scrubbed, but this sibling branch bypasses the same discipline. OAuth secrets are safe (they live in the Authorization header), so this is not a credential leak — but it does persist a financial identifier and breaks the invariant the PR itself documents.
Fix (one line): route the pre-send error through the same scrub — return nil, fmt.Errorf("x ads api %s %s: %s", method, path, safeTransportCause(err)). The %w can be dropped safely: classification is type-based (createOutcomeAmbiguous / isDuplicatePromotedTweetErr), nothing unwraps this error. A quick regression test asserting a connection-refused URL never reaches Steps would round it out.
🟡 Minor 2 — CreateCampaign header doc overstates the (nil, err) condition
client.go:1172 says "only a definite pre-send/validation failure returns (nil, err)." That's incomplete: (nil, err) is also returned for a findCampaignByName lookup failure (:1297) and for a definite 4xx rejection of the campaign POST (:1363). Behavior is correct (nothing was created in either case), but since the error contract is this PR's whole point, the doc should match. Suggest: "(nil, err) is returned only up to and including the initial campaign create when nothing has (or may have) been created — a validation/pre-send error, a name-lookup failure, or a definite 4xx/429 POST rejection. Once the campaign is created/reused or its create is ambiguous, a non-nil partial is always returned."
⚪ Nit 1 — ambiguous campaign/line-item branches don't append an UNCONFIRMED Steps entry
The promoted-tweet ambiguous branch (:1518) and the meta campaign path both append a matching Steps line, but the twitter campaign (:1360) and line-item (:1458) ambiguous/no-id branches carry the UNCONFIRMED signal only in err.Error(), not in result.Steps. Since the partial is explicitly returned for reconciliation, a parallel step would improve observability. Take-it-or-leave-it.
⚪ Nit 2 — safeTransportCause doc slightly overstates "carries no URL"
The comment says the unwrapped cause "does not" carry a URL. True for the signed request URL, but a mid-flight *net.OpError (read tcp <local>-><remote-ip>:443: connection reset) still renders the remote IP:port. Not the secret this type guards, so negligible — flagging only for phrasing accuracy.
🤝 Reconciliation with bot comments
- Copilot
client.go:538(peel every*url.Error, not just outermost) → agree; resolved in97d64bcwith the loop + nested test. My Minor 1 is a different branch (the non-ambiguous pre-send path), not this one. - Copilot
client.go:1161/1172((nil,err) contract inaccurate) → agree; captured as my Minor 2 above with a concrete rewording. - Copilot/Cursor — method-gate, body-leak, duplicate-on-5xx, 2xx-no-id, line-item/campaign ambiguity, concept-doc shallow-copy → all verified resolved across
51b7024,3c818d7,205fcd3,3a1abda,99109dd,1ac7c39and the PR #30 rebase. No re-litigation needed.
Structured issue count (all open items)
- 🔴 Blocking: 0
- 🟡 Minor: 2 — (1) pre-send dial branch leaks the request URL (incl.
funding_instrument_id) into persistedSteps; (2)CreateCampaignheader doc overstates the(nil, err)condition. - ⚪ Nit: 2 — (1) no UNCONFIRMED
Stepsentry on ambiguous campaign/line-item; (2)safeTransportCause"carries no URL" phrasing.
Final decision
🔴 Needs changes before approval — only for Minor 1. It's a one-line scrub (safeTransportCause) that closes the last URL-leak branch and makes the no-leak invariant actually hold across every path; it directly touches this PR's headline security claim and persists a financial identifier. Minor 2 and the two nits are bundled for the same push but are non-gating. Everything else is solid, consistent, and well-tested — thanks again for the careful iteration.
…URL (PR #31) Address review (copilot): the transportError fix covered the ambiguous branch, but the pre-send branch (isPreSendDialError) still did a raw %w of the *url.Error, so a DNS/connection-refused failure on a create rendered the full request URL (X puts create params in the query string) into PromotedTweetWarning and persisted Steps. Add a preSendError type: URL-free Error() via safeTransportCause, Unwrap() retains the cause so errors.Is/errors.As and isPreSendDialError still match. It is semantically DEFINITE (request never sent -> not applied), distinct from the ambiguous transportError, so createOutcomeAmbiguous still treats it as not-applied. Test added (TestPreSendError_DoesNotLeakURL). Concept doc + log updated. Resolves 1 review thread. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
dealako
left a comment
There was a problem hiding this comment.
Hi @mrautela365 👋 — follow-up on 0ef1cd5. You turned my one gating item around fast and, characteristically, did it thoroughly. Re-verified locally: go build, go vet, -race tests on twitter+meta all green, including the new TestPreSendError_DoesNotLeakURL.
✅ Revision tracking
- ✅ Minor 1 — pre-send dial branch leaked the request URL → Resolved in
0ef1cd5. Rather than a one-linesafeTransportCausepatch, you introduced a dedicatedpreSendErrortype — the right call. It:- renders URL-free via
safeTransportCause(so a DNS/connection-refused failure no longer foldsfunding_instrument_id+ the full create URL intoPromotedTweetWarning/Steps), and - keeps
Unwrap()returning the real cause, soisPreSendDialError,errors.Is, anderrors.Asstill match — proven by the new test asserting both no-leak anderrors.Is(pse, syscall.ECONNREFUSED). - Semantically it's DEFINITE (request never sent → not applied), distinct from the ambiguous
transportError. I confirmedcreateOutcomeAmbiguousstill classifies it as not-ambiguous (it matches only*transportError/*apiError, andpreSendErrorunwraps to*url.Error → *net.OpError), so the create-outcome logic is unchanged — no regression.
- renders URL-free via
- 🟡 Minor 2 —
CreateCampaignheader doc overstates the(nil, err)condition (client.go:1172) → still open, non-gating. My prior inline suggestion stands whenever you next touch the file. - ⚪ Nit 1 (no UNCONFIRMED
Stepsentry on ambiguous campaign/line-item) and ⚪ Nit 2 (safeTransportCause"carries no URL" phrasing) → still open, optional.
👏 Nice work
- Making
preSendErrora first-class type instead of inlining the scrub means the pre-send path now shares the exact URL-suppression discipline astransportError, and the type name documents the DEFINITE-vs-ambiguous distinction at the call site. - The regression test asserts the important invariant on both sides — the URL never surfaces, and the cause chain survives for classification. That's precisely the pair of properties that could regress.
- The knowledge-log note that reddit/meta (already merged) carry the same raw
%wpre-send render, tracked as a follow-up, is exactly the kind of transparent scoping I want to see.
🤝 Bot reconciliation
- Copilot
client.go:714(pre-send branch bypasses URL sanitization, posted 20:10) → same finding as my Minor 1; agree, resolved by0ef1cd5.
Structured issue count (all open items)
- 🔴 Blocking: 0
- 🟡 Minor: 1 —
CreateCampaignheader doc(nil, err)accuracy (non-gating; carried). - ⚪ Nit: 2 — UNCONFIRMED
Stepsentry on ambiguous campaign/line-item;safeTransportCausephrasing.
Final decision
✅ Approved with minor comments. The behavioral parity + no-URL/no-body-leak invariants now hold across every error branch (apiError, transportError, and preSendError), and the create-outcome classification is consistent and well-tested end to end. The one remaining minor is a doc-accuracy tweak and the two nits are optional — none block merge. Please fold Minor 2 into a future touch of this file (or ack). Thanks for the careful, well-documented iteration on this one.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
internal/platform/twitter/client.go:559
- After peeling
*url.Error, this renders an arbitrary underlying error. BecauseWithHTTPClientpreserves a caller-suppliedTransport, a RoundTripper can return an error containingreq.URL;http.Client.Dowraps it in*url.Error, and this line then re-emits the full signed query into warnings/Steps. That still violates the URL-leak guarantee. Render only fixed/allowlisted causes (and add a custom-transport regression) rather than arbitraryerr.Error()text.
return err.Error()
internal/platform/twitter/client.go:1197
- The “only” claim is inaccurate:
findCampaignByNamefailures at lines 1320–1323 and cancellation in the firstpacecall also return(nil, err). Document all pre-first-mutation failures so callers do not infer that every other error guarantees a partial result.
// only a definite pre-send/validation failure returns (nil, err).
internal/platform/twitter/client.go:621
- The current PR description explicitly says
createOutcomeAmbiguousis “not method-gated,” but this implementation (and its new GET test) method-gates 3xx responses. Since Meta and Reddit also use this gate, the code appears intentional; update the PR description so the stated contract matches the reviewed implementation.
return ae.StatusCode >= 300 && ae.StatusCode < 400 && isMutatingMethod(ae.Method)
Address review (@dealako): "only a definite pre-send/validation failure returns (nil, err)" was incomplete — (nil, err) is also returned for a findCampaignByName lookup failure and a definite 4xx/429 rejection of the initial campaign POST. Reword to state (nil, err) is returned only up to and including the initial campaign create (nothing created), and a non-nil partial once the campaign is created/reused or its create is ambiguous. Comment-only. Resolves 1 review thread. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/platform/twitter/client.go:559
- Unwrapping
*url.Errordoes not make an arbitrary underlying error string URL-free.WithHTTPClientaccepts custom transports, so aRoundTrippercan return a non-*url.Errorerror containingreq.URL;http.Client.Doadds the outer*url.Error, this loop removes it, and this line then persists the custom URL-bearing text in warnings/Steps. Render only whitelisted safe causes or a fixed label while retaining the original throughUnwrap; add a regression using a plain cause that contains the secret URL.
return err.Error()
internal/platform/twitter/client.go:621
- The current PR description still says
createOutcomeAmbiguousis “not method-gated,” while this line intentionally gates 3xx responses by method. The prior developer reply established that the gate is the intended Meta/Reddit-compatible contract, so update the PR description to remove the opposite acceptance criterion; as written, the review contract and implementation remain contradictory.
return ae.StatusCode >= 300 && ae.StatusCode < 400 && isMutatingMethod(ae.Method)

Summary
Brings the X/Twitter Ads client to behavioral parity with the reddit/meta/google-ads clients: typed errors + outcome-ambiguity classification wired into the create path.
Previously the client returned a bare
fmt.Errorffor every non-2xx and echoed the response body into the error (which can carry signed URLs / destination secrets and is persisted into a campaign's Steps), and had no ambiguity classification — so a mutating request that committed then failed looked identical to a definite rejection, inviting a duplicate-create retry.Changes
apiError(status/method/path + X's machine-readable error codes, e.g.DUPLICATE_PROMOTABLE_ENTITY; the raw body is not surfaced),transportError(ambiguous),isPreSendDialError. The exhausted/over-cap 429 paths return a typedapiError{429}.createOutcomeAmbiguousinto the create path — the non-idempotentpromoted_tweetsPOST now distinguishes an ambiguous failure (mutating 3xx/5xx or transport → UNCONFIRMED 'may exist', reconcile rather than retry) from a definite failure (4xx/pre-send → clean 'failed, add manually'). This is the behavioral parity, not just types.createOutcomeAmbiguousis not method-gated — mirrors meta/reddit exactly.isDuplicatePromotedTweetErrmatches the typed code (extracted to a const) instead of the removed body. Removed the now-unusedtruncate.Stacked on #30
This builds on the
noFollowchange in #30 (same twitter file). Rebase onto main after #30 merges, or review together.Verification
go build,go test -race,golangci-lintall clean. Includes aCreateCampaign-path test proving a 5xx promoted-tweet is UNCONFIRMED while a 4xx is a definite failure. No new dependencies.Reviewer-sim'd before push
Ran the reviewer simulation (dealako/rashad/asitha) which caught that the classifier was initially defined-but-unwired and had a silent method-gate divergence — both fixed here before opening.
LFXV2-2642