review/odysseus workspace fixes

core/atomic_io.py

@@ -15,31 +15,80 @@
 
 import json
 import os
+import stat
 from typing import Any, Optional
 
 
+def _fsync_parent_directory(path: str) -> None:
+    """Best-effort durability for a completed same-directory rename."""
+    if os.name != "posix":
+        return
+
+    directory = os.path.dirname(os.path.abspath(path)) or "."
+    flags = os.O_RDONLY
+    if hasattr(os, "O_DIRECTORY"):
+        flags |= os.O_DIRECTORY
+
+    descriptor = os.open(directory, flags)
+    try:
+        os.fsync(descriptor)
+    finally:
+        os.close(descriptor)
+
+
+def _atomic_write(path: str, writer, *, mode: Optional[int] = None) -> None:
+    import tempfile
+
+    directory = os.path.dirname(os.path.abspath(path)) or "."
+    os.makedirs(directory, exist_ok=True)
+
+    existing_mode = None
+    try:
+        existing_mode = stat.S_IMODE(os.stat(path, follow_symlinks=False).st_mode)
+    except FileNotFoundError:
+        pass
+
+    effective_mode = mode if mode is not None else existing_mode
+    descriptor, temporary = tempfile.mkstemp(
+        dir=directory,
+        prefix=f".{os.path.basename(path)}.",
+        suffix=".tmp",
+    )
+
+    try:
+        if effective_mode is not None:
+            os.fchmod(descriptor, effective_mode)
+
+        with os.fdopen(descriptor, "w", encoding="utf-8") as stream:
+            descriptor = -1
+            writer(stream)
+            stream.flush()
+            os.fsync(stream.fileno())
+
+        os.replace(temporary, path)
+        _fsync_parent_directory(path)
+    except BaseException:
+        if descriptor >= 0:
+            os.close(descriptor)
+        try:
+            os.unlink(temporary)
+        except FileNotFoundError:
+            pass
+        raise
+
+
 def atomic_write_json(path: str, data: Any, *, indent: Optional[int] = None) -> None:
-    """Atomically persist `data` as JSON at `path`.
+    def write(stream) -> None:
+        json.dump(data, stream, ensure_ascii=False, indent=indent)
 
-    The temp file uses the live PID as a suffix so two processes saving the
-    same file (e.g. unit tests) don't collide on the rename target.
-    """
-    os.makedirs(os.path.dirname(path) or ".", exist_ok=True)
-    tmp = f"{path}.tmp.{os.getpid()}"
-    with open(tmp, "w", encoding="utf-8") as f:
-        json.dump(data, f, indent=indent)
-        f.flush()
-        os.fsync(f.fileno())
-    os.replace(tmp, path)
+    _atomic_write(path, write)
 
 
-def atomic_write_text(path: str, text: str) -> None:
+def atomic_write_text(path: str, text: str, *, mode: Optional[int] = None) -> None:
     if not isinstance(text, str):
         raise TypeError("atomic_write_text expects a string")
-    os.makedirs(os.path.dirname(path) or ".", exist_ok=True)
-    tmp = f"{path}.tmp.{os.getpid()}"
-    with open(tmp, "w", encoding="utf-8") as f:
-        f.write(text)
-        f.flush()
-        os.fsync(f.fileno())
-    os.replace(tmp, path)
+
+    def write(stream) -> None:
+        stream.write(text)
+
+    _atomic_write(path, write, mode=mode)

core/database.py

@@ -8,7 +8,7 @@
 from sqlalchemy import event, create_engine, Column, String, Text, Boolean, DateTime, Integer, ForeignKey, JSON, Index, func, text
 from sqlalchemy.engine import Engine, make_url
 from sqlalchemy.types import TypeDecorator
-from sqlalchemy.ext.declarative import declarative_base, declared_attr
+from sqlalchemy.orm import declarative_base, declared_attr
 from sqlalchemy.orm import relationship, sessionmaker, backref
 
 from src.runtime_paths import get_app_root

pyproject.toml

@@ -1,4 +1,5 @@
 [tool.pytest.ini_options]
+asyncio_default_fixture_loop_scope = "function"
 testpaths = ["tests"]
 asyncio_mode = "auto"
 # Test-taxonomy markers added at collection time by tests/conftest.py. The
@@ -19,4 +20,7 @@ markers = [
     # taxonomy. The fast lane runs `not slow`; mark a test slow only with
     # duration evidence (see tests/run_focus.py --durations and tests/README.md).
     "slow: opt-in marker for known-slow tests; excluded by the fast lane (not slow)",
+    "asyncio: repository test taxonomy marker",
+    "parametrize: repository test taxonomy marker",
+    "skipif: repository test taxonomy marker",
 ]

