Fix correctness bugs found in code review

.github/workflows/ci.yml

@@ -0,0 +1,46 @@
+name: CI
+
+on:
+  push:
+    branches: [main]
+  pull_request:
+  workflow_dispatch:
+
+concurrency:
+  group: ci-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  lint:
+    name: Lint and format
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: "3.12"
+      - name: Install ruff
+        run: python -m pip install --upgrade ruff
+      - name: ruff check
+        run: ruff check .
+      - name: ruff format --check
+        run: ruff format --check .
+
+  test:
+    name: Test (Python ${{ matrix.python-version }})
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        python-version: ["3.11", "3.12", "3.13"]
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: ${{ matrix.python-version }}
+      - name: Install package with all extras
+        run: |
+          python -m pip install --upgrade pip
+          python -m pip install -e ".[all]"
+      - name: Run tests
+        run: pytest tests/ -v

bmlib/agents/base.py

@@ -41,11 +41,11 @@ def score(self, title: str, abstract: str, interests: list[str]) -> dict:
 
 import json
 import logging
-import re
 import time
 from typing import Any
 
 from bmlib.llm import LLMClient, LLMMessage, LLMResponse
+from bmlib.llm.utils import extract_json
 from bmlib.templates import TemplateEngine
 
 logger = logging.getLogger(__name__)
