[COOP-661] remove no media ncmec gate - #866
Conversation
calebmcquaid
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughRemoves the no-media enqueue skip, derives NCMEC incident timestamps from media and thread messages, permits empty reported media, and updates reviewer error classification and test coverage. ChangesNCMEC Text-Only Escalation Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/services/ncmecService/ncmecReporting.ts`:
- Around line 1485-1488: Text-only reports are still being rejected by
getNCMECAdditionalInfo because the helper treats an empty media list as
ALL_MEDIA_MISSING. Update submitReport and/or getNCMECAdditionalInfo so the
zero-request case (reportedMedia is empty for a legitimate text-only report)
bypasses that sentinel and does not surface ALL_MEDIA_MISSING, while still
preserving the existing missing-media behavior for requests that actually
included media. Use the submitReport flow and getNCMECAdditionalInfo helper as
the main places to locate and apply the fix.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4885388f-f4f5-40a8-88f1-7e3ebbc25e2d
📒 Files selected for processing (3)
server/services/ncmecService/ncmecEnqueueToMrt.tsserver/services/ncmecService/ncmecReporting.test.tsserver/services/ncmecService/ncmecReporting.ts
💤 Files with no reviewable changes (1)
- server/services/ncmecService/ncmecEnqueueToMrt.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/services/ncmecService/ncmecReporting.ts`:
- Around line 538-545: The timestamp fallback in ncmecReporting.ts is too
lenient: when every value in rawTimestamps is unparseable, the current logic in
the evidence timestamp helper returns a fresh ISO string instead of surfacing
invalid data. Update the helper used by submitReport/incidentDateTime handling
so it only derives a timestamp from valid createdAt/sentAt values and returns an
invalid/empty result when none are parseable, allowing the existing validation
error path to trigger rather than substituting now. Keep the change localized
around the evidence timestamp parsing logic and the submitReport flow that
consumes it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d5e9c94-4228-419a-bb62-b7f0bf815d8b
📒 Files selected for processing (5)
server/services/ncmecService/dbTypes.tsserver/services/ncmecService/ncmecReporting.test.tsserver/services/ncmecService/ncmecReporting.tsserver/services/ncmecService/ncmecReviewerErrors.test.tsserver/services/ncmecService/ncmecReviewerErrors.ts
taobojlen
left a comment
taobojlen
left a comment
There was a problem hiding this comment.
thank you!
some comments/questions. not blocking but would be good to address!
| if (rawTimestamps.length === 0) { | ||
| throw new Error('Report has neither media nor messages'); | ||
| } | ||
| // updated to allow a lenient approach to timestamps. If none parse, throw an |
There was a problem hiding this comment.
| // updated to allow a lenient approach to timestamps. If none parse, throw an | |
| // allow a lenient approach to timestamps. If none parse, throw an |
code comments should explain the current state of the code, not what was! as a reader i see this and think "updated from what?" -- but more importantly that question is just not important for the reader's understanding of the codebase, so it's only a distraction
| } | ||
| // updated to allow a lenient approach to timestamps. If none parse, throw an | ||
| // error and surface the bad data via the validation error path | ||
| const evidenceTimestampsMs = rawTimestamps |
There was a problem hiding this comment.
what's an example where an invalid date string might end up here? how might that happen?
(i am wondering if this handling is necessary or if it's unnecessarily defensive).
There was a problem hiding this comment.
Pull request overview
This PR removes the “must have media” gating for NCMEC reporting so text-only incident types can be enqueued and submitted, and it updates incident/evidence timestamp handling to consider both media and message timestamps.
Changes:
- Remove the enqueue-time “no media → skipped” behavior so human review jobs can still be created for text-only cases.
- Replace “latest media createdAt” logic with a unified
latestEvidenceTimestamp(media, threads)helper to derive incident time from either media or messages. - Update reviewer-facing error classification/messages and expand test coverage for timestamp selection and validation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/services/ncmecService/ncmecReviewerErrors.ts | Updates reviewer-friendly allowlist and reclassifies incidentDateTime timestamp errors under validation. |
| server/services/ncmecService/ncmecReviewerErrors.test.ts | Adds/updates tests for the new reviewer-friendly message and timestamp validation classification. |
| server/services/ncmecService/ncmecReporting.ts | Implements latestEvidenceTimestamp, removes media-only gating in submission flow, and allows storing reports with empty reported_media. |
| server/services/ncmecService/ncmecReporting.test.ts | Adds tests for latestEvidenceTimestamp and updates clamp error expectations. |
| server/services/ncmecService/ncmecEnqueueToMrt.ts | Removes the “no media → SKIPPED” gate so jobs can enqueue without media. |
| server/services/ncmecService/dbTypes.ts | Updates stored report schema typing to allow reported_media to be empty. |
Suppressed comments (1)
server/services/ncmecService/ncmecReporting.ts:1244
responseBody.media?.filter(...).lengthcan throw whenmediais undefined (optional chaining only applies to thefiltercall, not the subsequent.length). This makes the ALL_MEDIA_MISSING check crash on responses that omitmediaentirely. Use optional chaining/coalescing on the filtered result’s length.
if (
reportedMedia.length > 0 &&
responseBody.media?.filter(
(it) => it.missing === false || it.missing === undefined,
).length === 0
) {
| // updated to allow a lenient approach to timestamps. If none parse, throw an | ||
| // error and surface the bad data via the validation error path | ||
| const evidenceTimestampsMs = rawTimestamps | ||
| .map((raw) => (raw instanceof Date ? raw.getTime() : Date.parse(raw))) | ||
| .filter((ms) => !Number.isNaN(ms)); | ||
| if (evidenceTimestampsMs.length === 0) { | ||
| throw new Error('Invalid timestamp for incidentDateTime'); | ||
| } |
…-no-media-NCMEC-gate # Conflicts: # server/services/ncmecService/ncmecReporting.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/services/ncmecService/ncmecReporting.ts (3)
1934-1942: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftResolve the incompatible prior-report policy.
For production submissions, lines 1869-1880 reject a user when
getUserHasExistingNcmeReportfinds any non-test report.getPriorCTReportIdsuses the same org, user, item-type, and non-test predicates. Therefore this lookup always returns no rows when submission continues. Test submissions also forcepriorCTReportsto[].Revise the duplicate-report policy if later reports must include
priorCTReports. Otherwise remove this unreachable payload behavior.🤖 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/services/ncmecService/ncmecReporting.ts` around lines 1934 - 1942, Resolve the duplicate-report policy in the submission flow surrounding getPriorCTReportIds: either allow later production reports to continue and populate priorCTReports from existing non-test reports, or remove the unreachable priorCTReports lookup and payload behavior. Keep test submissions consistent with the selected policy and eliminate the unconditional empty result path if prior reports must be included.
1779-1781: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate the complete report ID before conversion.
parseInt('123invalid', 10)returns123. This violates the documented behavior and can add an incorrect prior report ID to the NCMEC payload. Require an all-digit value before conversion and reject unsafe integers.Proposed fix
- return rows - .map((r) => parseInt(r.report_id, 10)) - .filter((n) => Number.isFinite(n)); + return rows.flatMap(({ report_id }) => { + if (!/^\d+$/.test(report_id)) { + return []; + } + const reportId = Number(report_id); + return Number.isSafeInteger(reportId) ? [reportId] : []; + });🤖 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/services/ncmecService/ncmecReporting.ts` around lines 1779 - 1781, Update the report ID mapping near the NCMEC payload construction to validate each raw report_id as an all-digit value before converting it; reject malformed, empty, and unsafe-integer values, then retain only valid numeric IDs. Preserve the existing rows-to-number filtering flow while replacing permissive parseInt behavior.
894-922: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log raw media URLs.
When
NCMEC_DEBUG=1, these fallback paths log the full caller-provided URL. A media URL can contain signed query credentials, identifiers, or sensitive object paths. Log only the fallback reason and non-sensitive metadata.As per coding guidelines, “Do not log or expose secrets, JWTs, full
Authorizationheaders, full request/response bodies, or PII in logs.”Proposed fix
- ncmecDebugLog('deriveOriginalFileName.urlParseFailed', { url }); + ncmecDebugLog('deriveOriginalFileName.urlParseFailed', {}); ... - ncmecDebugLog('deriveOriginalFileName.emptyPath', { url }); + ncmecDebugLog('deriveOriginalFileName.emptyPath', {}); ... - ncmecDebugLog('deriveOriginalFileName.emptySegment', { url }); + ncmecDebugLog('deriveOriginalFileName.emptySegment', {}); ... - ncmecDebugLog('deriveOriginalFileName.decodeFailed', { url, raw: last }); + ncmecDebugLog('deriveOriginalFileName.decodeFailed', {});🤖 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/services/ncmecService/ncmecReporting.ts` around lines 894 - 922, Update deriveOriginalFileNameFromUrl logging to remove the caller-provided url from the urlParseFailed, emptyPath, emptySegment, and decodeFailed fallback logs. Preserve each reason-specific log and, for decodeFailed, retain only non-sensitive metadata such as safe segment characteristics rather than raw URL or path content.Source: Coding guidelines
🤖 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/services/manualReviewToolService/modules/JobRouting.test.ts`:
- Line 1: Split focused test groups from
server/services/manualReviewToolService/modules/JobRouting.test.ts and
server/services/moderationConfigService/moderationConfigService.test.ts into
sibling test files, extracting shared fixtures into reusable helpers so each
file remains under the active max-lines limit. Do not restore or broadly disable
lint exemptions; preserve all existing test coverage and behavior across the
split files.
In
`@server/services/manualReviewToolService/modules/ReporterInvalidation.test.ts`:
- Line 1: Add ReporterInvalidation.test.ts to the files array of the max-lines
rule suppression in server/.eslintrc.cjs, preserving the existing suppression
configuration.
---
Outside diff comments:
In `@server/services/ncmecService/ncmecReporting.ts`:
- Around line 1934-1942: Resolve the duplicate-report policy in the submission
flow surrounding getPriorCTReportIds: either allow later production reports to
continue and populate priorCTReports from existing non-test reports, or remove
the unreachable priorCTReports lookup and payload behavior. Keep test
submissions consistent with the selected policy and eliminate the unconditional
empty result path if prior reports must be included.
- Around line 1779-1781: Update the report ID mapping near the NCMEC payload
construction to validate each raw report_id as an all-digit value before
converting it; reject malformed, empty, and unsafe-integer values, then retain
only valid numeric IDs. Preserve the existing rows-to-number filtering flow
while replacing permissive parseInt behavior.
- Around line 894-922: Update deriveOriginalFileNameFromUrl logging to remove
the caller-provided url from the urlParseFailed, emptyPath, emptySegment, and
decodeFailed fallback logs. Preserve each reason-specific log and, for
decodeFailed, retain only non-sensitive metadata such as safe segment
characteristics rather than raw URL or path content.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e6020a24-ddcb-4190-b998-77ff369b9605
📒 Files selected for processing (6)
server/services/manualReviewToolService/manualReviewToolService.test.tsserver/services/manualReviewToolService/modules/JobRouting.test.tsserver/services/manualReviewToolService/modules/ReporterInvalidation.test.tsserver/services/moderationConfigService/moderationConfigService.test.tsserver/services/ncmecService/ncmecReporting.test.tsserver/services/ncmecService/ncmecReporting.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/services/ncmecService/ncmecReporting.test.ts
| @@ -1,4 +1,4 @@ | |||
| /* eslint-disable max-lines */ | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split these test files before removing the exemptions.
The active max-lines rule fails at 500 lines. server/services/manualReviewToolService/modules/JobRouting.test.ts reaches Line 714. server/services/moderationConfigService/moderationConfigService.test.ts reaches at least Line 1772. Removing the exemptions will fail ESLint.
server/services/manualReviewToolService/modules/JobRouting.test.ts#L1-L1: Move focused routing test groups into sibling test files and share fixture helpers.server/services/moderationConfigService/moderationConfigService.test.ts#L1-L1: Move focused service test groups into sibling test files and share fixture helpers.
As per coding guidelines, do not disable lint rules broadly; split the files instead of restoring the exemptions.
📍 Affects 2 files
server/services/manualReviewToolService/modules/JobRouting.test.ts#L1-L1(this comment)server/services/moderationConfigService/moderationConfigService.test.ts#L1-L1
🤖 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/services/manualReviewToolService/modules/JobRouting.test.ts` at line
1, Split focused test groups from
server/services/manualReviewToolService/modules/JobRouting.test.ts and
server/services/moderationConfigService/moderationConfigService.test.ts into
sibling test files, extracting shared fixtures into reusable helpers so each
file remains under the active max-lines limit. Do not restore or broadly disable
lint exemptions; preserve all existing test coverage and behavior across the
split files.
Source: Coding guidelines
| @@ -1,4 +1,4 @@ | |||
| /* eslint-disable max-lines */ | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in \
server/services/manualReviewToolService/modules/ReporterInvalidation.test.ts \
server/services/manualReviewToolService/modules/JobRouting.test.ts \
server/services/moderationConfigService/moderationConfigService.test.ts
do
printf '%s: ' "$file"
wc -l < "$file"
done
rg -n -C 2 "'max-lines'" server/.eslintrc.cjsRepository: roostorg/coop
Length of output: 783
Keep the ESLint max-lines suppression.
ReporterInvalidation.test.ts is 740 lines, and the active max-lines limit in server/.eslintrc.cjs is 500. Add this file back to the files array under the max-lines suppression.
🤖 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/services/manualReviewToolService/modules/ReporterInvalidation.test.ts`
at line 1, Add ReporterInvalidation.test.ts to the files array of the max-lines
rule suppression in server/.eslintrc.cjs, preserving the existing suppression
configuration.
Context & Requests for Reviewers
ENQUEUE_TO_NCMECsilently refused any account with no media, and the downstream submission path failed on a payload with no media. This blocked text-only incident types that NCMEC accepts like Online Enticement, Misleading Words, grooming chat logs, and DM-based harassment.This PR removes the media requirement from both the enqueue and submission paths so text-only reports flow through. (This is server only. The client reviewer UI and reported-message capture will follow in separate PRs.)
Tests
ncmecReporting.test.ts: 41/41 passThere will be follow-on PRs to cover client side gating (
canSendReport), capture reported message identifiers, and surface enqueue skip reasonsSummary by CodeRabbit