* fix(mcp): reject malformed Args on Add MCP Server instead of silently defaulting to []
* test(mcp): pass every Form param add_server reads past args validation
CI's pytest run showed test_add_server_still_accepts_valid_json_args and
test_add_server_still_defaults_empty_args_to_empty_list failing with
TypeError: the JSON object must be str, bytes or bytearray, not Form.
Calling the endpoint function directly bypasses FastAPI's dependency
resolution, so an unpassed Form(...) parameter (url, oauth_file,
oauth_config) arrives as the Form marker object itself rather than its
declared default, and add_server's later `if oauth_file:` check reads
that marker as truthy. The malformed-args test never hit this because it
raises before reaching that code. Not a production bug: a real HTTP
request resolves these through FastAPI before add_server ever runs.
* fix(mcp): reject non-list args and surface the new 400 in the Admin panel
o3LL's review on #6215 found two gaps in the args validation this PR adds:
the Admin panel posts to the same /api/mcp/servers endpoint but never
validates Args client-side, so the new 400 falls into the generic failure
branch and shows "Added but connection failed: unknown". Mirror the same
JSON.parse guard settings.js already has.
Also add an isinstance(list) check next to the existing JSON parse, since
valid-but-wrong-shaped JSON (args=5) reaches StdioServerParameters(args=5)
and 500s in the error formatter. Pre-existing on dev, same validation site
this PR already touches.
* fix(admin): surface the server's 400 detail instead of a generic connection-failed message
The Admin add-server handler read needs_oauth/connected/error but never
res.ok, so a request rejected by the isinstance(list) check added for
#6211 (args=5, a valid-JSON-but-non-list value the client-side JSON.parse
guard cannot catch) fell into the same-shape else branch as a successful
add whose connection attempt failed, and the form fields were cleared as
if the server had accepted it.
atomic_write_json/atomic_write_text build their temp filename as
"{path}.tmp.{os.getpid()}". os.getpid() is constant for the life of a
process, so it only ever distinguishes concurrent writers that live in
different OS processes. Odysseus runs as a single long-lived process
per container, so two concurrent writers to the same path (e.g. two
request handlers racing a settings save) always compute the identical
temp path. Whichever finishes os.replace() first removes the shared
tmp file out from under the other, which then raises FileNotFoundError
on its own os.replace() instead of landing its write.
Fix: derive the temp suffix from uuid4() instead of the PID, so every
call gets a distinct temp path regardless of process/thread identity.
routes/prefs_routes.py's _save() had an independent, hand-rolled copy
of the exact same PID-suffix logic (not the shared core.atomic_io
helper other routes already use, e.g. routes/auth_routes.py) with the
same bug. Replaced it with a call to atomic_write_json.
Fixes#5596
Co-authored-by: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com>
Amir Fathi