test(web): cover the error and response builders, and stop dropping empty values

errors.py and error_handler.py's response builders had no direct tests,
though every API response passes through them. Two bugs surfaced.

WebInterfaceError set suggested_fixes with `or`, so a caller passing []
to mean "I have no suggestions for this one" got the default list
instead. Only None should fall back.

create_success_response gated `data` on `is not None` but `message` and
`metadata` on truthiness, so an explicitly-passed "" or {} vanished from
the response while 0 and False survived — the response shape depended on
the value. api_helpers.success_response() then re-gated metadata the same
way, which is the path every api_v3 endpoint actually calls, so fixing
only the inner function would have changed nothing observable. Both now
use `is not None`.

That wrapper also merged request timing into the caller's own metadata
dict in place. A caller reusing a dict across requests would accumulate
previous responses' timings; it now copies before adding.

79 tests: category inference for every error code, mapped vs fallback
suggestions, the JSON shape including which keys are omitted when empty,
exception-to-code inference, and the success/error builders end to end.
Two behaviours are pinned as deliberate rather than fixed: an empty
context stays out of the response body, and from_exception's `message`
is the fixed per-code string, never the raw exception text.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh

src/web_interface/api_helpers.py

@@ -29,18 +29,16 @@ def success_response(
         Flask jsonify response
     """
     response_data = create_success_response(data, message, metadata)
-    
-    # Add request metadata if available
-    if metadata is None:
-        metadata = {}
-    
-    # Add timing if request start time is available
+
+    # Timing is merged into whatever the caller passed, without inventing a
+    # metadata block for responses that have neither.
+    enriched = dict(metadata) if metadata is not None else {}
     if hasattr(request, 'start_time'):
-        metadata['response_time_ms'] = int((time.time() - request.start_time) * 1000)
-    
-    if metadata:
-        response_data['metadata'] = metadata
-    
+        enriched['response_time_ms'] = int((time.time() - request.start_time) * 1000)
+
+    if metadata is not None or enriched:
+        response_data['metadata'] = enriched
+
     return jsonify(response_data)
 
 

src/web_interface/error_handler.py

@@ -142,14 +142,17 @@ def create_success_response(
         "status": "success"
     }
     
+    # All three use `is not None` rather than truthiness: "" and {} are
+    # values a caller chose to send, and dropping them silently would make
+    # the response shape depend on the data.
     if data is not None:
         response["data"] = data
-    
-    if message:
+
+    if message is not None:
         response["message"] = message
-    
-    if metadata:
+
+    if metadata is not None:
         response["metadata"] = metadata
-    
+
     return response
 

src/web_interface/errors.py

@@ -89,7 +89,11 @@ class WebInterfaceError:
         self.category = category or self._infer_category(error_code)
         self.details = details
         self.context = context or {}
-        self.suggested_fixes = suggested_fixes or self._get_default_suggestions(error_code)
+        # `is None`, not truthiness: an explicit [] means "this caller has
+        # no suggestions to offer", which the default list would override.
+        self.suggested_fixes = (
+            suggested_fixes if suggested_fixes is not None
+            else self._get_default_suggestions(error_code))
         self.original_error = original_error
     
     def _infer_category(self, error_code: ErrorCode) -> ErrorCategory:

test/web_interface/test_error_handler.py

@@ -0,0 +1,149 @@
+"""
+Tests for the response builders in src/web_interface/error_handler.py and
+the success path in src/web_interface/api_helpers.py.
+
+describe_exception() in the same module is already covered by
+test/test_web_error_detail.py and is not duplicated here.
+
+Regression coverage for one fixed bug: create_success_response used
+truthiness for `message` and `metadata` while using `is not None` for
+`data`, so an explicitly-passed "" or {} was silently dropped —
+api_helpers.success_response() repeated the same gate, which is the path
+every api_v3 endpoint actually calls.
+"""
+
+import pytest
+from flask import Flask
+
+from src.web_interface.api_helpers import success_response
+from src.web_interface.error_handler import (
+    create_error_response,
+    create_success_response,
+)
+from src.web_interface.errors import ErrorCode, WebInterfaceError
+
+
+@pytest.fixture
+def app():
+    return Flask(__name__)
+
+
+class TestCreateErrorResponse:
+    def test_returns_response_and_status_tuple(self, app):
+        with app.test_request_context():
+            response, status = create_error_response(
+                ErrorCode.CONFIG_SAVE_FAILED, "could not save")
+        assert status == 500
+        assert response.get_json()["message"] == "could not save"
+
+    def test_status_code_passthrough(self, app):
+        with app.test_request_context():
+            _, status = create_error_response(
+                ErrorCode.INVALID_INPUT, "bad", status_code=400)
+        assert status == 400
+
+    def test_body_matches_the_error_dataclass(self, app):
+        with app.test_request_context():
+            response, _ = create_error_response(
+                ErrorCode.NETWORK_ERROR, "offline",
+                details="connection refused", context={"url": "http://x"})
+        expected = WebInterfaceError(
+            error_code=ErrorCode.NETWORK_ERROR, message="offline",
+            details="connection refused", context={"url": "http://x"}).to_dict()
+        assert response.get_json() == expected
+
+    def test_none_context_produces_no_context_key(self, app):
+        with app.test_request_context():
+            response, _ = create_error_response(ErrorCode.SYSTEM_ERROR, "boom")
+        assert "context" not in response.get_json()
+
+    def test_suggested_fixes_passed_through(self, app):
+        with app.test_request_context():
+            response, _ = create_error_response(
+                ErrorCode.SYSTEM_ERROR, "boom", suggested_fixes=["Try again"])
+        assert response.get_json()["suggested_fixes"] == ["Try again"]
+
+
+class TestCreateSuccessResponse:
+    def test_bare_success(self):
+        assert create_success_response() == {"status": "success"}
+
+    def test_data_included(self):
+        assert create_success_response(data={"a": 1})["data"] == {"a": 1}
+
+    @pytest.mark.parametrize("falsy", [0, "", False, {}, []])
+    def test_falsy_data_is_still_included(self, falsy):
+        assert create_success_response(data=falsy)["data"] == falsy
+
+    def test_none_data_omitted(self):
+        assert "data" not in create_success_response(data=None)
+
+    def test_message_included(self):
+        assert create_success_response(message="done")["message"] == "done"
+
+    def test_empty_message_is_still_included(self):
+        # Regression: `if message:` dropped an explicitly-passed "".
+        assert create_success_response(message="")["message"] == ""
+
+    def test_none_message_omitted(self):
+        assert "message" not in create_success_response(message=None)
+
+    def test_metadata_included(self):
+        assert create_success_response(metadata={"v": 1})["metadata"] == {"v": 1}
+
+    def test_empty_metadata_is_still_included(self):
+        # Regression: `if metadata:` dropped an explicitly-passed {}.
+        assert create_success_response(metadata={})["metadata"] == {}
+
+    def test_none_metadata_omitted(self):
+        assert "metadata" not in create_success_response(metadata=None)
+
+
+class TestSuccessResponseHelper:
+    """api_helpers.success_response — the wrapper every endpoint calls."""
+
+    def test_plain_response_has_no_metadata_block(self, app):
+        with app.test_request_context():
+            body = success_response(data={"a": 1}).get_json()
+        assert body == {"status": "success", "data": {"a": 1}}
+
+    def test_explicit_empty_metadata_survives_the_wrapper(self, app):
+        # Regression: the wrapper re-gated metadata on truthiness after
+        # create_success_response had already included it, so {} was
+        # dropped again on the way out.
+        with app.test_request_context():
+            body = success_response(data=None, metadata={}).get_json()
+        assert body["metadata"] == {}
+
+    def test_caller_metadata_preserved(self, app):
+        with app.test_request_context():
+            body = success_response(metadata={"version": "1.2"}).get_json()
+        assert body["metadata"]["version"] == "1.2"
+
+    def test_timing_added_when_request_has_start_time(self, app):
+        with app.test_request_context() as ctx:
+            ctx.request.start_time = 0.0
+            body = success_response(data={"a": 1}).get_json()
+        assert "response_time_ms" in body["metadata"]
+
+    def test_timing_merges_with_caller_metadata(self, app):
+        with app.test_request_context() as ctx:
+            ctx.request.start_time = 0.0
+            body = success_response(metadata={"version": "1.2"}).get_json()
+        assert body["metadata"]["version"] == "1.2"
+        assert "response_time_ms" in body["metadata"]
+
+    def test_caller_metadata_dict_is_not_mutated(self, app):
+        # The helper used to add response_time_ms straight into the dict the
+        # caller passed, so a module-level or reused metadata dict would
+        # accumulate timings from previous requests.
+        caller_metadata = {"version": "1.2"}
+        with app.test_request_context() as ctx:
+            ctx.request.start_time = 0.0
+            success_response(metadata=caller_metadata)
+        assert caller_metadata == {"version": "1.2"}
+
+    def test_message_passed_through(self, app):
+        with app.test_request_context():
+            body = success_response(message="saved").get_json()
+        assert body["message"] == "saved"

test/web_interface/test_errors.py

@@ -0,0 +1,208 @@
+"""
+Tests for src/web_interface/errors.py — the structured error type behind
+every API error response (category inference, default suggestions, the
+JSON shape, and exception conversion).
+
+Pure logic; no Flask context needed.
+
+Regression coverage for one fixed bug: suggested_fixes used `or`, so a
+caller passing [] to mean "no suggestions" silently got the default list.
+"""
+
+import pytest
+
+from src.web_interface.errors import ErrorCategory, ErrorCode, WebInterfaceError
+
+
+class TestCategoryInference:
+    @pytest.mark.parametrize("code,expected", [
+        (ErrorCode.CONFIG_SAVE_FAILED, ErrorCategory.CONFIGURATION),
+        (ErrorCode.CONFIG_ROLLBACK_FAILED, ErrorCategory.CONFIGURATION),
+        (ErrorCode.PLUGIN_NOT_FOUND, ErrorCategory.PLUGIN),
+        (ErrorCode.PLUGIN_OPERATION_CONFLICT, ErrorCategory.PLUGIN),
+        (ErrorCode.VALIDATION_ERROR, ErrorCategory.VALIDATION),
+        (ErrorCode.SCHEMA_VALIDATION_FAILED, ErrorCategory.VALIDATION),
+        (ErrorCode.INVALID_INPUT, ErrorCategory.VALIDATION),
+        (ErrorCode.NETWORK_ERROR, ErrorCategory.NETWORK),
+        (ErrorCode.API_ERROR, ErrorCategory.NETWORK),
+        (ErrorCode.TIMEOUT, ErrorCategory.NETWORK),
+        (ErrorCode.PERMISSION_DENIED, ErrorCategory.PERMISSION),
+        (ErrorCode.FILE_PERMISSION_ERROR, ErrorCategory.PERMISSION),
+        (ErrorCode.SYSTEM_ERROR, ErrorCategory.SYSTEM),
+        (ErrorCode.SERVICE_UNAVAILABLE, ErrorCategory.SYSTEM),
+        (ErrorCode.UNKNOWN_ERROR, ErrorCategory.UNKNOWN),
+    ])
+    def test_every_code_prefix_maps_to_its_category(self, code, expected):
+        assert WebInterfaceError(code, "msg").category is expected
+
+    def test_explicit_category_overrides_inference(self):
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", category=ErrorCategory.SYSTEM)
+        assert error.category is ErrorCategory.SYSTEM
+
+    def test_every_error_code_gets_a_category(self):
+        # No code may fall through uncategorized as the enum grows.
+        for code in ErrorCode:
+            assert isinstance(WebInterfaceError(code, "msg").category, ErrorCategory)
+
+
+class TestDefaultSuggestions:
+    def test_mapped_code_gets_specific_suggestions(self):
+        fixes = WebInterfaceError(ErrorCode.CONFIG_SAVE_FAILED, "msg").suggested_fixes
+        assert "Check available disk space" in fixes
+
+    def test_unmapped_code_gets_generic_fallback(self):
+        # PLUGIN_UPDATE_FAILED has no entry in suggestions_map.
+        fixes = WebInterfaceError(ErrorCode.PLUGIN_UPDATE_FAILED, "msg").suggested_fixes
+        assert fixes == ["Review error details and try again"]
+
+    def test_explicit_suggestions_win(self):
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=["Do the thing"])
+        assert error.suggested_fixes == ["Do the thing"]
+
+    def test_explicit_empty_list_is_respected(self):
+        # Regression: `suggested_fixes or default` treated [] as "unset",
+        # so a caller could not express "I have no suggestions".
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=[])
+        assert error.suggested_fixes == []
+
+    def test_none_still_gets_defaults(self):
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=None)
+        assert len(error.suggested_fixes) > 0
+
+
+class TestToDict:
+    def test_base_keys_always_present(self):
+        result = WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom").to_dict()
+        assert result["status"] == "error"
+        assert result["error_code"] == "SYSTEM_ERROR"
+        assert result["error_category"] == "system"
+        assert result["message"] == "boom"
+
+    def test_details_included_when_set(self):
+        result = WebInterfaceError(
+            ErrorCode.SYSTEM_ERROR, "boom", details="disk full").to_dict()
+        assert result["details"] == "disk full"
+
+    def test_details_omitted_when_absent(self):
+        assert "details" not in WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom").to_dict()
+
+    def test_context_included_when_non_empty(self):
+        result = WebInterfaceError(
+            ErrorCode.SYSTEM_ERROR, "boom", context={"path": "/tmp/x"}).to_dict()
+        assert result["context"] == {"path": "/tmp/x"}
+
+    def test_empty_context_is_omitted(self):
+        # Pinned as intentional, not a bug: __init__ normalizes context to
+        # {}, and an empty context carries no information, so it is left out
+        # rather than padding every error body with "context": {}.
+        result = WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom", context={}).to_dict()
+        assert "context" not in result
+
+    def test_empty_suggestions_omitted(self):
+        result = WebInterfaceError(
+            ErrorCode.SYSTEM_ERROR, "boom", suggested_fixes=[]).to_dict()
+        assert "suggested_fixes" not in result
+
+    def test_is_json_serializable(self):
+        import json
+        error = WebInterfaceError(
+            ErrorCode.NETWORK_ERROR, "boom",
+            details="timeout", context={"url": "http://x"})
+        assert json.loads(json.dumps(error.to_dict()))["error_code"] == "NETWORK_ERROR"
+
+
+class TestFromException:
+    @pytest.mark.parametrize("exc_name,expected", [
+        ("ConfigError", ErrorCode.CONFIG_LOAD_FAILED),
+        ("PluginError", ErrorCode.PLUGIN_LOAD_FAILED),
+        ("PermissionError", ErrorCode.PERMISSION_DENIED),
+        ("AccessDenied", ErrorCode.PERMISSION_DENIED),
+        ("ValidationError", ErrorCode.VALIDATION_ERROR),
+        ("SchemaError", ErrorCode.VALIDATION_ERROR),
+        ("NetworkError", ErrorCode.NETWORK_ERROR),
+        ("ConnectionError", ErrorCode.NETWORK_ERROR),
+        ("TimeoutError", ErrorCode.TIMEOUT),
+        ("SomethingElse", ErrorCode.UNKNOWN_ERROR),
+    ])
+    def test_code_inferred_from_exception_class_name(self, exc_name, expected):
+        exc = type(exc_name, (Exception,), {})("boom")
+        assert WebInterfaceError.from_exception(exc).error_code is expected
+
+    def test_explicit_code_skips_inference(self):
+        error = WebInterfaceError.from_exception(
+            ValueError("boom"), error_code=ErrorCode.PLUGIN_NOT_FOUND)
+        assert error.error_code is ErrorCode.PLUGIN_NOT_FOUND
+
+    def test_message_is_the_safe_one_not_the_exception_text(self):
+        # The raw exception text is not echoed into `message`; that field is
+        # a fixed, user-facing string per code.
+        error = WebInterfaceError.from_exception(ValueError("secret-ish detail"))
+        assert error.message == "An unexpected error occurred"
+        assert "secret-ish" not in error.message
+
+    def test_exception_type_recorded_in_context(self):
+        error = WebInterfaceError.from_exception(ValueError("boom"))
+        assert error.context["exception_type"] == "ValueError"
+
+    def test_caller_context_is_preserved_alongside_type(self):
+        error = WebInterfaceError.from_exception(
+            ValueError("boom"), context={"plugin_id": "clock"})
+        assert error.context["plugin_id"] == "clock"
+        assert error.context["exception_type"] == "ValueError"
+
+    def test_caller_supplied_exception_type_is_overwritten(self):
+        error = WebInterfaceError.from_exception(
+            ValueError("boom"), context={"exception_type": "Fake"})
+        assert error.context["exception_type"] == "ValueError"
+
+    def test_original_error_retained(self):
+        exc = ValueError("boom")
+        assert WebInterfaceError.from_exception(exc).original_error is exc
+
+    def test_every_code_has_a_safe_message(self):
+        for code in ErrorCode:
+            assert WebInterfaceError._safe_message(code)
+
+
+class TestExceptionDetails:
+    def test_context_dict_is_flattened(self):
+        exc = ValueError("boom")
+        exc.context = {"config_path": "/etc/x.json", "line": 4}
+        details = WebInterfaceError._get_exception_details(exc)
+        assert "config_path: /etc/x.json" in details
+        assert "line: 4" in details
+        assert "; " in details
+
+    def test_exception_type_key_excluded(self):
+        exc = ValueError("boom")
+        exc.context = {"exception_type": "ValueError", "path": "/tmp/x"}
+        details = WebInterfaceError._get_exception_details(exc)
+        assert "exception_type" not in details
+        assert details == "path: /tmp/x"
+
+    def test_context_with_only_exception_type_gives_none(self):
+        exc = ValueError("boom")
+        exc.context = {"exception_type": "ValueError"}
+        assert WebInterfaceError._get_exception_details(exc) is None
+
+    def test_no_context_attribute_gives_none(self):
+        assert WebInterfaceError._get_exception_details(ValueError("boom")) is None
+
+    def test_non_dict_context_gives_none(self):
+        exc = ValueError("boom")
+        exc.context = "not a dict"
+        assert WebInterfaceError._get_exception_details(exc) is None
+
+    def test_empty_context_gives_none(self):
+        exc = ValueError("boom")
+        exc.context = {}
+        assert WebInterfaceError._get_exception_details(exc) is None
+
+    def test_details_flow_into_from_exception(self):
+        exc = ValueError("boom")
+        exc.context = {"config_path": "/etc/x.json"}
+        assert "config_path" in WebInterfaceError.from_exception(exc).details