[ms-bing-ads-audiences] Add temporary debug logging of API responses

[harsh-joshi99]

Summary

Non-bulk destinations don't persist the responses received from the destination, which makes debugging the MS Bing Ads Audiences sync difficult — in particular, getting Microsoft's tracking/request id needed to raise Bing Ads support tickets.

This PR adds temporary debug logging, gated behind the actions-ms-bing-ads-audiences-debug-logging feature flag (off by default). It logs only successful responses from each CustomerListUserData/Apply call to the centralized logging pipeline:

• Microsoft's tracking/request id (from the TrackingId / x-ms-trackingid / RequestId response header, or a top-level body id field) — logged in full, never truncated, for quoting to Bing Ads support
• non-sensitive request metadata: action (Add/Remove), audience id, identifier type, item count — not the CustomerListItems themselves
• a redacted summary of any PartialErrors (ErrorCode / Code / Index / Type only)

Per-item failures are reported by Bing as PartialErrors on the 200 response, so the success log covers them too. The error path (hard HTTP failures — auth, 5xx) is intentionally not logged; those flow through the existing handleHttpError / rethrow path unchanged.

Safety

• No PII. CustomerListItems (hashed emails / unhashed CRM ids) are never logged, and the PartialError free-text fields (Message / Details / FieldPath) — which can echo back an identifier — are dropped.
• No log injection. Every interpolated value is control-char stripped; large fields are length-capped (without splitting a surrogate pair).
• No behavior change. Logging is read-only, wrapped so a throwing logger can never alter the action's control flow, and is a complete no-op unless the flag is enabled. All log lines are prefixed [ms-bing-ads-audiences][DEBUG] for easy searching.

This is intended to be removed once debugging is complete — every added block is marked with a // TEMPORARY comment.

Flagon: https://flagon.segment.com/families/centrifuge-destinations/gates/actions-ms-bing-ads-audiences-debug-logging

⚠️ Note: this is a customer-match destination. Logging is gated and intended to be on only while actively debugging, and is scoped to non-sensitive metadata + redacted error codes + the opaque tracking id.

Testing

• Unit tests pass (npx jest src/destinations/ms-bing-ads-audiences — 41 passed)
• Typechecks (tsc --noEmit) and lints clean
• Added unit tests for new functionality (flag on/off, tracking id from header + body + full-length, PII redaction, control-char stripping, strict cap, throwing-logger safety, empty body, error path logs nothing)
• Tested end-to-end using the local server
• [If destination is already live] Tested for backward compatibility — no field changes; logging is a no-op unless the flag is enabled.

Security Review

• Reviewed all field definitions for sensitive data — no field definitions changed.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds feature-flagged (“actions-ms-bing-ads-audiences-debug-logging”, default off) debug logging to the MS Bing Ads Audiences syncAudiences action so request/response details can be captured in centralized logs for troubleshooting; the PR also introduces a new destination metadata.json file.

Changes:

• Gate and emit temporary debug logs for Apply request payloads and Bing Ads responses (success + error paths) in syncAudiences.
• Thread features + logger into syncUser to control logging behavior via feature flag.
• Add packages/destination-actions/src/destinations/ms-bing-ads-audiences/metadata.json (new file).

Reviewed changes

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

File	Description
packages/destination-actions/src/destinations/ms-bing-ads-audiences/syncAudiences/index.ts	Adds feature-flagged temporary debug logging and plumbs features/logger through execution paths.
packages/destination-actions/src/destinations/ms-bing-ads-audiences/metadata.json	Adds a generated-looking destination metadata payload file (needs confirmation it’s intended to be committed).

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[harsh-joshi99]

Note

Self-Review
Posted by PR author (harsh-joshi99) — self-reviews cannot apply the APPROVED label. Intended verdict: Manual Review Needed ⚠️. Ask a teammate to run plugin-review for the APPROVED label.

Code Review Personae Verdict: Manual Review Needed ⚠️

