Review findings (ai:codex)\n\nNo findings.\n\nResidual test risk: the PR test covers uppercase .XML filename handling, but not the other normalized paths: content types with case/parameters such as Application/XML; charset=utf-8, and uppercase .PDF dispatch. The implementation looks straightforward, but those cases are currently covered by inspection rather than tests.\n\nVerification: go test ./internal/xrechnung passed on the PR branch.

heiko reviewed

2026-05-11 21:54:11 +02:00

heiko left a comment

Review: fix/14-normalize-attachment-name

Correct and well-scoped. Lowercasing is confined to IsXRechnung and does not leak into Detect() (original filename still passed downstream).

Observations

contentType not normalized: Content-types like Application/PDF (valid per RFC 2045) would not match the lowercase comparisons. Consider contentType = strings.ToLower(contentType) as a follow-up.
No downstream leakage: Original filename is passed to Detect(), so flavor/profile results remain correct. Scanner logs use att.Filename (the original).
Adjacent gap in pdf.go: ExtractEmbeddedXML filters embedded filenames through a case-sensitive map. If a PDF embeds Factur-X.XML, it would be missed. Worth a follow-up issue.
Test coverage: Regression test covers uppercase .XML. Consider adding a case for uppercase .PDF to exercise both HasSuffix paths.

No correctness issues found.

— reviewed by ai:claude

## Review: fix/14-normalize-attachment-name **Correct and well-scoped.** Lowercasing is confined to `IsXRechnung` and does not leak into `Detect()` (original `filename` still passed downstream). ### Observations 1. **contentType not normalized:** Content-types like `Application/PDF` (valid per RFC 2045) would not match the lowercase comparisons. Consider `contentType = strings.ToLower(contentType)` as a follow-up. 2. **No downstream leakage:** Original `filename` is passed to `Detect()`, so flavor/profile results remain correct. Scanner logs use `att.Filename` (the original). 3. **Adjacent gap in `pdf.go`:** `ExtractEmbeddedXML` filters embedded filenames through a case-sensitive map. If a PDF embeds `Factur-X.XML`, it would be missed. Worth a follow-up issue. 4. **Test coverage:** Regression test covers uppercase `.XML`. Consider adding a case for uppercase `.PDF` to exercise both `HasSuffix` paths. No correctness issues found. — reviewed by ai:claude

heiko added 1 commit

2026-05-11 22:01:56 +02:00

test: add coverage for uppercase .PDF extension normalization bf195535e2

heiko closed this pull request

2026-05-11 22:25:44 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

IUS/xr-invoiced!20

No description provided.

Rows
Columns