feat/face-compare-api-addition#1331
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new face comparison endpoint is introduced across the Idfy client library and verification interface. Request and response types are defined at both the Idfy provider level and the generic interface level. The Idfy client implements the HTTP call via Servant. An adapter bridges interface types to the Idfy client, and a dispatcher routes requests to Idfy or returns "Not Implemented" for other providers. ChangesFace Comparison Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant IdfyClient
participant IdfyService
Client->>IdfyClient: faceCompare(FaceCompareRequest)
IdfyClient->>IdfyService: POST /v3/tasks/sync/compare/face
IdfyService-->>IdfyClient: FaceCompareResponse
IdfyClient-->>Client: FaceCompareResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@ClaudeWeb please review |
|
I'll fetch the PR details, diff, and run a security scan in parallel.Let me try using the GitHub MCP tools instead.Now let me get the changed files and look at the diff.Now let me look at the surrounding context in the existing source files to compare patterns.Now let me check the existing Idfy request/response types and the Interface/Idfy.hs for patterns around PR Review:
|
|
Checked ClaudeWeb comments . nothing critical |
c35c0ce to
3caec28
Compare
|
@ClaudeWeb Can you review, if errors are handled properly ? |
|
Let me do a focused error handling review across all changed files.Let me fetch Error Handling Review —
|
| Layer | Verdict |
|---|---|
Client.hs HTTP transport errors |
✓ callApiUnwrappingApiError handles 4xx/5xx/network |
decrypt cfg.apiKey / cfg.accountId |
✓ throws on decryption failure |
buildIdfyRequest (GUID gen) |
✓ monadic, no silent failure path |
Interface.hs dispatcher throwError "Not Implemented!" |
✓ non-Idfy configs fail loudly |
JSON decode of FaceCompareResponseData |
✓ all fields Maybe, Servant throws on parse failure |
0e06507 to
a7dd9d8
Compare
![coderabbitai[bot]](https://avatars.github.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 `@lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs`:
- Line 26: IdfyCfg's faceCompareRetryLimit is currently Maybe Int with no
validation; update the decode/constructor to enforce a non-negative bounded
value (e.g., change the field to Maybe Natural/Word or validate in the FromJSON
instance for IdfyCfg) and apply a max cap constant (e.g.,
MAX_FACE_COMPARE_RETRIES) or reject out-of-range values, then ensure faceCompare
still uses retryLimit = fromMaybe 1 cfg.faceCompareRetryLimit but with the
validated/clamped value so negative or excessively large inputs are prevented;
modify the FromJSON for IdfyCfg (or a smart constructor) to perform this check
and return a parse error or clamp to the cap.
🪄 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: CHILL
Plan: Pro
Run ID: 2886cb79-21cf-47f5-88de-12f63591df37
📒 Files selected for processing (7)
lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hslib/mobility-core/src/Kernel/External/Verification/Interface.hslib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hslib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs
| secret :: EncryptedField 'AsEncrypted Text, | ||
| url :: BaseUrl | ||
| url :: BaseUrl, | ||
| faceCompareRetryLimit :: Maybe Int |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all uses of faceCompareRetryLimit and whether any non-negative/bounded validation exists.
rg -n -C3 '\bfaceCompareRetryLimit\b|fromMaybe\s+1\s+cfg\.faceCompareRetryLimit|callWithRetry\s*\(' \
lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs \
lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hsRepository: nammayatri/shared-kernel
Length of output: 2361
🏁 Script executed:
#!/bin/bash
# Inspect callWithRetry definition and any handling of retryLimit/attemptsLeft <= 0
rg -n "callWithRetry|attemptsLeft" lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs -nRepository: nammayatri/shared-kernel
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Show the surrounding implementation area for callWithRetry
sed -n '470,560p' lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hsRepository: nammayatri/shared-kernel
Length of output: 3378
🏁 Script executed:
#!/bin/bash
# Inspect Config.hs around the IdfyCfg definition and the faceCompareRetryLimit field
sed -n '1,120p' lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hsRepository: nammayatri/shared-kernel
Length of output: 1269
🏁 Script executed:
#!/bin/bash
# Search whole repo for any other uses/validation of faceCompareRetryLimit
rg -n "\bfaceCompareRetryLimit\b" -S .Repository: nammayatri/shared-kernel
Length of output: 310
Constrain faceCompareRetryLimit to a non-negative, bounded value
IdfyCfg defines faceCompareRetryLimit :: Maybe Int with derived FromJSON (no validation), and the only consumer is retryLimit = fromMaybe 1 cfg.faceCompareRetryLimit in faceCompare. Negative values make attemptsLeft > 0 false immediately (no retries, early failure), while very large values can amplify external-call retries and latency. Add non-negative/bounded validation at the config decode boundary (e.g., Maybe Natural/Word plus a max cap) and/or clamp/explicitly reject out-of-range values.
🤖 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 `@lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs` at line
26, IdfyCfg's faceCompareRetryLimit is currently Maybe Int with no validation;
update the decode/constructor to enforce a non-negative bounded value (e.g.,
change the field to Maybe Natural/Word or validate in the FromJSON instance for
IdfyCfg) and apply a max cap constant (e.g., MAX_FACE_COMPARE_RETRIES) or reject
out-of-range values, then ensure faceCompare still uses retryLimit = fromMaybe 1
cfg.faceCompareRetryLimit but with the validated/clamped value so negative or
excessively large inputs are prevented; modify the FromJSON for IdfyCfg (or a
smart constructor) to perform this check and return a parse error or clamp to
the cap.
a7dd9d8 to
06bb283
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs`:
- Around line 551-553: The code returns FaceCompareResp {faceComparedData = Just
result} for any Just result; update the case handling for resp.result to first
verify the face-detection flags on the returned result (the detection fields on
result) are present and true, and only then set faceComparedData to Just result;
if any detection flag is false or missing, throw/return InternalError instead of
returning faceComparedData. Ensure this validation is implemented where
resp.result is matched (the Just result branch) and that the error uses the
existing InternalError type/constructor used elsewhere in this module.
- Around line 548-555: The current retry loop regenerates idfyReq (and its
task_id) each attempt in callWithRetry by calling buildIdfyRequest(req.driverId
reqData) inside the recursion; change the logic to build idfyReq once before
starting retries and pass that same idfyReq into callWithRetry (or convert
callWithRetry to accept an idfyReq parameter) so Idfy.faceCompare always
receives the same payload; update references to idfyReq inside callWithRetry and
keep the same retry/decrement logic and error handling around Idfy.faceCompare
and the attemptsLeft parameter.
🪄 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: CHILL
Plan: Pro
Run ID: cb64f86a-75d1-4562-a88c-c08cada6e8d3
📒 Files selected for processing (7)
lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hslib/mobility-core/src/Kernel/External/Verification/Interface.hslib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hslib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface.hs
- lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
| case resp.result of | ||
| Just result -> pure FaceCompareResp {faceComparedData = Just result} | ||
| Nothing |
There was a problem hiding this comment.
Gate success on face-detection validity before returning match data.
At Line 552, success is returned for any Just result, but face-compare outputs are unreliable when one or both images have no detected face. Please enforce a validation guard on the detection flags and throw InternalError when detection is false/missing instead of returning faceComparedData.
🤖 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 `@lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs` around
lines 551 - 553, The code returns FaceCompareResp {faceComparedData = Just
result} for any Just result; update the case handling for resp.result to first
verify the face-detection flags on the returned result (the detection fields on
result) are present and true, and only then set faceComparedData to Just result;
if any detection flag is false or missing, throw/return InternalError instead of
returning faceComparedData. Ensure this validation is implemented where
resp.result is matched (the Just result branch) and that the error uses the
existing InternalError type/constructor used elsewhere in this module.
| deriving (Show, Generic, FromJSON, ToJSON, ToSchema) | ||
|
|
||
| data FaceCompareReq = FaceCompareReq | ||
| { selfieImage :: Text, |
There was a problem hiding this comment.
We can keep generic names. Order doesn't matter anyway
06bb283 to
a14d877
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs (1)
551-553:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftValidate face detection before returning match data.
The code returns
faceComparedData = Just resultfor anyJust resultwithout verifying that faces were actually detected in both images. According to the Idfy face-compare contract, whenface_detectedisFalseor missing for either image, theis_a_matchandmatch_scorefields are unreliable. Add a validation guard to check the face-detection flags onresultand throwInternalErrorwhen detection fails, rather than returning potentially misleading biometric match data.🛡️ Suggested validation logic
case resp.result of - Just result -> pure FaceCompareResp {faceComparedData = Just result} + Just result -> do + -- Validate face detection before returning result + -- Access result.face_detected fields and verify both are True + -- If either is False/Nothing, throw InternalError + -- Example (adjust based on actual FaceCompareResponseData structure): + -- when (result.face_detected /= Just True) $ + -- throwError $ InternalError "Face detection failed in one or both images." + pure FaceCompareResp {faceComparedData = Just result} NothingNote: You'll need to inspect the
Idfy.FaceCompareResponseDatastructure to determine the exact field names for face detection flags.As per coding guidelines and PR objectives, biometric APIs should validate detection success before returning match results.
🤖 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 `@lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs` around lines 551 - 553, The current branch unconditionally returns FaceCompareResp {faceComparedData = Just result} when resp.result is Just result; instead validate detection flags on the Idfy.FaceCompareResponseData (inspect fields like face_detected / faceDetection / per-image flags inside the result) before returning: if either image's face-detected flag is False or missing, throw an InternalError (use the same error type used elsewhere) rather than returning match data; update the case in the function handling resp.result to perform this guard and only set faceComparedData = Just result when detection succeeded for both images.
🧹 Nitpick comments (2)
lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs (2)
349-353: ⚡ Quick winConsider domain-meaningful field names to improve clarity.
The current field names
documentImage1anddocumentImage2are vendor-opaque and don't communicate intent. Following the pattern ofNameCompareReq(which usesextractedNameandverifiedName), consider renaming these fields to convey their semantic roles—e.g.,selfieImageanddocumentImage—and map them to the Idfy provider'sdocument1/document2fields in the adapter layer atInterface/Idfy.hs.♻️ Suggested refactor
data FaceCompareReq = FaceCompareReq - { documentImage1 :: Text, - documentImage2 :: Text, + { selfieImage :: Text, + documentImage :: Text, driverId :: Text }Then update the adapter in
Interface/Idfy.hslines 544-546:let reqData = Idfy.FaceCompareRequestBody - { document1 = req.documentImage1, - document2 = req.documentImage2 + { document1 = req.selfieImage, + document2 = req.documentImage }As per coding guidelines, this aligns with the domain-driven naming pattern seen in
NameCompareReq.🤖 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 `@lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs` around lines 349 - 353, Rename the vendor-opaque fields in FaceCompareReq to domain-meaningful names (e.g., change documentImage1/documentImage2 to selfieImage/documentImage or similar) and update any construction/consumption sites accordingly; keep the type FaceCompareReq and its Text fields but adjust usages in the Idfy adapter (Interface/Idfy.hs) to map the new names to the provider's document1/document2 fields (similar to how NameCompareReq uses extractedName/verifiedName) so external-facing semantics are clear while preserving provider mapping in the adapter.
356-359: Response type exposes vendor-specific structure.
FaceCompareRespdirectly wrapsIdfy.FaceCompareResponseData, which mirrors the existingNameCompareResppattern but couples the interface to the Idfy provider. If multi-provider support is anticipated, consider abstracting the response structure to domain types.🤖 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 `@lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs` around lines 356 - 359, FaceCompareResp currently exposes the vendor-specific Idfy.FaceCompareResponseData (same anti-pattern as NameCompareResp); replace the direct wrapper with a domain-level type (e.g., FaceComparisonResult or FaceCompareRespDomain) that captures only the fields our domain needs, keep or derive FromJSON/ToJSON/ToSchema for that domain type, and update the Idfy adapter/translator to map Idfy.FaceCompareResponseData -> FaceComparisonResult before returning; ensure references to FaceCompareResp in callers are updated to the new domain type or a small shim constructor to avoid leaking Idfy types into the interface.
🤖 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.
Duplicate comments:
In `@lib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hs`:
- Around line 551-553: The current branch unconditionally returns
FaceCompareResp {faceComparedData = Just result} when resp.result is Just
result; instead validate detection flags on the Idfy.FaceCompareResponseData
(inspect fields like face_detected / faceDetection / per-image flags inside the
result) before returning: if either image's face-detected flag is False or
missing, throw an InternalError (use the same error type used elsewhere) rather
than returning match data; update the case in the function handling resp.result
to perform this guard and only set faceComparedData = Just result when detection
succeeded for both images.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs`:
- Around line 349-353: Rename the vendor-opaque fields in FaceCompareReq to
domain-meaningful names (e.g., change documentImage1/documentImage2 to
selfieImage/documentImage or similar) and update any construction/consumption
sites accordingly; keep the type FaceCompareReq and its Text fields but adjust
usages in the Idfy adapter (Interface/Idfy.hs) to map the new names to the
provider's document1/document2 fields (similar to how NameCompareReq uses
extractedName/verifiedName) so external-facing semantics are clear while
preserving provider mapping in the adapter.
- Around line 356-359: FaceCompareResp currently exposes the vendor-specific
Idfy.FaceCompareResponseData (same anti-pattern as NameCompareResp); replace the
direct wrapper with a domain-level type (e.g., FaceComparisonResult or
FaceCompareRespDomain) that captures only the fields our domain needs, keep or
derive FromJSON/ToJSON/ToSchema for that domain type, and update the Idfy
adapter/translator to map Idfy.FaceCompareResponseData -> FaceComparisonResult
before returning; ensure references to FaceCompareResp in callers are updated to
the new domain type or a small shim constructor to avoid leaking Idfy types into
the interface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7eaba28a-4405-4974-9f84-bd2368956e77
📒 Files selected for processing (7)
lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hslib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hslib/mobility-core/src/Kernel/External/Verification/Interface.hslib/mobility-core/src/Kernel/External/Verification/Interface/Idfy.hslib/mobility-core/src/Kernel/External/Verification/Interface/Types.hs
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/mobility-core/src/Kernel/External/Verification/Interface.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Config.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Request.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Types/Response.hs
- lib/mobility-core/src/Kernel/External/Verification/Idfy/Client.hs
a14d877 to
c6a8b60
Compare
4 participants
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
New Features
Configuration