feat(platform): add X/Twitter Ads client (OAuth 1.0a)

[mrautela365]

First Phase-2 platform client. Ports x-ads.service.ts from the Express BFF to Go as a standalone package internal/platform/twitter/.

What it does: OAuth 1.0a HMAC-SHA1 signing (stdlib), campaign→line-item→promoted-tweet creation with name-based idempotency, micro-currency conversion, ISO8601 UTC dates, 1 req/sec write delay + 429 exponential-backoff retry (context-aware).

Design: credentials are injected (Credentials+AccountConfig), not read from env — in production they come from the decrypted stored connection (D7). No new deps (stdlib only). Does not import the orchestrator; a thin PlatformDispatcher adapter lands with the wiring PR once #11 merges.

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

Tests: 14 unit tests (deterministic OAuth1 signature, currency round-trip, date formatting, 429-retry via httptest) — no network/real creds.

Not ported (separate later PR): status-toggle + analytics/monitoring read path.

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

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adds the first Phase-2 platform client: a standalone internal/platform/twitter/ package that ports the Express BFF's x-ads.service.ts to Go. It implements OAuth 1.0a (HMAC-SHA1) request signing with stdlib only and drives the X Ads campaign → line_item → promoted_tweet creation flow with name-based idempotency. Credentials and account config are injected via NewClient (never read from env/DB), keeping the package decoupled from the orchestrator until the wiring PR (#11) lands.

Changes:

• New X Ads client with OAuth 1.0a signing, micro-currency/ISO8601 helpers, 1 req/sec write delay, and 429 exponential-backoff retry that honors context cancellation.
• Public API NewClient(...) → CreateCampaign(ctx, CampaignInput) returning a step-logged CampaignResult, reusing existing campaigns/line items by name for idempotency.
• 14 deterministic unit tests (OAuth signature golden value, currency round-trip, date formatting, UTM building, 429 retry/exhaustion, context cancellation, full create flow, idempotency).

Reviewed changes

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

File	Description
internal/platform/twitter/client.go	New OAuth 1.0a signer + X Ads campaign creation flow, retry/rate-limit handling, and conversion/idempotency helpers.
internal/platform/twitter/client_test.go	Unit tests covering signing determinism, validation, retry behavior, and the end-to-end create/idempotency flows via httptest.

Notes for reviewers: One moderate correctness issue is flagged inline — X-Rate-Limit-Reset is a Unix epoch timestamp but is treated as a delay in seconds, so 429 retries relying on that header always wait the full 60s cap. This is also security-sensitive OAuth signing code whose golden signature value could not be independently verified during this review.

---

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[MRashad26]

Two findings on the X/Twitter Ads client.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[dealako]

@mrautela365 thanks for the persistence through this many review rounds — the OAuth1 signing, idempotent find-or-create flow, pagination, rate-limit header precedence, and input validation are all in solid shape at this point, and the test suite is thorough (deterministic signature goldens, 429-retry via httptest, guard-branch coverage). Nice work getting this porting job this tight.

How this review was done: I ran a security-focused subagent and a general code-quality subagent in parallel against the current diff (gh pr diff 19) and the full client.go/client_test.go, cross-checked their output against the existing Copilot review threads on this PR, and traced the two still-open findings myself in the code before posting.

Revision history: This PR has been through 13 rounds of fixes already (see commit history: rate-limit header precedence, pagination with q= filter, whitespace/format validation on IDs, response-body read-error surfacing, naming-convention date segment, etc.) — all confirmed resolved, no regressions found. The two items below are new, from the Copilot pass that landed after your most recent fix commit (1282828), so they haven't been addressed yet.

Issue count:

• 🔴 Blocking: 2 — Retry-After integer overflow bypassing maxRetryWait abort; confirmed campaign/line-item IDs discarded on downstream failure paths
• 🟡 Minor: 0
• ⚪ Nit: 0
• ❔ Question: 0

Both blocking items were independently surfaced by Copilot's latest pass and confirmed by both my own trace and the code-reviewer-pro subagent — no false positives found, security-auditor subagent found nothing further (credential handling, injection risks, and defaults all check out clean).

Decision: 🔴 Needs changes before approval — both blocking items are inline above with proof and suggested fixes.

[Copilot]

Pull request overview

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

[mrautela365]

Both blocking findings addressed — ready for re-review @dealako

Both [blocking] items from your review landed in 19774f8 (pushed right after your review submitted, so it reviewed the prior commit):