routes/gallery/gallery_routes.py

@@ -257,43 +257,62 @@ async def gallery_upload(request: Request):
     @router.post("/api/gallery/{image_id}/replace")
     async def gallery_replace(request: Request, image_id: str):
         """Replace an existing gallery image file with a new one."""
-        user = get_current_user(request)
+        form = None
         db = SessionLocal()
         try:
-            img = db.query(GalleryImage).filter(GalleryImage.id == image_id).first()
+            user = get_current_user(request)
+            img = (
+                db.query(GalleryImage)
+                .filter(GalleryImage.id == image_id)
+                .first()
+            )
             if not img:
                 raise HTTPException(404, "Image not found")
             if not user or img.owner != user:
                 raise HTTPException(403, "Not your image")
 
             form = await request.form()
             file = form.get("image")
-            if not file or not hasattr(file, 'read'):
+            if not file or not hasattr(file, "read"):
                 raise HTTPException(400, "No image provided")
 
-            content = await read_upload_limited(file, GALLERY_UPLOAD_MAX_BYTES, "Gallery replacement")
+            content = await read_upload_limited(
+                file,
+                GALLERY_UPLOAD_MAX_BYTES,
+                "Gallery replacement",
+            )
             GALLERY_IMAGE_DIR.mkdir(parents=True, exist_ok=True)
             img_path = _gallery_image_path(img.filename)
             img_path.write_bytes(content)
 
             # Refresh dimensions in case the editor resized the canvas.
             # updated_at auto-bumps via TimestampMixin's onupdate hook.
             try:
-                from PIL import Image
                 from io import BytesIO
+
+                from PIL import Image
+
                 with Image.open(BytesIO(content)) as new_im:
                     img.width = new_im.width
                     img.height = new_im.height
             except Exception:
                 pass
+
             try:
                 db.commit()
             except Exception:
                 db.rollback()
                 logger.exception("gallery_replace: DB commit failed")
                 raise HTTPException(500, "Image update failed")
-            return {"ok": True, "width": img.width, "height": img.height}
+
+            return {
+                "ok": True,
+                "width": img.width,
+                "height": img.height,
+            }
         finally:
+            if form is not None:
+                await form.close()
             db.close()
 
     # ---- POST /api/gallery/{image_id}/rename ----

routes/mcp_routes.py

@@ -21,6 +21,67 @@
 router = APIRouter(prefix="/api/mcp", tags=["mcp"])
 
 
+
+_SUPPORTED_MCP_TRANSPORTS = frozenset({"stdio", "sse", "http"})
+
+
+def _validate_mcp_transport(raw_transport) -> str:
+    transport = str(raw_transport or "").lower()
+    if transport not in _SUPPORTED_MCP_TRANSPORTS:
+        raise HTTPException(
+            status_code=400,
+            detail=(
+                "Unsupported MCP transport; expected one of: "
+                "stdio, sse, http"
+            ),
+        )
+    return transport
+
+
+def _parse_mcp_args(raw_args) -> list[str]:
+    try:
+        parsed = json.loads(raw_args)
+    except (TypeError, ValueError, json.JSONDecodeError) as exc:
+        raise HTTPException(
+            status_code=400,
+            detail="Invalid MCP args: expected a JSON string list",
+        ) from exc
+
+    if (
+        not isinstance(parsed, list)
+        or not all(isinstance(item, str) for item in parsed)
+    ):
+        raise HTTPException(
+            status_code=400,
+            detail="Invalid MCP args: expected a JSON string list",
+        )
+
+    return parsed
+
+
+def _parse_mcp_env(raw_env) -> dict[str, str]:
+    try:
+        parsed = json.loads(raw_env)
+    except (TypeError, ValueError, json.JSONDecodeError) as exc:
+        raise HTTPException(
+            status_code=400,
+            detail="Invalid MCP env: expected a JSON string mapping",
+        ) from exc
+
+    if (
+        not isinstance(parsed, dict)
+        or not all(
+            isinstance(key, str) and isinstance(value, str)
+            for key, value in parsed.items()
+        )
+    ):
+        raise HTTPException(
+            status_code=400,
+            detail="Invalid MCP env: expected a JSON string mapping",
+        )
+
+    return parsed
+
 def _mcp_oauth_base_dir() -> Path:
     """Directory that may contain OAuth files managed by Odysseus."""
     return Path(MCP_OAUTH_DIR).resolve(strict=False)