No production-crashing bugs: all NPE candidates are guarded and the response.clone()/handleHttpError ordering is verified safe (ms-bing-ads does not set skipResponseCloning, so the original error body is left intact for handleHttpError). However, the new debug logging has two real issues that should be fixed before merge: (1) raw upstream response/error bodies are interpolated into log lines with no control-character sanitization (log injection), and (2) those full bodies can contain PII — Bing PartialErrors may echo identifiers and CRM IDs are unhashed — which undercuts the PR's stated 'no CustomerListItems are logged' guarantee.

Both are gated behind a default-off, temporary feature flag, which lowers real-world impact. A medium robustness gap (unguarded logger throw in the catch path can mask the original error and skip handleHttpError) was independently flagged by 2 of 3 passes.

Warning

Required Actions

1. Sanitize control chars/newlines before logging response and error bodies (packages/destination-actions/src/destinations/ms-bing-ads-audiences/syncAudiences/index.ts:83)
2. Avoid logging full response/error bodies that can echo unhashed CRM IDs / identifiers, or redact identifier-bearing fields (packages/destination-actions/src/destinations/ms-bing-ads-audiences/syncAudiences/index.ts:105)

---

📝 Deep Code Review (3 Passes) — 2 High, 2 Medium ❌

Three independent bug-finders reviewed the diff plus the surrounding ms-bing-ads-audiences source and @segment/actions-core Response semantics.

Consensus / verified-safe (all passes agree): no NPE risk — sentPayload.CustomerListUserData always exists (preparePayload returns it unconditionally), response.content is always a string and is additionally guarded with ?? '', error.response is guarded with ?., and attempted?.CustomerListUserData is gated before dereference. The response.clone()/handleHttpError double-read is correctly ordered and safe: ms-bing-ads does not set skipResponseCloning, so prepareResponse reads only a throwaway clone and leaves error.response unconsumed; logBingAdsError reads a second clone and handleHttpError reads the original via .json(). attemptedPayload tracking matches the in-flight operation across all branches, and the existing 'throw non-HTTP errors' test is unaffected (no logger passed -> early return).

The findings below are about logging behaviour, not crashes. Note all of them sit behind the default-off, temporary actions-ms-bing-ads-audiences-debug-logging flag.

The 'unguarded logger throw' finding was raised by 2 of 3 passes. A clone-under-skipResponseCloning concern is a pre-existing risk in handleHttpError (utils.ts), not introduced here, so it is noted but not listed as a required action.

Findings

• 🔴 HIGH: Log injection: raw response body logged without control-char sanitization (packages/destination-actions/src/destinations/ms-bing-ads-audiences/syncAudiences/index.ts:83)
  logBingAdsResponse interpolates truncate(response.content ?? '') directly into a single log string. response.content is the raw, upstream-controlled response text from Bing (prepareResponse sets it to await clone.text()). truncate() only caps length; it does not strip newlines or control characters. A body containing \n[ms-bing-ads-audiences][DEBUG] ... status=200 can forge fake log lines or inject ANSI/control sequences into the centralized logging pipeline. The same applies to the error body at line 113 (error=${truncate(body)}).

Mitigating context: gated behind a default-off, temporary debug flag.
Suggestion: Sanitize before logging, e.g. JSON.stringify(truncated) or .replace(/[\r\n\t\x00-\x1f]/g, ' '), applied to both the response-body branch (line 83) and the error-body branch (line 113).

• 🔴 HIGH: Logged response/error bodies can contain PII (unhashed CRM IDs), contradicting the PR's stated guarantee (packages/destination-actions/src/destinations/ms-bing-ads-audiences/syncAudiences/index.ts:105)
  The header comment and PR description state that CustomerListItems (hashed emails / CRM IDs) are never logged. That is true for the explicit metadata fields, but the code also logs the full body: response.content on success (line 83) and error.response.clone().text() on failure (line 105/113). The Bing CustomerListUserData/Apply API returns PartialError objects (see types.ts) with Message/FieldPath/Details fields that can echo the offending CustomerListItem value back on validation errors. CRM IDs in particular are NOT hashed (utils.ts: p.crm_id is used verbatim), so a CRM-identifier validation error could place a raw customer CRM ID into centralized logs. The 4096-char cap does not prevent a single echoed identifier from being logged.

