refactor(web): one error-response path for api_v3 (#624)

* refactor(web): answer unhandled api_v3 errors from one blueprint handler

Fifty-three api_v3 routes ended in a copy of the same catch-all: log the
traceback, return {status, "An error occurred; see logs for details",
details: describe_exception(e)} with a 500. They are replaced by one
errorhandler on the api_v3 blueprint that returns exactly that body.

It lives on the blueprint rather than falling through to app.py's global
handler because the two answers differ: the global one adds
error_code: UNKNOWN_ERROR, and api_client.js sends a body with an
error_code to the error modal and one without to a plain toast. A
blueprint handler also gives tests that mount api_v3 on a bare Flask app
the same answer the real app gives.

Only handlers that were byte-for-byte that shape were removed (matched on
the AST, and each rewritten function re-parsed and compared). Handlers
with their own message, extra keys, operation-history records or cleanup
stay, as does execute_plugin_action's step-1 handler, which sits inside
an `except subprocess.TimeoutExpired` arm that would otherwise turn a
plugin's timeout into a 408.

HTTPExceptions raised inside a route go back as themselves in the global
handler's 4xx shape. Where a removed catch-all used to swallow one (only
delete_plugin_asset's non-silent get_json() is reachable), a malformed
request now gets its 415/400 instead of a 500.

Most of the diff is re-indentation from unwrapping the try blocks;
`git diff -w` shows the real change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(web): plugin action errors name the real failure, not UnboundLocalError

execute_plugin_action bound a local `logger` in its JSON-parsing arm,
which made `logger` local to the whole function. Every other
`logger.error` in it then raised UnboundLocalError, so a failing OAuth
step-1 script was reported as "UnboundLocalError: cannot access local
variable 'logger'" -- from the step-1 handler, and before the previous
commit from the route's outer catch-all too. Use the module logger.

Found by comparing every api_v3 route's forced-failure response before
and after the catch-all consolidation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(web): drop the error category and exception-name code guessing

WebInterfaceError derived an ErrorCategory from every error code and put
it in each structured error body as `error_category`. Nothing reads it:
not the web UI (static/ and templates/), not the tests beyond the ones
pinning the mapping itself, and not any plugin in ledmatrix-plugins. The
enum, the inference table and the JSON key go.

from_exception() could also guess an error code from the exception's
class name ("Config" -> CONFIG_LOAD_FAILED, and so on). Every caller
passes a code, so the guess never ran; error_code is now required.

suggested_fixes stays: the error dialog in static/v3/js/utils/
error_handler.js lists them.

The REST reference loses error_category and says what an unanticipated
exception in an /api/v3 route answers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(web): one call for the from_exception error responses

Nine plugin routes built a structured error by hand:

    from src.web_interface.errors import WebInterfaceError
    error = WebInterfaceError.from_exception(e, ErrorCode.X)
    return error_response(error.error_code, error.message,
                          details=error.details, context=error.context,
                          status_code=500)

That is now exception_error_response(e, ErrorCode.X) in api_helpers, so
error_response() is the only structured-error entry point the routes
use. The three operation-history routes never passed the context, and
with_context=False keeps their bodies exactly as they were; a test
compares the helper against the hand-written pair for both forms.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs(changelog): one api_v3 error-response path

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-09-24 15:53:19 -04:00
committed by GitHub
co-authored by Claude Opus 5.5
parent ece416c4e5
commit 4e61d7248a
17 changed files with 2437 additions and 2418 deletions
+46
View File
@@ -230,6 +230,52 @@ class TestHandlersCarryDetail:
"handlers returning the generic message without %s: %r"
% ("both a traceback log and the detail", offenders))
def test_no_api_v3_route_copies_the_blueprint_handler(self):
"""The generic catch-all lives once, on the blueprint.
Fifty-three routes carried their own copy of it -- log with exc_info,
return {status, "An error occurred; see logs for details",
describe_exception(e)}, 500 -- until they were folded into
`_api_v3_unhandled_exception`. A new copy changes nothing a caller
sees, so nothing else would notice it; this does. A handler that says
something *different* (its own message, extra keys, cleanup) is fine.
"""
import ast
import pathlib
generic = "An error occurred; see logs for details"
copies = []
for path in sorted(pathlib.Path("web_interface/blueprints/api_v3").glob("*.py")):
tree = ast.parse(path.read_text(encoding="utf-8"))
for fn in [n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef)]:
for h in ast.walk(fn):
if not (isinstance(h, ast.ExceptHandler)
and isinstance(h.type, ast.Name)
and h.type.id == "Exception"):
continue
for r in [n for n in h.body if isinstance(n, ast.Return)]:
v = r.value
if not (isinstance(v, ast.Tuple) and len(v.elts) == 2
and isinstance(v.elts[0], ast.Call)
and getattr(v.elts[0].func, "id", None) == "jsonify"
and v.elts[0].args
and isinstance(v.elts[0].args[0], ast.Dict)):
continue
d = v.elts[0].args[0]
keys = {k.value for k in d.keys if isinstance(k, ast.Constant)}
message = [val.value for k, val in zip(d.keys, d.values)
if isinstance(k, ast.Constant) and k.value == "message"
and isinstance(val, ast.Constant)]
if keys == {"status", "message", "details"} and message == [generic]:
copies.append((path.name, fn.name))
# One is not a copy: execute_plugin_action's step-1 handler sits
# inside the route's `except subprocess.TimeoutExpired` arm, which
# would turn a plugin's own timeout into a 408 if this let it through.
assert copies == [("plugins.py", "execute_plugin_action")], (
"these handlers duplicate the api_v3 blueprint's errorhandler; "
"delete them and let the exception propagate: %r" % copies)
def test_client_errors_keep_their_own_status(self):
"""A 405 must not be reported as a server-side UNKNOWN_ERROR.