• Retry-After overflow (client.go:473) — parseRetryAfter now uses strconv.ParseInt(v,10,64) and guards the seconds→Duration multiply: if n > int64(maxRetryWait/time.Second) { return maxRetryWait + time.Second }. A huge value returns above the cap so the > maxRetryWait abort fires instead of wrapping negative and retrying early. Epoch header paths use time.Time.Sub (Go clamps, no wrap). Test: TestParseRetryAfterOverflowGuard.
• campaignID/lineItemID discarded on downstream failures (client.go:~1014) — added a partialResult() closure after the campaign is created; every downstream fatal return now returns a partial *CampaignResult (CampaignID, LineItemID once created, + Steps) alongside an error naming the created resource, instead of nil. Mirrors the meta/reddit pattern. Tests: TestPartialResultAfterCampaignCreated, TestPartialResultAfterLineItemCreated.

Both threads resolved. CI green on HEAD (CodeQL / MegaLinter / License / DCO / OKF; Build in progress). Re-requesting your review.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[dealako]

@mrautela365 great turnaround — both blocking items from my last review landed correctly in 19774f8, and I re-verified rather than trusting the commit message.

How this re-review was done: fetched the branch tip (64184c1, 6 commits past what I last reviewed at 1282828), ran security-auditor and code-reviewer-pro subagents in parallel against the full diff, traced the two fixes myself in the code, cross-checked every Copilot thread opened since my last pass, and ran go build/go vet/go test locally.

👏 Nice work:

• parseRetryAfter now caps the Retry-After seconds value against maxRetryWait before multiplying by time.Second, returning an explicit over-cap sentinel rather than letting the multiply wrap — exactly closes the overflow.
• CreateCampaign now threads a partialResult() closure through every downstream failure point after campaignID (and later lineItemID) is known — all 5 failure paths verified to return the partial result, not just the two I flagged.
• The RFC 5849 URI-normalization work (lowercase scheme/host, default-port stripping, IPv6 re-bracketing, EscapedPath() for the signature base string) and the multi-valued query-param signing fix are both correct and non-trivial to get right — verified by tracing normalizeSigningURL and generateOAuthSignature directly, not just from the tests.
• Every Copilot thread opened since my last review has a reply pointing to the resolving commit, and the timestamps line up cleanly with the fix commits — no dangling threads.

Revision tracking:

• ✅ Retry-After int64 overflow bypassing maxRetryWait — resolved in 19774f8
• ✅ Confirmed campaignID/lineItemID discarded on downstream failure — resolved in 19774f8
• ✅ All Copilot findings since (naming-date-segment reuse divergence wording, line-item-reuse status drift, race in test counters, OAuth base-string normalization/IPv6/escaped-path, multi-valued query signing, base-URL trailing slash) — resolved across daf75fa..64184c1, each with a linked reply

Issue count:

• 🔴 Blocking: 0
• 🟡 Minor: 1 — HTTP-date branch of parseRetryAfter isn't symmetrically guarded against the same overflow class (theoretical, requires a ~292-years-out date; inline comment has the fix)
• ⚪ Nit: 1 — no test exercises the "reused, PRE-EXISTING" wording on a failure path (only the create-then-fail and reuse-then-success paths are covered); not required, just a coverage gap for the next person debugging a reused-resource failure

Decision: ✅ Approved with minor comments — the one minor item is optional polish, not a blocker; the two prior blocking issues are correctly and thoroughly fixed.

[dealako]

[minor] HTTP-date branch of parseRetryAfter isn't guarded against the same overflow class you just fixed

Issue: The seconds-form branch above (line ~526-527) now caps n against maxRetryWait before multiplying, to avoid time.Duration wrapping. The HTTP-date branch (t.Sub(c.timeFn())) has no equivalent guard.

Proof: time.Duration is an int64 count of nanoseconds, so a Retry-After HTTP-date more than ~292 years out would make t.Sub(...) overflow and wrap to a negative/small value, which — like the seconds-form bug — would slip past the caller's waitDur > maxRetryWait abort and trigger an immediate retry instead of aborting.

Why it matters: this is the same bug class as the one you just fixed, just reached via the HTTP-date format instead of the seconds format. Low likelihood (requires an adversarial/badly-misconfigured upstream sending a far-future date), but cheap to close for consistency now that the seconds path is hardened.

Fix: apply the same cap:

if t, err := http.ParseTime(v); err == nil {
    if d := t.Sub(c.timeFn()); d > 0 {
        if d > maxRetryWait {
            return maxRetryWait + time.Second
        }
        return d
    }
}

Not blocking — flagging for consistency; up to you whether it's worth a follow-up.