Fix save_run_checks returning 500 on invalid checks

[Swayam-arora-2004]

###Changes

1. Fix error handling for save_run_checks: wrap DQEngine.save_checks in a try/except and map InvalidCheckError to an HTTP 400 response, aligning behavior with get_run_checks.
2. Add regression unit test: new test test_save_run_checks_maps_invalid_check_error_to_http_400 verifies that when save_run_checks receives invalid checks and DQEngine.save_checks raises InvalidCheckError, the endpoint returns 400 with an appropriate error message.
3. Keep scope minimal: no changes to other endpoints or shared abstractions; only the specific error mapping and its focused test.

###Tests
[ ] manually tested
[x] added unit tests
[ ] added integration tests
[ ] added end-to-end tests
[ ] added performance tests

[mwojtyczka]

test

[mwojtyczka]

Code Review

Overview

Code (router.py)

The change is correct and minimal. The error message format "Invalid checks format: {e}" matches the existing pattern in get_run_checks (line 176). ✓

Potential gap — InvalidConfigError not handled:

get_run_checks catches two error types after engine.load_checks:

except InvalidCheckError as e:
    raise HTTPException(status_code=400, detail=f"Invalid checks format: {e}")
except InvalidConfigError as e:
    raise HTTPException(status_code=400, detail=f"Invalid configuration: {e}")

save_run_checks only catches InvalidCheckError. If engine.save_checks can also raise InvalidConfigError, the endpoint would still return a 500. Worth verifying whether save_checks can throw this and, if so, adding the same handler for consistency.

---

Tests (test_save_run_checks.py)

Structural issue — wrong file location:
The existing backend router tests live in tests/unit/test_app_backend.py. This new test should be added as a class there (e.g., class TestSaveRunChecks) rather than in a standalone file, to match the established convention.

Fragile assertion in fake:

# _FakeSerializer.load_run_config
assert isinstance(install_folder, str)

Using assert as a runtime guard inside a test double is unusual and will produce a cryptic AssertionError if install_folder is ever None (a valid call path). This can just be removed.

Unused class body in _FakeChecksStorageConfig:
The stored attributes (run_config_name, install_folder) are never used by the test. The class body can be simplified to pass or replaced with a lambda in the monkeypatch call.

---

Summary

Severity	Issue
Medium	Missing InvalidConfigError handler — may still 500 if save_checks raises it
Low	New test should be added to test_app_backend.py, not a new standalone file
Low	assert isinstance(install_folder, str) in _FakeSerializer is fragile
Nit	Unused attributes in _FakeChecksStorageConfig

The core fix is correct. The InvalidConfigError gap is the main thing worth clarifying before merge.

[mwojtyczka]

Code review with inline comments.

[mwojtyczka]

Consider also catching InvalidConfigError here for consistency with get_run_checks, which handles both:

except InvalidCheckError as e:
    raise HTTPException(status_code=400, detail=f"Invalid checks format: {e}")
except InvalidConfigError as e:
    raise HTTPException(status_code=400, detail=f"Invalid configuration: {e}")

If save_checks can raise InvalidConfigError, the endpoint would still return a 500 without this handler.

[mwojtyczka]

This file should be added as a class inside tests/unit/test_app_backend.py (e.g. class TestSaveRunChecks) rather than as a standalone file — that's the established convention for backend router tests in this project.

[mwojtyczka]

This assert inside a test double is fragile — if install_folder is ever None (a valid call path in the router), this raises a cryptic AssertionError instead of a clean test failure. It can safely be removed.

[mwojtyczka]

I would avoid using monkeypatch. Every time we patch we introduce a technical debt.

The stored attributes (run_config_name, install_folder) are never read by the test. The __init__ body can be replaced with pass.

[mwojtyczka]

Thank you for the PR! Kindly please address the comments