Skip to content

feat(platform): add Reddit Ads client (OAuth2 + token refresh)#21

Merged
dealako merged 46 commits into
mainfrom
feat/platform-reddit-client
Jul 14, 2026
Merged

feat(platform): add Reddit Ads client (OAuth2 + token refresh)#21
dealako merged 46 commits into
mainfrom
feat/platform-reddit-client

Conversation

@mrautela365

Copy link
Copy Markdown
Contributor

Phase-2 platform client. Ports reddit-ads.service.ts to Go as standalone package internal/platform/reddit/.

What it does: OAuth2 with token refresh + expiry buffer (cached token reused while now < expireAt-60s, else refreshed; mutex-guarded, basic-auth token endpoint). Campaign→ad-group→promoted-post hierarchy (PAUSED), lifetime microdollar budget, objective-aware params, community-fallback retry, UTM builder, post-ID extraction.

Design: injected credentials, no env, stdlib only, context-aware, injectable clock for deterministic expiry tests. Standalone; dispatcher adapter with the wiring PR.

Public API: NewClient(creds, account, opts...)CreateCampaign(ctx, CampaignInput) (*CampaignResult, error).

Tests: token reuse/refresh (controllable clock), basic-auth shape, campaign happy path, community fallback, validation, post-ID extraction, UTM, microdollars.

Not ported (later PR): status-toggle + analytics.

Ports lfx-v2-ui/apps/lfx-one/src/server/services/reddit-ads.service.ts. LFXV2-2638

🤖 Generated with Claude Code

@mrautela365 mrautela365 requested a review from a team as a code owner July 9, 2026 20:33
Copilot AI review requested due to automatic review settings July 9, 2026 20:33
Comment thread internal/platform/reddit/client.go Fixed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR ports the upstream TypeScript reddit-ads.service.ts to a new standalone Go package internal/platform/reddit/. It introduces a Reddit Ads API v3 client focused on OAuth 2.0 token refresh (with a 60s expiry buffer, mutex-guarded, basic-auth token endpoint) and PAUSED campaign creation across the Campaign → Ad Group → Promoted Post hierarchy. The client is standalone (no dispatcher wiring yet), injects all credentials/config, uses only the standard library, and exposes an injectable clock for deterministic expiry tests.

Changes:

  • Adds Client with NewClient(creds, account, opts...) and CreateCampaign(ctx, CampaignInput), including token refresh/caching, objective-aware campaign params, targeting with community-fallback retry, UTM builder, microdollar/timestamp helpers, and post-ID extraction.
  • Adds functional options (WithHTTPClient, WithBaseURL, WithTokenURL, WithNowFunc) for testability.
  • Adds table/httptest-driven unit tests covering token reuse/refresh, basic-auth request shape, the full campaign happy path, community fallback, input validation, post-ID extraction, UTM building, and microdollar conversion.

Reviewed changes

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

File Description
internal/platform/reddit/client.go New Reddit Ads client: OAuth token refresh with expiry buffer, campaign creation flow, and pure helper functions.
internal/platform/reddit/client_test.go Unit tests for token refresh, campaign creation, fallback, validation, and helpers using httptest and an injectable clock.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Copilot AI review requested due to automatic review settings July 9, 2026 20:52
@mrautela365 mrautela365 force-pushed the feat/platform-reddit-client branch from d7cdbb1 to 65c1020 Compare July 9, 2026 20:52
@mrautela365 mrautela365 force-pushed the feat/platform-reddit-client branch from 65c1020 to 0453030 Compare July 9, 2026 20:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go Outdated
Copilot AI review requested due to automatic review settings July 9, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Copilot AI review requested due to automatic review settings July 9, 2026 21:05
@mrautela365 mrautela365 force-pushed the feat/platform-reddit-client branch from 0453030 to 691fd45 Compare July 9, 2026 21:05
@mrautela365

Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed (round 2)

  • client.go: removed unused bidValue (TS uses BIDLESS and never sends it) (per copilot[bot])
  • client.go: token response validated — error on empty access_token, guard non-positive expires_in (per copilot[bot])
  • client.go: GeoTargets + subreddit inputs trimmed, empties dropped (per copilot[bot])
  • client.go: extractRedditPostID validates the part after t3_ (rejects t3_!!!) (per copilot[bot])
  • Added 4 tests. golangci-lint 0 issues, build/test/gofmt clean. All threads resolved.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client_test.go Outdated
