fix(verify): move spec parsing inside the try block so relative URLs 400, not 500 - #333
Conversation
| ) -> dict[str, int | str | float]: | ||
| spec = LLMSpec.from_string(info.spec) | ||
| try: | ||
| spec = LLMSpec.from_string(info.spec) |
There was a problem hiding this comment.
Sorry, it's incorrect error handling for this situation LLMSpec.from_string
There was a problem hiding this comment.
Sorry for the confusion, want to make sure I address the right thing. Looking again at from_string(): it already wraps any exception, bad input or an internal bug in parse_http_spec alike, into InvalidHTTPSpecError. Moving the call inside the try means that branch now returns a clean 400 with no logger call, where the generic except Exception branch below it does call logger.exception. So a genuine internal bug hiding inside parse_http_spec would now return a quiet 400 instead of the loud 500 it produced before, with no server-side trace either way.
Is that the concern, or did you have something else in mind, maybe the status code itself, or a different split between parse errors and verify() errors? Happy to add the missing log line if that is it, or take a different direction if I am off base.
…400, not 500 Fixes msoedov#149
2ed871d to
e52ff30
Compare
|
Rebased onto main to clear the conflict with the new logging PR (#338); the fix itself is unchanged, just carried the spec parsing into the try block alongside the new log lines. No rush, just checking in on the question above about the missing logger call on the InvalidHTTPSpecError branch, happy to add it if that is the concern. |
LLMSpec.from_string(info.spec)on line 32 ofroutes/scan.pysits one line above thetry:block that already catchesInvalidHTTPSpecErrorand turns it into a 400.from_stringre-raises every parse failure asInvalidHTTPSpecError(a relative URL, an empty spec, a malformed header line), so any of those inputs propagates straight past the existing handler and out of the endpoint. With no app-level exception handler in the repo, FastAPI's default handler turns that into a raw 500 instead of the 400 the route is clearly meant to return.Fix: move the
from_stringcall inside the try block so it shares the handler that already exists one line below it. One-line move, no new branches.Verification:
tests/integration/routes/test_scan.py, posting the relative-URL spec from the issue to/verify. Fails on main (500, same traceback as the issue), passes on the branch (400 with the parse-failure detail).poetry install && poetry run pytest .(Python 3.14, matching CI): 442 passed, 4 skipped, 41 deselected.black,flake8,pyupgrade --py314-pluson the changed files: clean.verify()(spec.verify()itself), since the bug and fix are entirely in the parsing path before that call.Fixes #149