@@ -136,7 +136,10 @@ def chat_json(
                 delay = 2 ** (attempt - 1)  # 1s, 2s, 4s …
                 logger.warning(
                     "Retry %d/%d after %.0fs (previous: %s)",
-                    attempt + 1, max_retries, delay, last_error,
+                    attempt + 1,
+                    max_retries,
+                    delay,
+                    last_error,
                 )
                 time.sleep(delay)
 
@@ -154,7 +157,8 @@ def chat_json(
                 last_error = "empty response from model"
                 logger.warning(
                     "LLM returned empty response (attempt %d/%d)",
-                    attempt + 1, max_retries,
+                    attempt + 1,
+                    max_retries,
                 )
                 continue
 
@@ -163,9 +167,10 @@ def chat_json(
             except ValueError:
                 last_error = "unparseable response"
                 logger.error(
-                    "LLM returned unparseable response (attempt %d/%d), "
-                    "full response: %s",
-                    attempt + 1, max_retries, content,
+                    "LLM returned unparseable response (attempt %d/%d), full response: %s",
+                    attempt + 1,
+                    max_retries,
+                    content,
                 )
                 continue
 
@@ -176,9 +181,7 @@ def chat_json(
     def render_template(self, template_name: str, **variables: Any) -> str:
         """Render a prompt template.  Raises if no template engine configured."""
         if self.templates is None:
-            raise RuntimeError(
-                f"No template engine configured — cannot render {template_name!r}"
-            )
+            raise RuntimeError(f"No template engine configured — cannot render {template_name!r}")
         return self.templates.render(template_name, **variables)
 
     # --- JSON parsing ---
@@ -195,19 +198,12 @@ def parse_json(text: str) -> dict:
         except json.JSONDecodeError:
             pass
 
-        # Try extracting from code block
-        match = re.search(r"```(?:json)?\s*\n?(.*?)\n?```", text, re.DOTALL)
-        if match:
-            try:
-                return json.loads(match.group(1).strip())
-            except json.JSONDecodeError:
-                pass
-
-        # Try finding a JSON object
-        match = re.search(r"\{.*\}", text, re.DOTALL)
-        if match:
+        # Fall back to the shared extractor (code-block aware + balanced-brace
+        # scanning that picks the first parseable object).
+        candidate = extract_json(text)
+        if candidate != text:
             try:
-                return json.loads(match.group(0))
+                return json.loads(candidate)
             except json.JSONDecodeError:
                 pass
 

bmlib/db/__init__.py

@@ -29,6 +29,7 @@
 """
 
 from bmlib.db.connection import connect_postgresql, connect_sqlite
+from bmlib.db.migrations import Migration, run_migrations
 from bmlib.db.operations import (
     create_tables,
     execute,
@@ -38,7 +39,6 @@
     fetch_scalar,
     table_exists,
 )
-from bmlib.db.migrations import Migration, run_migrations
 from bmlib.db.transactions import transaction
 
 __all__ = [

bmlib/db/connection.py

@@ -82,9 +82,7 @@ def connect_postgresql(
         import psycopg2
         import psycopg2.extras
     except ImportError:
-        raise ImportError(
-            "psycopg2 not installed. Install with: pip install bmlib[postgresql]"
-        )
+        raise ImportError("psycopg2 not installed. Install with: pip install bmlib[postgresql]")
 
     if dsn:
         conn = psycopg2.connect(dsn, cursor_factory=psycopg2.extras.RealDictCursor)

bmlib/db/migrations.py

@@ -35,8 +35,9 @@ def _m001_create_users(conn):
 from __future__ import annotations
 
 import logging
+from collections.abc import Callable
 from dataclasses import dataclass
-from typing import Any, Callable
+from typing import Any
 
 from bmlib.db.operations import create_tables, execute, fetch_all, table_exists
 from bmlib.db.transactions import transaction
@@ -125,9 +126,7 @@ def run_migrations(conn: Any, migrations: list[Migration]) -> int:
         if migration.version in applied:
             continue
 
-        logger.info(
-            "Applying migration %d: %s", migration.version, migration.name
-        )
+        logger.info("Applying migration %d: %s", migration.version, migration.name)
         with transaction(conn):
             migration.up(conn)
             execute(

bmlib/db/operations.py

@@ -96,16 +96,82 @@ def table_exists(conn: Any, name: str) -> bool:
     return row is not None
 
 
+def _split_sql_statements(script: str) -> list[str]:
+    """Split a multi-statement SQL script into individual statements.
+
+    Splits on semicolons that are outside string literals and comments.
+    Handles single/double-quoted strings (with SQL ``''`` escaping), ``--``
+    line comments, and ``/* */`` block comments. This is sufficient for the
+    plain ``CREATE TABLE``/``CREATE INDEX`` schemas used in this project; it
+    does not attempt to parse trigger bodies (``BEGIN ... END;``).
+    """
+    statements: list[str] = []
+    buf: list[str] = []
+    i = 0
+    n = len(script)
+    quote: str | None = None
+    while i < n:
+        ch = script[i]
+        if quote:
+            buf.append(ch)
+            if ch == quote:
+                # A doubled quote is an escaped quote, not a terminator.
+                if i + 1 < n and script[i + 1] == quote:
+                    buf.append(script[i + 1])
+                    i += 2
+                    continue
+                quote = None
+            i += 1
+            continue
+        # Not inside a string literal.
+        if ch in ("'", '"'):
+            quote = ch
+            buf.append(ch)
+            i += 1
+        elif ch == "-" and i + 1 < n and script[i + 1] == "-":
+            # Line comment: skip to end of line.
+            j = script.find("\n", i)
+            i = n if j == -1 else j
+        elif ch == "/" and i + 1 < n and script[i + 1] == "*":
+            # Block comment: skip to closing */.
+            j = script.find("*/", i + 2)
+            i = n if j == -1 else j + 2
+        elif ch == ";":
+            stmt = "".join(buf).strip()
+            if stmt:
+                statements.append(stmt)
+            buf = []
+            i += 1
+        else:
+            buf.append(ch)
+            i += 1
+    tail = "".join(buf).strip()
+    if tail:
+        statements.append(tail)
+    return statements
+
+
 def create_tables(conn: Any, schema_sql: str) -> None:
     """Execute a (possibly multi-statement) schema DDL string.
 
-    For SQLite the entire string is executed via ``executescript()``.
-    For PostgreSQL each statement is executed individually within an
-    implicit transaction.
+    Statements are executed individually via a cursor for both backends.
+    SQLite's ``executescript()`` is deliberately avoided because it issues an
+    implicit ``COMMIT`` before running, which would break the atomicity of a
+    surrounding :func:`~bmlib.db.transactions.transaction` (e.g. during
+    migrations). Executing statements one at a time keeps the DDL inside the
+    active transaction — SQLite supports transactional DDL — so a failure
+    rolls back cleanly.
     """
     module_name = type(conn).__module__
     if "sqlite3" in module_name:
-        conn.executescript(schema_sql)
+        cur = conn.cursor()
+        for stmt in _split_sql_statements(schema_sql):
+            cur.execute(stmt)
+        # Persist when called standalone, but leave commit to the caller when
+        # an explicit transaction is active (e.g. the migration runner), so
+        # the DDL stays atomic with the rest of that transaction.
+        if not conn.in_transaction:
+            conn.commit()
     else:
         cur = conn.cursor()
         cur.execute(schema_sql)