Mitigating context: gated behind a default-off, temporary debug flag — but if enabled in production it can log unhashed identifiers.
Suggestion: Do not log raw response/error bodies, or redact identifier-bearing fields (Message/FieldPath/Details) and log only error codes + counts. At minimum, correct the PR description/comment so they don't overstate the PII guarantee, and document that CRM IDs are unhashed and may appear in logged bodies.

• 🟡 MEDIUM: Unguarded logger call in catch path can mask the original error and skip handleHttpError [Found by 2/3 passes] (packages/destination-actions/src/destinations/ms-bing-ads-audiences/syncAudiences/index.ts:179)
  In syncUser's catch block, await logBingAdsError(...) (line 179) runs before the error-handling branch. Inside logBingAdsError, only the body-read is wrapped in try/catch; the logger.error(...) call itself (line 113) is not guarded. If the injected logger throws (it is an external dependency this code does not control), that exception propagates out of the catch, the original HTTPError is lost, and the if (!isBatch) throw error / handleHttpError(...) logic never runs — so a batch call that should produce a MultiStatusResponse with per-item errors instead rejects. The same applies to logBingAdsResponse's unguarded logger.info on the success path. Debug/observability logging should never be able to change the action's outcome.
  Suggestion: Wrap the logger.info/logger.error calls (or the whole helper bodies) in a try/catch that swallows any error, so temporary debug logging can never alter control flow.
• 🟡 MEDIUM: truncate() slices on UTF-16 code units and can emit a lone surrogate (packages/destination-actions/src/destinations/ms-bing-ads-audiences/syncAudiences/index.ts:61)
  value.slice(0, MAX_LOGGED_BODY_LENGTH) cuts on UTF-16 code-unit boundaries. If the 4096th code unit lands inside a surrogate pair (emoji / non-BMP char in the body), the truncated string ends with a lone surrogate. This does not crash the logger call, but downstream log serialization/JSON encoding of a lone surrogate can produce replacement characters or be rejected by stricter sinks. Low priority — only affects the temporary debug text.
  Suggestion: Optional hardening: trim a trailing lone surrogate after slicing, or use Array.from(value).slice(0, N).join('').

🛡️ Policy Compliance Review — No violations (3 policies evaluated)

Evaluated 3 policies across all review surfaces. No violations detected.

Review Stages Executed

• ✅ Deep Code Review (3 Passes)
• ✅ Policy Compliance Review
  _{Generated by code-review-personae v0.8.7 | Deep Code Review (3 Passes), Policy Compliance Review}

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[harsh-joshi99]

Note

Self-Review
Posted by PR author (harsh-joshi99) — self-reviews cannot apply the APPROVED label. Intended verdict: Manual Review Needed ⚠️. Ask a teammate to run plugin-review for the APPROVED label.

Code Review Personae Verdict: Manual Review Needed ⚠️ (Round 2)

Delta from previous review: 2 resolved

Three independent bug-finder passes and a policy scan all came back clean against the current code (head aa5b94b). This is the first round with zero findings, following a deliberate simplification that removed the error-path logging entirely.

The remaining debug logging is success-path only, flag-gated (default off), and temporary. No production-crashing bugs, no PII leak, no log injection, no behavior change.

(Posted as COMMENT rather than APPROVE because GitHub does not allow an approving self-review.)

---

📝 Deep Code Review (3 Passes) — 0 Critical, 0 High, 0 Medium — clean

Three independent passes reviewed syncAudiences/index.ts plus the surrounding ms-bing-ads-audiences source (utils.ts, types.ts) and the @segment/actions-core Response semantics. All three returned no findings.

What was verified:

• PII: The prior HIGH leak is resolved. Error-path logging was removed, so the non-batch PartialError throw — IntegrationError(PartialErrors[0].Message) — now reaches an unlogged catch and is rethrown; the raw Message leaves the action only via the thrown error (framework-level, as it always did), never via logger. On the success path, summarizeErrors drops the free-text Message/Details/FieldPath fields, extractTrackingId pulls only the opaque TrackingId/RequestId, and CustomerListItems are never logged. Cross-confirmed by the test that plants a CRM id in a PartialError and asserts it is not logged.
• NPE: response.data / .headers / .status / sentPayload.CustomerListUserData are all accessed safely (optional chaining or guaranteed by preparePayload / the Response type).
• Log injection: every interpolated value is control-char stripped or sanitized; large fields are length-capped without splitting a surrogate pair (suffix reserved, output guaranteed <= 4096).
• Behavior change: logBingAdsResponse is read-only, flag-gated (no-op by default), and wrapped in safeLog so a throwing logger cannot alter control flow. summarizeErrors/extractTrackingId/sanitizeForLog are evaluated lazily inside the safeLog closure, so even a throw on malformed input is swallowed.
• No dead code: after the error-logging removal, every remaining import and helper (HTTPError in the catch, summarizeErrors, extractTrackingId, safeLog, sanitizeForLog, stripControlChars) is still in use.