Comment thread internal/platform/reddit/client_test.go
Comment thread internal/platform/reddit/client.go Outdated
Copilot AI review requested due to automatic review settings July 9, 2026 21:35
@mrautela365 mrautela365 force-pushed the feat/platform-reddit-client branch from 691fd45 to 731a0cc Compare July 9, 2026 21:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go
mrautela365 added a commit that referenced this pull request Jul 11, 2026
1. Compute the adjusted start time once, up front, so a same-day (past
   midnight-UTC) start is nudged to now+buffer before the campaign POST
   and reused by the ad group, instead of sending a past start.
2. Build the ad-group label from the same normalized (trimmed, uppercased)
   geos used for targeting, so padded input can't desync name vs targeting.
3. Treat an /ads 2xx response missing data.id as a manual-action warning
   rather than a silent success; do not count it as a created ad.
4. Merge UTM params into the URL query via url.Parse so a URL carrying a
   fragment keeps the fragment at the end instead of nesting the query.
5. Validate the post URL's parsed host (reddit.com/redd.it and subdomains
   only) rather than substring-matching the whole URL, closing an SSRF/
   spoofing hole.
6. Add the internal/platform/reddit OKF concept doc, index entry, and log
   entry so the knowledge bundle stays conformant.

Address Copilot review comments on PR #21.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings July 11, 2026 05:36
… (PR #21)

Address the latest Copilot round:

- client.go: validateRegistrationURL now rejects a URL whose query has invalid
  percent-encoding (e.g. ?token=%zz) before any mutation. url.Parse accepts it,
  but u.Query() in buildRedditUTMURL silently drops the malformed pair, so the
  paid ad would be created with a different destination than the caller gave.
- client.go: redactURL no longer echoes an unparseable URL that carries userinfo
  (e.g. https://user:password@example.com/%zz fails url.Parse and its authority
  would otherwise be returned) — it returns a fixed "[unparseable-url-redacted]"
  placeholder when a credential "@" remains, so a secret can't leak via an error.
- client_test.go: added the malformed-query rejection test and a redactURL case
  for the unparseable-with-userinfo input.

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Comment thread internal/platform/reddit/client.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go
Comment thread internal/platform/reddit/client.go Outdated
…nfirmed ad (PR #21)

Address the latest review round:

- client.go: a campaign create returning 2xx with no data.id now returns a
  partial *CampaignResult carrying the campaign NAME (and steps) alongside the
  error, so an orphaned PAUSED campaign is reconcilable by name — instead of
  (nil, err) which discarded everything (mirrors the ad-group path). (cursor)
- client.go: token-refresh failures no longer echo the OAuth response body —
  the request carried the client id/secret + refresh token and a token/proxy
  diagnostic body may reflect credential material, and CreateCampaign persists
  this error into Steps. Only the HTTP status (and a body-read failure) is
  exposed now. (per copilot[bot])
- client.go: a per-request ad failure (transport timeout / dropped response)
  does not prove the ad was not created, so the AdWarning + step now report the
  ad as UNCONFIRMED and require verification before manual creation, avoiding a
  duplicate promoted-post ad. (per copilot[bot])
- client_test.go: added TestCreateCampaign_CampaignWithoutIDReturnsPartial;
  updated the live-ctx ad-failure test to assert the UNCONFIRMED wording.

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client_test.go Outdated
Comment thread internal/platform/reddit/client.go
…oup name in partial (PR #21)

Address the latest Copilot round:

- client.go: request() now bounds EACH attempt with context.WithTimeout(ctx,
  redditRequestTimeout). Relying solely on http.Client.Timeout let a
  WithHTTPClient override with no Timeout (and a background caller ctx) hang a
  create indefinitely, and redditWorstCaseCreateWait assumes each attempt is
  capped by redditRequestTimeout.
- client.go: an ambiguous campaign-create failure (transport timeout, decode
  failure, or 5xx — where Reddit may have created the campaign) now returns a
  partial result carrying the campaign NAME with an UNCONFIRMED note, instead of
  (nil, err); a definite 4xx still returns (nil, err). Mirrors the ad path so a
  caller doesn't blindly retry and duplicate the campaign.
- client.go: the 2xx-without-ad-id path now reports the ad as UNCONFIRMED (verify
  before manual creation) rather than "add manually", matching the transport
  path — a malformed success may still have created the ad.
- client.go: partialResult now includes AdGroupName (computed up front) — on an
  ad-group failure/malformed success the ad group's deterministic name is the
  only reconciliation handle, so dropping it risked a duplicate.
- client_test.go: bounded the token-coalescer test's leader-entry wait with a
  timed select (was an unbounded receive that could hang the suite).

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Comment thread internal/platform/reddit/client.go
… Reddit (PR #21)

Address Cursor: the ambiguous "campaign may exist" partial-UNCONFIRMED branch was
too broad — it also fired for pre-send failures (token refresh, body
encode/build), a 429 over-cap abort, and context cancellation, where the
campaign definitely was NOT created, wrongly telling callers a resource may
exist.

- client.go: added a transportError type wrapping httpClient.Do failures (the
  request was SENT, outcome ambiguous). The campaign-create failure now treats a
  create as UNCONFIRMED ONLY for a transportError or a 5xx apiError — i.e. the
  request actually reached Reddit. Every other error (pre-send, 429 over-cap,
  4xx, ctx cancel) returns (nil, err) so a caller can retry safely.
- client_test.go: TestCreateCampaign_5xxOnCampaignIsUnconfirmed and
  TestCreateCampaign_TokenRefreshFailureNotUnconfirmed (pre-send failure returns
  nil, not UNCONFIRMED); added a validRedditInput() helper.

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go

@dealako dealako left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mrautela365 — thanks for the extensive round of hardening on this one. This is a well-engineered port: the OAuth token single-flight coalescing, the partial-result contract for orphan-campaign identifiability, the redaction discipline across every error/Steps path, and the 400-gated community fallback are all handled carefully and are backed by 64 tests (all pass locally under -race, go vet clean, golangci-lint 0 issues). The OKF concept doc, code index, log, and api-catalog updates are all present and accurate, and go run ./cmd/okfvalidate ./docs/knowledge passes.

I ran two parallel subagents (a security-focused pass and a general code-quality pass) alongside my own read of client.go/client_test.go in full, then cross-checked every finding against the repo's internal/platform/twitter sibling client for convention drift. All 100 existing review threads on this PR are already resolved, and Cursor Bugbot's summary (medium risk, paid-resource creation) aligns with what's already mitigated here.

🔴 Blocking: 1 issue

  • CI: MegaLinter/secretlint is failing on client_test.go:2920 — the https://user:s3cr3t@example.com/... test fixture matches secretlint's basic-auth URL pattern. Confirmed as a false positive (verified locally against the actual secretlint rule), with a verified fix (repo-root .secretlintrc.json allowlist entry) posted inline.

🟡 Minor: 1 issue

  • No length validation on EventName/Project before they're composed into the campaign/ad-group name — every other orphan-risk field in CreateCampaign is validated before the campaign POST, but this one isn't, unlike the Twitter client's maxEventNameLen/validateEntityName pattern. Posted inline with specifics.

⚪ Nit: 0

❔ Question: 0

Security

No findings from either the security-focused review or my own pass — credential handling (OAuth basic-auth header, cached token, refresh coalescing), URL/host validation (registration URL, post URL, SSRF-safe isRedditHost), path sanitization, and error/Steps redaction are all solid and specifically test-covered.

Final decision: 🔴 Needs changes before approval

Both issues are small and mechanical — the secretlint fix is a config addition (verified working), and the name-length check is a short validation mirroring code that already exists in this repo. Happy to re-review as soon as those land.

Comment thread internal/platform/reddit/client_test.go
Comment thread internal/platform/reddit/client.go

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ 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 b4c936b. Configure here.

Comment thread internal/platform/reddit/client.go
Address Copilot: a caller context cancellation during the (non-fatal) account-
verification GET was downgraded to a warning, so execution continued to the
campaign step, which then returned an "unconfirmed, may exist" partial even
though no campaign POST occurred — contradicting the pre-POST (nil, err)
contract.

- client.go: after the account-verification GET, a ctx.Err() cancellation now
  aborts with (nil, err) before any mutating call. The campaign-create error
  branch also checks ctx.Err() first, so a cancellation that propagated there
  returns (nil, err) rather than an ambiguous "may exist" partial.
- client_test.go: TestCreateCampaign_CtxCancelDuringVerifyIsFatal (cancel during
  the verify GET -> nil result, not UNCONFIRMED, zero campaign POSTs).

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
…front (PR #21)

Address @dealako's review:

- client_test.go: compose userinfo URLs at runtime via a urlWithUserinfo helper
  (and switch TestRedactURL to a slice) so no literal "user:pass@host" appears —
  secretlint's basic-auth rule (MegaLinter) was flagging the test fixtures and
  failing CI. Reworded a client.go comment for the same reason. [blocking]
- client.go: length-validate EventName (<=120) and Project (<=40) by rune count
  up front, before any mutating call — both are folded into the campaign/ad-group
  names, and an over-long value would otherwise be rejected by Reddit only at the
  ad-group POST, after the campaign already exists (orphan). Mirrors the meta
  client's copy-limit checks. [minor]
- client_test.go: TestCreateCampaign_RejectsOverlongNameBeforeAnyPOST.

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go
…name limit (PR #21)

Address a cluster of Copilot/Cursor findings by applying ONE consistent
"was the resource created?" model across the campaign, ad-group, and ad paths.

- client.go: added createOutcomeAmbiguous(err) — true only when the request
  plausibly reached Reddit (transportError, or a 5xx apiError). Used by all three
  create paths so they classify identically.
- client.go: isPreSendDialError — a DNS/connection-refused/no-route Do failure is
  NOT wrapped as transportError (the request was never sent), so it's treated as
  not-created, not "may exist".
- client.go: a 2xx response whose body can't be read/decoded is now wrapped as
  transportError (the mutation succeeded; result unreadable) -> UNCONFIRMED,
  instead of a plain error that a caller could retry into a duplicate.
- client.go: ad-group failures and the 2xx-no-id case are worded UNCONFIRMED for
  ambiguous errors (ad group may exist; partialResult carries its name) vs FAILED
  for definite errors. The ad path likewise reports UNCONFIRMED only for
  transport/5xx and FAILED for a definite 4xx; a ctx-cancel that interrupts an
  in-flight ad/campaign POST is UNCONFIRMED (may exist), a pre-send cancel is a
  clean abort.
- client.go: apiError.Error() no longer includes the response Body (retained on
  the struct for internal matching only) so a reflective upstream body can't leak
  request material (click_url secret / bearer token) into Steps/errors/logs.
- client.go: enforce Reddit's 200-char limit on the COMPOSED campaign/ad-group
  names before the respective POSTs (the per-field caps don't guarantee it).
- client_test.go: conn-refused (not UNCONFIRMED), 2xx-undecodable (UNCONFIRMED),
  ad 4xx (FAILED not UNCONFIRMED).

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go Outdated
… (PR #21)

Address Copilot: the composed ad-group-name length check ran AFTER the campaign
POST, so a caller supplying enough valid geo codes (GeoTargets has no count
limit) to push adGroupName past 200 runes would create the campaign and then
fail the locally-predictable length check — orphaning a paid campaign.

- client.go: compute campaignName AND adGroupName and validate both lengths up
  front, before the campaign POST (all inputs are available then), so an
  over-long name fails before any paid resource exists. Removed the now-duplicate
  later computation + post-POST check.
- client_test.go: TestCreateCampaign_OverlongAdGroupNameFailsBeforeAnyPOST (many
  geos -> over-long name -> rejected with zero POSTs).

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
@mrautela365

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review, @dealako — much appreciated. Both items you flagged are now fixed and CI is green:

🔴 secretlint (blocking) — Fixed in 770d893. Rather than an allowlist entry, I removed the literal https://user:s3cr3t@… fixtures entirely: the test now composes userinfo URLs at runtime via a urlWithUserinfo() helper (and TestRedactURL moved from a map to a slice), so no user:pass@host literal appears in source for secretlint's basic-auth rule to match. Same for the meta client. MegaLinter now passes on the current head (c6dc0ea).

🟡 EventName/Project length (minor) — Fixed in 770d893. Added maxEventNameRunes (120) and maxProjectRunes (40) rune-count checks up front, before any mutating call (mirroring the Twitter client's validateEntityName pattern). Follow-up in c6dc0ea also validates the composed campaign/ad-group names against Reddit's 200-char limit before the campaign POST, so a large GeoTargets list can't push the ad-group name over-limit and orphan a campaign.

Everything else stayed as you reviewed it. Ready for re-review whenever you have a moment — thanks again!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
Comment thread internal/platform/reddit/client.go Outdated
…ail wording (PR #21)

Address David's minor Copilot items:

- client.go: the coalesced token refresh detached with context.Background(),
  dropping request-scoped values so an injected tracing transport couldn't
  correlate the token request. Now uses context.WithoutCancel(ctx) — keeps the
  request-scoped VALUES (tracing/observability) while still detaching from the
  caller's cancellation, with a fresh bounded timeout.
- client.go: the "Campaign created ... lifetime" step used %.2f, misreporting a
  sub-cent accepted budget as $0.00; switched to %g so the step reflects the
  value actually sent.
- client.go: the definite-failure ad branch said "Reddit rejected the ad" even
  for pre-send failures (token/build/dial — Reddit never received it). Now words
  a 4xx as "Reddit rejected" and a pre-send failure as "failed before it reached
  Reddit / not created".

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
@mrautela365

Copy link
Copy Markdown
Contributor Author

Thanks @dealako — addressed the latest Copilot round (all 3, in bc5f1ee), CI green:

  • Token-refresh tracing — the coalesced refresh detached with context.Background(), dropping request-scoped values; it now uses context.WithoutCancel(ctx) so an injected tracing/observability transport can still correlate the token request with the campaign operation, while keeping the single-flight detachment from any one caller's cancellation.
  • Budget precision — the "Campaign created … lifetime" step used %.2f, misreporting a sub-cent accepted budget as $0.00; switched to %g so it reflects the value actually sent.
  • Ad-failure wording — the definite-failure branch said "Reddit rejected the ad" even for pre-send failures (token/build/dial); it now distinguishes a 4xx ("Reddit rejected") from a pre-send failure ("failed before it reached Reddit; the ad was not created").

All threads resolved; gofmt/go vet/golangci-lint (0 issues)/go test -race clean. Ready for re-review — thanks!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

dealako
dealako previously approved these changes Jul 14, 2026

@dealako dealako left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mrautela365 — great follow-up round, thanks for turning this around so fast.

👏 Nice work:

  • The secretlint fix is better than what I would've suggested. Rather than an allowlist/config workaround, you removed the literal user:pass@host fixtures entirely and compose them at runtime via urlWithUserinfo() — that keeps the test's real intent (exercising the userinfo-rejection/redaction paths) while giving the linter nothing to false-positive on. Verified: MegaLinter/secretlint is green on bc5f1ee.
  • The EventName/Project length validation (770d893) is correctly placed before any mutating call, uses rune counts (Unicode-safe), and you went further than asked by also validating the composed campaign/ad-group names (region + objective + geo-list can push a name past Reddit's 200-char limit even when the per-field inputs are within bounds) — good catch, that's a real orphan-prevention gap the per-field check alone wouldn't have caught.
  • The apiError/transportError split (createOutcomeAmbiguous / isPreSendDialError) is a genuinely nice piece of design — it gives a single, consistent answer across the campaign/ad-group/ad create paths to "did this maybe get created or definitely not," instead of the ad-hoc ctx.Err()-only check from before. Traced through all three call sites; the classification is sound and each branch (pre-send dial error / definite 4xx / ambiguous 5xx-or-transport) is covered by a dedicated test.
  • context.WithoutCancel(ctx) for the token-refresh detach is the correct fix for the tracing-context-loss issue Copilot flagged — preserves request-scoped values for correlation while still not letting one caller's cancel tear down a refresh other waiters depend on.

I spawned a fresh security-auditor and code-reviewer-pro pass against just this diff (a411c1d..bc5f1ee), traced the key claims myself, and confirmed CI (all checks) + local go build/go vet/go test -race are green.

Revision tracking (from my initial review round)

  • Resolved — secretlint CI failure (770d893)
  • Resolved — missing EventName/Project length validation (770d893, extended in 0f5c218/c6dc0ea to the composed names)
  • Resolved — the 3 Copilot findings from the bc5f1ee round (subreddit-ID/name contract doc, ConversionPixelID gating x2, Project normalization, partial-result contract docs, flaky test) — all threads show resolved on GitHub.

Issue count

  • 🔴 Blocking: 0
  • 🟡 Minor: 1 — composed campaign name length guard is correct but untested (inline comment on client.go:1256)
  • ⚪ Nit: 1 — context.WithoutCancel cancel-survival isn't directly asserted by a dedicated test (inline comment on client.go:451)
  • No new security findings from either subagent or my own pass.

Final decision

Approved with minor comments — nothing here blocks merge; the one minor item is a coverage gap on an already-correct guard, not a correctness or security issue.

Comment thread internal/platform/reddit/client.go
Comment thread internal/platform/reddit/client.go
…el-survival (PR #21)

Address David's minor/nit test-coverage items:

- client_test.go: added a case to TestCreateCampaign_RejectsOverlongNameBeforeAnyPOST
  where the per-field EventName/Project caps PASS but the COMPOSED campaign name
  (with template + region + objective segments) exceeds redditMaxNameRunes, so the
  reachable composed-campaign-name check is now exercised.
- client_test.go: TestRefreshToken_LeaderCancelSurvives directly asserts the
  context.WithoutCancel behavior — the leader's caller ctx is cancelled mid-fetch;
  the cancelled leader returns its ctx error promptly (by design), but the detached
  fetch still completes, proven by a follower on the same refresh receiving the
  token and the endpoint being hit exactly once.

Verified: gofmt, go build, go vet, golangci-lint (0 issues),
go test -race ./internal/platform/reddit.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
@mrautela365

Copy link
Copy Markdown
Contributor Author

Both test items added in b72fffc:

  • Composed campaign-name limit — added a case where the per-field EventName(120)/Project(40) caps pass but the composed campaign name exceeds 200, exercising the reachable check.
  • Cancel-survivalTestRefreshToken_LeaderCancelSurvives now directly cancels the leader's caller ctx mid-fetch and confirms the detached WithoutCancel fetch still completes (follower gets the token, endpoint hit once), while the cancelled leader returns its ctx error promptly.

go test -race clean (0 issues). Thanks!

@dealako dealako left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mrautela365b72fffc closes out both remaining items from my last pass:

  • TestCreateCampaign_RejectsOverlongNameBeforeAnyPOST now includes the case where per-field caps pass but the composed campaign name still exceeds 200 runes — verified the math (max EventName + "NA" region + "Traffic" objective + max Project + template ≈ 212 runes), confirmed it actually trips the check, and ran it locally: passes.
  • TestRefreshToken_LeaderCancelSurvives directly proves the context.WithoutCancel cancel-survival property (leader's ctx cancelled mid-fetch, follower still gets the token, endpoint hit exactly once) — exactly the gap I flagged. Ran it locally: passes.

Full suite (go test -race ./internal/platform/reddit/...), go vet, and go build all green locally, and all CI checks (including MegaLinter/secretlint) are passing on b72fffc. No new bot comments since the last round, and no open threads remain.

🔴 Blocking: 0
🟡 Minor: 0
⚪ Nit: 0

Approved — this is ready to merge from my side.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread internal/platform/reddit/client.go
Comment thread internal/platform/reddit/client.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants