Make NCMEC and MRT enqueue work for users with no submission record#494
Merged
juanmrad merged 4 commits intoMay 18, 2026
Conversation
juanmrad
requested review from
cassidyjames,
dom-notion,
julietshen and
vinaysrao1
as code owners
Contributor
📝 WalkthroughWalkthroughAdds recovery paths so ActionPublisher can enqueue MRT/NCMEC even when submission records are missing (synthesizing USER submissions or inferring creators from content), extends adapters and types for creator lookup, updates the client bulk-action failure message, and adds tests and structured error logging. ChangesAction Publisher Resilience for Missing Submissions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates action publishing so NCMEC/MRT enqueue actions can proceed for USER targets that do not have a stored item submission, using synthetic user submissions and improved failure messaging/logging.
Changes:
- Exports synthetic user submission creation from Item Investigation.
- Adds fallback submission resolution in
ActionPublisherfor NCMEC/MRT enqueue paths. - Adds tests for synthetic USER enqueue behavior and updates client failure copy.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
server/services/itemInvestigationService/index.ts |
Re-exports synthetic user submission helper. |
server/rule_engine/ActionPublisher.ts |
Adds synthetic/fallback resolution and structured error logging for failed actions. |
server/rule_engine/ActionPublisher.test.ts |
Adds coverage for synthetic enqueue behavior and failure logging. |
client/src/components/ItemAction.tsx |
Updates failed-action modal text and formatting. |
Comments suppressed due to low confidence (1)
server/rule_engine/ActionPublisher.ts:424
ENQUEUE_TO_MRTis the built-in action that enqueues the matched item directly, including CONTENT targets. ReusinggetFullItemOrSyntheticUserhere means that if the fallback returns a USER for a CONTENT target, the action will enqueue the creator instead of the matched content; only the NCMEC path should convert content to its associated user.
case ActionType.ENQUEUE_TO_MRT:
const fullItemForMrt = await getFullItemOrSyntheticUser();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/rule_engine/ActionPublisher.test.ts`:
- Line 1: The file-level eslint disable "/* eslint-disable max-lines */" in
ActionPublisher.test.ts should be removed and replaced with a narrowly-scoped
suppression or test split: either move the large synthetic-submission test cases
into a new test file (e.g., SyntheticSubmission.test.ts) or keep them here but
place "// eslint-disable-next-line max-lines -- reason: [brief rationale]"
immediately above the specific long test block or describe/it block that exceeds
the limit; update the top of ActionPublisher.test.ts to remove the file-wide
directive and add a short comment explaining the chosen approach, and ensure any
moved tests still reference the same functions/classes (e.g., ActionPublisher,
publishAction, synthetic submission helpers) so behavior remains identical.
In `@server/rule_engine/ActionPublisher.ts`:
- Line 424: The const declarations inside switch case clauses (e.g., the const
fullItemForMrt = await getFullItemOrSyntheticUser() and the other const at the
second case) cause switch-scoped declaration hazards; fix by wrapping each case
body that declares const/let/await in its own block (add { ... } around the
statements in those case branches) or hoist the declarations outside the switch,
ensuring getFullItemOrSyntheticUser() is awaited inside the new block so the
switch no longer contains top-level const bindings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e450d56c-e356-4135-826d-38ea8e33aebb
📒 Files selected for processing (4)
client/src/components/ItemAction.tsxserver/rule_engine/ActionPublisher.test.tsserver/rule_engine/ActionPublisher.tsserver/services/itemInvestigationService/index.ts
julietshen
approved these changes
May 18, 2026
Member
julietshen
left a comment
julietshen
left a comment
There was a problem hiding this comment.
Approving solely on the business logic and intended functionality - relying on copilot/coderabbit for code quality checks
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Comment on lines
+17
to
+24
| export async function synthesizeUserItemFromContentTarget(opts: { | ||
| orgId: string; | ||
| itemId: string; | ||
| itemTypeId: string; | ||
| actionExecutionsAdapter: Pick< | ||
| IActionExecutionsAdapter, | ||
| 'findContentCreatorIdentity' | 'findInferredUserIdentity' | ||
| >; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/plugins/warehouse/queries/ClickhouseActionExecutionsAdapter.ts (1)
236-238: ⚡ Quick winMake
item_type_idmatching case-insensitive for consistency.Line 237 uses exact-case comparison, which can miss valid historical rows if type IDs were ingested with different casing and cause unnecessary recovery failures.
Suggested change
- AND item_type_id = ? + AND lower(item_type_id) = lower(?)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/plugins/warehouse/queries/ClickhouseActionExecutionsAdapter.ts` around lines 236 - 238, The WHERE clause in ClickhouseActionExecutionsAdapter that currently uses "AND item_type_id = ?" should be made case-insensitive to match the surrounding lower(item_id) check; update the SQL to compare lower(item_type_id) = lower(?) (keeping the same parameter placeholder) in the function that builds this query so historical rows with different casing are matched correctly and recovery failures avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/plugins/warehouse/queries/ClickhouseActionExecutionsAdapter.ts`:
- Around line 236-238: The WHERE clause in ClickhouseActionExecutionsAdapter
that currently uses "AND item_type_id = ?" should be made case-insensitive to
match the surrounding lower(item_id) check; update the SQL to compare
lower(item_type_id) = lower(?) (keeping the same parameter placeholder) in the
function that builds this query so historical rows with different casing are
matched correctly and recovery failures avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52d86acc-82ab-42d4-8000-6a628f454db2
📒 Files selected for processing (7)
server/plugins/warehouse/queries/ClickhouseActionExecutionsAdapter.test.tsserver/plugins/warehouse/queries/ClickhouseActionExecutionsAdapter.tsserver/plugins/warehouse/queries/IActionExecutionsAdapter.tsserver/rule_engine/ActionPublisher.test.tsserver/rule_engine/ActionPublisher.tsserver/services/itemInvestigationService/itemInvestigationServiceAdapter.tsserver/services/itemInvestigationService/synthesizeUserItemFromContentTarget.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/rule_engine/ActionPublisher.test.ts
juanmrad
added a commit
that referenced
this pull request
May 18, 2026
…orts dashboard (#492) * [NCMEC] Persist failed submissions and surface retry in the NCMEC Reports dashboard * address code review comments by copilot and coderabbit * Make NCMEC and MRT enqueue work for users with no submission record (#494) * Make NCMEC and MRT enqueue work for users with no submission record * make it work with content type also * code review changes * code review feedback
juanmrad
added a commit
that referenced
this pull request
May 19, 2026
…ode 24 multipart submission (#477) * [NCMEC] Fix report ordering required due to js2xml * code review fixes * code review changes * revert URL match as it can cause errors given urls can be signed/ephemeralso they don't byte-match across loads * [NCMEC] Persist failed submissions and surface retry in the NCMEC Reports dashboard (#492) * [NCMEC] Persist failed submissions and surface retry in the NCMEC Reports dashboard * address code review comments by copilot and coderabbit * Make NCMEC and MRT enqueue work for users with no submission record (#494) * Make NCMEC and MRT enqueue work for users with no submission record * make it work with content type also * code review changes * code review feedback * code review changes * code review changes * code review adjustments
cassidyjames
added
NCMEC
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
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.
Context & Requests for Reviewers
Moderators triaging a user the integration never POSTed (a "synthetic" user surfaced by Item Investigation) could not enqueue them for NCMEC review or MRT. The action returned success: false with a misleading "callback URL may have returned an error" toast and nothing in server logs, while still creating an audit row in
analytics.ACTION_EXECUTIONSso every retry looked like a new entry in Recent Actions on this User despite none of them taking effect.ActionPublisher.publishActionnow synthesizes a minimalUserItemsubmission viamakeSyntheticUserSubmissionwhen the target is a USER andgetItemByIdentifierreturns nothing, soENQUEUE_TO_NCMECandENQUEUE_TO_MRTenqueue successfully. The downstreamNcmecEnqueueToMrtservice already handles minimal user submissions.Summary by CodeRabbit
Bug Fixes
New Features