Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughAdds new Firestore rules and test suites for coordination/report workflows, extends validators with session/SMS/ops fields and geohash-aware inbox processing, and updates trigger logic to persist geohash, SMS consent municipality, and resiliently handle missing pending media. Changes
Sequence DiagramsequenceDiagram
participant Client
participant InboxTrigger as Inbox Trigger\n(process-inbox-item)
participant Geohash as Geohash\nEncoder
participant ReportOps as report_ops\nCollection
participant Firestore as Firestore\nDatabase
Client->>InboxTrigger: Invoke with payload (may include exactLocation)
alt exactLocation present
InboxTrigger->>Geohash: Encode lat/lng at precision 6
Geohash-->>InboxTrigger: Return geohash
InboxTrigger->>ReportOps: Materialize report_ops (reportType, locationGeohash)
else exactLocation absent
InboxTrigger->>ReportOps: Materialize report_ops (reportType, locationGeohash: undefined)
end
InboxTrigger->>Firestore: Write report_sms_consent (include municipalityId if available)
InboxTrigger->>Firestore: Read pending_media ids (log warning if missing)
Firestore-->>InboxTrigger: Acknowledge writes/transaction
InboxTrigger-->>Client: Return processing result or moderation error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/shared-validators/lib/sms.d.ts (1)
53-61:⚠️ Potential issue | 🟠 MajorRegenerate the
.d.tsdeclaration files—they are out of sync with the source.The source file already has correct semantic ordering:
queued → sending → sent → delivered. However, the declaration file showsqueued → sent → sending, which is stale. Runnpm run buildin thepackages/shared-validatorsdirectory to regenerate the.d.tsfiles from the current source. This ensures that downstream consumers of this package see correct type definitions matching the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-validators/lib/sms.d.ts` around lines 53 - 61, The generated declaration for the status enum in sms.d.ts (status: z.ZodEnum<...>) is out of sync with the source (it shows queued→sent→sending instead of queued→sending→sent→delivered); fix by regenerating the declarations for the shared-validators package—run the package build (npm run build for the shared-validators package) so the lib/sms.d.ts is recreated with the correct enum ordering for status, verify the z.ZodEnum values match the source, and commit the updated .d.ts output.functions/src/triggers/process-inbox-item.ts (1)
238-245:⚠️ Potential issue | 🟠 MajorMissing
municipalityIdinreport_sms_consentdocument.The
reportSmsConsentDocSchemainpackages/shared-validators/src/users.tsrequiresmunicipalityIdas a mandatory field, but this write omits it. This will cause schema validation failures when reading these documents.🐛 Proposed fix
tx.set(db.collection('report_sms_consent').doc(reportId), { reportId, phone: payload.contact.phone, locale: muniLocale, smsConsent: true, + municipalityId: geo.municipalityId, createdAt, schemaVersion: 1, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/triggers/process-inbox-item.ts` around lines 238 - 245, The write to report_sms_consent via tx.set is missing the required municipalityId field (per reportSmsConsentDocSchema); update the object passed to tx.set in process-inbox-item.ts to include municipalityId (e.g., municipalityId: payload.municipalityId or municipalityId variable used elsewhere in this file) so the stored document matches report_sms_consent schema and avoids validation failures.functions/src/__tests__/rules/report-sharing.rules.test.ts (1)
104-117: 🧹 Nitpick | 🔵 TrivialRemove redundant dynamic import of
setDoc.
setDocis already imported at the top of the file (line 2). The dynamic import on line 110 is unnecessary and adds overhead.♻️ Proposed fix
it('any client write fails', async () => { const db = authed( env, 'daet-admin', staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }), ) - const { setDoc } = await import('firebase/firestore') await assertFails( setDoc(doc(db, 'report_sharing/new'), { ownerMunicipalityId: 'daet', sharedWith: ['mercedes'], }), ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/src/__tests__/rules/report-sharing.rules.test.ts` around lines 104 - 117, The test contains a redundant dynamic import of setDoc; remove the line that does "const { setDoc } = await import('firebase/firestore')" inside the "any client write fails" test and use the already imported top-level setDoc (and existing doc/db helpers) instead so the test relies on the module-scoped import rather than performing an unnecessary dynamic import.packages/shared-validators/lib/reports.js (1)
176-188:⚠️ Potential issue | 🟡 MinorUse dynamic validation for
expiresAtmax constraint instead ofDate.now()at schema definition time.The schema's
expiresAtmax constraint:.max(Date.now() + 365 * 24 * 60 * 60 * 1000)This value is computed once when the module loads. On servers running longer than ~275 days, new documents with
expiresAt: createdAt + 90 * 24 * 60 * 60 * 1000will have timestamps exceeding the stale schema constraint, causing valid documents to fail validation.Use
.refine()or a custom validator to check the constraint dynamically at validation time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-validators/lib/reports.js` around lines 176 - 188, The expiresAt max is computed at module load in reportLookupDocSchema (expiresAt: .max(Date.now() + ...)), causing stale validation; replace the static .max(...) with a runtime check by removing the .max(...) and adding a dynamic validator (e.g., use z.number().int().refine(...) or .superRefine(...) on reportLookupDocSchema) that compares expiresAt against Date.now() + 365*24*60*60*1000 at validation time and returns a clear error message; keep the existing integer requirement and schemaVersion checks intact.infra/firebase/firestore.rules.template (1)
1-1:⚠️ Potential issue | 🟠 MajorPipeline failure: Rules file out of sync with build script.
The CI indicates
firestore.rules is out of sync with scripts/build-rules.ts. Run the build script locally and commit the regenerated rules file:pnpm exec tsx scripts/build-rules.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/firestore.rules.template` at line 1, The firestore rules file is out of sync with the build script; run the build script scripts/build-rules.ts locally (e.g., pnpm exec tsx scripts/build-rules.ts) to regenerate the rules (producing firestore.rules from infra/firebase/firestore.rules.template), verify the updated rules, and commit the regenerated firestore.rules file so CI and scripts/build-rules.ts are in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/src/__tests__/rules/report-notes.rules.test.ts`:
- Around line 97-104: The test "allows muni admin to read notes for their
municipality" misstates scope—Firestore rules permit any active privileged admin
to read all report_notes; update the it() description in the test case using
authed(..., staffClaims(...)) and assertSucceeds(getDocs(collection(db,
'report_notes'))) to something like "allows muni admin to read notes" to reflect
actual behavior, and optionally add a separate test that creates a different
municipality context and asserts that a municipal_admin from one municipality
can still read report_notes from another municipality to document
cross-municipality reads.
In `@functions/src/__tests__/triggers/process-inbox-item.test.ts`:
- Around line 146-176: The test currently only asserts the shape of
locationGeohash; enhance it by creating a second inbox document (e.g., id
'ibx-geohash-2') or updating the original with a different exactLocation
(different lat/lng), call processInboxItemCore again (using db, inboxId, now)
and assert the new report_ops document's locationGeohash is both shape-valid and
not equal to the first geohash; reference the existing test flow and symbols
(processInboxItemCore, report_inbox, exactLocation, report_ops, locationGeohash)
so you compare two produced geohashes to ensure they change when exactLocation
changes.
In `@functions/src/triggers/process-inbox-item.ts`:
- Around line 17-26: The isExactLocation type guard and ExactLocation interface
are redundant because inboxPayloadSchema.safeParse(...) already guarantees
payload.exactLocation's shape; remove the isExactLocation function and
ExactLocation interface and instead use the schema-inferred type by assigning
const exactLocation = payload.exactLocation (or explicitly type it using the
inferred type from inboxPayloadSchema) wherever isExactLocation was used; if you
still want defense-in-depth, keep a minimal runtime check but prefer the
schema-typed value from inboxPayloadSchema to avoid duplicate runtime/type
logic.
In `@infra/firebase/firestore.rules`:
- Around line 149-166: The generated Firestore rules file is out of sync with
the build script; run the rules generator and commit the updated artifact:
execute the build script (scripts/build-rules.ts) to regenerate firestore.rules,
verify the produced content matches the reviewed rule blocks (e.g., the match
/field_mode_sessions/{uid} and match /report_notes/{n} rules), and commit the
updated firestore.rules output so CI no longer fails.
- Around line 135-140: The create rule for match /events/{eventId} allows any
municipal admin to append audit events, so tighten it by adding a parent-level
ownership check: when allowing create (in addition to isActivePrivileged() &&
(isMuniAdmin() || isSuperadmin())), require that the requesting admin’s
municipality matches the parent report_sharing document’s municipality (fetch
the parent report_sharing/{r} and compare its municipality id to the user’s
municipality id or to request.resource.data.targetMunicipalityId/sharedBy as
appropriate). Update the allow create clause to call the existing auth helpers
(isActivePrivileged(), isMuniAdmin(), isSuperadmin()) plus this parent-ownership
check so only admins from the same municipality as the parent can create events.
- Around line 106-108: The create rule for /reports/{reportId}/messages
currently only checks isActivePrivileged(), role predicates (isMuniAdmin(),
isAgencyAdmin(), isSuperadmin()), and authorUid equality, which lets privileged
users write messages under arbitrary report IDs; update the allow create rule to
include the same report-level authorization used by the read rule (i.e., verify
the user is scoped to that specific report ID/its parent muni/agency) in
addition to request.auth.uid == request.resource.data.authorUid so writes are
only allowed when the report-level check (the same predicate used by read)
passes.
- Around line 160-164: The create rule for report_notes currently includes
isAgencyAdmin() and isSuperadmin() but then only checks
report_ops.data.municipalityId == myMunicipality(), which rejects agency and
superadmin tokens that don't carry myMunicipality(); update the final predicate
to authorize based on the report's municipality using the shared admin helpers
instead of myMunicipality(): e.g. fetch reportId municipalityId into a variable
and allow when isMuniAdmin() || adminOf(municipalityId) || (isAgencyAdmin() &&
request.auth.token.agencyIds has municipalityId) || isSuperadmin(), while still
enforcing isActivePrivileged() and request.auth.uid ==
request.resource.data.authorUid so the logic in isActivePrivileged(),
isMuniAdmin(), isAgencyAdmin(), isSuperadmin(), adminOf(...), and agencyIds is
respected.
In `@infra/firebase/firestore.rules.template`:
- Around line 130-136: The write rule for match /field_mode_sessions/{uid}
allows request.auth.uid == uid but doesn't ensure the document's embedded uid
matches the path; update the write condition in that match (the rule using
request.auth.uid, isActivePrivileged(), and isSuperadmin()) to also require
request.resource.data.uid == uid so the stored uid field is consistent with the
document ID (use request.resource.data.uid for creates/updates).
In `@packages/shared-validators/lib/responders.js`:
- Around line 11-12: The schema currently allows fcmTokens and hasFcmToken to
drift out of sync; update the Zod schema in responders.js (the fcmTokens and
hasFcmToken fields) so hasFcmToken is derived from fcmTokens rather than an
independent default: remove the independent default for hasFcmToken and either
add a transform that sets hasFcmToken = fcmTokens.length > 0 after parsing or
add a refinement that enforces hasFcmToken === (fcmTokens.length > 0); reference
the fcmTokens and hasFcmToken fields inside that transform/refine to guarantee
the invariant at schema boundary.
In `@packages/shared-validators/lib/responders.test.js`:
- Around line 21-36: Add a new test in responders.test.js that calls
responderDocSchema.parse with an object where fcmTokens is a non-empty array but
hasFcmToken is omitted (use the same base fields as the existing test: uid,
agencyId, municipalityId, displayCode, specialisations, availabilityStatus,
isActive, schemaVersion, createdAt, updatedAt). Assert that the parsed result
sets hasFcmToken to true and that fcmTokens equals the provided non-empty array;
use the same ts fixture and expect(result.hasFcmToken).toBe(true) and
expect(result.fcmTokens).toEqual([...]) to ensure token-only input stays
consistent with responderDocSchema.parse.
In `@packages/shared-validators/src/reports.ts`:
- Around line 4-11: The reportOpsReportTypeSchema enum is too narrow and rejects
valid persisted report types; update reportOpsReportTypeSchema to include the
full set of persisted types (add 'medical', 'accident', 'structural',
'security', 'other' alongside the existing
'flood','fire','earthquake','typhoon','landslide','storm_surge'), and ensure any
other enums/validators around the same symbol (the other occurrence at lines
referenced near the same declaration) are updated consistently; alternatively,
if you prefer narrowing at ingestion, change process-inbox-item to explicitly
map/validate payload.reportType into the narrower enum before writing to
report_ops instead of changing the shared validator.
In `@packages/shared-validators/src/responders.ts`:
- Around line 12-13: The schema field hasFcmToken is unused; either remove it
from the responder schema in responders.ts and any derived types/usages, or keep
it and make writes/reads consistent: when creating/updating responders (e.g.,
the responder creation paths that set fcmTokens) also set hasFcmToken =
fcmTokens.length > 0, and in the FCM sending logic (the fcm-send service) update
both fcmTokens and hasFcmToken together (read length of fcmTokens to compute
hasFcmToken, and persist both atomically). Ensure any factories/bootstrap code
that seeds responders also respects this invariant so the field is never out of
sync.
---
Outside diff comments:
In `@functions/src/__tests__/rules/report-sharing.rules.test.ts`:
- Around line 104-117: The test contains a redundant dynamic import of setDoc;
remove the line that does "const { setDoc } = await
import('firebase/firestore')" inside the "any client write fails" test and use
the already imported top-level setDoc (and existing doc/db helpers) instead so
the test relies on the module-scoped import rather than performing an
unnecessary dynamic import.
In `@functions/src/triggers/process-inbox-item.ts`:
- Around line 238-245: The write to report_sms_consent via tx.set is missing the
required municipalityId field (per reportSmsConsentDocSchema); update the object
passed to tx.set in process-inbox-item.ts to include municipalityId (e.g.,
municipalityId: payload.municipalityId or municipalityId variable used elsewhere
in this file) so the stored document matches report_sms_consent schema and
avoids validation failures.
In `@infra/firebase/firestore.rules.template`:
- Line 1: The firestore rules file is out of sync with the build script; run the
build script scripts/build-rules.ts locally (e.g., pnpm exec tsx
scripts/build-rules.ts) to regenerate the rules (producing firestore.rules from
infra/firebase/firestore.rules.template), verify the updated rules, and commit
the regenerated firestore.rules file so CI and scripts/build-rules.ts are in
sync.
In `@packages/shared-validators/lib/reports.js`:
- Around line 176-188: The expiresAt max is computed at module load in
reportLookupDocSchema (expiresAt: .max(Date.now() + ...)), causing stale
validation; replace the static .max(...) with a runtime check by removing the
.max(...) and adding a dynamic validator (e.g., use z.number().int().refine(...)
or .superRefine(...) on reportLookupDocSchema) that compares expiresAt against
Date.now() + 365*24*60*60*1000 at validation time and returns a clear error
message; keep the existing integer requirement and schemaVersion checks intact.
In `@packages/shared-validators/lib/sms.d.ts`:
- Around line 53-61: The generated declaration for the status enum in sms.d.ts
(status: z.ZodEnum<...>) is out of sync with the source (it shows
queued→sent→sending instead of queued→sending→sent→delivered); fix by
regenerating the declarations for the shared-validators package—run the package
build (npm run build for the shared-validators package) so the lib/sms.d.ts is
recreated with the correct enum ordering for status, verify the z.ZodEnum values
match the source, and commit the updated .d.ts output.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 774a8be0-5cb9-4316-8d55-685cd0cd1e99
⛔ Files ignored due to path filters (17)
packages/shared-validators/lib/coordination.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.js.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/index.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/index.js.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.js.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.test.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/users.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/users.js.mapis excluded by!**/*.mappackages/shared-validators/lib/users.test.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/users.test.js.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
docs/learnings.mddocs/progress.mdfunctions/package.jsonfunctions/src/__tests__/rules/coordination.rules.test.tsfunctions/src/__tests__/rules/field-mode-sessions.rules.test.tsfunctions/src/__tests__/rules/report-messages.rules.test.tsfunctions/src/__tests__/rules/report-notes.rules.test.tsfunctions/src/__tests__/rules/report-sharing.rules.test.tsfunctions/src/__tests__/triggers/process-inbox-item.test.tsfunctions/src/triggers/process-inbox-item.tsinfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-validators/lib/coordination.d.tspackages/shared-validators/lib/coordination.jspackages/shared-validators/lib/coordination.test.jspackages/shared-validators/lib/index.d.tspackages/shared-validators/lib/index.jspackages/shared-validators/lib/moderation.d.tspackages/shared-validators/lib/reports.d.tspackages/shared-validators/lib/reports.jspackages/shared-validators/lib/reports.test.jspackages/shared-validators/lib/responders.d.tspackages/shared-validators/lib/responders.jspackages/shared-validators/lib/responders.test.d.tspackages/shared-validators/lib/responders.test.jspackages/shared-validators/lib/sms.d.tspackages/shared-validators/lib/users.d.tspackages/shared-validators/lib/users.jspackages/shared-validators/lib/users.test.d.tspackages/shared-validators/lib/users.test.jspackages/shared-validators/src/coordination.test.tspackages/shared-validators/src/coordination.tspackages/shared-validators/src/index.tspackages/shared-validators/src/reports.test.tspackages/shared-validators/src/reports.tspackages/shared-validators/src/responders.test.tspackages/shared-validators/src/responders.tspackages/shared-validators/src/users.test.tspackages/shared-validators/src/users.ts
Derived hasFcmToken boolean from fcmTokens array; widened persisted report type enum; added runtime lookup expiry check; regenerated lib/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Report messages, sharing events, field-mode sessions, and report notes: fixed map-key lookups, auth checks, and data validation constraints. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Switched collection list tests to getDoc where rule uses resource.data; added missing auth scenarios for report messages, notes, and sharing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removed redundant exact-location guard; added report_sms_consent municipalityId write; strengthened geohash regression test coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Added tsc lib/ stale-artifact learning; documented PR #63 follow-up scope and verification results; added CodeRabbit review summary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/firebase/firestore.rules (1)
1-438:⚠️ Potential issue | 🔴 CriticalCommit requires explicit approval before emulator deployment.
This is a new comprehensive Firestore security rules file governing access to all collections. Per deployment protocol, show the diff first for review and approval before proceeding.
Full diff of infra/firebase/firestore.rules
diff --git a/infra/firebase/firestore.rules b/infra/firebase/firestore.rules new file mode 100644 index 0000000..92be880 --- /dev/null +++ b/infra/firebase/firestore.rules @@ -0,0 +1,438 @@ +rules_version = '2'; +service cloud.firestore { + match /databases/{database}/documents { + + // ================================================================ + // Helper functions — names and role literals match spec §5.7 exactly. + // + // DO NOT ADD: `dispatcher`, `provincial_admin`, or platform-level + // `super_admin` — those roles do not exist in the spec's 5-role model. + // The dispatcher concept is folded into municipal_admin's duties. + // Platform and provincial superadmin are one unified role. + // ================================================================ + + function isAuthed() { + return request.auth != null + && request.auth.token.accountStatus == 'active'; + } + function role() { return request.auth.token.role; } + function uid() { return request.auth.uid; } + function myMunicipality() { return request.auth.token.municipalityId; } + function myAgency() { return request.auth.token.agencyId; } + function permittedMunis() { + return request.auth.token.permittedMunicipalityIds != null + ? request.auth.token.permittedMunicipalityIds : []; + } + function isCitizen() { return isAuthed() && role() == 'citizen'; } + function isResponder() { return isAuthed() && role() == 'responder'; } + function isMuniAdmin() { return isAuthed() && role() == 'municipal_admin'; } + function isAgencyAdmin(){ return isAuthed() && role() == 'agency_admin'; } + function isSuperadmin() { return isAuthed() && role() == 'provincial_superadmin'; } + function isActivePrivileged() { + return exists(/databases/$(database)/documents/active_accounts/$(uid())) + && get(/databases/$(database)/documents/active_accounts/$(uid())) + .data.accountStatus == 'active'; + } + function adminOf(muniId) { + return (isMuniAdmin() && myMunicipality() == muniId) + || (isSuperadmin() && muniId in permittedMunis()); + } + function canReadReportDoc(data) { + return (data.visibilityClass == 'public_alertable' && isAuthed()) + || adminOf(data.municipalityId); + } + function canReadEventDoc(data) { + return isActivePrivileged() + && (isMuniAdmin() || isSuperadmin() + || (isAgencyAdmin() && data.agencyId == myAgency())); + } + function validResponderTransition(from, to) { + return (from == 'pending' && to == 'accepted') + || (from == 'pending' && to == 'declined') + || (from == 'pending' && to == 'cancelled') + || (from == 'pending' && to == 'timed_out') + || (from == 'pending' && to == 'superseded') + || (from == 'accepted' && to == 'acknowledged') + || (from == 'accepted' && to == 'cancelled') + || (from == 'accepted' && to == 'superseded') + || (from == 'acknowledged' && to == 'en_route') + || (from == 'acknowledged' && to == 'cancelled') + || (from == 'acknowledged' && to == 'superseded') + || (from == 'en_route' && to == 'on_scene') + || (from == 'en_route' && to == 'cancelled') + || (from == 'en_route' && to == 'superseded') + || (from == 'on_scene' && to == 'resolved') + || (from == 'on_scene' && to == 'cancelled') + || (from == 'on_scene' && to == 'superseded') + || false; + } + + // ================================================================ + // Phase 2: citizen inbox + triptych + // ================================================================ + + match /report_inbox/{inboxId} { + allow read: if false; + allow create: if isCitizen() && request.resource.data.reporterUid == uid(); + allow update, delete: if false; + } + + match /reports/{reportId} { + allow read: if canReadReportDoc(resource.data); + allow create: if false; + + // reports.status is written by: + // - system triggers: processInboxItem, dispatchMirrorToReport + // - callables only: verifyReport, dispatchResponder, cancelDispatch, closeReport + // Responders NEVER write reports.status directly. + allow update: if adminOf(resource.data.municipalityId) + && request.resource.data.diff(resource.data) + .affectedKeys() + .hasOnly(['status', 'updatedAt', 'verifiedAt', 'assignedAt', 'closedAt', 'rejectedAt', 'rejectedReason', 'barangayId', 'severity', 'mediaRefs', 'hazardTagList']); + allow delete: if false; + + match /status_log/{e} { + allow read: if canReadReportDoc(get(/databases/$(database)/documents/reports/$(reportId)).data); + allow write: if false; + } + match /media/{m} { + allow read: if canReadReportDoc(get(/databases/$(database)/documents/reports/$(reportId)).data); + allow write: if false; + } + match /messages/{m} { + allow read: if isActivePrivileged() + && (adminOf(get(/databases/$(database)/documents/reports/$(reportId)).data.municipalityId) + || (isAgencyAdmin() + && exists(/databases/$(database)/documents/report_ops/$(reportId)) + && myAgency() in get(/databases/$(database)/documents/report_ops/$(reportId)).data.agencyIds)); + allow create: if isActivePrivileged() + && request.auth.uid == request.resource.data.authorUid + && ( + adminOf(get(/databases/$(database)/documents/reports/$(reportId)).data.municipalityId) + || ( + isAgencyAdmin() + && exists(/databases/$(database)/documents/report_ops/$(reportId)) + && myAgency() in get(/databases/$(database)/documents/report_ops/$(reportId)).data.agencyIds + ) + ); + allow update, delete: if false; + } + match /field_notes/{n} { + allow read: if isActivePrivileged() + && (isMuniAdmin() || isSuperadmin() + || (isResponder() && exists(/databases/$(database)/documents/dispatches/$(reportId + '_' + uid())))); + allow write: if false; + } + } + + match /report_private/{r} { + allow read: if adminOf(resource.data.municipalityId); + allow write: if false; + } + + match /report_ops/{r} { + allow read: if adminOf(resource.data.municipalityId) + || (isAgencyAdmin() && myAgency() in resource.data.agencyIds); + allow write: if false; + } + + match /report_sharing/{r} { + allow read: if adminOf(resource.data.ownerMunicipalityId) + || (isActivePrivileged() && resource.data.sharedWith.hasAny([myMunicipality()])); + allow write: if false; + + match /events/{eventId} { + allow read: if isActivePrivileged() + && (isMuniAdmin() || isAgencyAdmin() || isSuperadmin()); + allow create: if isActivePrivileged() + && request.auth.uid == request.resource.data.sharedBy + && exists(/databases/$(database)/documents/report_sharing/$(r)) + && adminOf(get(/databases/$(database)/documents/report_sharing/$(r)).data.ownerMunicipalityId); + allow update, delete: if false; + } + } + + match /report_contacts/{r} { + allow read: if adminOf(resource.data.municipalityId); + allow write: if false; + } + + match /field_mode_sessions/{uid} { + allow read: if isAuthed() && (request.auth.uid == uid || isSuperadmin()); + allow write: if isAuthed() + && request.auth.uid == uid + && request.resource.data.uid == uid + && isActivePrivileged() + && !isSuperadmin(); + } + + match /report_notes/{n} { + allow read: if isActivePrivileged() + && (isMuniAdmin() || isAgencyAdmin() || isSuperadmin()); + allow create: if isActivePrivileged() + && request.auth.uid == request.resource.data.authorUid + && exists(/databases/$(database)/documents/report_ops/$(request.resource.data.reportId)) + && ( + adminOf( + get(/databases/$(database)/documents/report_ops/$(request.resource.data.reportId)) + .data.municipalityId + ) + || ( + isAgencyAdmin() + && myAgency() in get(/databases/$(database)/documents/report_ops/$(request.resource.data.reportId)) + .data.agencyIds + ) + ); + allow update, delete: if false; + } + + match /report_lookup/{publicRef} { + allow read: if isAuthed(); + allow write: if false; + } + + // ================================================================ + // Phase 2: dispatches, responders, users + // ================================================================ + + // --- Dispatches --- + match /dispatches/{d} { + allow read: if isActivePrivileged() && ( + (isResponder() && resource.data.assignedTo.uid == uid()) + || adminOf(resource.data.municipalityId) + || (isAgencyAdmin() && myAgency() == resource.data.agencyId) + ); + + function isAssignedResponder() { + return isResponder() + && isActivePrivileged() + && resource.data.assignedTo.uid == request.auth.uid; + } + + function isAllowedDispatchFieldSet(newStatus) { + let base = ['status','lastStatusAt']; + let withSummary = base.concat(['resolutionSummary']); + return newStatus == 'resolved' + ? request.resource.data.diff(resource.data).affectedKeys().hasOnly(withSummary) + && request.resource.data.resolutionSummary is string + && request.resource.data.resolutionSummary.size() >= 1 + && request.resource.data.resolutionSummary.size() <= 2000 + : request.resource.data.diff(resource.data).affectedKeys().hasOnly(base); + } + + allow update: if isAssignedResponder() + && validResponderTransition(resource.data.status, request.resource.data.status) + && isAllowedDispatchFieldSet(request.resource.data.status); + allow create, delete: if false; + } + + // --- Responders and Users --- + match /responders/{rUid} { + allow read: if isAuthed() && ( + uid() == rUid + || (isAgencyAdmin() && myAgency() == resource.data.agencyId) + || (isMuniAdmin() && myMunicipality() == resource.data.municipalityId) + || isSuperadmin() + ); + allow update: if uid() == rUid + && request.resource.data.diff(resource.data).affectedKeys() + .hasOnly(['availabilityStatus']); + allow create, delete: if false; + } + + match /users/{uUid} { + allow read: if isAuthed() && ( + uid() == uUid + || (isMuniAdmin() && myMunicipality() == resource.data.municipalityId) + || isSuperadmin() + ); + allow update: if uid() == uUid + && request.resource.data.diff(resource.data).affectedKeys() + .hasOnly(['displayName','phone','barangayId']); + allow create, delete: if false; + } + + // ================================================================ + // Phase 1: identity spine — alerts, system_config, active_accounts, + // claim_revocations, rate_limits. + // ================================================================ + + match /system_config/{configId} { + allow read: if isAuthed(); + allow write: if isSuperadmin() && isActivePrivileged(); + } + + match /active_accounts/{accountUid} { + allow read: if isAuthed() && uid() == accountUid; + allow write: if false; + } + + match /claim_revocations/{accountUid} { + allow read: if isAuthed() && uid() == accountUid; + allow write: if false; + } + + match /rate_limits/{rateKey} { + allow read, write: if false; + } + + // ================================================================ + // Phase 2: public collections, audit, event streams + // ================================================================ + + match /alerts/{a} { + allow read: if isAuthed(); + allow write: if false; + } + + match /emergencies/{e} { + allow read: if isAuthed(); + allow write: if false; + } + + match /agencies/{a} { + allow read: if isAuthed(); + allow write: if isSuperadmin() && isActivePrivileged(); + } + + match /audit_logs/{l} { + allow read: if isSuperadmin() && isActivePrivileged(); + allow write: if false; + } + + match /dead_letters/{d} { + allow read: if isSuperadmin() && isActivePrivileged(); + allow write: if false; + } + + match /hazard_signals/{s} { + allow read: if isAuthed(); + allow write: if false; + } + + match /pending_media/{uploadId} { + allow read, write: if false; + } + + match /moderation_incidents/{m} { + allow read: if isActivePrivileged() && (isMuniAdmin() || isSuperadmin()); + allow write: if false; + } + + match /breakglass_events/{id} { + allow read: if isSuperadmin() && isActivePrivileged(); + allow write: if false; + } + + match /incident_response_events/{id} { + allow read: if isSuperadmin() && isActivePrivileged(); + allow write: if false; + } + + match /report_events/{eventId} { + allow read: if canReadEventDoc(resource.data); + allow write: if false; + } + + match /dispatch_events/{eventId} { + allow read: if canReadEventDoc(resource.data); + allow write: if false; + } + + // ================================================================ + // Phase 2: SMS layer — sms_inbox, sms_outbox, sms_sessions, + // sms_provider_health. + // ================================================================ + + match /sms_inbox/{msgId} { + allow read, write: if false; + } + + match /sms_outbox/{msgId} { + allow read: if isSuperadmin() && isActivePrivileged(); + allow write: if false; + } + + match /sms_sessions/{msisdnHash} { + allow read, write: if false; + } + + match /sms_provider_health/{providerId} { + allow read: if isSuperadmin(); + allow write: if false; + } + + // ================================================================ + // Phase 2: coordination — agency_assistance_requests, + // command_channel_threads, command_channel_messages, + // mass_alert_requests, shift_handoffs. + // ================================================================ + + match /agency_assistance_requests/{requestId} { + allow read: if isActivePrivileged() + && ((isMuniAdmin() && myMunicipality() == resource.data.requestedByMunicipality) + || (isAgencyAdmin() && myAgency() == resource.data.targetAgencyId) + || isSuperadmin()); + allow write: if false; + } + + match /command_channel_threads/{threadId} { + allow read: if isActivePrivileged() + && (isMuniAdmin() || isAgencyAdmin() || isSuperadmin()) + && resource.data.participantUids[request.auth.uid] == true; + allow write: if false; + } + + match /command_channel_messages/{messageId} { + allow read: if isActivePrivileged() + && (isMuniAdmin() || isAgencyAdmin() || isSuperadmin()) + && get(/databases/$(database)/documents/command_channel_threads/$(resource.data.threadId)) + .data.participantUids[request.auth.uid] == true; + allow write: if false; + } + + match /mass_alert_requests/{requestId} { + allow read: if isActivePrivileged() + && (isSuperadmin() + || (isMuniAdmin() && myMunicipality() == resource.data.requestedByMunicipality)); + allow write: if false; + } + + match /shift_handoffs/{handoffId} { + allow read: if isActivePrivileged() + && (request.auth.uid == resource.data.fromUid + || request.auth.uid == resource.data.toUid + || isSuperadmin()); + allow write: if false; + } + + // ================================================================ + // Phase 2: hazard zones — read-only reference and custom flood zones + // ================================================================ + + match /hazard_zones/{zoneId} { + allow read: if isActivePrivileged() + && (isSuperadmin() + || (resource.data.zoneType == 'reference' && isMuniAdmin()) + || (resource.data.zoneType == 'custom' && resource.data.scope == 'municipality' && isMuniAdmin() && resource.data.municipalityId == myMunicipality())); + allow write: if false; + match /history/{version} { + allow read: if isActivePrivileged() + && (isSuperadmin() + || (resource.data.zoneType == 'reference' && isMuniAdmin()) + || (resource.data.zoneType == 'custom' && resource.data.scope == 'municipality' && isMuniAdmin() && resource.data.municipalityId == myMunicipality())); + allow write: if false; + } + } + + // ================================================================ + // Default deny — every collection not explicitly matched above. + // Phase 2+ will add specific match blocks for reports, report_private, + // report_ops, dispatches, responders, etc. + // ================================================================ + + match /{document=**} { + allow read, write: if false; + } + } +}After approval, deploy to dev emulator with full test suite:
# Test the rules against the emulator npm run test:rules:firestore # Deploy to dev emulator (after approval only) firebase emulators:startRollback: Delete the file and commit:
git rm infra/firebase/firestore.rulesRequires explicit staging approval before production deployment. Minimum overnight soak in dev after verification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/firestore.rules` around lines 1 - 438, Add an explicit pre-deploy approval gate so this new comprehensive Firestore rules (rules_version/service cloud.firestore block) cannot be pushed to an emulator or CI deploy automatically: modify the deployment/emulator npm scripts (the one that runs firebase emulators:start or the test script that runs test:rules:firestore) to require an environment variable or PR approval flag (e.g. FIRESTORE_RULES_APPROVED=true) or to check for a specific PR label before starting the emulator, and update CI workflow to fail the deploy step unless that flag/label is present; also add a prominent comment at the top of the rules file (near rules_version) noting the required approval and the exact env var/label name so reviewers know the gating mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reviews/2026-04-24-pr63-phase5-pre-b-review.md`:
- Around line 26-37: Add a blank line before the fenced code block that contains
the snippet using payload.exactLocation and isExactLocation so the code fence is
surrounded by blank lines per MD031; locate the block that logs
INBOX_EXACT_LOCATION_MALFORMED (uses variables inboxId, payload.exactLocation
and calls log) and insert a single empty line immediately above the opening ```
to satisfy the markdown linter.
In `@functions/src/__tests__/rules/coordination.rules.test.ts`:
- Around line 277-300: The two test suites
describe('command_channel_threads/messages participant lookup') and
describe('command_channel_threads/messages — participant map key lookup') both
seed documents with the same IDs ('thread-1' and 'msg-1') in their beforeAll
blocks, causing potential data collisions; update the second suite to use
distinct document IDs (e.g., 'thread-2' and 'msg-2') or explicitly clear/reset
those docs before seeding so that functions like setDoc(doc(ctx.firestore(),
'command_channel_threads', 'thread-1')) and setDoc(doc(ctx.firestore(),
'command_channel_messages', 'msg-1')) in the second describe do not overwrite
the first suite's data.
In `@functions/src/__tests__/rules/report-messages.rules.test.ts`:
- Around line 96-106: Add an explicit test that unauthenticated users cannot
read messages by adding an it('denies unauthenticated reads', ...) case that
uses unauthed(env) and calls assertFails(getDoc(doc(db, 'reports', 'report-1',
'messages', 'msg-1'))); ensure you import unauthed from the rules-harness
alongside the existing helpers (e.g., authed) so the test runs correctly and
verifies read denial for the messages subcollection.
In `@functions/src/__tests__/rules/report-sharing.rules.test.ts`:
- Around line 182-184: The test uses staffClaims({ role: 'citizen' }) which is
semantically misleading for a citizen user; replace this with a proper citizen
claims helper (e.g., citizenClaims() or userClaims({ role: 'citizen' })) or
update the helper to explicitly support non-staff roles so the call is clear;
update the test to call authed(env, 'citizen-1', citizenClaims()) (or the
appropriate userClaims helper) and/or add a dedicated citizenClaims helper used
by other tests to avoid confusion around staffClaims.
In `@infra/firebase/firestore.rules`:
- Around line 160-167: The path wildcard {uid} in match
/field_mode_sessions/{uid} is shadowing the helper function uid(), so rename the
wildcard to a non-conflicting identifier (e.g., {userId} or {sessionUid}) and
update all uses inside that match: replace occurrences like request.auth.uid ==
uid and request.resource.data.uid == uid with request.auth.uid == userId (or
sessionUid) to preserve the same "self-only" semantics while avoiding the helper
name collision with uid().
In `@packages/shared-validators/src/responders.ts`:
- Around line 12-20: The fcmTokens element currently accepts empty/whitespace
strings and hasFcmToken is computed from the raw array length; strengthen
validation by making fcmTokens an array of non-blank strings and compute
hasFcmToken from the filtered, validated tokens: change the fcmTokens entry to
use z.string().trim().min(1) (so blank/whitespace tokens are rejected) and in
the schema .transform((d) => ...) replace the simple hasFcmToken derivation with
logic that first filters/uses the validated d.fcmTokens (e.g., filtered =
d.fcmTokens.filter(t => t.length > 0)) then return { ...d, fcmTokens: filtered,
hasFcmToken: filtered.length > 0 } so only usable tokens mark hasFcmToken true.
---
Outside diff comments:
In `@infra/firebase/firestore.rules`:
- Around line 1-438: Add an explicit pre-deploy approval gate so this new
comprehensive Firestore rules (rules_version/service cloud.firestore block)
cannot be pushed to an emulator or CI deploy automatically: modify the
deployment/emulator npm scripts (the one that runs firebase emulators:start or
the test script that runs test:rules:firestore) to require an environment
variable or PR approval flag (e.g. FIRESTORE_RULES_APPROVED=true) or to check
for a specific PR label before starting the emulator, and update CI workflow to
fail the deploy step unless that flag/label is present; also add a prominent
comment at the top of the rules file (near rules_version) noting the required
approval and the exact env var/label name so reviewers know the gating
mechanism.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1523c9a2-0540-458c-9d44-a3eac46f3302
⛔ Files ignored due to path filters (8)
packages/shared-validators/lib/coordination.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/coordination.js.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.js.mapis excluded by!**/*.mappackages/shared-validators/lib/reports.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.js.mapis excluded by!**/*.mappackages/shared-validators/lib/responders.test.js.mapis excluded by!**/*.map
📒 Files selected for processing (25)
docs/learnings.mddocs/progress.mddocs/reviews/2026-04-24-pr63-phase5-pre-b-review.mdfunctions/src/__tests__/rules/coordination.rules.test.tsfunctions/src/__tests__/rules/field-mode-sessions.rules.test.tsfunctions/src/__tests__/rules/report-messages.rules.test.tsfunctions/src/__tests__/rules/report-notes.rules.test.tsfunctions/src/__tests__/rules/report-sharing.rules.test.tsfunctions/src/__tests__/triggers/process-inbox-item.test.tsfunctions/src/triggers/process-inbox-item.tsinfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-validators/lib/coordination.jspackages/shared-validators/lib/reports.d.tspackages/shared-validators/lib/reports.jspackages/shared-validators/lib/reports.test.jspackages/shared-validators/lib/responders.d.tspackages/shared-validators/lib/responders.jspackages/shared-validators/lib/responders.test.jspackages/shared-validators/lib/sms.d.tspackages/shared-validators/src/coordination.tspackages/shared-validators/src/reports.test.tspackages/shared-validators/src/reports.tspackages/shared-validators/src/responders.test.tspackages/shared-validators/src/responders.ts
Comment on lines
+26
to
+37
| - **Fix:** Log a warning when `payload.exactLocation` is present but fails the type guard: | ||
| ```typescript | ||
| const exactLocation = isExactLocation(payload.exactLocation) ? payload.exactLocation : undefined | ||
| if (payload.exactLocation && !exactLocation) { | ||
| log({ | ||
| severity: 'WARN', | ||
| code: 'INBOX_EXACT_LOCATION_MALFORMED', | ||
| message: `exactLocation malformed for inbox ${inboxId}`, | ||
| data: { inboxId, received: payload.exactLocation }, | ||
| }) | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add blank line before the fenced code block.
Markdown linting flags MD031: fenced code blocks should be surrounded by blank lines. Add a blank line before the code fence on line 27 for consistent formatting.
📝 Proposed fix
- **Fix:** Log a warning when `payload.exactLocation` is present but fails the type guard:
+
```typescript
const exactLocation = isExactLocation(payload.exactLocation) ? payload.exactLocation : undefined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Fix:** Log a warning when `payload.exactLocation` is present but fails the type guard: | |
| ```typescript | |
| const exactLocation = isExactLocation(payload.exactLocation) ? payload.exactLocation : undefined | |
| if (payload.exactLocation && !exactLocation) { | |
| log({ | |
| severity: 'WARN', | |
| code: 'INBOX_EXACT_LOCATION_MALFORMED', | |
| message: `exactLocation malformed for inbox ${inboxId}`, | |
| data: { inboxId, received: payload.exactLocation }, | |
| }) | |
| } | |
| ``` | |
| - **Fix:** Log a warning when `payload.exactLocation` is present but fails the type guard: |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 27-27: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reviews/2026-04-24-pr63-phase5-pre-b-review.md` around lines 26 - 37,
Add a blank line before the fenced code block that contains the snippet using
payload.exactLocation and isExactLocation so the code fence is surrounded by
blank lines per MD031; locate the block that logs INBOX_EXACT_LOCATION_MALFORMED
(uses variables inboxId, payload.exactLocation and calls log) and insert a
single empty line immediately above the opening ``` to satisfy the markdown
linter.
Comment on lines
+182
to
+184
| const db = authed(env, 'citizen-1', staffClaims({ role: 'citizen' })) | ||
| await assertFails(addDoc(collection(db, 'report_sharing', 'r-share-1', 'events'), validEvent)) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
staffClaims helper may not be appropriate for citizen role.
The staffClaims({ role: 'citizen' }) call appears semantically incorrect—citizens are not staff. If this helper sets accountStatus: 'active' in claims, the test still works, but for clarity consider using a dedicated citizenClaims helper or verifying that staffClaims is intentionally designed to handle non-staff roles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/rules/report-sharing.rules.test.ts` around lines 182
- 184, The test uses staffClaims({ role: 'citizen' }) which is semantically
misleading for a citizen user; replace this with a proper citizen claims helper
(e.g., citizenClaims() or userClaims({ role: 'citizen' })) or update the helper
to explicitly support non-staff roles so the call is clear; update the test to
call authed(env, 'citizen-1', citizenClaims()) (or the appropriate userClaims
helper) and/or add a dedicated citizenClaims helper used by other tests to avoid
confusion around staffClaims.
Comment on lines
+12
to
+20
| fcmTokens: z.array(z.string()).default([]), | ||
| hasFcmToken: z.boolean().optional(), | ||
| lastTelemetryAt: z.number().int().optional(), | ||
| schemaVersion: z.number().int().positive(), | ||
| createdAt: z.number().int(), | ||
| updatedAt: z.number().int(), | ||
| }) | ||
| .strict() | ||
| .transform((d) => ({ ...d, hasFcmToken: d.fcmTokens.length > 0 })) |
Contributor
There was a problem hiding this comment.
Harden fcmTokens validation to reject empty/blank tokens.
At Line 12, z.string() allows ''/whitespace tokens, but Line 20 derives hasFcmToken from array length only. This can mark responders as token-enabled with unusable tokens.
Proposed fix
- fcmTokens: z.array(z.string()).default([]),
+ fcmTokens: z.array(z.string().trim().min(1)).default([]),As per coding guidelines: "Defensive Programming: Assume external input is malicious/broken. Validate at the boundary."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-validators/src/responders.ts` around lines 12 - 20, The
fcmTokens element currently accepts empty/whitespace strings and hasFcmToken is
computed from the raw array length; strengthen validation by making fcmTokens an
array of non-blank strings and compute hasFcmToken from the filtered, validated
tokens: change the fcmTokens entry to use z.string().trim().min(1) (so
blank/whitespace tokens are rejected) and in the schema .transform((d) => ...)
replace the simple hasFcmToken derivation with logic that first filters/uses the
validated d.fcmTokens (e.g., filtered = d.fcmTokens.filter(t => t.length > 0))
then return { ...d, fcmTokens: filtered, hasFcmToken: filtered.length > 0 } so
only usable tokens mark hasFcmToken true.
z.string().trim().min(1) ensures only usable tokens contribute to hasFcmToken; a whitespace-only string no longer marks a responder as token-enabled. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…essions Avoids shadowing the uid() helper function in the same rules scope; semantically identical but prevents silent confusion if uid() is ever called inside this match block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… read test Second command_channel suite now uses thread-2/msg-2 to avoid overwriting the first suite's seed data. Report messages test now explicitly verifies unauthenticated read is denied. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
infra/firebase/firestore.rules (1)
1-5:⚠️ Potential issue | 🟠 MajorRegenerate the built rules artifact to fix CI.
The pipeline is failing with:
firestore.rules is out of sync with scripts/build-rules.ts. Run the build script and commit the output:pnpm exec tsx scripts/build-rules.tsBased on learnings, security rules changes should be deployed to dev emulator first and the full test suite run before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/firebase/firestore.rules` around lines 1 - 5, The checked-in firestore.rules artifact is out of sync with the generator; run the build script scripts/build-rules.ts (e.g., pnpm exec tsx scripts/build-rules.ts) to regenerate infra/firebase/firestore.rules, review the generated output for the intended security changes, commit the updated firestore.rules file, then deploy the rules to the dev emulator and run the full test suite before merging to ensure CI and security tests pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reviews/2026-04-24-pr63-phase5-pre-b-review.md`:
- Around line 100-105: Update the review comment to point to the actual source
location: change the file reference from
`infra/firebase/firestore.rules:102-109` to `firestore.rules.template:83-99`
(the messages read rule) and keep the description about the
`get(/databases/$(database)/documents/report_ops/$(reportId)).data.agencyIds`
access needing an `exists(...)` guard so readers can find and fix the `messages`
read rule that uses `report_ops` without checking existence.
In `@functions/src/__tests__/rules/coordination.rules.test.ts`:
- Around line 213-274: Tests for participant lookup lack explicit
unauthenticated access checks; add two tests that use unauthed(env) to assert
reads are denied on 'command_channel_threads' ('thread-1') and
'command_channel_messages' ('msg-1'). Import the unauthed helper from
../helpers/rules-harness.js if not already present, then add an it('denies
unauthenticated reads on threads', ...) and an it('denies unauthenticated reads
on messages', ...) that call assertFails(getDoc(doc(db,
'command_channel_threads', 'thread-1'))) and assertFails(getDoc(doc(db,
'command_channel_messages', 'msg-1'))) respectively to strengthen coverage of
isActivePrivileged() enforcement.
- Around line 292-298: The test inserts a `msg-2` document with fields {message,
sentBy, sentAt} that are inconsistent with the expected
commandChannelMessageDocSchema used elsewhere (fields like authorUid,
authorRole, body, createdAt as in `msg-1`); update the setDoc call that writes
`doc(ctx.firestore(), 'command_channel_messages', 'msg-2')` in the
coordination.rules.test to use the same schema fields (authorUid, authorRole,
body, createdAt and schemaVersion) and values consistent with `msg-1` so both
test messages follow the `commandChannelMessageDocSchema`.
In `@functions/src/__tests__/rules/report-messages.rules.test.ts`:
- Around line 50-57: Add a negative test that verifies an agency_admin with a
different agencyId cannot read a message: create a seeded account using
seedActiveAccount with uid (e.g., 'pnp-admin'), role 'agency_admin',
municipalityId 'daet' and agencyId 'pnp-daet', build an authenticated DB using
authed with staffClaims({ role: 'agency_admin', municipalityId: 'daet',
agencyId: 'pnp-daet' }) and assert the read is denied by calling
assertFails(getDoc(doc(db, 'reports', 'report-1', 'messages', 'msg-1'))); name
the test something like "denies agency admin read when their agency is not in
report_ops.agencyIds" to mirror the existing positive test and ensure it uses
the same report id ('report-1') which only includes 'bfp-daet' in its agencyIds.
- Around line 50-57: Add a new test in report-messages.rules.test.ts that
asserts an agency_admin can create a message when their agency is listed in
report_ops.agencyIds: use authed(env, 'bfp-admin', staffClaims({ role:
'agency_admin', municipalityId: 'daet', agencyId: 'bfp-daet' })), then call
assertSucceeds(addDoc(collection(db, 'reports', 'report-1', 'messages'), {
authorUid: 'bfp-admin', body: 'Agency response.', createdAt: ts, schemaVersion:
1 })) to mirror the read test and validate the write path governed by the
Firestore rules around report_ops.agencyIds.
In `@infra/firebase/firestore.rules.template`:
- Around line 125-133: The events read rule is too broad; restrict it to only
allow privileged users who are admins of the parent report_sharing's
ownerMunicipalityId or whose municipality is included in the parent
report_sharing.sharedWith. Update the match /events/{eventId} read rule to fetch
the parent report_sharing document (use
get(/databases/$(database)/documents/report_sharing/$(r)) or the correct parent
path variable used in this rules file) and require
adminOf(parent.data.ownerMunicipalityId) || (request.auth.token.municipalityId
in parent.data.sharedWith) in addition to isActivePrivileged() and existing
isMuniAdmin()/isAgencyAdmin()/isSuperadmin() checks so reads are scoped to the
parent document's ownerMunicipalityId/sharedWith rather than all events.
---
Duplicate comments:
In `@infra/firebase/firestore.rules`:
- Around line 1-5: The checked-in firestore.rules artifact is out of sync with
the generator; run the build script scripts/build-rules.ts (e.g., pnpm exec tsx
scripts/build-rules.ts) to regenerate infra/firebase/firestore.rules, review the
generated output for the intended security changes, commit the updated
firestore.rules file, then deploy the rules to the dev emulator and run the full
test suite before merging to ensure CI and security tests pass.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba5eb7f4-6be0-44ef-8567-32e408c8f4e6
⛔ Files ignored due to path filters (1)
packages/shared-validators/lib/responders.js.mapis excluded by!**/*.map
📒 Files selected for processing (7)
docs/reviews/2026-04-24-pr63-phase5-pre-b-review.mdfunctions/src/__tests__/rules/coordination.rules.test.tsfunctions/src/__tests__/rules/report-messages.rules.test.tsinfra/firebase/firestore.rulesinfra/firebase/firestore.rules.templatepackages/shared-validators/lib/responders.jspackages/shared-validators/src/responders.ts
Comment on lines
+100
to
+105
| ### 10. `messages` read rule: `get()` on `report_ops` without `exists()` guard | ||
|
|
||
| - **File:** `infra/firebase/firestore.rules:102-109` | ||
| - **Agent:** silent-failure-hunter (Severity: MEDIUM) | ||
| - **Issue:** The rule reads `get(/databases/$(database)/documents/report_ops/$(reportId)).data.agencyIds` without verifying the doc exists. Accessing `.data` on a non-existent resource evaluates to `null`; the `in` operator on `null` behaves unpredictably. | ||
| - **Fix:** Guard with `exists(...)` before accessing `.data.agencyIds`, or restructure to avoid the nested `get()` without existence check. |
Contributor
There was a problem hiding this comment.
Issue #10 references incorrect line numbers.
Issue #10 references infra/firebase/firestore.rules:102-109 but the actual messages rules in the template are at lines 83-99. The line numbers should reference firestore.rules.template for accuracy, as that's the source file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reviews/2026-04-24-pr63-phase5-pre-b-review.md` around lines 100 - 105,
Update the review comment to point to the actual source location: change the
file reference from `infra/firebase/firestore.rules:102-109` to
`firestore.rules.template:83-99` (the messages read rule) and keep the
description about the
`get(/databases/$(database)/documents/report_ops/$(reportId)).data.agencyIds`
access needing an `exists(...)` guard so readers can find and fix the `messages`
read rule that uses `report_ops` without checking existence.
Comment on lines
+213
to
+274
| describe('command_channel_threads/messages participant lookup', () => { | ||
| beforeAll(async () => { | ||
| await env.withSecurityRulesDisabled(async (ctx) => { | ||
| await setDoc(doc(ctx.firestore(), 'command_channel_threads', 'thread-1'), { | ||
| threadId: 'thread-1', | ||
| reportId: 'report-1', | ||
| threadType: 'agency_assistance', | ||
| subject: 'Need help', | ||
| participantUids: { 'daet-admin': true }, | ||
| createdBy: 'daet-admin', | ||
| createdAt: ts, | ||
| updatedAt: ts, | ||
| schemaVersion: 1, | ||
| }) | ||
|
|
||
| await setDoc(doc(ctx.firestore(), 'command_channel_messages', 'msg-1'), { | ||
| threadId: 'thread-1', | ||
| authorUid: 'daet-admin', | ||
| authorRole: 'municipal_admin', | ||
| body: 'hello', | ||
| createdAt: ts, | ||
| schemaVersion: 1, | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| it('allows participant to read thread', async () => { | ||
| const db = authed( | ||
| env, | ||
| 'daet-admin', | ||
| staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }), | ||
| ) | ||
| await assertSucceeds(getDoc(doc(db, 'command_channel_threads', 'thread-1'))) | ||
| }) | ||
|
|
||
| it('denies non-participant from reading thread', async () => { | ||
| const db = authed( | ||
| env, | ||
| 'other-admin', | ||
| staffClaims({ role: 'municipal_admin', municipalityId: 'mercedes' }), | ||
| ) | ||
| await assertFails(getDoc(doc(db, 'command_channel_threads', 'thread-1'))) | ||
| }) | ||
|
|
||
| it('allows participant to read message through parent thread lookup', async () => { | ||
| const db = authed( | ||
| env, | ||
| 'daet-admin', | ||
| staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }), | ||
| ) | ||
| await assertSucceeds(getDocs(collection(db, 'agency_assistance_requests'))) | ||
| await assertSucceeds(getDoc(doc(db, 'command_channel_messages', 'msg-1'))) | ||
| }) | ||
|
|
||
| it('denies non-participant from reading message', async () => { | ||
| const db = authed( | ||
| env, | ||
| 'other-admin', | ||
| staffClaims({ role: 'municipal_admin', municipalityId: 'mercedes' }), | ||
| ) | ||
| await assertFails(getDoc(doc(db, 'command_channel_messages', 'msg-1'))) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding unauthenticated read denial tests.
The participant lookup tests cover authorized/unauthorized participants but don't explicitly test unauthenticated access denial. While isActivePrivileged() implicitly requires authentication, explicit tests would strengthen coverage.
📝 Suggested test addition
it('denies unauthenticated reads on threads', async () => {
const db = unauthed(env)
await assertFails(getDoc(doc(db, 'command_channel_threads', 'thread-1')))
})
it('denies unauthenticated reads on messages', async () => {
const db = unauthed(env)
await assertFails(getDoc(doc(db, 'command_channel_messages', 'msg-1')))
})Note: Requires importing unauthed from ../helpers/rules-harness.js.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/rules/coordination.rules.test.ts` around lines 213 -
274, Tests for participant lookup lack explicit unauthenticated access checks;
add two tests that use unauthed(env) to assert reads are denied on
'command_channel_threads' ('thread-1') and 'command_channel_messages' ('msg-1').
Import the unauthed helper from ../helpers/rules-harness.js if not already
present, then add an it('denies unauthenticated reads on threads', ...) and an
it('denies unauthenticated reads on messages', ...) that call
assertFails(getDoc(doc(db, 'command_channel_threads', 'thread-1'))) and
assertFails(getDoc(doc(db, 'command_channel_messages', 'msg-1'))) respectively
to strengthen coverage of isActivePrivileged() enforcement.
Comment on lines
+292
to
+298
| await setDoc(doc(ctx.firestore(), 'command_channel_messages', 'msg-2'), { | ||
| threadId: 'thread-2', | ||
| message: 'hello', | ||
| sentBy: 'daet-admin', | ||
| sentAt: ts, | ||
| schemaVersion: 1, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Message document fields inconsistent with schema.
The msg-2 document uses { message, sentBy, sentAt } fields which don't match commandChannelMessageDocSchema from packages/shared-validators/src/coordination.ts that expects { authorUid, authorRole, body, createdAt } (as used in msg-1).
While Firestore rules don't enforce schema, using inconsistent test data may:
- Mask future regressions if rules are updated to validate fields
- Confuse readers about the expected document structure
💡 Suggested alignment with schema
await setDoc(doc(ctx.firestore(), 'command_channel_messages', 'msg-2'), {
threadId: 'thread-2',
- message: 'hello',
- sentBy: 'daet-admin',
- sentAt: ts,
+ authorUid: 'daet-admin',
+ authorRole: 'municipal_admin',
+ body: 'hello',
+ createdAt: ts,
schemaVersion: 1,
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/rules/coordination.rules.test.ts` around lines 292 -
298, The test inserts a `msg-2` document with fields {message, sentBy, sentAt}
that are inconsistent with the expected commandChannelMessageDocSchema used
elsewhere (fields like authorUid, authorRole, body, createdAt as in `msg-1`);
update the setDoc call that writes `doc(ctx.firestore(),
'command_channel_messages', 'msg-2')` in the coordination.rules.test to use the
same schema fields (authorUid, authorRole, body, createdAt and schemaVersion)
and values consistent with `msg-1` so both test messages follow the
`commandChannelMessageDocSchema`.
Comment on lines
+50
to
+57
| it('allows agency admin to read a message when report_ops agencyIds includes their agency', async () => { | ||
| const db = authed( | ||
| env, | ||
| 'bfp-admin', | ||
| staffClaims({ role: 'agency_admin', municipalityId: 'daet', agencyId: 'bfp-daet' }), | ||
| ) | ||
| await assertSucceeds(getDoc(doc(db, 'reports', 'report-1', 'messages', 'msg-1'))) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding agency admin read denial test.
The test verifies an agency admin with matching agencyId can read, but doesn't verify that an agency admin with a different agencyId is denied. This negative case would strengthen confidence in the agency scoping logic.
📝 Suggested test addition
it('denies agency admin read when their agency is not in report_ops.agencyIds', async () => {
await seedActiveAccount(env, {
uid: 'pnp-admin',
role: 'agency_admin',
municipalityId: 'daet',
agencyId: 'pnp-daet', // Different agency
})
const db = authed(
env,
'pnp-admin',
staffClaims({ role: 'agency_admin', municipalityId: 'daet', agencyId: 'pnp-daet' }),
)
// report-1 only has bfp-daet in agencyIds
await assertFails(getDoc(doc(db, 'reports', 'report-1', 'messages', 'msg-1')))
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/rules/report-messages.rules.test.ts` around lines 50
- 57, Add a negative test that verifies an agency_admin with a different
agencyId cannot read a message: create a seeded account using seedActiveAccount
with uid (e.g., 'pnp-admin'), role 'agency_admin', municipalityId 'daet' and
agencyId 'pnp-daet', build an authenticated DB using authed with staffClaims({
role: 'agency_admin', municipalityId: 'daet', agencyId: 'pnp-daet' }) and assert
the read is denied by calling assertFails(getDoc(doc(db, 'reports', 'report-1',
'messages', 'msg-1'))); name the test something like "denies agency admin read
when their agency is not in report_ops.agencyIds" to mirror the existing
positive test and ensure it uses the same report id ('report-1') which only
includes 'bfp-daet' in its agencyIds.
🧹 Nitpick | 🔵 Trivial
Missing agency admin write test.
The test verifies agency admin can read messages but doesn't test that agency admin can create messages when their agency is in report_ops.agencyIds. This is a valid code path per the rules at firestore.rules.template:108-117.
📝 Suggested test addition
it('allows agency admin to write a message when report_ops agencyIds includes their agency', async () => {
const db = authed(
env,
'bfp-admin',
staffClaims({ role: 'agency_admin', municipalityId: 'daet', agencyId: 'bfp-daet' }),
)
await assertSucceeds(
addDoc(collection(db, 'reports', 'report-1', 'messages'), {
authorUid: 'bfp-admin',
body: 'Agency response.',
createdAt: ts,
schemaVersion: 1,
}),
)
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/__tests__/rules/report-messages.rules.test.ts` around lines 50
- 57, Add a new test in report-messages.rules.test.ts that asserts an
agency_admin can create a message when their agency is listed in
report_ops.agencyIds: use authed(env, 'bfp-admin', staffClaims({ role:
'agency_admin', municipalityId: 'daet', agencyId: 'bfp-daet' })), then call
assertSucceeds(addDoc(collection(db, 'reports', 'report-1', 'messages'), {
authorUid: 'bfp-admin', body: 'Agency response.', createdAt: ts, schemaVersion:
1 })) to mirror the read test and validate the write path governed by the
Firestore rules around report_ops.agencyIds.
Comment on lines
+125
to
+133
| match /events/{eventId} { | ||
| allow read: if isActivePrivileged() | ||
| && (isMuniAdmin() || isAgencyAdmin() || isSuperadmin()); | ||
| allow create: if isActivePrivileged() | ||
| && request.auth.uid == request.resource.data.sharedBy | ||
| && exists(/databases/$(database)/documents/report_sharing/$(r)) | ||
| && adminOf(get(/databases/$(database)/documents/report_sharing/$(r)).data.ownerMunicipalityId); | ||
| allow update, delete: if false; | ||
| } |
Contributor
There was a problem hiding this comment.
Events read rule may be overly permissive.
The events read rule (lines 126-127) allows any active privileged user with isMuniAdmin || isAgencyAdmin || isSuperadmin to read all events under any report_sharing document. This doesn't scope reads to the parent document's ownerMunicipalityId or sharedWith municipalities.
Consider tightening to match the parent document's access pattern:
💡 Suggested tightening
match /events/{eventId} {
- allow read: if isActivePrivileged()
- && (isMuniAdmin() || isAgencyAdmin() || isSuperadmin());
+ allow read: if isActivePrivileged()
+ && (adminOf(get(/databases/$(database)/documents/report_sharing/$(r)).data.ownerMunicipalityId)
+ || resource.data.targetMunicipalityId == myMunicipality());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/firebase/firestore.rules.template` around lines 125 - 133, The events
read rule is too broad; restrict it to only allow privileged users who are
admins of the parent report_sharing's ownerMunicipalityId or whose municipality
is included in the parent report_sharing.sharedWith. Update the match
/events/{eventId} read rule to fetch the parent report_sharing document (use
get(/databases/$(database)/documents/report_sharing/$(r)) or the correct parent
path variable used in this rules file) and require
adminOf(parent.data.ownerMunicipalityId) || (request.auth.token.municipalityId
in parent.data.sharedWith) in addition to isActivePrivileged() and existing
isMuniAdmin()/isAgencyAdmin()/isSuperadmin() checks so reads are scoped to the
parent document's ownerMunicipalityId/sharedWith rather than all events.
This was referenced Apr 24, 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
This PR lands the PRE-B schema and rules foundation for Phase 5.
report_ops.reportTypeand a 6-charlocationGeohashfromexactLocationwhen present.Validation
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation