feat(fb_custom_audience): add hashing consistency validation#5018
Merged
shekhar-rudder merged 3 commits intodevelopfrom Mar 10, 2026
Merged
feat(fb_custom_audience): add hashing consistency validation#5018shekhar-rudder merged 3 commits intodevelopfrom
shekhar-rudder merged 3 commits intodevelopfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Destination config src/v0/destinations/fb_custom_audience/config.ts |
Adds and exports DESTINATION = 'fb_custom_audience'. |
Audience validation util src/v0/util/audienceUtils.ts |
New validation utility with HASHED_VALUE_REGEX, env flag reader, and validateHashingConsistency(propertyName, normalizedValue, destination) which emits audience_hashing_inconsistency metrics and optionally throws InstrumentationError when enabled. |
FB custom audience utils src/v0/destinations/fb_custom_audience/util.ts |
Imports DESTINATION and validateHashingConsistency; extends getUpdatedDataElement and prepareDataField signatures to accept workspaceId and destinationId; computes isHashableField/shouldHash; calls validation when applicable; preserves existing hashing/pushing logic. |
FB custom audience tests src/v0/destinations/fb_custom_audience/util.test.ts |
Updates tests to mock stats.increment, add workspace/destination IDs, exercise validateHashingConsistency behavior for ON/OFF env flag combinations, and assert metric emissions per case. |
Record & transform propagation src/v0/destinations/fb_custom_audience/recordTransform.ts, src/v0/destinations/fb_custom_audience/transform.ts |
Propagates workspaceId and destinationId from record/destination into getUpdatedDataElement / prepareDataField call sites and adjusted preparePayload invocation. |
Package manifest package.json |
Minor single-line change referenced in diff. |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant RecordProcessor
participant Updater as getUpdatedDataElement
participant Validator as validateHashingConsistency
participant Env as "Env (AUDIENCE_HASHING_VALIDATION_ENABLED)"
participant Stats
Client->>RecordProcessor: send record (contains workspaceId, destinationId)
RecordProcessor->>Updater: update data element (passes workspaceId,destinationId)
Updater->>Validator: validate(propertyName, normalizedValue, destination)
Validator->>Env: read AUDIENCE_HASHING_VALIDATION_ENABLED
Env-->>Validator: boolean
alt inconsistency detected
Validator->>Stats: increment audience_hashing_inconsistency
Stats-->>Validator: ack
alt validation enabled
Validator-->>Updater: throw InstrumentationError
else
Validator-->>Updater: return (no throw)
end
else consistent
Validator-->>Updater: return OK
end
Updater-->>RecordProcessor: updated data element
RecordProcessor-->>Client: continue processing
Estimated Code Review Effort
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description covers the key changes (validateHashingConsistency function, stats metric, conditional error throwing) but lacks the required template sections like objectives, Linear task reference, and developer checklist. | Complete the PR description using the repository template, including Linear task reference (INT-XXX), detailed objectives, dependency changes, technical considerations, and developer/reviewer checklists. | |
| Docstring Coverage | Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately captures the main change: adding hashing consistency validation to the fb_custom_audience destination. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
int-5920-fb-audience-hashing
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/v0/destinations/fb_custom_audience/util.ts (1)
27-28: Consider case-insensitivity for hash detection.The regex only matches lowercase hex characters. If users send pre-hashed values with uppercase hex (e.g.,
B94D27B9...), they won't be detected as hashed. This could lead to false positive metrics forunhashed_when_hash_disabled.Consider using the case-insensitive flag:
💡 Suggested improvement
-const HASHED_VALUE_REGEX = /^[\da-f]{64}$/; +const HASHED_VALUE_REGEX = /^[\da-f]{64}$/i;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.ts` around lines 27 - 28, The current HASHED_VALUE_REGEX only matches lowercase hex and will miss uppercase pre-hashed values; update HASHED_VALUE_REGEX (the constant used for hash detection) to be case-insensitive (e.g., add the /i flag) or include A-F in the character class so it matches both upper- and lowercase hex, and verify any code paths that use HASHED_VALUE_REGEX for the unhashed_when_hash_disabled metric still behave correctly after this change.src/v0/destinations/fb_custom_audience/util.test.ts (1)
215-275: Comprehensive test coverage for the validation logic.The tests cover all four combinations of hashing configuration and input state, plus the validation toggle behavior. The use of
beforeEach/afterEachensures proper test isolation.One minor observation: the
validateHashingConsistencytests are nested insidegetUpdatedDataElement function tests. This is acceptable since the validation is invoked throughgetUpdatedDataElement, but you might consider moving it to a siblingdescribeblock for better organization if the suite grows.💡 Optional: Consider restructuring test organization
- describe('getUpdatedDataElement function tests', () => { - // ... existing test cases ... - - describe('validateHashingConsistency function tests', () => { - // ... new tests ... - }); - }); + describe('getUpdatedDataElement function tests', () => { + // ... existing test cases ... + }); + + describe('validateHashingConsistency function tests', () => { + // ... new tests ... + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.test.ts` around lines 215 - 275, The validateHashingConsistency tests are nested inside the getUpdatedDataElement suite; extract the block named "describe('validateHashingConsistency function tests')" into its own top-level sibling describe so it's not nested under the getUpdatedDataElement tests, ensure the usages of getUpdatedDataElement and stats.increment mocks remain in scope (move or duplicate the beforeEach/afterEach setup/teardown for process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED and mockStatsIncrement.mockClear() as needed), and keep all assertions identical so the test behavior for validateHashingConsistency and its calls to getUpdatedDataElement remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/v0/destinations/fb_custom_audience/util.test.ts`:
- Around line 215-275: The validateHashingConsistency tests are nested inside
the getUpdatedDataElement suite; extract the block named
"describe('validateHashingConsistency function tests')" into its own top-level
sibling describe so it's not nested under the getUpdatedDataElement tests,
ensure the usages of getUpdatedDataElement and stats.increment mocks remain in
scope (move or duplicate the beforeEach/afterEach setup/teardown for
process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED and
mockStatsIncrement.mockClear() as needed), and keep all assertions identical so
the test behavior for validateHashingConsistency and its calls to
getUpdatedDataElement remain unchanged.
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 27-28: The current HASHED_VALUE_REGEX only matches lowercase hex
and will miss uppercase pre-hashed values; update HASHED_VALUE_REGEX (the
constant used for hash detection) to be case-insensitive (e.g., add the /i flag)
or include A-F in the character class so it matches both upper- and lowercase
hex, and verify any code paths that use HASHED_VALUE_REGEX for the
unhashed_when_hash_disabled metric still behave correctly after this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63ef84bc-fe6a-4f27-a114-8a73107f753d
⛔ Files ignored due to path filters (3)
test/integrations/destinations/fb_custom_audience/processor/data.tsis excluded by!**/test/**test/integrations/destinations/fb_custom_audience/router/data.tsis excluded by!**/test/**test/integrations/destinations/fb_custom_audience/router/eventStream.tsis excluded by!**/test/**
📒 Files selected for processing (3)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/util.test.tssrc/v0/destinations/fb_custom_audience/util.ts
…shing inconsistency metrics Threads workspaceId and destinationId through validateHashingConsistency so every fb_custom_audience_hashing_inconsistency metric emission is tagged with both IDs, enabling per-workspace and per-destination observability. - Made the new params required (no empty-string defaults) to catch missing call sites at compile time. - Updated recordTransform.processRecord to extract workspaceId from record.metadata and destinationId from record.destination.ID. - Updated all call sites and unit tests accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/v0/destinations/fb_custom_audience/util.test.ts`:
- Around line 227-233: The afterEach currently deletes
process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED unconditionally which
can leak global state; modify the test to snapshot the original value before
tests (e.g., in a variable set in beforeEach or a separate beforeAll) and in
afterEach restore process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED to
that original value (or delete it only if it was undefined originally). Update
the block that references mockStatsIncrement and the existing
beforeEach/afterEach surrounding it so the environment flag is restored instead
of always deleted.
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 27-38: The current HASHED_VALUE_REGEX only matches lowercase hex
and causes uppercase SHA-256 strings to be treated as unhashed in
validateHashingConsistency; update the regex to be case-insensitive (e.g.,
change HASHED_VALUE_REGEX to /^[\da-f]{64}$/i or include A-F in the character
class) so isAlreadyHashed correctly detects both lowercase and uppercase hex;
modify the constant declaration near validateHashingConsistency and run relevant
tests to ensure no behavioral regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: efea0b08-c496-46e4-8d6d-9b5c1df7cf8c
📒 Files selected for processing (4)
src/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/transform.tssrc/v0/destinations/fb_custom_audience/util.test.tssrc/v0/destinations/fb_custom_audience/util.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5018 +/- ##
===========================================
- Coverage 92.28% 92.27% -0.01%
===========================================
Files 655 657 +2
Lines 35851 35927 +76
Branches 8425 8473 +48
===========================================
+ Hits 33086 33153 +67
+ Misses 2548 2536 -12
- Partials 217 238 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
achettyiitr
reviewed
Mar 10, 2026
test/integrations/destinations/fb_custom_audience/router/data.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/v0/util/audienceUtils.ts (1)
4-4:⚠️ Potential issue | 🟠 MajorUse case-insensitive regex to detect hashed values.
The current regex only matches lowercase hex characters (
a-f). SHA-256 hex strings can also use uppercase (A-F), which would cause false detection of unhashed values and potentially trigger re-hashing of already-hashed data.🔧 Proposed fix
-const HASHED_VALUE_REGEX = /^[\da-f]{64}$/; +const HASHED_VALUE_REGEX = /^[\da-f]{64}$/i;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/util/audienceUtils.ts` at line 4, The regex constant HASHED_VALUE_REGEX currently only matches lowercase hex (a-f) and will miss uppercase SHA-256 hex strings; update the definition of HASHED_VALUE_REGEX (the constant named HASHED_VALUE_REGEX) to accept uppercase letters too (either by adding A-F to the character class or by applying the case-insensitive flag) so that hashed values with uppercase hex are correctly recognized and not re-hashed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/v0/util/audienceUtils.ts`:
- Line 4: The regex constant HASHED_VALUE_REGEX currently only matches lowercase
hex (a-f) and will miss uppercase SHA-256 hex strings; update the definition of
HASHED_VALUE_REGEX (the constant named HASHED_VALUE_REGEX) to accept uppercase
letters too (either by adding A-F to the character class or by applying the
case-insensitive flag) so that hashed values with uppercase hex are correctly
recognized and not re-hashed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adf37a38-4c1d-466e-8466-ffc6423c23ec
⛔ Files ignored due to path filters (2)
test/integrations/destinations/fb_custom_audience/router/data.tsis excluded by!**/test/**test/integrations/destinations/fb_custom_audience/router/eventStream.tsis excluded by!**/test/**
📒 Files selected for processing (5)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/util.test.tssrc/v0/destinations/fb_custom_audience/util.tssrc/v0/util/audienceUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/v0/destinations/fb_custom_audience/recordTransform.ts
shekhar-rudder
force-pushed
the
int-5920-fb-audience-hashing
branch
from
154679e to
5b2f9c8
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/v0/destinations/fb_custom_audience/util.ts (1)
207-214: Consider consistent parameter ordering across functions.The parameter order differs between functions:
prepareDataField:(destinationId, workspaceId)getUpdatedDataElement:(workspaceId, destinationId)This inconsistency could lead to accidental swaps. Consider standardizing on one order (e.g.,
workspaceIdbeforedestinationId) for better maintainability.♻️ Suggested parameter order alignment
const prepareDataField = ( userSchema: string[], userUpdateList: Record<string, unknown>[], isHashRequired: boolean, disableFormat: boolean, - destinationId: string, workspaceId: string, + destinationId: string, ): unknown[][] => {Note: This would require updating all call sites in
recordTransform.tsandtransform.tsaccordingly.Also applies to: 230-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.ts` around lines 207 - 214, The parameters for prepareDataField are ordered (destinationId, workspaceId) which is inconsistent with getUpdatedDataElement (workspaceId, destinationId); change prepareDataField to accept workspaceId before destinationId (i.e., ...isHashRequired, disableFormat, workspaceId, destinationId) and apply the same ordering change to the other functions around lines 230-237 to standardize signature ordering; update all affected call sites (notably in recordTransform.ts and transform.ts) to pass workspaceId then destinationId and run tests/type checks to ensure no swapped-argument bugs remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 207-214: The parameters for prepareDataField are ordered
(destinationId, workspaceId) which is inconsistent with getUpdatedDataElement
(workspaceId, destinationId); change prepareDataField to accept workspaceId
before destinationId (i.e., ...isHashRequired, disableFormat, workspaceId,
destinationId) and apply the same ordering change to the other functions around
lines 230-237 to standardize signature ordering; update all affected call sites
(notably in recordTransform.ts and transform.ts) to pass workspaceId then
destinationId and run tests/type checks to ensure no swapped-argument bugs
remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4072d0fe-c5ab-4389-aa9c-55aaf675eaac
⛔ Files ignored due to path filters (2)
test/integrations/destinations/fb_custom_audience/router/data.tsis excluded by!**/test/**test/integrations/destinations/fb_custom_audience/router/eventStream.tsis excluded by!**/test/**
📒 Files selected for processing (5)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/util.test.tssrc/v0/destinations/fb_custom_audience/util.tssrc/v0/util/audienceUtils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/v0/util/audienceUtils.ts
- src/v0/destinations/fb_custom_audience/recordTransform.ts
shekhar-rudder
force-pushed
the
int-5920-fb-audience-hashing
branch
from
5b2f9c8 to
3f5237c
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/v0/destinations/fb_custom_audience/util.test.ts (1)
233-235:⚠️ Potential issue | 🟡 MinorRestore the original hashing-validation env flag after each test.
This still wipes a process-wide setting unconditionally. If the suite starts with
AUDIENCE_HASHING_VALIDATION_ENABLEDalready set, these tests mutate global state for sibling tests. Snapshot the previous value and restore it inafterEachinstead of always deleting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.test.ts` around lines 233 - 235, The afterEach currently unconditionally deletes process.env.AUDIENCE_HASHING_VALIDATION_ENABLED which mutates global state; fix by capturing the original value in a top-scope variable (e.g. let originalAudienceHashingValidationEnabled) in a beforeEach or at test-suite setup, then in afterEach restore it: if original is undefined delete the env var, otherwise set process.env.AUDIENCE_HASHING_VALIDATION_ENABLED = original; update the afterEach in util.test.ts accordingly and ensure the snapshot variable is declared where the tests can access it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/v0/destinations/fb_custom_audience/util.test.ts`:
- Around line 233-235: The afterEach currently unconditionally deletes
process.env.AUDIENCE_HASHING_VALIDATION_ENABLED which mutates global state; fix
by capturing the original value in a top-scope variable (e.g. let
originalAudienceHashingValidationEnabled) in a beforeEach or at test-suite
setup, then in afterEach restore it: if original is undefined delete the env
var, otherwise set process.env.AUDIENCE_HASHING_VALIDATION_ENABLED = original;
update the afterEach in util.test.ts accordingly and ensure the snapshot
variable is declared where the tests can access it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7d6de36-0cfb-4625-809b-afdcf20a1685
⛔ Files ignored due to path filters (2)
test/integrations/destinations/fb_custom_audience/router/data.tsis excluded by!**/test/**test/integrations/destinations/fb_custom_audience/router/eventStream.tsis excluded by!**/test/**
📒 Files selected for processing (5)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/util.test.tssrc/v0/destinations/fb_custom_audience/util.tssrc/v0/util/audienceUtils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/v0/destinations/fb_custom_audience/config.ts
- src/v0/destinations/fb_custom_audience/util.ts
- src/v0/util/audienceUtils.ts
|
achettyiitr
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validateHashingConsistencyfunction that detects mismatches between theisHashRequiredsetting and the actual input data (e.g. sending pre-hashed SHA-256 values when hashing is enabled, or plaintext when it is disabled).fb_custom_audience_hashing_inconsistencystats metric on detection, labelled withpropertyNameandtype(hashed_when_hash_enabled/unhashed_when_hash_disabled), so misconfigurations can be observed and alerted on in production without breaking traffic.InstrumentationErroronly when theFB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLEDenv var istrue, enabling a safe, gradual rollout.isHashRequired=false.🤖 Generated with Claude Code