Project: doctolib-doctolib · Branch: .invalid · Mode: ` · **Model:**claude-opus-4-7`
Stats: 1 user messages, 26 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Review the following files in the packages/@doctolib/national-health-records package for style, guidelines, and best practices compliance (PR #242170, branch fhir-2245_observability-search-documents). This is an NHR adapter observability increment that adds structured Datadog monitoring to searchDocuments via a Proxy-based factory.
Files to review:
1. packages/@doctolib/national-health-records/src/types/nhrMonitoring.ts — pure type definitions for monitoring (NhrAdapterName, NhrAdapterAction, mapped types for request/response fields)
2. packages/@doctolib/national-health-records/src/types/index.ts — barrel re-export of all public types
3. packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts — runtime send helpers (sendNhrRequest, sendNhrResponse) with fire-and-forget .catch(() => {})
4. packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts — Proxy-based factory function withNhrObservability(adapter, adapterName) with a two-phase generic observe().execute() helper
5. packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts — 36 unit tests covering observe() invariants, searchDocuments specifics, and transparent Proxy delegation
6. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts — wires withNhrObservability around DmpNhrAdapter
7. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts — verifies the wiring
Focus on: TypeScript correctness, code style compliance with the project (airbnb/prettier), patterns (no class components, functional only), potential runtime issues, security, and any obvious design smells. Do NOT suggest adding comments that explain "what" code does — the project convention is no comments unless the WHY is non-obvious.
Turn 1 — Assistant
I'll review the files in the NHR observability PR. Let me start by reading them all.
Tools used:
Bash:ls /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/ /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-record …(truncated, 111 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/: DmpNhrAdapter.test.ts DmpNhrAdapter.ts NhrAdapterWithObservability.test.ts NhrAdapterWithObservability.ts nhrMonitoring.test.ts nhrMonitoring.ts parseHl7DateToIso.test.ts parseHl7DateToIso.ts
/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/: useDmpNhr.test.ts useDmpNhr.ts useGetDocumentsContent.test.ts useGetDocumentsContent.ts useInvalidateNhrD …(truncated, 366 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/types/nhrMonitoring.ts
Tool results:
→ result (ok): 1 import type { NhrResultStatus } from '@doctolib/tpf-national-health-records'
2
3 export type NhrAdapterName = 'dmp'
4 // Extend when adding adapters: | 'germany-kim' | 'italy-fse'
5
6 export type NhrAdapterAction = 'search_documents'
7 // Extend in follow-up increments: | 'get_documents_content' | 'update_document_metadata'
8
9 // ── Infrastructure base types ──
10
11 type NhrRequestBase = {
12 trace_id: string
13 adapter: NhrAdapterName
14 phase: 'request'
15 // patient_ins (raw I
…(truncated, 1892 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/types/index.ts
Tool results:
→ result (ok): 1 export type { NhrActionModalConfig } from './nhrDocumentActions' 2 export type { 3 NhrAdapterName, 4 NhrAdapterAction, 5 NhrRequestDetails, 6 NhrResponseDetails, 7 NhrRequestActionFields, 8 NhrResponseActionFields, 9 } from './nhrMonitoring' 10
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import type { NhrRequestDetails, NhrResponseDetails } from '../../types'
3
4 export type {
5 NhrAdapterName,
6 NhrAdapterAction,
7 NhrRequestDetails,
8 NhrResponseDetails,
9 NhrRequestActionFields,
10 NhrResponseActionFields,
11 } from '../../types'
12
13 // ── Send helpers ──
14 // Fire-and-forget: sendMonitoringEvent returns Promise<unknown>.
15 // .catch(() => {}) follows the ti_connect pattern — m
…(truncated, 410 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts
Tool results:
→ result (ok): 1 import type { NhrAdapter, NhrDocumentSearchProps, NhrDocumentSearchResult } from '@doctolib/tpf-national-health-records'
2 import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring'
3 import type {
4 NhrAdapterName,
5 NhrAdapterAction,
6 NhrRequestDetails,
7 NhrResponseDetails,
8 NhrRequestActionFields,
9 NhrResponseActionFields,
10 } from '../../types'
11
12 // Phase 1: lock in TAction.
13 // Returns a typed runner whose .execute() locks in TResult.
14 // The 'as' casts ar
…(truncated, 3330 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import type { NhrAdapter, NhrDocumentSearchProps } from '@doctolib/tpf-national-health-records'
3 import { withNhrObservability } from './NhrAdapterWithObservability'
4
5 jest.mock('monolith_DEPRECATED/services/monitoring', () => ({
6 sendMonitoringEvent: jest.fn().mockResolvedValue(undefined),
7 }))
8
9 const mockSendMonitoringEvent = sendMonitoringEvent as jest.MockedFunction<typeof sendMonitoringEvent>
10
1
…(truncated, 13944 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts
Tool results:
→ result (ok): 1 import { useMemo } from 'react' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import { DmpApiImpl as DmpApi, useDmpSetUp } from '@doctolib/dmp' 4 import { createNhrService } from '@doctolib/tpf-national-health-records' 5 import { useDocumentCategorizationFacetsIndex } from 'engines/document_categorization/queries/document_categorization/facets' 6 import { DmpNhrAdapter } from '../business/adapters/DmpNhrAdapter' 7 import { withNhrObservability } from '../busin …(truncated, 932 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts
Tool results:
→ result (ok): 1 import { renderHook } from '@testing-library/react' 2 import { useDmpSetUp } from '@doctolib/dmp' 3 import { createNhrService } from '@doctolib/tpf-national-health-records' 4 import { useDocumentCategorizationFacetsIndex } from 'engines/document_categorization/queries/document_categorization/facets' 5 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 6 import { DmpNhrAdapter } from '../business/adapters/DmpNhrAdapter' 7 import { withNhrObservability } from '../busines …(truncated, 3900 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.test.ts
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring'
3
4 jest.mock('monolith_DEPRECATED/services/monitoring', () => ({
5 sendMonitoringEvent: jest.fn().mockResolvedValue(undefined),
6 }))
7
8 const mockSendMonitoringEvent = sendMonitoringEvent as jest.MockedFunction<typeof sendMonitoringEvent>
9
10 beforeEach(() => {
11 jest.clearAllMocks()
12 mockSendMonitoringEvent.mockResolvedValue(undefine
…(truncated, 2521 more chars)
Tools used:
Bash:git diff origin/master -- packages/@doctolib/national-health-records/ 2>/dev/null | head -200
Tool results:
→ result (ok): diff --git a/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts b/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts new file mode 100644 index 000000000000..fc85376812e9 --- /dev/null +++ b/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts @@ -0,0 +1,327 @@ +import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' +import typ …(truncated, 8173 more chars)
Now I have all the information I need. Let me check a few more things to validate concerns.
Tools used:
Bash:grep -rn "sendMonitoringEvent" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/ /Users/USER/workspace/doctolib/doctolib/app/javascript/monolith_DEPRECATED/ …(truncated, 44 more chars)
Tool results:
→ result (ok): (eval):1: no matches found: /Users/USER/workspace/doctolib/doctolib/app/javascript/monolith_DEPRECATED/services/monitoring*
Tools used:
Bash:find /Users/USER/workspace/doctolib/doctolib/app/javascript/monolith_DEPRECATED/services -name "monitoring*" -type f 2>/dev/null | head -5
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:grep -rn "ti_connect" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib --include="*.ts" 2>/dev/null | head -10
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/secure_messaging_inbox/src/components/doctor/desktop/de/SignatureVerificationButton/useVerifySignature.ts:12:import useTiConnector from '@doctolib/ti_connect/public/useTiConnector' /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/e_rezept/src/business/useERezeptSignAndSendFromDwl.ts:3:import { TiService, usePromisifyTiSpecialistService } from '@doctolib/ti_connect/public' /Users/USER/workspace/doctolib/doctolib/packages/@doctol …(truncated, 1440 more chars)
Tools used:
Bash:find /Users/USER/workspace/doctolib/doctolib -name "monitoring.ts" -path "*monolith_DEPRECATED*" 2>/dev/null | head -5
Tools used:
Bash:grep -rn "sendMonitoringEvent" /Users/USER/workspace/doctolib/doctolib --include="*.ts" -l 2>/dev/null | head -10
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/app/assets/javascripts/types/commons/window.ts /Users/USER/workspace/doctolib/doctolib/app/assets/javascripts/contexts/doctor/desktop/tlsiServices/EPrescription/helpers/helper.ts /Users/USER/workspace/doctolib/doctolib/app/assets/javascripts/contexts/doctor/desktop/tlsiServices/AATi/useAatiTransmission.ts /Users/USER/workspace/doctolib/doctolib/app/assets/javascripts/contexts/doctor/desktop/events/sendNewRelicErrorEvent.ts /Users/USER/workspace/doctolib/do …(truncated, 878 more chars)
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:find /Users/USER/workspace/doctolib/doctolib -path "*monolith_DEPRECATED/services/monitoring*" 2>/dev/null | head -5
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:grep -rn "monolith_DEPRECATED/services/monitoring" /Users/USER/workspace/doctolib/doctolib --include="*.ts" 2>/dev/null | head -10
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/app/assets/javascripts/modules/doctor/mobile/document-upload/NewDocumentModule/variations/useTrackDocumentUploadVariation.ts:3:import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' /Users/USER/workspace/doctolib/doctolib/app/assets/javascripts/modules/doctor/mobile/DocumentEditModule/variations/useTrackDocumentUpdatedVariation.ts:3:import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' /Users/USER/workspace/do …(truncated, 1596 more chars)
Good — the import path is consistent with other packages. Now let me verify a few more concerns.
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/documents-ui/src/helpers/monitoring/documentEncryptionMonitoring.ts
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import { isFeatureEnabled } from '@doctolib/variation-management/legacy'
3 import type { ShareWith } from '@doctolib/deepwater_core/public'
4 import type { EncryptableDocumentField, DocumentEncryptionFeature } from '../types'
5
6 interface FormDataInspectionResult {
7 hasTankerEncryptedFile: boolean
8 hasClearTextFile: boolean
9 hasTankerEncryptedMetadata: boolean
10 hasClearTextMetadata: boolean
11 }
12
…(truncated, 3218 more chars)
Tools used:
Bash:grep -rn "crypto.randomUUID" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib --include="*.ts" 2>/dev/null | head -10
Tools used:
Bash:grep -rn "performance.now" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib --include="*.ts" 2>/dev/null | head -5
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/secure_messaging_inbox/src/modules/MssanteLoginModule/useMssanteLoginModuleController.ts:144: const startTime = performance.now() /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/secure_messaging_inbox/src/modules/MssanteLoginModule/useMssanteLoginModuleController.ts:189: const startTime = performance.now() /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/e_rezept/src/business/hooks/useERezeptSign.ts:37: co …(truncated, 341 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/secure_messaging_inbox/src/components/doctor/desktop/de/SignatureVerificationButton/useVerifySignature.ts:79: crypto.randomUUID() /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/e_rezept/src/hooks/useDigitalHealthApplicationRenewalData.ts:78: frontEndUUID: crypto.randomUUID(), /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/ehr_rich_texts/src/business/dataCaptureIfConsent.ts:19: if (!uuidMapping …(truncated, 1448 more chars)
Tools used:
Bash:grep -rn "NhrAdapter\b\|export.*NhrAdapter\b" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/tpf-national-health-records --include="*.ts" 2>/dev/null | head -20
Tool results:
→ result (ok): (Bash completed with no output)
Both crypto.randomUUID and performance.now are used elsewhere in the codebase. Let me also verify the test asserting getDocumentsContent does not emit events — that the patient identifier check is robust, and the unknown_error cast is type-safe.
Tools used:
Bash:grep -rn "NhrResultStatus\|status:.*'success'\|status:.*'error'" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/tpf-national-health-records/src --include="*.ts" 2>/dev/null | head -20
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:find /Users/USER/workspace/doctolib/doctolib -path "*tpf-national-health-records*" -name "*.ts" 2>/dev/null | head -20
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/index.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/src/index.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records-ui/dist/entry.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records-ui/dist/types.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-na …(truncated, 1807 more chars)
Tools used:
Bash:grep -rn "NhrResultStatus\|NhrAdapter\b" /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist 2>/dev/null | head -20
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/index.d.ts:2:export type { NhrResult, NhrResultStatus, NhrDocumentSearchParams, NhrDocumentSearchProps, NhrDocumentSearchResult, NhrGetDocumentsContentParams, NhrGetDocumentsContentProps, NhrDocumentContentResult, NhrDocumentMetadataUpdatePayload, NhrDocumentMetadataUpdateParams, NhrDocumentMetadataUpdateProps, NhrDocumentMetadataUpdatePayloadResult, NhrDocumentMetadataUpdateResult, NhrServiceInformat …(truncated, 2506 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/business/types/service.d.ts
Tool results:
→ result (ok): 1 import type { NhrDocumentContentResult, NhrDocumentMetadataUpdateProps, NhrDocumentMetadataUpdateResult, NhrDocumentSearchProps, NhrDocumentSearchResult, NhrGetDocumentsContentProps } from '#business/types/api'; 2 export type CountryCode = 'fr' | 'mock'; 3 export type NhrService = { 4 searchDocuments(props: NhrDocumentSearchProps): Promise<NhrDocumentSearchResult>; 5 getDocumentsContent(props: NhrGetDocumentsContentProps): Promise<NhrDocumentContentResult[]>; 6 updateDocumentMetada …(truncated, 466 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/business/types/api.d.ts
Tool results:
→ result (ok): 1 import type { DateRange, NhrDocument, NhrDocumentFile } from '#business/types/document'; 2 import type { NhrPatientInformation } from '#business/types/participant'; 3 export type NhrResultStatus = 'success' | 'skipped' | 'error' | 'partial success'; 4 export type NhrResult = { 5 status: NhrResultStatus; 6 message?: string; 7 code?: string; 8 }; 9 export type NhrDocumentSearchParams = { 10 formats?: string[]; 11 categories?: string[]; 12 status?: string; 13 serviceSt …(truncated, 1627 more chars)
Now I have full context. Let me write up the review findings.
I've reviewed the seven files. Below are the issues that meet the ≥80 confidence threshold for reporting. Issues are ordered by severity.
Critical (90-100)
1. Type safety hole: as NhrResponseDetails cast erases the error_code/documents_count requirements (Confidence: 92)
File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts (lines 34–43, 49–54)
NhrResponseActionFields['search_documents'] does NOT include phase, trace_id, adapter, action, or duration_ms — those are added by the wrapper. But NhrResponseDetails is an intersection that requires all of them plus action: 'search_documents'. The cast as NhrResponseDetails works today only because TypeScript can't narrow the spread.
The bigger issue is the catch path:
sendResponse({
status: 'error',
error_message: error instanceof Error ? error.message : 'unknown_error',
} as NhrResponseActionFields[TAction])
Casting an arbitrary object to a generic mapped type is unsafe — if a future action requires a non-optional field in NhrResponseActionFields, this catch path will silently violate the contract because the cast suppresses the error. Today every field in search_documents response is optional, but the design encourages adding new actions. Same risk applies to requestFields-side casts at the request emission.
Fix: Either drop NhrResponseDetails/NhrRequestDetails (the discriminated wrappers around the action fields) and let the helper build the wire shape directly, or constrain NhrResponseActionFields[TAction] to require a Partial<...>-compatible shape so the catch-path object is well-typed without a cast. At minimum, make the catch object conform without a cast: a pragmatic fix is
const errorFields = { status: 'error', error_message: ... } satisfies Partial<NhrResponseActionFields[NhrAdapterAction]>
sendResponse(errorFields as NhrResponseActionFields[TAction])
— still imperfect, but at least the source object is schema-validated.
2. Proxy get does not handle Symbol-keyed accesses safely (Confidence: 88)
File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts (lines 89–97)
return new Proxy(adapter, {
get(target, prop: string | symbol) {
if (typeof prop === 'string' && prop in overrides) {
return overrides[prop as keyof NhrAdapter]
}
const value = Reflect.get(target, prop)
return typeof value === 'function' ? value.bind(target) : value
},
})
Reflect.get correctly takes symbols, but rebinding non-method symbol-keyed values (e.g., Symbol.toPrimitive, Symbol.iterator) with value.bind(target) is fine. The actual concern: when the override is returned, it is returned as a bare function reference, not bound to anything. When the consumer calls wrapper.searchDocuments(...), the call's this is the Proxy (not target), and inside the override adapter.searchDocuments(props) is called with adapter bound — this works. But if someone destructures const { searchDocuments } = wrapper and calls it standalone, the override function still works because it captures adapter in closure. So this is actually fine.
However, the override functions defined as async searchDocuments(...) in the overrides literal are method shorthand — when retrieved via overrides[prop], their this is no longer the overrides object. Today searchDocuments does not reference this, so it works, but it is a latent footgun: any override that uses this (e.g., to call another override) will silently break. Recommendation: convert overrides to plain arrow functions or document the constraint.
Confidence is 88 because while the current code works, the test at line 309–326 (preserves this binding for forwarded methods) explicitly asserts behavior for the forwarding path but not for overrides — a future override using this would slip through.
Important (80-89)
3. fire-and-forget send is silent on synchronous throws (Confidence: 85)
File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts (line 18)
sendMonitoringEvent({ name: ..., details }).catch(() => {})
This handles only promise rejection. If sendMonitoringEvent throws synchronously (e.g., the global window.dataLayer is undefined or some validation throws before the promise is constructed), .catch is never reached and the error propagates up through the observe().execute() request emission path, leaking into the caller's stack. The test at line 117–125 (does not propagate monitoring failures) only covers the async-rejection case.
Fix: Wrap in a try/catch:
const send = (details: NhrRequestDetails | NhrResponseDetails): void => {
try {
sendMonitoringEvent({ name: `nhr_adapter_${details.action}_${details.phase}`, details }).catch(() => {})
} catch {
/* monitoring must never affect the caller */
}
}
4. PII leak risk: error.message is forwarded verbatim to Datadog (Confidence: 82)
File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts (line 52) and DMP result.message at line 73.
The types file (src/types/nhrMonitoring.ts:15, :25) explicitly notes the patient INS is excluded pending DPO sign-off. But error.message from the upstream DMP call can contain unsanitized server payloads — including, in some DMP error shapes, the patient INS or other identifiers echoed back by the upstream API. The same applies to result.message at line 73 (passed straight into error_message).
This is a privacy posture concern that the explicit DPO note in the types file flags as material. At minimum the WHY comment should be updated to acknowledge that error_message is also potentially-sensitive and is being knowingly emitted; ideally error.message and result.message are mapped through a safelist of known DMP error codes so the wire never carries free-form text.
Confidence 82 because I cannot prove DMP echoes INS in messages without inspecting the upstream contract, but the asymmetric treatment (refuse INS, allow free-form message) is a design smell worth addressing in this PR.
5. NhrAdapter test mock is module-scoped and shared across tests (Confidence: 80)
File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts (lines 13–17)
const mockAdapter: NhrAdapter = {
searchDocuments: jest.fn(),
getDocumentsContent: jest.fn(),
updateDocumentMetadata: jest.fn(),
}
mockAdapter is created once at module load. jest.clearAllMocks() in beforeEach clears call records but not implementations set by mockResolvedValue/mockRejectedValue from prior tests (those persist on the same jest.fn() reference). This works today because every test sets its own .mockResolvedValue / .mockRejectedValue before calling, but if a future test omits the setup, it will pick up the previous test's implementation in unpredictable order. The test at line 281 (getDocumentsContent forwards…) is particularly exposed — it relies on mockResolvedValue(expectedResult) to override whatever leaked in.
Fix: Recreate the mock in beforeEach, or use jest.resetAllMocks() (which clears implementations) instead of jest.clearAllMocks().
Summary
The PR is well-structured, the Proxy pattern is appropriate, and test coverage is impressively broad (36 tests across two test files covering invariants, action-specifics, and transparent delegation). The hooks wiring is minimal and correct. The non-blocking concerns above are worth addres …(truncated, 1470 more chars)