@@ -174,6 +235,8 @@ async def add_server(
         server_id = str(uuid.uuid4())[:8]
 
         # Validate
+        transport = _validate_mcp_transport(transport)
+
         if transport == "stdio" and not command:
             raise HTTPException(400, "command is required for stdio transport")
         if transport == "sse" and not url:
@@ -182,16 +245,8 @@ async def add_server(
             raise HTTPException(400, "url is required for HTTP transport")
 
         # Parse JSON fields
-        try:
-            parsed_args = json.loads(args) if args else []
-        except json.JSONDecodeError:
-            parsed_args = []
-        try:
-            parsed_env = json.loads(env) if env else {}
-        except json.JSONDecodeError:
-            parsed_env = {}
-        if not isinstance(parsed_env, dict):
-            parsed_env = {}
+        parsed_args = _parse_mcp_args(args if args else "[]")
+        parsed_env = _parse_mcp_env(env if env else "{}")
 
         # Parse OAuth config
         parsed_oauth_config = None

routes/skills_routes.py

@@ -1625,7 +1625,7 @@ async def update_skill(request: Request, skill_id: str, body: SkillUpdateRequest
             raise HTTPException(404, "Skill not found")
         _verify_owner(match, user)
 
-        updates = body.dict(exclude_none=True)
+        updates = body.model_dump(exclude_none=True)
         if not updates:
             return {"ok": True}
         ok = skills_manager.update_skill(match.get("name"), updates, owner=user)

services/memory/skill_format.py

@@ -186,12 +186,14 @@ def _emit_scalar(v: Any) -> str:
     return s
 
 
-def _as_list(v: Any) -> List[str]:
+def _as_string_list(v: Any, field_name: str) -> List[str]:
     if v is None:
         return []
     if isinstance(v, list):
-        return [str(x) for x in v if x not in (None, "")]
-    return [str(v)]
+        if not all(isinstance(x, str) for x in v):
+            raise ValueError(f"{field_name} must be a list of strings")
+        return [x for x in v if x != ""]
+    raise ValueError(f"{field_name} must be a list of strings")
 
 
 def _as_float(v: Any, default: float = 0.8) -> float:
@@ -284,12 +286,21 @@ def _parse_list_lines(text: str) -> List[str]:
             items.append(m.group(1).strip())
         elif items:
             # continuation of previous bullet
-            items[-1] = items[-1] + " " + s
+            items[-1] = items[-1] + "\n" + s
         else:
             items.append(s)
     return items
 
 
+def _emit_list_item(prefix: str, item: Any) -> str:
+    lines = str(item).splitlines() or [""]
+    first, rest = lines[0], lines[1:]
+    if not rest:
+        return f"{prefix} {first}"
+    continuation = "\n".join(f"   {line}" if line else "" for line in rest)
+    return f"{prefix} {first}\n{continuation}"
+
+
 def emit_body(sections: Dict[str, Any]) -> str:
     parts: List[str] = []
     when = (sections.get("when_to_use") or "").strip()
@@ -301,9 +312,9 @@ def emit_body(sections: Dict[str, Any]) -> str:
             continue
         heading = _KEY_TO_HEADING[key]
         if key == "procedure":
-            body = "\n".join(f"{i + 1}. {x}" for i, x in enumerate(items))
+            body = "\n".join(_emit_list_item(f"{i + 1}.", x) for i, x in enumerate(items))
         else:
-            body = "\n".join(f"- {x}" for x in items)
+            body = "\n".join(_emit_list_item("-", x) for x in items)
         parts.append(f"## {heading}\n\n{body}")
     extra = (sections.get("body_extra") or "").strip()
     if extra:
@@ -410,10 +421,10 @@ def from_markdown(cls, text: str, *, path: Optional[str] = None) -> "Skill":
             description=str(fm.get("description", "") or ""),
             version=str(fm.get("version", "1.0.0") or "1.0.0"),
             category=str(fm.get("category", "general") or "general"),
-            tags=_as_list(fm.get("tags")),
-            platforms=_as_list(fm.get("platforms")),
-            requires_toolsets=_as_list(fm.get("requires_toolsets")),
-            fallback_for_toolsets=_as_list(fm.get("fallback_for_toolsets")),
+            tags=_as_string_list(fm.get("tags"), "tags"),
+            platforms=_as_string_list(fm.get("platforms"), "platforms"),
+            requires_toolsets=_as_string_list(fm.get("requires_toolsets"), "requires_toolsets"),
+            fallback_for_toolsets=_as_string_list(fm.get("fallback_for_toolsets"), "fallback_for_toolsets"),
             status=str(fm.get("status", "draft") or "draft"),
             confidence=_as_float(fm.get("confidence", 0.8), 0.8),
             source=str(fm.get("source", "learned") or "learned"),