Repository navigation
feat(epcis): SEL-598 lossless lot identity normalizer (H2 Preview) - #3728
PetrefiedThunder wants to merge 1 commit into
Conversation
Add shared.epcis_lot_identity so canonical, legacy extraction, and export agree on full LGTIN / Digital Link as the stored identity. Bare lot is derived for rebuild and UI. LOT_IDENTITY_LOSSLESS=0 restores segment-only writes. Synthetic sel598 fixtures cover both-pass and lenient-only TLC. Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Caution Review failedThe pull request is closed. Finishing Touches
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44c5c9af4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _LGTIN_RE = re.compile( | ||
| r"^urn:epc:class:lgtin:(\d{6,12})\.(\d+)\.([A-Za-z0-9_~!$&'()*+,;=:@%\-]+)$", | ||
| re.IGNORECASE, |
There was a problem hiding this comment.
For an LGTIN such as urn:epc:class:lgtin:0614141.107346.Lot.A, the new character class rejects the dot in the lot component. Previously, legacy extraction captured everything after the company-prefix and item-reference separators and returned Lot.A with the full URI. Now _lot_from_identifier returns None, and canonical normalization raises ValueError when this is the event's only lot identifier. Preserve support for dots in the lot component so previously accepted lot identifiers remain ingestible.
Useful? React with 👍 / 👎.
| def _prefer_class_uri(ident: LotIdentity) -> LotIdentity: | ||
| if parse_class_uri(ident.canonical_id): | ||
| return ident | ||
| for other in class_idents: | ||
| if other.lot_token == ident.lot_token: |
There was a problem hiding this comment.
When an event contains ILMD lotNumber: "LotA" and a matching quantity class, setting LOT_IDENTITY_LOSSLESS=0 still writes a full URI. With a Digital Link, identity_from_parts builds a URI and this early return bypasses the flag; with an LGTIN, the matching-class return below also bypasses it. Consequently, the advertised rollback only works for events without ILMD or flattened lot fields, leaving common events on the new storage format. Apply the flag before either full-URI return and add rollback coverage for events containing ILMD.
Useful? React with 👍 / 👎.
|
Closing as duplicate of #3727 (cursor/sel598-phase-b). VS Code Lead coordination: SEL-598 agent owns the lossless lot normalizer files ( A-2165 §7.12/§9 fixture coverage should land via #3727 (or a follow-up on that head), not a second normalizer PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 44c5c9a. Configure here.
| _LGTIN_RE = re.compile( | ||
| r"^urn:epc:class:lgtin:(\d{6,12})\.(\d+)\.([A-Za-z0-9_~!$&'()*+,;=:@%\-]+)$", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
LGTIN parser drops dotted lots
Medium Severity
parse_class_uri rejects valid LGTIN lots that contain ., so extraction no longer returns those identifiers and canonical ingest raises if ILMD is absent. The old extraction regex accepted any lot segment, and Digital Links still have a permissive fallback, but LGTINs do not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 44c5c9a. Configure here.
| if value is not None and str(value).strip(): | ||
| ident = identity_from_parts(tlc=str(value).strip(), gtin=gtin_hint) | ||
| if ident: | ||
| return _prefer_class_uri(ident) |
There was a problem hiding this comment.
ILMD path fabricates Digital Links
High Severity
normalize_from_epcis_event feeds the first Digital Link GTIN on the event into identity_from_parts with the ILMD TLC. That helper invents a new Digital Link, and _prefer_class_uri then returns it without checking that the lot tokens match, so a stored canonical id can pair the wrong GTIN with the ILMD lot.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 44c5c9a. Configure here.
| return value | ||
| parsed = parse_class_uri(value) | ||
| if parsed: | ||
| return parsed.canonical_id |
There was a problem hiding this comment.
Export emits placeholder class URIs
Medium Severity
_epc_class still rejects placeholder LGTINs containing 0000000.000000 in _valid_class_uri, then immediately accepts the same value through parse_class_uri and returns it. Retailer EPCIS export can now emit dummy class URIs that this filter was meant to block.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 44c5c9a. Configure here.
Summary
Phase B H2 (SEL-598): one shared lossless lot identity normalizer so legacy extraction, canonical ingest, and EPCIS export agree.
01+10) when present._epc_classprefers a stored class URI unchanged and never nests a full URI under/10/.LOT_IDENTITY_LOSSLESS=0restores segment-only canonical writes.sel598-both-pass,sel598-lenient-only). No partner data. No production ingest.A-2165 fixture tests
tests/shared/test_epcis_lot_identity_sel598.pyplus extraction/export extensions).Tests
pytest tests/shared/test_epcis_lot_identity_sel598.py services/ingestion/tests/test_epcis_extraction_1342.py services/ingestion/tests/test_epcis_export_gs1_2.py::test_export_epc_class_equals_stored_canonical_full_id --override-ini=addopts=Out of scope
Files
services/shared/epcis_lot_identity.py(new)services/shared/canonical_event.pyservices/ingestion/app/epcis/extraction.pyservices/ingestion/app/epcis_export.pyservices/ingestion/tests/fixtures/sel598/*tests/shared/test_epcis_lot_identity_sel598.pyNote
Medium Risk
Changes what gets persisted in
traceability_lot_codeand exportepcClassfor EPCIS events (full URI vs bare segment), which can affect lot matching and downstream consumers until rollout is understood; rollback viaLOT_IDENTITY_LOSSLESS=0limits blast radius.Overview
Introduces
shared.epcis_lot_identityso legacy EPCIS extraction, canonical ingest, and export use one SEL-598 normalizer: stored TLC becomes the full LGTIN URN or GS1 Digital Link when the event carries it, with a derived bare lot token for UI, validation, and rebuilding/01/.../10/...links.Canonical path (
canonical_event) now resolves lot codes vianormalize_from_epcis_eventinstead of inline ILMD/EPC parsing; transformation inputs addlot_tokenand honor lossless keys.LOT_IDENTITY_LOSSLESS=0rolls back to segment-only canonical writes.Legacy extraction delegates LGTIN/Digital Link parsing to
parse_class_uribut still returns the full identifier as TLC. Export_epc_classparses stored class URIs unchanged, derives tokens before GTIN+lot rebuilds, and blocks nesting a full URI under/10/.Synthetic sel598 fixtures and tests cover round-trip, strict vs lenient TLC, flag rollback, and export edge cases.
Reviewed by Cursor Bugbot for commit 44c5c9a. Bugbot is set up for automated code reviews on this repo. Configure here.