Skip to content

feat(platform): add Meta Ads client (Graph API)#20

Merged
mrautela365 merged 61 commits into
mainfrom
feat/platform-meta-client
Jul 14, 2026
Merged

feat(platform): add Meta Ads client (Graph API)#20
mrautela365 merged 61 commits into
mainfrom
feat/platform-meta-client

Conversation

@mrautela365

@mrautela365 mrautela365 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

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

What it does: Graph API bearer client; Campaign→Ad Set→Ad(s) flow (PAUSED), budget-cents conversion, daily-vs-lifetime budget, objective→parameter mapping (5 objectives), promoted-object logic (page_id/pixel_id/none), placement→targeting builder, geo validation + US default + regulated-country skipping, UTM builder. Typed *APIError from the Graph error envelope; non-fatal per-variant/account-verify steps.

Design: injected credentials, no env, stdlib only, context-aware. Objective params + default placements verified against @lfx-one/shared campaign.constants.ts — 4 of 5 objectives are an exact match; leads deliberately diverges (implemented as an interim OUTCOME_TRAFFIC campaign optimizing LINK_CLICKS to the registration URL with no promoted object, because this client cannot yet build the instant lead form the TS LEAD_GENERATION/page_id contract requires — and OUTCOME_LEADS+LINK_CLICKS would need a pixel_id+custom_event_type this flow doesn't supply, which would orphan the campaign at ad-set creation; full lead-gen parity tracked in LFXV2-2665). Standalone; dispatcher adapter with the wiring PR.

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

Tests: objective mapping, campaign happy path (httptest), error mapping.

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

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

🤖 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

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 adds a new, self-contained Go package internal/platform/meta/ that ports the upstream TypeScript meta-ads.service.ts into a Meta (Facebook/Instagram) Ads Graph API client. It is the first Phase‑2 platform client and is intentionally standalone: credentials and account config are injected via NewClient, nothing reads the environment, and it uses only the standard library while remaining context-aware. The client drives the Campaign → Ad Set → Ad(s) creation flow (all PAUSED), including objective→parameter mapping, budget-cents conversion, geo validation with regulated-country skipping, placement→targeting building, promoted-object logic, and UTM URL construction, with a typed *APIError surfaced from the Graph error envelope.

Changes:

  • New Client with NewClient(creds, account, opts...) and CreateCampaign(ctx, CampaignInput) (*CampaignResult, error), plus functional options for HTTP client / base URLs.
  • Faithful port of business logic: objective mapping (5 objectives), daily-vs-lifetime budget, geo validation + US default + regulated-country skipping, placement targeting, and UTM builder; per-variant/account-verify steps are non-fatal.
  • Comprehensive httptest-based unit tests covering objective mapping, placement/geo/URL helpers, the campaign happy path, regulated-geo handling, error mapping, and input validation.

Reviewed changes

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

File Description
internal/platform/meta/client.go New Meta Graph API client: types, validation helpers, objective/placement/UTM builders, and the CreateCampaign flow.
internal/platform/meta/client_test.go Unit tests for mappings, helpers, and the campaign creation flow using an httptest server; contains a minor hand-rolled itoa helper.

Note: the port claims exact-match fidelity against @lfx-one/shared constants and meta-ads.service.ts, which live in a different repository not accessible from this checkout. Details such as budget encoding (integer cents vs. string), the JSON request body shape, and the exact objective parameter values could not be verified against that upstream source here and warrant human confirmation.


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

Comment thread internal/platform/meta/client_test.go Outdated
Copilot AI review requested due to automatic review settings July 9, 2026 20:52
@mrautela365 mrautela365 force-pushed the feat/platform-meta-client branch from ed7b94f to a5944fd Compare July 9, 2026 20:52
@mrautela365 mrautela365 force-pushed the feat/platform-meta-client branch from a5944fd to aa36994 Compare July 9, 2026 20:56

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.

Comments suppressed due to low confidence (1)

internal/platform/meta/client_test.go:473

  • This helper reimplements strconv.Itoa from the standard library. Prefer using strconv.Itoa(n) at the call sites (adding "strconv" to the imports) and removing this helper to reduce hand-rolled code.

Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/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 no new comments.

Copilot AI review requested due to automatic review settings July 9, 2026 21:05
@mrautela365 mrautela365 force-pushed the feat/platform-meta-client branch from aa36994 to 7f8257d Compare July 9, 2026 21:05
@mrautela365

Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed (round 2)

  • client.go: account-verify + per-variant ad errors now surface the actual error text (rune-safe truncateErr) instead of the misleading "check server logs" (package does no logging) (per copilot[bot])
  • client.go: fixed the godoc blank-line so the comment attaches to collapseSpacesToDash (per copilot[bot])

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 1 comment.

Comment thread internal/platform/meta/client.go
Copilot AI review requested due to automatic review settings July 9, 2026 21:16
@mrautela365 mrautela365 force-pushed the feat/platform-meta-client branch from 7f8257d to a9a3e57 Compare July 9, 2026 21:16

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.

@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.

Quick pass focused specifically on resilience, comparing this client against the sibling platform clients (#19 Twitter, #21 Reddit, #22 LinkedIn) built in the same batch.

🟡 Minor: 1 issue — doRequest has no 429 rate-limit backoff/retry, unlike the Twitter client in #19 which handles it explicitly. CreateCampaign here makes several sequential Graph API calls (campaign → ad set → ad), so a transient rate limit mid-flow fails the whole operation instead of recovering.

No blocking or security findings from this pass — credential injection looks clean (no env reads, no logging of the bearer token).

Suggest either porting the Twitter client's retry/backoff helper here (and to #22, which has the same gap), or factoring it into a shared helper so all four platform clients get the same rate-limit behavior for free.

Comment thread internal/platform/meta/client.go Outdated
mrautela365 added a commit that referenced this pull request Jul 10, 2026
Address David's resilience finding on PR #20: doRequest treated a 429 the
same as any other non-2xx and returned immediately, so a transient rate
limit during CreateCampaign's sequential Graph API calls (campaign -> ad
set -> ad) aborted the whole flow.

- Retry a 429 up to retryMax (3) times with bounded backoff, honoring
  Retry-After (delay-seconds or HTTP-date) and capping any single wait at
  maxRetryWait (60s); fall back to exponential backoff otherwise.
- Add an injectable clock (WithClock) for the HTTP-date path and an
  unexported withRetryBaseDelay so tests exercise retries without real
  wall-clock waits.
- sleepCtx honors context cancellation during backoff.
- Tests: retry-then-succeed, exhaust-after-retryMax, parseRetryAfter
  header matrix, and context-cancel-during-backoff (go test -race clean).

Mirrors the retry/backoff approach in the Twitter client (#19).

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings July 10, 2026 23:03
mrautela365 added a commit that referenced this pull request Jul 10, 2026
Address David's resilience finding on PR #22: doRequest mapped every
non-2xx status, including 429, straight to a returned error with no retry.
CreateCampaign drives several sequential Marketing API calls (campaign
group, campaign, dark post, creative) - exactly the burst that trips a
per-account rate limit - so a single transient 429 aborted the whole flow.

- Retry a 429 up to retryMax (3) times with bounded backoff, honoring
  Retry-After (delay-seconds or HTTP-date) and capping any single wait at
  maxRetryWait (60s); fall back to exponential backoff otherwise. The
  final attempt returns the standard non-2xx error rather than looping.
- Reuse the existing injectable clock (WithClock) for the HTTP-date path;
  add an unexported withRetryBaseDelay so tests exercise retries without
  real wall-clock waits.
- sleepCtx honors context cancellation during backoff. The response body
  is now closed explicitly on every path (the retry loop precludes defer).
- Tests: retry-then-succeed, exhaust-after-retryMax, parseRetryAfter
  header matrix, and context-cancel-during-backoff (go test -race clean).

Mirrors the retry/backoff approach in the Twitter client (#19) and the
matching fix in the Meta client (#20).

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 2 out of 2 changed files in this pull request and generated 6 comments.

Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/client.go Outdated
mrautela365 added a commit that referenced this pull request Jul 10, 2026
- buildUTMURL: preserve URL fragment when appending UTM params
- validate calendar dates (reject impossible dates like 2026-13-40)
- reject sub-cent budgets that round to zero before any API call
- run deterministic placement/promoted-object validation before the
  first mutating campaign create
- require PageID up front (creatives need it)
- surface the raw error body instead of pointing at nonexistent logs
- tests for each

Address Copilot review comments on PR #20.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings July 10, 2026 23:33
- Fail before any mutating call when the account preflight reports a known-inactive
  account_status (disabled/closed/pending review, etc.) instead of proceeding to
  paid creation that Meta rejects later; status 0/unknown is allowed through.
  Credit cursor.
- Update the meta knowledge doc so the currency section matches the authoritative
  supported-currency map (no fall-through default; unknown codes fail closed) and
  documents the account_status check. Credit copilot[bot].
- Add TestCreateCampaignRejectsInactiveAccountBeforeAnyPost.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Add the remaining Meta-supported two-decimal ad-account currencies (ARS, BDT,
  BOB, COP, CRC, DZD, EGP, GTQ, HNL, KES, MOP, NGN, NIO, PEN, PKR, QAR, TWD, UYU)
  to the authoritative currencyMinorUnitOffset table so persisted connections for
  those accounts (which carry no CurrencyOffset) aren't blocked by the fail-closed
  path. Extended TestCurrencyOffsetForAuthoritativeMap. Credit copilot[bot].
- Move the internal/platform/meta concept-doc Creation entry under the 2026-07-13
  heading (its timestamp) to keep the knowledge log chronological. Credit
  copilot[bot].

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Trim and lowercase in.Objective in place before all consumers, so a padded or
  mixed-case value like " Traffic" resolves to the canonical objective instead of
  failing the objectiveParams lookup as unknown, and a whitespace-only value
  correctly defaults to traffic. Mirrors the EventName normalization. Credit
  cursor.
- Add TestCreateCampaignNormalizesObjective.

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…set (PR #20)

Address the latest Copilot round (both real 100× overspend risks):

- client.go: moved IDR, HUF, COP, CRC, TWD from the offset-100 group to
  offset-1. They have minor units in general ISO usage but Meta's Marketing
  API bills ad amounts in whole units for them (verified against
  developers.facebook.com/docs/marketing-api/currencies). At 100 a 500,000 IDR
  budget was encoded as 50,000,000 — a 100× overspend.
- client.go: when an explicit AccountConfig.CurrencyOffset is set AND the
  preflight returns a recognized currency whose true offset differs, reject
  before any mutation instead of trusting the override. A stale override (e.g.
  CurrencyOffset:100 on an account whose currency is now JPY, true offset 1)
  would otherwise mis-scale the budget 100×. The account currency is
  authoritative; the override is only trusted when the preflight currency is
  unrecognized.
- client_test.go: corrected the currency-map assertions (COP/TWD/IDR/HUF/CRC=1);
  replaced the override-bypasses-preflight test with a
  must-match-preflight-currency test (conflict rejected with 0 POSTs, agreeing
  override accepted); the overflow test now uses an unrecognized preflight
  currency so the overflow guard remains the thing under test.

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

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Address Cursor: when GeoTargets mixed eligible + ineligible codes (e.g. US +
IR), validateGeoTargets silently removed the ineligible/sanctioned entries and
creation proceeded with no step — unlike regulated countries (SG/TW/KR), which
already emit an explicit "skipped" step. A caller could therefore believe an
excluded country was being targeted.

- client.go: after validateGeoTargets, compute the supplied-but-dropped codes
  (bogus/non-ISO, or Meta-ineligible/sanctioned like IR/CU/KP/RU) and add an
  explicit "Geo targets dropped" step naming them, mirroring the regulated-geo
  step. The empty-input US fallback is unaffected.
- client_test.go: TestCreateCampaignReportsDroppedIneligibleGeos (US+IR -> IR
  named in a drop step, targeting keeps US).

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

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…fset (PR #20)

Address the latest review round:

- client.go: a server-DECLARED Retry-After that exceeds maxRetryWait now ABORTS
  with the rate-limit error instead of clamping to maxRetryWait and retrying
  early — clamping retried while Meta was still throttling, burning attempts and
  stalling the synchronous flow. The exponential fallback (no server reset) still
  caps. Mirrors the twitter/reddit clients. (per copilot[bot])
- client.go + internal-platform-meta.md + CurrencyOffset field doc: corrected the
  now-stale precedence wording — the account currency is authoritative, a
  conflicting explicit CurrencyOffset is rejected, and the explicit value is only
  a fallback when the preflight currency can't be resolved. Prevents a future
  change from restoring the 100× budget-scaling risk. (per copilot[bot])
- client_test.go: TestDoRequestAbortsOnOverCapRetryAfter (one request, no
  clamp+retry).

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

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…aign (PR #20)

Address Copilot: the interim `leads` mapping used OUTCOME_LEADS + LINK_CLICKS,
but Meta requires a pixel_id + custom_event_type for that objective/optimization
pairing — which this flow (no lead-form/pixel support yet) does not supply, so
the campaign POST would succeed and the ad-set POST would then reject every
leads request, orphaning the paused campaign.

- client.go: remapped `leads` -> OUTCOME_TRAFFIC + LINK_CLICKS (no promoted
  object). OUTCOME_TRAFFIC cleanly supports LINK_CLICKS optimization with no
  pixel requirement, so the ad-set POST always succeeds — a spendable interim
  website-traffic configuration. Full LEAD_GENERATION / instant-form (or
  OUTCOME_LEADS + pixel) parity stays deferred to LFXV2-2665. Updated the mapping
  comment, the top-of-table note, and the Objective field doc.
- internal-platform-meta.md: corrected the section to describe the OUTCOME_TRAFFIC
  interim mapping and why OUTCOME_LEADS+LINK_CLICKS is avoided.
- client_test.go: updated the objective assertions to OUTCOME_TRAFFIC.

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

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…(PR #20)

MegaLinter's secretlint basic-auth rule flagged two literal
"https://user:pass@..." test fixtures in client_test.go, failing CI. Compose
them at runtime via a urlWithUserinfo() helper (as done for the reddit client)
so no literal credential appears in source; the userinfo-rejection paths are
exercised identically.

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

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…; partial on no-id (PR #20)

Address the latest Copilot/Cursor round:

- client.go: the all-geos-invalid error now NAMES the dropped codes (the
  "Geo targets dropped" step is discarded when returning nil, so the caller
  otherwise got no specifics). (cursor)
- client.go: the over-cap Retry-After abort now preserves the Graph envelope's
  Type/Code/FBTraceID and original message (prepended with the rate-limit
  context) instead of discarding them — support diagnostics are needed exactly
  when a rate limit is hit. (per copilot[bot])
- client.go: a 2xx campaign create with no id now returns a partial result
  carrying the campaign NAME (reconcilable by name) with an UNCONFIRMED note,
  not a bare (nil, err). Full retry-safety needs a provider idempotency key Meta
  doesn't expose — tracked in LFXV2-2665. (per copilot[bot])
- client_test.go: TestCreateCampaignNoIDReturnsPartial,
  TestCreateCampaignAllGeosInvalidNamesThem.

Also updated the PR description's leads mapping to OUTCOME_TRAFFIC (was stale).

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

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
…query URL (PR #20)

Address Copilot (both verified against Meta docs):

- client.go: removed 201 (ANY_ACTIVE) and 202 (ANY_CLOSED) from the
  inactive-account-status map. 201/202 are Meta AGGREGATE/filter values, not
  per-account statuses — 201 denotes an ACTIVE aggregate, so listing it wrongly
  rejected active accounts.
- client.go: validateRegistrationURL now rejects a URL whose query can't be
  cleanly parsed (url.ParseQuery error, e.g. bad percent-encoding or an
  unescaped ';'), before any mutating call — buildUTMURL rebuilds via u.Query()
  which silently drops such a pair, so the ad's click URL could otherwise differ
  from the caller's.
- client_test.go: TestCreateCampaignAcceptsAnyActiveAccountStatus (201 accepted),
  TestValidateRegistrationURLRejectsMalformedQuery.

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

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

Address Copilot: the ad-creative name is composed ("<EventName> - Variant N")
and Meta caps creative names at 255 chars, but only the per-field copy
(primary/headline/description) was length-validated — a long EventName would
pass, create the campaign + ad set, then fail at every creative (orphaning both).

- client.go: added maxCreativeNameChars (255) and validate the composed creative
  name for each variant in the existing up-front, before-any-POST copy-limit loop.
- client_test.go: TestCreateCampaignRejectsOverlongCreativeNameBeforeAnyPost.

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

Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Comment thread internal/platform/meta/client.go Outdated
buildCampaignName appended a 9th StartDate segment after Funnel, diverging
from the TypeScript reference (meta-ads.service.ts) and every other Go
platform client, which all emit the 8-segment name ending in MoFU. Drop
the trailing Date segment to restore cross-client consistency.

Addresses Cursor review on PR #20.

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 4 out of 5 changed files in this pull request and generated 5 comments.

Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go Outdated
Comment thread docs/knowledge/log.md Outdated
Comment thread internal/platform/meta/client.go
Address Copilot review on PR #20:
- compose the ad-creative name from the trimmed EventName so the length
  check matches what is actually sent
- trim whitespace in validateRegistrationURL, matching every other input
- normalize (trim+lowercase) the objective inside buildCampaignName so the
  directly-unit-tested helper is robust to un-normalized callers
- collapse the redundant meta concept-doc log entry into a single Creation

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

@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 6 total unresolved issues (including 5 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 f31a4a9. Configure here.

Comment thread internal/platform/meta/client.go
validateRegistrationURL trims its own copy, but buildUTMURL reads
in.RegistrationURL directly, so a padded URL passed validation yet was
concatenated un-trimmed into the creative click URL. Normalize
RegistrationURL in place at the top of CreateCampaign, ahead of both
validation and UTM construction, and assert a padded URL yields a clean
creative link.

Addresses Cursor review on PR #20.

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 4 out of 5 changed files in this pull request and generated 3 comments.

Comment thread docs/knowledge/log.md
Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/client.go Outdated
…Retry-After

Address Copilot review on PR #20:
- docs/knowledge/log.md: restore the two internal/platform/reddit entries
  (2026-07-13 frontmatter Update and 2026-07-10 Creation) that the union
  rebase dropped; the log is append-only, so the meta entry is added
  alongside them rather than replacing history
- client.go: route audience_network through the addPlatform helper so it
  shares the seenPlatforms de-duplication like every other placement
- client.go: include the raw Retry-After header value alongside the parsed
  duration in the rate-limit abort message for clearer upstream debugging

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 4 out of 5 changed files in this pull request and generated 4 comments.

Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client_test.go
…mming

Address Copilot review on PR #20:
- client.go: parseRetryAfter clamps an oversized reset to maxRetryWait+1s, so
  the abort message's "parsed" duration could read 1m1s even when the header
  said 600s or a far-future date. Report the raw Retry-After header as the
  authoritative value instead of the clamped sentinel.
- client.go: trim RegistrationURL and EventName inside buildUTMURL so the
  helper is robust to un-normalized direct callers (tests), rather than
  relying only on CreateCampaign to pre-normalize.

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 4 out of 5 changed files in this pull request and generated 4 comments.

Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/client.go Outdated
Address Copilot review on PR #20:
- client.go: base the "caller supplied geos" checks on the count of NON-BLANK
  entries, so GeoTargets like []string{"   "} default to US like empty input
  instead of erroring with an empty "(dropped: )" list
- client.go: reject a variant missing primary text or headline by naming its
  1-based index rather than silently dropping it, which would renumber the
  surviving variants (ad numbering, "Variant N", utm_content=variant-N)
- client.go: compare parsed time.Time values (!endDate.After(startDate))
  instead of lexicographic string ordering for the end>start check
- tests: blank-only geos succeed with no "dropped" step; a partial variant
  errors naming "variant 2"

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 4 out of 5 changed files in this pull request and generated 5 comments.

Comment thread internal/platform/meta/client.go Outdated
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go Outdated
…docs

Address Copilot review on PR #20:
- treat an ambiguous campaign-create failure (timeout/5xx) as reconcilable:
  return a partial result carrying the campaign name + UNCONFIRMED note
  instead of discarding it, mirroring the Reddit client
- ad/creative create errors with ambiguous outcomes now emit an UNCONFIRMED
  step rather than a definite "create manually" that could duplicate
- add displayMetaUTMURL (allowlist-rebuilt query, no userinfo/fragment) so
  the display Step can't leak a caller's ?token=... via persisted logs;
  the real ad click URL still uses the caller's full destination
- raise adSetStartBuffer to 10m so a same-day start_time survives the worst-
  case retry budget
- document CreateCampaign's non-nil-result-with-error partial contract

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 4 out of 5 changed files in this pull request and generated 2 comments.

Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/client.go
… out

Address Copilot review on PR #20:
- doRequest only unmarshals the Graph error envelope on non-2xx responses;
  a 2xx success body never populates it, so the per-response unmarshal was
  wasted work on the common path
- move the large iso3166Alpha2 lookup table into a dedicated countries.go so
  the static data no longer sits inline with the core client logic

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 6 comments.

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

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/meta/client.go
Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/countries.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.

Hi @mrautela365 — comprehensive review of the Meta Ads Graph API client (round 3+, HEAD 5c8d874).

Overall impression

This is exceptionally well-hardened, defensively-written code. The validation-before-mutation ordering (nothing deterministic runs after the first mutating POST), the partial-result / UNCONFIRMED reconcile-by-name contract, the currency-offset resolution with authoritative-account precedence, the int64 overflow guard on the scaled budget, and the 429 + Graph-throttle-code retry with bounded backoff and context-aware sleep are all correct and thoroughly tested. go build, go vet, go test, and gofmt -l are all clean. I ran parallel security and code-quality subagents plus my own trace-through of doRequest, the budget/currency path, parseRetryAfter, and the ambiguous-outcome classification — no blocking correctness, security, or contract defects.

Security is solid: the bearer token only ever rides in the Authorization header (never a URL/query/error/log), request bodies are all json.Marshal'd (no string-concatenated JSON), AccountID/PageID/PixelID are anchored-regex validated before any path interpolation, responses are capped at 10 MiB, and the Steps output uses the sanitized displayMetaUTMURL allowlist so a secret in a RegistrationURL query never lands in a persisted step.

Issue count

  • 🔴 Blocking: 0
  • 🟡 Minor: 2 — both are test-coverage gaps on defensive branches (memory-safety cap; the "not sent ⇒ no partial" dial-error arm). No behavioral defect.
  • ⚪ Nit: 3 — all previously raised by Copilot and still open (transportError.Path prefix, objectiveKeys drift, date-format error wording). Reconciliation below.
  • ❔ Question: 0

AI comment reconciliation

182 of 187 review threads are resolved. The 5 open threads are all Copilot nits, and I agree with three of them:

  • client.go:1964/1983transportError.Path is "/adcreatives" / "/ads" but the real request path is account-prefixed (/act_.../adcreatives); the error string is slightly less actionable. Agree, nit.
  • client.go:1988objectiveKeys() is a hand-maintained list that can drift from the authoritative objectiveParams map. Agree, nit — deriving+sorting from the map (or a keyset-equality test) removes the trap.
  • client.go:1462 — the "invalid ... date format" message also fires for well-formed-but-impossible dates (e.g. 2026-13-40), which time.Parse correctly rejects; the wording could distinguish invalid value from invalid format. Agree, nit.

The other two open Copilot threads (large test file split; label-map drift) are optional and I'd leave to author preference.

Revision tracking

Every substantive finding from prior rounds — orphaned-campaign-on-input-error (validation reordered), UTM fragment/secret handling, past-start-date rejection, currency-offset scaling, budget overflow, 429 + Graph-throttle retry, connection-drain on retry, per-variant and account-verify non-fatal paths — has been resolved with an accompanying test. No regressions spotted in the new commits.

Decision

Approved with minor comments. None of the open items block merge; the two test-coverage gaps and the three nits are worth a quick follow-up (this PR or a fast-follow) but shouldn't hold the batch. Nice work absorbing an unusually heavy review load.

Comment thread internal/platform/meta/client.go
Comment thread internal/platform/meta/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.

4 participants