[Copilot]

NFC/OCR troubleshooting sometimes requires document identity data, but ordinary debug logging must remain PII-free. This change separates development-only PII diagnostics from production-safe debug logging.

• Opt-in: Add logging.include_pii / LOG_INCLUDE_PII, defaulting to false. Both scan paths log parsed document fields only when the toggle and debug logging are enabled.
• Safeguards: Emit a startup warning when enabled. Portraits, raw chip data, FaceMaps, and request/response bodies remain excluded; request and audit logging are unchanged.
• Operator guidance: Document development-only use, sensitive-log retention, and the restart requirement.

Development-only configuration:

logging:
  level: debug
  include_pii: true # Never enable in production

• Fixes Support debug logging of sensitive PII #74

[Copilot]

🟡 Changes recommended

Error-level logging suppresses the required startup warning, and the test currently expects that suppression.

1 open finding

• PII warning is suppressed when log level is error · New

What changed in this PR

Adds an explicit development-only opt-in for sensitive NFC/OCR diagnostics while keeping ordinary debug logging PII-free.

Changes:

• Gates document logging on both debug level and logging.include_pii, excluding portraits.
• Adds startup warnings and operator guidance.
• Adds configuration and logging tests, including server tests in CI.

File	Description
README.md	Documents opt-in usage and safeguards.
PRIVACY.md	Describes sensitive-log handling and retention.
Makefile	Includes server tests.
internal/​config/​config.go	Adds the PII logging toggle.
internal/​config/​config_test.go	Tests defaults and configuration precedence.
internal/​apiv1/​logging_test.go	Tests logging gates and portrait exclusion.
internal/​apiv1/​client.go	Adds gated diagnostics to both scan paths.
configs/​config.yaml	Documents the disabled-by-default setting.
cmd/​server/​main.go	Adds the startup warning.
cmd/​server/​main_test.go	Tests warning behavior across log levels.
.github/​workflows/​ci.yml	Includes server tests in CI coverage.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/server/main.go	83.33%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[jessevanmuijden]

Approving: a careful, well-tested opt-in that keeps ordinary debug logging PII-free.

What I checked:

• Off by default. include_pii defaults to false, and Validate refuses it unless logging.level is debug.
• Startup warning. It is logged whenever the toggle is on.
• What is logged. Only DocumentData: names, dates, document number, nationality and MRZ lines. The portrait is replaced by its size, the caller's copy is left untouched, and chip data, FaceMaps and request bodies are never included.
• Both scan paths. /process-request and /v1/id-scan both log it, before the gates, so refused scans can be debugged too.
• Checks. go vet and go test -race ./... pass locally, and CI is green.

Non-blocking notes:

1. Stale docs: the README config row says the toggle "emits a startup warning even at higher log levels", and the PR description says "regardless of log level". Since "Require debug level for PII logging", Validate rejects include_pii at any level other than debug, so both lines should say that instead.
2. #72: it adds debug logging of the same document data, and #74 exists because that conflicts with PRIVACY.md. Once this merges, #72 should put its document-data logging behind include_pii (or reuse logDocumentData), or be closed if this covers what it needed.

[s-jairl]

Approving: a careful, well-tested opt-in that keeps ordinary debug logging PII-free.

What I checked:

• Off by default. include_pii defaults to false, and Validate refuses it unless logging.level is debug.
• Startup warning. It is logged whenever the toggle is on.
• What is logged. Only DocumentData: names, dates, document number, nationality and MRZ lines. The portrait is replaced by its size, the caller's copy is left untouched, and chip data, FaceMaps and request bodies are never included.
• Both scan paths. /process-request and /v1/id-scan both log it, before the gates, so refused scans can be debugged too.
• Checks. go vet and go test -race ./... pass locally, and CI is green.

Non-blocking notes:

1. Stale docs: the README config row says the toggle "emits a startup warning even at higher log levels", and the PR description says "regardless of log level". Since "Require debug level for PII logging", Validate rejects include_pii at any level other than debug, so both lines should say that instead.
2. Add debug logging for processRequest, IDScan and documentExpiryRejection #72: it adds debug logging of the same document data, and Support debug logging of sensitive PII #74 exists because that conflicts with PRIVACY.md. Once this merges, Add debug logging for processRequest, IDScan and documentExpiryRejection #72 should put its document-data logging behind include_pii (or reuse logDocumentData), or be closed if this covers what it needed.

Thanks for the review. @copilot : Address "non-blocking note" number 1.

[Copilot]

Approving: a careful, well-tested opt-in that keeps ordinary debug logging PII-free.

What I checked:
...

Updated the README configuration row to state that include_pii is accepted only with logging.level: debug; other log levels prevent startup. Commit: 5a180d3. The available GitHub tools do not provide an operation to edit the PR description, so its wording remains unchanged.

[sonarqubecloud]

Quality Gate failed

Failed conditions
72.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[s-jairl]

Quality Gate failed

Failed conditions 72.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

False positive - triggers detection on old additions and is not picking up the main tests added in this change.

[s-jairl]

Failed govulncheck is unrelated - will create a separate PR to address that.