You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(edi): report what ingest_events stored in the response status - #3819
Both EDI routes (POST /api/v1/ingest/edi and /document) reported success when ingest_events stored nothing. EDIIngestResponse.status defaulted to "accepted", so a document with every event rejected returned 201 with status: "accepted" and ingestion_result.accepted == 0. A partly stored document looked identical to a fully stored one. #3781 fixes the KDE names and listed this as out of scope.
Contract change
status is now derived from the ingest result. All three values arrive with HTTP 201.
status
Stored
Where the errors are
accepted
Every event
None
partial (new)
Some events
ingestion_result.events[], status and errors per event
rejected
Nothing
ingestion_result.events[], or rejection for ?strict=false
Behavior change: a document that used to say accepted with accepted: 0 now says rejected. A partly stored one now says partial, a value partners have not seen before.
rejected is reused from the existing advisory path, which already returns 201 with status: "rejected". Its two shapes stay mutually exclusive: ingestion_result means the document reached ingest, and rejection means it was stopped before ingest.
status has no default and is typed as a Literal, so a new return path fails loudly instead of claiming success.
201 stays. I found no reason it must change. The advisory path already pairs 201 with rejected, and the body carries everything a 4xx would. Partners that check only the HTTP code stay misled and must read status, which TRACEABILITY_FILE_INTAKE.md now says.
Consumers checked
No frontend code calls these routes or reads this response. frontend/src mentions EDI only as a source-type label and a capability flag.
No other backend code reads EDIIngestResponse.status, and all three constructors are in routes.py.
No test asserted the old value. The only status="accepted" matches are per-event EventResult stubs.
External partners may read status. partial is new to them.
Tests
test_edi_ingest_status.py posts through the routes into the real ingest_events and real _validate_event_kdes, faking only auth, rate limit, database, canonical store and graph sync. The existing EDI tests stub ingest_events, which is why this went unseen.
All rejected: the ASN fixture on both routes. Asserts the KDE error and that nothing reached storage.
Partial: a batched 945 on /document where one set has no N1*ST, so only that event fails KDE validation.
Rejections at ingest are not written to the EDI rejection log. Only pre-ingest rejections are, so rejected documents in the new shape have no rejection_id. Per-event errors are in the response.
An idempotent replay counts as accepted.ingest_events counts it that way, and the data is stored.
Separate bug seen in the verify run: the API logged data_gap_write_failed ... error_type=ProgrammingError after commit. It is fail-open and this PR does not touch it.
Jev sidecar was off: TYPESAFE_API_KEY is not set in this session.
Medium Risk
This is an external API contract change (partial is new; zero-store documents flip from accepted to rejected) while HTTP 201 is unchanged, so integrations that ignore body fields may mis-handle failures.
Overview
Fixes a misleading EDI API contract where EDIIngestResponse.status always defaulted to "accepted" even when ingest_events stored nothing or only some lines.
Both POST /api/v1/ingest/edi and /document now set status from ingest_status(ingestion_result): accepted when every event persisted, partial (new) when some did, rejected when accepted == 0. HTTP 201 is unchanged; partners must read status and ingestion_result, not the status code alone. status is a required Literal with no default so new return paths cannot silently claim success.
TRACEABILITY_FILE_INTAKE.md documents the three outcomes and where errors live (ingestion_result.events[] vs advisory rejection). test_edi_ingest_status.py exercises the real ingest_events / KDE validation path (existing EDI tests stub ingest and missed this).
Reviewed by Cursor Bugbot for commit d140073. Bugbot is set up for automated code reviews on this repo. Configure here.
Both EDI routes returned status "accepted" by default, so a document
whose events were all rejected answered 201 accepted with
ingestion_result.accepted == 0. A partly stored document looked the same
as a fully stored one.
status is now derived from the ingest result: accepted, partial, or
rejected. The field has no default, so a new return path must state its
outcome. HTTP 201 is unchanged.
The new tests post through the routes into the real ingest_events and
_validate_event_kdes. The existing EDI tests stub ingest_events, which
is why this went unseen.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No actionable comments were generated in the recent review.
Walkthrough
The EDI response model now defines accepted, partial, and rejected statuses and derives the status from accepted and rejected event counts. The 856 and unified document routes return the derived status. Tests cover the three outcomes, and the documentation describes their stored-event conditions, error locations, and HTTP 201 behavior.
Both EDI routes report ingestion outcomes while retaining HTTP 201. No merge-blocking risk remains.
Security architecture risk: Low · up to d1400
The change improves outcome reporting without changing ingestion permissions or storage behavior. The main remaining risk is partner compatibility with the new partial status. Clients that inspect only HTTP 201 will still miss rejected events.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The changed behavior is confined to reporting outcomes on the two existing EDI endpoints. The production diff does not add an ingestion caller, tenant selector, write operation, privileged action, or downstream service path.
Trust Boundaries and Controls
observed — Both endpoints retain the edi.ingest permission dependency and pass requested tenant identifiers together with the principal tenant to the existing resolver before reading and parsing the upload. The PR does not modify these admission calls or move response derivation ahead of ingestion.
Resilience and Maintainability Implications
observed — Retry behavior is pre-existing and unchanged: persistence can count an already durable idempotent event as accepted, while interchange deduplication can reject a repeated ISA13 identifier before ingestion with HTTP 409. The interchange reservation is separate from the database transaction, so a later failure can still obstruct retransmission; this PR changes neither that reservation nor its recovery behavior.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title clearly and concisely describes the main change: reporting the events stored by ingest_events in the EDI response status.
Description check
The description is detailed, relevant, and covers the problem, contract change, affected consumers, tests, verification results, and out-of-scope items. It does not use the template headings or explic…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Description check
Explanation
The description is detailed, relevant, and covers the problem, contract change, affected consumers, tests, verification results, and out-of-scope items. It does not use the template headings or explicitly complete the Type of Change, Checklist, or Related Issues sections, but the required information is mostly present.
Full details: Docstring Coverage
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Both EDI routes (
POST /api/v1/ingest/ediand/document) reported success wheningest_eventsstored nothing.EDIIngestResponse.statusdefaulted to"accepted", so a document with every event rejected returned 201 withstatus: "accepted"andingestion_result.accepted == 0. A partly stored document looked identical to a fully stored one. #3781 fixes the KDE names and listed this as out of scope.Contract change
statusis now derived from the ingest result. All three values arrive with HTTP 201.statusacceptedpartial(new)ingestion_result.events[],statusanderrorsper eventrejectedingestion_result.events[], orrejectionfor?strict=falseacceptedwithaccepted: 0now saysrejected. A partly stored one now sayspartial, a value partners have not seen before.rejectedis reused from the existing advisory path, which already returns 201 withstatus: "rejected". Its two shapes stay mutually exclusive:ingestion_resultmeans the document reached ingest, andrejectionmeans it was stopped before ingest.statushas no default and is typed as aLiteral, so a new return path fails loudly instead of claiming success.rejected, and the body carries everything a 4xx would. Partners that check only the HTTP code stay misled and must readstatus, whichTRACEABILITY_FILE_INTAKE.mdnow says.Consumers checked
frontend/srcmentions EDI only as a source-type label and a capability flag.EDIIngestResponse.status, and all three constructors are inroutes.py.status="accepted"matches are per-eventEventResultstubs.status.partialis new to them.Tests
test_edi_ingest_status.pyposts through the routes into the realingest_eventsand real_validate_event_kdes, faking only auth, rate limit, database, canonical store and graph sync. The existing EDI tests stubingest_events, which is why this went unseen./documentwhere one set has noN1*ST, so only that event fails KDE validation./document.acceptedandpartialcases stubingest_events, because no real 856 passes KDE validation until fix(ingestion): give EDI events the shipping KDEs validation requires #3781 lands.Verification
services/ingestion/tests: 6,243 tests, 0 failures, 0 errors, 63 skipped (live Postgres tests).statusis forced back to alwaysaccepted.201 rejected, 0 accepted, 1 rejected.201 accepted, 2 accepted.201 partial, 1 accepted, 1 rejected.fsma.cte_events, none from the rejected 856s._record_edi_rejectionwith or without this PR.Not in this PR
rejecteddocument still burns its ISA13, so a corrected resend gets a 409. fix(edi): release the ISA13 claim when an interchange is not stored #3782 readsingestion_result.accepted, notstatus, so the two are independent. After fix(edi): release the ISA13 claim when an interchange is not stored #3782, apartialdocument keeps its claim, so a partner resending the failed lines needs a new ISA13.rejecteddocuments in the new shape have norejection_id. Per-event errors are in the response.accepted.ingest_eventscounts it that way, and the data is stored.data_gap_write_failed ... error_type=ProgrammingErrorafter commit. It is fail-open and this PR does not touch it.Jev sidecar was off:
TYPESAFE_API_KEYis not set in this session.🤖 Generated with Claude Code
Note
Medium Risk
This is an external API contract change (
partialis new; zero-store documents flip fromacceptedtorejected) while HTTP 201 is unchanged, so integrations that ignore body fields may mis-handle failures.Overview
Fixes a misleading EDI API contract where
EDIIngestResponse.statusalways defaulted to"accepted"even wheningest_eventsstored nothing or only some lines.Both
POST /api/v1/ingest/ediand/documentnow setstatusfromingest_status(ingestion_result):acceptedwhen every event persisted,partial(new) when some did,rejectedwhenaccepted == 0. HTTP 201 is unchanged; partners must readstatusandingestion_result, not the status code alone.statusis a requiredLiteralwith no default so new return paths cannot silently claim success.TRACEABILITY_FILE_INTAKE.mddocuments the three outcomes and where errors live (ingestion_result.events[]vs advisoryrejection).test_edi_ingest_status.pyexercises the realingest_events/ KDE validation path (existing EDI tests stub ingest and missed this).Reviewed by Cursor Bugbot for commit d140073. Bugbot is set up for automated code reviews on this repo. Configure here.