fix: apply deep code review findings (3 critical, 5 warning)

CR-01: Lifespan error handler re-raises instead of SystemExit(1)
CR-02: Use INSERT OR IGNORE for symbols (matches DELETE+re-insert pattern)
CR-03: Validate version format before sorting (prevents ValueError crash)
WR-02: Add DocsServerError catch to list_versions for consistent isError:true
WR-03: Escape double quotes in logfmt values to prevent injection
WR-04: Validate version format in ingest_inventory before URL construction
WR-05: Add defensive assertion for FTS table name identifiers
WR-07: Validate YAML structure in populate_synonyms
WR-08: Add clear warning when sphinx-build fails but symbols ingested
IN-05: Replace dead _meta code block with deferred comment

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: ayhammouda

src/mcp_server_python_docs/__main__.py

@@ -118,6 +118,15 @@ def build_index(versions: str, skip_content: bool) -> None:
         logger.error("No valid versions specified. Example: --versions 3.13")
         raise SystemExit(1)
 
+    # Validate version format before sorting (CR-03, WR-04)
+    for v in version_list:
+        parts = v.split(".")
+        if len(parts) != 2 or not all(p.isdigit() for p in parts):
+            logger.error(
+                "Invalid version format %r. Expected 'X.Y' (e.g., 3.13)", v
+            )
+            raise SystemExit(1)
+
     # Determine default version: highest version number (MVER-02)
     sorted_versions = sorted(version_list, key=lambda v: [int(x) for x in v.split(".")])
     default_version = sorted_versions[-1]
@@ -228,7 +237,12 @@ def build_index(versions: str, skip_content: bool) -> None:
                             version,
                             result.stderr[-2000:] if result.stderr else "(no output)",
                         )
-                        any_version_succeeded = True  # symbols still ingested
+                        logger.warning(
+                            "Symbols were ingested for %s but sections/examples "
+                            "will be missing. Smoke tests may fail.",
+                            version,
+                        )
+                        any_version_succeeded = True
                         continue
 
                     logger.info("sphinx-build complete for Python %s", version)

src/mcp_server_python_docs/ingestion/inventory.py

@@ -111,6 +111,14 @@ def ingest_inventory(
     # Clear existing symbols for this doc_set (re-ingestion support)
     conn.execute("DELETE FROM symbols WHERE doc_set_id = ?", (doc_set_id,))
 
+    # Validate version format before URL construction (WR-04)
+    import re
+
+    if not re.match(r"^\d+\.\d+$", version):
+        from mcp_server_python_docs.errors import IngestionError
+
+        raise IngestionError(f"Invalid version format: {version!r}")
+
     # Download and parse objects.inv (INGR-I-01)
     url = f"https://docs.python.org/{version}/objects.inv"
     logger.info(f"Downloading {url}...")
@@ -141,7 +149,7 @@ def ingest_inventory(
         anchor_part = uri.split("#", 1)[1] if "#" in uri else None
 
         conn.execute(
-            "INSERT OR REPLACE INTO symbols "
+            "INSERT OR IGNORE INTO symbols "
             "(doc_set_id, qualified_name, normalized_name, module, "
             "symbol_type, uri, anchor) "
             "VALUES (?, ?, ?, ?, ?, ?, ?)",

src/mcp_server_python_docs/ingestion/sphinx_json.py

@@ -398,6 +398,13 @@ def populate_synonyms(conn: sqlite3.Connection) -> int:
     with importlib.resources.as_file(ref) as path:
         data = yaml.safe_load(path.read_text())
 
+    if not isinstance(data, dict):
+        from mcp_server_python_docs.errors import IngestionError
+
+        raise IngestionError(
+            f"synonyms.yaml must be a YAML mapping, got {type(data).__name__}"
+        )
+
     count = 0
     for concept, expansion in data.items():
         if isinstance(expansion, list):

src/mcp_server_python_docs/server.py

@@ -96,15 +96,15 @@ async def app_lifespan(server: FastMCP) -> AsyncIterator[AppContext]:
             version_service=version_svc,
         )
     except Exception:
-        # HYGN-05: log lifespan errors, write last-error.log
+        # HYGN-05: log lifespan errors, write last-error.log, re-raise original
         error_msg = traceback.format_exc()
         logger.error("Lifespan error: %s", error_msg)
         try:
             error_log = cache_dir / "last-error.log"
             error_log.write_text(error_msg)
         except Exception:
             pass
-        raise SystemExit(1)
+        raise
     finally:
         db.close()
 
@@ -167,24 +167,15 @@ def list_versions(
     ) -> ListVersionsResult:
         """List Python documentation versions available in this index."""
         app_ctx: AppContext = ctx.request_context.lifespan_context
-        return app_ctx.version_service.list_versions()
+        try:
+            return app_ctx.version_service.list_versions()
+        except DocsServerError as e:
+            raise ToolError(str(e))
 
     # SRVR-07: _meta hint for get_docs tool.
     # FastMCP 1.27 does not expose a public API for setting _meta on tool
-    # definitions via the decorator. The _meta is documented as a client hint
-    # for expected max response size. We set it by accessing the tool manager's
-    # internal tool registry. This may need updating on mcp SDK version bumps.
-    try:
-        tool_mgr = mcp._tool_manager
-        if hasattr(tool_mgr, "_tools") and "get_docs" in tool_mgr._tools:
-            tool_def = tool_mgr._tools["get_docs"]
-            if not hasattr(tool_def, "_meta") or tool_def._meta is None:
-                # Store as attribute for now — the MCP protocol layer reads
-                # _meta from the Tool definition when building tools/list response
-                pass
-            # The _meta hint is advisory and may not be supported by all SDK
-            # versions. Log but don't fail if we can't set it.
-    except Exception:
-        logger.debug("Could not set _meta on get_docs tool — advisory hint only")
+    # definitions. Deferred until the mcp SDK adds _meta support to the
+    # decorator API or tool manager. The hint is advisory — clients work
+    # correctly without it.
 
     return mcp

src/mcp_server_python_docs/services/observability.py

@@ -30,7 +30,8 @@ def _format_logfmt(**fields: Any) -> str:
         elif isinstance(value, float):
             parts.append(f"{key}={value:.1f}")
         elif isinstance(value, str) and " " in value:
-            parts.append(f'{key}="{value}"')
+            escaped = str(value).replace('"', '\\"')
+            parts.append(f'{key}="{escaped}"')
         else:
             parts.append(f"{key}={value}")
     return " ".join(parts)

src/mcp_server_python_docs/storage/db.py

@@ -124,7 +124,9 @@ def bootstrap_schema(conn: sqlite3.Connection) -> None:
     # Drop FTS5 virtual tables first so they can be recreated with the
     # correct tokenizer. IF NOT EXISTS would skip recreation if the table
     # exists with a different tokenizer -- there is no ALTER for FTS5.
-    for fts_table in ("sections_fts", "symbols_fts", "examples_fts"):
+    _FTS_TABLES = ("sections_fts", "symbols_fts", "examples_fts")
+    for fts_table in _FTS_TABLES:
+        assert fts_table.isidentifier(), f"Invalid table name: {fts_table}"
         conn.execute(f"DROP TABLE IF EXISTS {fts_table}")
 
     # Load and execute the full schema DDL