Note: all of this is gated behind a default-off, temporary feature flag and is marked // TEMPORARY for later removal. One item is outside automated review scope: the TrackingId header/field names should be confirmed against a real Bing response in staging (logs trackingId=none if the name doesn't match).

🛡️ Policy Compliance Review — No violations (3 policies evaluated)

Evaluated 3 policies across all review surfaces. No violations detected.

Review Stages Executed

• ✅ Deep Code Review (3 Passes)
• ✅ Policy Compliance Review
  _{Generated by code-review-personae v0.8.7 | Deep Code Review (3 Passes), Policy Compliance Review}

[harsh-joshi99]

Note

Self-Review
Posted by PR author (harsh-joshi99) — self-reviews cannot apply the APPROVED label. Intended verdict: Manual Review Needed ⚠️. Ask a teammate to run plugin-review for the APPROVED label.

Code Review Personae Verdict: Manual Review Needed ⚠️ (Round 3)

Delta from previous review: 1 resolved

Fresh run: three independent bug-finder passes and a policy scan all came back clean against the current code (head aa5b94b). No production-crashing bugs, no PII leak, no log injection, no behavior change, no dead code.

The debug logging is success-path only, flag-gated (default off), and temporary. (Posted as COMMENT rather than APPROVE because GitHub does not allow an approving self-review.)

---

📝 Deep Code Review (3 Passes) — 0 Critical, 0 High, 0 Medium — clean

Three independent passes reviewed syncAudiences/index.ts plus the surrounding ms-bing-ads-audiences source (utils.ts, types.ts) and the @segment/actions-core Response semantics. All three returned no findings.

Verified:

• PII: On the success path, summarizeErrors drops the free-text Message/Details/FieldPath fields, extractTrackingId reads only the opaque TrackingId/RequestId, and only CustomerListItems.length is logged (never the values). In non-batch mode, handleMultistatusResponse throws IntegrationError(PartialErrors[0].Message) AFTER logBingAdsResponse has already run with redacted data; that throw rethrows through an unlogged catch, so the raw Message never reaches a logger.
• NPE: response.data/.headers/.status and sentPayload.CustomerListUserData are accessed safely (optional chaining or guaranteed by preparePayload / the Response type). String/array response.data is handled without crashing.
• Log injection: every interpolated value is control-char stripped or sanitized; status/itemCount are numbers and action/identifierType are constrained enums/literals.
• Truncation math: '…' is one UTF-16 unit, suffix length 12, end=4084, output guaranteed <= 4096, surrogate-pair split handled; end is never <=0/NaN (branch only entered when length > 4096).
• Behavior change: logBingAdsResponse is read-only, flag-gated (no-op by default), and wrapped in safeLog so a throwing logger or a malformed-PartialErrors throw is swallowed and cannot mask the action's control flow.
• No dead code: after the error-logging removal, every import and helper (HTTPError in the catch, summarizeErrors, extractTrackingId, safeLog, sanitizeForLog, stripControlChars) is still in use.

Note: all gated behind a default-off, temporary feature flag and marked // TEMPORARY. One item is outside automated review scope — confirm the TrackingId header/field names against a real Bing response in staging (logs trackingId=none if the name doesn't match).

🛡️ Policy Compliance Review — No violations (3 policies evaluated)

Evaluated 3 policies across all review surfaces. No violations detected.

Review Stages Executed

• ✅ Deep Code Review (3 Passes)
• ✅ Policy Compliance Review
  _{Generated by code-review-personae v0.8.7 | Deep Code Review (3 Passes), Policy Compliance Review}