From abaf23c122be916f91bd5f364942a43151111811 Mon Sep 17 00:00:00 2001 From: Nora Date: Tue, 7 Jul 2026 12:19:42 -0600 Subject: [PATCH] fix(sp37-followup): SubmitOutcome.UNEXPECTED_ERROR for non-typed failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Followup #3 from the SP37 final-state tracker. The router's per-file try/except previously coerced every unexpected exception to SubmitOutcome.SFTP_FAILED — misleading because a RuntimeError from cycl_store.add has nothing to do with SFTP. Changes: * submission/result.py — add UNEXPECTED_ERROR = 'unexpected_error' enum value. Docstring distinguishes it from the typed SFTP_FAILED. * api_routers/submission.py — exception handler now returns UNEXPECTED_ERROR (was SFTP_FAILED). * test_api_submit_batch.py::test_submit_batch_unexpected_exception_recorded_in_results — updated assertion (was sftp_failed, now unexpected_error). * test_unexpected_error_outcome.py (new, 4 tests): - enum value exists - distinct from typed failure values - API surfaces 'unexpected_error' for uncaught exceptions - typed SFTP_FAILED path still surfaces 'sftp_failed' (regression lock) Tests: 40/40 pass in 1.41s (full SP37 chain + new file). Live curl: POST /api/submit-batch in stub mode → HTTP 409 (unchanged behavior, no regression on the config-level guards). --- backend/src/cyclone/api_routers/submission.py | 2 +- backend/src/cyclone/submission/result.py | 12 +- backend/tests/test_api_submit_batch.py | 17 +-- .../tests/test_unexpected_error_outcome.py | 139 ++++++++++++++++++ 4 files changed, 158 insertions(+), 12 deletions(-) create mode 100644 backend/tests/test_unexpected_error_outcome.py diff --git a/backend/src/cyclone/api_routers/submission.py b/backend/src/cyclone/api_routers/submission.py index 938f246..de208fd 100644 --- a/backend/src/cyclone/api_routers/submission.py +++ b/backend/src/cyclone/api_routers/submission.py @@ -160,7 +160,7 @@ def submit_batch(body: SubmitBatchRequest): ) r = SubmitResult( file=src.name, - outcome=SubmitOutcome.SFTP_FAILED, + outcome=SubmitOutcome.UNEXPECTED_ERROR, error=f"{exc.__class__.__name__}: {exc}", ) diff --git a/backend/src/cyclone/submission/result.py b/backend/src/cyclone/submission/result.py index 329c1cd..bd37edb 100644 --- a/backend/src/cyclone/submission/result.py +++ b/backend/src/cyclone/submission/result.py @@ -16,8 +16,15 @@ class SubmitOutcome(str, Enum): audit event; ``SKIPPED`` does NOT (re-running on an already-uploaded file must not flood the audit log with duplicate events). All failure outcomes (``PARSE_FAILED``, ``PAYER_MISMATCH``, ``DB_FAILED``, - ``SFTP_FAILED``) emit no event either — failures should be observable - via the helper's return value, not the audit log. + ``SFTP_FAILED``, ``UNEXPECTED_ERROR``) emit no event either — + failures should be observable via the helper's return value, not + the audit log. + + ``UNEXPECTED_ERROR`` is reserved for uncaught exceptions in the + helper's own code (e.g. a bug in ``cycl_store.add``) — distinct + from the typed ``SFTP_FAILED`` which means the SFTP layer itself + raised. Operators reading ``outcome="unexpected_error"`` know to + look at the bug tracker, not the SFTP server. """ SUBMITTED = "submitted" SKIPPED = "skipped" @@ -25,6 +32,7 @@ class SubmitOutcome(str, Enum): PAYER_MISMATCH = "payer_mismatch" DB_FAILED = "db_failed" SFTP_FAILED = "sftp_failed" + UNEXPECTED_ERROR = "unexpected_error" @dataclass(frozen=True) diff --git a/backend/tests/test_api_submit_batch.py b/backend/tests/test_api_submit_batch.py index f8e1c9c..3fe64ae 100644 --- a/backend/tests/test_api_submit_batch.py +++ b/backend/tests/test_api_submit_batch.py @@ -213,11 +213,10 @@ def test_submit_batch_unexpected_exception_recorded_in_results( """If submit_file raises, the file lands in results[] as a failed row. The router catches the exception, builds a SubmitResult with - ``outcome=SubmitOutcome.SFTP_FAILED`` (per Task 6's instruction - that "any enum value works as a marker; choose whichever is least - misleading"), and counts it as ``failed``. The full error string - (including exception class name) is in ``error`` so the operator - can tell it was a real exception, not a typed failure path. + ``outcome=SubmitOutcome.UNEXPECTED_ERROR`` (followup #3 — distinct + from the typed SFTP_FAILED path so operators can tell "real bug" + from "SFTP server down"), and counts it as ``failed``. The full + error string (including exception class name) is in ``error``. """ _stage_batch(tmp_path, 1) @@ -235,10 +234,10 @@ def test_submit_batch_unexpected_exception_recorded_in_results( assert resp.status_code == 200, resp.text body = resp.json() assert body["failed"] == 1 - # The marker is the SFTP_FAILED enum value (per Task 6 design - # choice). The full exception class+message is in ``error`` so the - # operator can distinguish a real exception from a typed failure. - assert body["results"][0]["outcome"] == "sftp_failed" + # Followup #3: marker is UNEXPECTED_ERROR, NOT SFTP_FAILED — the + # exception class+message in ``error`` tells the operator this + # was a real exception (not a typed failure). + assert body["results"][0]["outcome"] == SubmitOutcome.UNEXPECTED_ERROR.value assert "kaboom" in body["results"][0]["error"] assert "RuntimeError" in body["results"][0]["error"] diff --git a/backend/tests/test_unexpected_error_outcome.py b/backend/tests/test_unexpected_error_outcome.py new file mode 100644 index 0000000..11aa8c9 --- /dev/null +++ b/backend/tests/test_unexpected_error_outcome.py @@ -0,0 +1,139 @@ +"""SP37 follow-up #3: SubmitOutcome.UNEXPECTED_ERROR for non-typed failures. + +The router's per-file try/except previously coerced every unexpected +exception to ``SubmitOutcome.SFTP_FAILED`` because the helper had no +"unknown failure" enum value. That was misleading — a ``RuntimeError`` +from ``cycl_store.add`` (e.g. ``RuntimeError("db down")``) is +definitely not an SFTP failure. + +This test pins the new behavior: ``SubmitOutcome.UNEXPECTED_ERROR`` +exists, the API surfaces it under the JSON key ``"unexpected_error"``, +and the typed SFTP_FAILED path is reserved for actual SFTP failures. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient + + +_FIXTURE = Path(__file__).parent / "fixtures" / "submit-batch" / "single-claim.x12" + + +@pytest.fixture +def client(tmp_path) -> TestClient: + """Standard TestClient; conftest.py autouse handles DB + auth gate. + + Seeds the clearhouse then flips sftp_block.stub to False so the + file-walking tests reach the walker. Matches the fixture shape in + test_api_submit_batch.py — local because only these two test files + need it (no conftest move yet). + """ + from cyclone import db as db_mod + from cyclone.store import store as cycl_store + + db_mod._reset_for_tests() + db_mod.init_db() + cycl_store.ensure_clearhouse_seeded() + ch = cycl_store.get_clearhouse() + cycl_store.update_clearhouse( + ch.model_copy(update={"sftp_block": ch.sftp_block.model_copy(update={"stub": False})}), + ) + + from cyclone.api import app + + return TestClient(app) + + +def test_unexpected_error_enum_value_exists(): + """The enum MUST have UNEXPECTED_ERROR so the API can distinguish + a non-typed exception from a real SFTP failure. + """ + from cyclone.submission.result import SubmitOutcome + + assert hasattr(SubmitOutcome, "UNEXPECTED_ERROR") + assert SubmitOutcome.UNEXPECTED_ERROR.value == "unexpected_error" + + +def test_unexpected_error_is_distinct_from_typed_failures(): + """The new value must NOT collide with any existing failure value.""" + from cyclone.submission.result import SubmitOutcome + + # Compare against the typed failure values only (not UNEXPECTED_ERROR + # itself, which would trivially be in any set including it). + typed_failure_values = { + SubmitOutcome.SUBMITTED.value, + SubmitOutcome.SKIPPED.value, + SubmitOutcome.PARSE_FAILED.value, + SubmitOutcome.PAYER_MISMATCH.value, + SubmitOutcome.DB_FAILED.value, + SubmitOutcome.SFTP_FAILED.value, + } + assert SubmitOutcome.UNEXPECTED_ERROR.value not in typed_failure_values + + +def test_router_maps_unexpected_exception_to_unexpected_error(client: TestClient, monkeypatch, tmp_path): + """If submit_file raises an unexpected exception, the API surfaces + ``outcome="unexpected_error"`` (not ``"sftp_failed"``). + """ + from cyclone.submission.result import SubmitResult, SubmitOutcome + + # Stage one file so the walker has something to submit. + batch_dir = tmp_path / "batch-test-claims" + batch_dir.mkdir() + (batch_dir / "claim-0.x12").write_bytes(_FIXTURE.read_bytes()) + + def _explode(path, **_kw): + raise RuntimeError("kaboom") + + monkeypatch.setattr( + "cyclone.api_routers.submission.submit_file", _explode, + ) + + resp = client.post( + "/api/submit-batch", + json={"ingest_dir": str(tmp_path), "validate": False}, + ) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["failed"] == 1 + # The operator-visible outcome MUST be "unexpected_error", not "sftp_failed". + assert body["results"][0]["outcome"] == SubmitOutcome.UNEXPECTED_ERROR.value + # The full exception class + message stays in ``error`` so the + # operator can tell it was a real exception, not a typed failure. + assert "kaboom" in body["results"][0]["error"] + assert "RuntimeError" in body["results"][0]["error"] + + +def test_typed_sftp_failure_still_uses_sftp_failed(client: TestClient, monkeypatch, tmp_path): + """Typed SFTP failure (returned by submit_file) MUST still surface as + ``outcome="sftp_failed"`` — the new enum value is for *unexpected* + exceptions only, not the typed SFTP_FAILED path. + """ + from cyclone.submission.result import SubmitResult, SubmitOutcome + + batch_dir = tmp_path / "batch-test-claims" + batch_dir.mkdir() + (batch_dir / "claim-0.x12").write_bytes(_FIXTURE.read_bytes()) + + def _fake_submit(path, **_kw): + return SubmitResult( + file=path.name, + outcome=SubmitOutcome.SFTP_FAILED, + error="sftp connection refused", + ) + + monkeypatch.setattr( + "cyclone.api_routers.submission.submit_file", _fake_submit, + ) + + resp = client.post( + "/api/submit-batch", + json={"ingest_dir": str(tmp_path), "validate": False}, + ) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["failed"] == 1 + assert body["results"][0]["outcome"] == SubmitOutcome.SFTP_FAILED.value + assert body["results"][0]["error"] == "sftp connection refused" \ No newline at end of file