diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..064c451 --- /dev/null +++ b/.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 diff --git a/bmlib/agents/base.py b/bmlib/agents/base.py index 8779eee..3a04754 100644 --- a/bmlib/agents/base.py +++ b/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 diff --git a/bmlib/db/__init__.py b/bmlib/db/__init__.py index 2b757ad..8425219 100644 --- a/bmlib/db/__init__.py +++ b/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__ = [ diff --git a/bmlib/db/connection.py b/bmlib/db/connection.py index 8b9c49f..87676b8 100644 --- a/bmlib/db/connection.py +++ b/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) diff --git a/bmlib/db/migrations.py b/bmlib/db/migrations.py index 1f7f7e2..977128d 100644 --- a/bmlib/db/migrations.py +++ b/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( diff --git a/bmlib/db/operations.py b/bmlib/db/operations.py index 4dee81f..e0abc28 100644 --- a/bmlib/db/operations.py +++ b/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) diff --git a/bmlib/fulltext/jats_parser.py b/bmlib/fulltext/jats_parser.py index f0b57dd..c44d6c4 100644 --- a/bmlib/fulltext/jats_parser.py +++ b/bmlib/fulltext/jats_parser.py @@ -31,8 +31,6 @@ from html import escape as html_escape from io import BytesIO -logger = logging.getLogger(__name__) - from bmlib.fulltext.models import ( JATSAbstractSection, JATSArticle, @@ -43,6 +41,8 @@ JATSTableInfo, ) +logger = logging.getLogger(__name__) + MAX_HEADING_LEVEL = 6 @@ -111,6 +111,8 @@ class _TableBuilder: in_row: bool = False in_cell: bool = False current_row_has_header_cells: bool = False + current_row_cell_count: int = 0 + current_row_header_cell_count: int = 0 current_colspan: int = 1 def start_header(self) -> None: @@ -131,27 +133,38 @@ def start_row(self) -> None: self.in_row = True self.current_row = [] self.current_row_has_header_cells = False + self.current_row_cell_count = 0 + self.current_row_header_cell_count = 0 def end_row(self) -> None: if self.in_row and self.current_row: - if self.in_header or ( - self.current_row_has_header_cells - and not self.in_body - and not self.header_rows - ): + # A row is a header when it is inside an explicit , or — for + # tables lacking / wrappers — when it is the first row + # AND *every* cell is a header cell. Requiring all cells to be header + # cells avoids misclassifying a normal data row that merely starts + # with a single row-label. + all_header_cells = ( + self.current_row_cell_count > 0 + and self.current_row_header_cell_count == self.current_row_cell_count + ) + if self.in_header or (all_header_cells and not self.in_body and not self.header_rows): self.header_rows.append(self.current_row) else: self.body_rows.append(self.current_row) self.in_row = False self.current_row = [] self.current_row_has_header_cells = False + self.current_row_cell_count = 0 + self.current_row_header_cell_count = 0 def start_cell(self, is_header: bool = False, colspan: int = 1) -> None: self.in_cell = True self.current_cell_text = "" self.current_colspan = max(1, colspan) + self.current_row_cell_count += 1 if is_header or self.in_header: self.current_row_has_header_cells = True + self.current_row_header_cell_count += 1 def end_cell(self) -> None: if self.in_cell: @@ -460,12 +473,7 @@ def startElement(self, name: str, attrs: xml.sax.xmlreader.AttributesImpl) -> No self.current_figure = _FigureBuilder(id=attrs.get("id", "")) elif name == "graphic": if self.in_figure and self.current_figure is not None: - href = ( - attrs.get("xlink:href") - or attrs.get("href") - or attrs.get("xlink-href") - or "" - ) + href = attrs.get("xlink:href") or attrs.get("href") or attrs.get("xlink-href") or "" if href: self.current_figure.graphic_href = href elif name == "table-wrap": @@ -556,8 +564,11 @@ def endElement(self, name: str) -> None: if id_type == "doi": self.doi = text elif id_type in ( - "pmc", "pmcid", "pmcid-ver", - "pmcaid", "pmcaiid", + "pmc", + "pmcid", + "pmcid-ver", + "pmcaid", + "pmcaiid", ): # All PMC-related identifiers — store the canonical # PMC ID only from "pmc" or "pmcid" variants (not @@ -573,7 +584,10 @@ def endElement(self, name: str) -> None: self.current_article_id_type = None elif name == "abstract": - if self.current_abstract_text: + # Flush the final section when it has a title OR body text, so a + # titled-but-empty trailing subsection (or a title-only abstract) + # is not silently dropped. + if self.current_abstract_text or self.current_abstract_title: content = " ".join(self.current_abstract_text) self.abstract_sections.append( JATSAbstractSection(title=self.current_abstract_title, content=content) @@ -581,12 +595,10 @@ def endElement(self, name: str) -> None: self.in_abstract = False elif name == "title": if self.in_abstract: - if self.current_abstract_text: + if self.current_abstract_text or self.current_abstract_title: content = " ".join(self.current_abstract_text) self.abstract_sections.append( - JATSAbstractSection( - title=self.current_abstract_title, content=content - ) + JATSAbstractSection(title=self.current_abstract_title, content=content) ) self.current_abstract_text = [] self.current_abstract_title = text @@ -599,8 +611,12 @@ def endElement(self, name: str) -> None: elif (self.in_body or self.in_back) and self.section_stack: self.section_stack[-1].paragraphs.append(normalized_text) elif self.in_figure and self.current_figure: + if self.current_figure.caption and normalized_text: + self.current_figure.caption += " " self.current_figure.caption += normalized_text elif self.in_table_wrap and self.current_table: + if self.current_table.caption and normalized_text: + self.current_table.caption += " " self.current_table.caption += normalized_text elif name == "body": @@ -920,9 +936,7 @@ def _format_journal_html(h: _JATSHandler) -> str: def _format_identifiers_html(h: _JATSHandler) -> str: ids: list[str] = [] if h.doi: - ids.append( - f'DOI: {html_escape(h.doi)}' - ) + ids.append(f'DOI: {html_escape(h.doi)}') if h.pmc_id: pmc_num = h.pmc_id[3:] if h.pmc_id.startswith("PMC") else h.pmc_id ids.append( diff --git a/bmlib/fulltext/models.py b/bmlib/fulltext/models.py index 3509693..0bad605 100644 --- a/bmlib/fulltext/models.py +++ b/bmlib/fulltext/models.py @@ -189,7 +189,7 @@ def from_dict(cls, data: dict[str, Any]) -> FullTextSourceEntry: class FullTextResult: """Result of a full-text retrieval attempt.""" - source: str # "europepmc", "unpaywall", "doi", "cached" + source: str # "europepmc", "unpaywall", "doi", "pubmed", "cached" html: str | None = None pdf_url: str | None = None web_url: str | None = None diff --git a/bmlib/fulltext/service.py b/bmlib/fulltext/service.py index 79301b0..5aa8101 100644 --- a/bmlib/fulltext/service.py +++ b/bmlib/fulltext/service.py @@ -25,6 +25,7 @@ from __future__ import annotations +import hashlib import logging import re from urllib.parse import quote @@ -49,8 +50,16 @@ class FullTextError(Exception): def _sanitize_identifier(raw: str) -> str: - """Turn a DOI or other identifier into a safe filename component.""" - return re.sub(r"[^\w.\-]", "_", raw) + """Turn a DOI or other identifier into a safe, collision-free filename. + + A readable prefix is kept for debuggability, but because many distinct + identifiers sanitise to the same string (every character outside + ``[\\w.\\-]`` maps to ``_``), a short hash of the *raw* identifier is + appended so two different identifiers can never share a cache file. + """ + safe = re.sub(r"[^\w.\-]", "_", raw) + digest = hashlib.sha1(raw.encode("utf-8")).hexdigest()[:10] + return f"{safe}_{digest}" def _extract_free_pdf_url(result: dict[str, object]) -> str | None: @@ -150,8 +159,8 @@ def fetch_fulltext( pdf_render_url: str | None = None if not pmc_id and (doi or pmid): try: - discovered_pmc_id, pdf_render_url = ( - self._resolve_pmc_id_and_pdf_url(doi=doi, pmid=pmid) + discovered_pmc_id, pdf_render_url = self._resolve_pmc_id_and_pdf_url( + doi=doi, pmid=pmid ) if discovered_pmc_id: html = self._fetch_europepmc(discovered_pmc_id) @@ -164,14 +173,17 @@ def fetch_fulltext( except Exception: logger.debug( "Europe PMC discovery failed for doi=%s pmid=%s", - doi, pmid, exc_info=True, + doi, + pmid, + exc_info=True, ) # When XML failed with a known PMC ID, search for PDF render URL if xml_failed and not pdf_render_url and (doi or pmid): try: _, pdf_render_url = self._resolve_pmc_id_and_pdf_url( - doi=doi, pmid=pmid, + doi=doi, + pmid=pmid, ) except Exception: logger.debug("PDF URL resolution failed", exc_info=True) @@ -202,7 +214,7 @@ def fetch_fulltext( # Final fallback: PubMed URL if pmid: logger.info("Falling back to PubMed URL for PMID %s", pmid) - return FullTextResult(source="doi", web_url=f"{PUBMED_BASE}/{pmid}/") + return FullTextResult(source="pubmed", web_url=f"{PUBMED_BASE}/{pmid}/") raise FullTextError("No identifiers provided") @@ -219,7 +231,8 @@ def _try_known_sources( """ priority = {"xml": 0, "pdf": 1, "html": 2} sorted_sources = sorted( - sources, key=lambda s: priority.get(s.format, 99), + sources, + key=lambda s: priority.get(s.format, 99), ) for entry in sorted_sources: @@ -239,7 +252,9 @@ def _try_known_sources( return FullTextResult(source=entry.source, web_url=entry.url) except Exception: logger.debug( - "Known source %s (%s) failed", entry.source, entry.url, + "Known source %s (%s) failed", + entry.source, + entry.url, exc_info=True, ) continue @@ -269,7 +284,10 @@ def _cache_html(self, html: str, cache_id: str | None) -> None: logger.debug("Failed to cache HTML for %s", cache_id, exc_info=True) def _download_and_cache_pdf( - self, pdf_url: str, cache_id: str | None, result: FullTextResult, + self, + pdf_url: str, + cache_id: str | None, + result: FullTextResult, ) -> None: """Download a PDF and save it to the disk cache. @@ -304,7 +322,10 @@ def _fetch_jats_xml(self, url: str) -> str: return parser.to_html() def _resolve_pmc_id_and_pdf_url( - self, *, doi: str | None = None, pmid: str = "", + self, + *, + doi: str | None = None, + pmid: str = "", ) -> tuple[str | None, str | None]: """Search Europe PMC to discover a PMC ID and free PDF URL. @@ -358,7 +379,8 @@ def _fetch_europepmc(self, pmc_id: str) -> str: def _fetch_unpaywall(self, doi: str) -> str: """Query Unpaywall for open-access PDF URL.""" encoded_doi = quote(doi, safe="") - url = f"{UNPAYWALL_BASE}/{encoded_doi}?email={self.email}" + encoded_email = quote(self.email, safe="") + url = f"{UNPAYWALL_BASE}/{encoded_doi}?email={encoded_email}" resp = self._http_get(url, headers={"Accept": "application/json"}) if resp.status_code == 404: diff --git a/bmlib/llm/client.py b/bmlib/llm/client.py index 1f46ef0..6e3ee80 100644 --- a/bmlib/llm/client.py +++ b/bmlib/llm/client.py @@ -70,7 +70,9 @@ def __init__( api_key: str | None = None, base_url: str | None = None, ) -> None: - self.default_provider = default_provider + # Provider keys in the registry are lowercase; normalise here so a + # mixed-case default ("Anthropic") routes and aggregates consistently. + self.default_provider = default_provider.lower() self._provider_config: dict[str, dict[str, object]] = { "anthropic": {"api_key": anthropic_api_key or api_key}, "ollama": {"base_url": ollama_host}, @@ -92,7 +94,7 @@ def _parse_model_string(self, model: str | None) -> tuple[str, str]: if model and ":" in model: provider, model_name = model.split(":", 1) return provider.lower(), model_name - provider = self.default_provider + provider = self.default_provider.lower() provider_instance = self._get_provider(provider) model_name = model or provider_instance.default_model return provider, model_name @@ -190,9 +192,7 @@ def chat( # Track token usage tracker = get_token_tracker() - cost = provider.calculate_cost( - model_name, response.input_tokens, response.output_tokens - ) + cost = provider.calculate_cost(model_name, response.input_tokens, response.output_tokens) tracker.record_usage( model=f"{provider_name}:{model_name}", input_tokens=response.input_tokens, @@ -246,7 +246,8 @@ def embed( return provider.embed(text=text, model=model_name, **kwargs) def test_connection( - self, provider: str | None = None, + self, + provider: str | None = None, ) -> bool | dict[str, tuple[bool, str]]: """Test connectivity to one or all providers.""" if provider: @@ -267,7 +268,8 @@ def test_connection( return results def list_models( - self, provider: str | None = None, + self, + provider: str | None = None, ) -> list[str] | list[ModelMetadata]: """List available models for one or all providers.""" if provider: @@ -287,12 +289,14 @@ def list_models( return all_models def get_model_metadata( - self, model: str, provider: str | None = None, + self, + model: str, + provider: str | None = None, ) -> ModelMetadata | None: """Return metadata for *model*, or ``None`` if unavailable.""" if provider is None and ":" in model: provider, model = model.split(":", 1) - provider = provider or self.default_provider + provider = (provider or self.default_provider).lower() try: p = self._get_provider(provider) return p.get_model_metadata(model) @@ -321,6 +325,18 @@ def get_provider_info(self, provider: str) -> dict[str, object]: # Helpers # --------------------------------------------------------------------------- +# Allowlist of providers known to support OpenAI-style tool calling. +# Add new providers here when their tool-calling implementation lands. +_TOOL_CAPABLE_PROVIDERS = { + "anthropic", + "openai", + "deepseek", + "mistral", + "gemini", + "ollama", +} + + def _provider_supports_tools(provider: BaseProvider) -> bool: """Return True if *provider* declares tool-calling capability. @@ -335,16 +351,6 @@ def _provider_supports_tools(provider: BaseProvider) -> bool: wasteful. Providers in the allowlist have been verified to support OpenAI-style tool calling on at least one current model. """ - # Allowlist of providers known to support OpenAI-style tool calling. - # Add new providers here when their tool-calling implementation lands. - _TOOL_CAPABLE_PROVIDERS = { - "anthropic", - "openai", - "deepseek", - "mistral", - "gemini", - "ollama", - } name = getattr(provider, "PROVIDER_NAME", "").lower() return name in _TOOL_CAPABLE_PROVIDERS diff --git a/bmlib/llm/data_types.py b/bmlib/llm/data_types.py index c20baf9..cbcc802 100644 --- a/bmlib/llm/data_types.py +++ b/bmlib/llm/data_types.py @@ -70,7 +70,7 @@ class LLMMessage: role: Literal["system", "user", "assistant", "tool"] content: str tool_call_id: str | None = None - tool_calls: list["LLMToolCall"] | None = None + tool_calls: list[LLMToolCall] | None = None @dataclass diff --git a/bmlib/llm/providers/__init__.py b/bmlib/llm/providers/__init__.py index 4233376..dd562bd 100644 --- a/bmlib/llm/providers/__init__.py +++ b/bmlib/llm/providers/__init__.py @@ -63,9 +63,7 @@ def get_provider(name: str, **kwargs: object) -> BaseProvider: _ensure_builtins() cls = _REGISTRY.get(name) if cls is None: - raise ValueError( - f"Unknown provider {name!r}. Available: {list(_REGISTRY.keys())}" - ) + raise ValueError(f"Unknown provider {name!r}. Available: {list(_REGISTRY.keys())}") return cls(**kwargs) @@ -77,6 +75,7 @@ def _ensure_builtins() -> None: # Anthropic try: from bmlib.llm.providers.anthropic import AnthropicProvider + _REGISTRY["anthropic"] = AnthropicProvider except ImportError: pass @@ -84,6 +83,7 @@ def _ensure_builtins() -> None: # Ollama try: from bmlib.llm.providers.ollama import OllamaProvider + _REGISTRY["ollama"] = OllamaProvider except ImportError: pass @@ -91,6 +91,7 @@ def _ensure_builtins() -> None: # OpenAI try: from bmlib.llm.providers.openai_provider import OpenAIProvider + _REGISTRY["openai"] = OpenAIProvider except ImportError: pass @@ -98,6 +99,7 @@ def _ensure_builtins() -> None: # DeepSeek try: from bmlib.llm.providers.deepseek import DeepSeekProvider + _REGISTRY["deepseek"] = DeepSeekProvider except ImportError: pass @@ -105,6 +107,7 @@ def _ensure_builtins() -> None: # Mistral try: from bmlib.llm.providers.mistral import MistralProvider + _REGISTRY["mistral"] = MistralProvider except ImportError: pass @@ -112,6 +115,7 @@ def _ensure_builtins() -> None: # Gemini try: from bmlib.llm.providers.gemini import GeminiProvider + _REGISTRY["gemini"] = GeminiProvider except ImportError: pass diff --git a/bmlib/llm/providers/anthropic.py b/bmlib/llm/providers/anthropic.py index db6427f..b542fca 100644 --- a/bmlib/llm/providers/anthropic.py +++ b/bmlib/llm/providers/anthropic.py @@ -123,6 +123,7 @@ def _get_client(self) -> Any: if self._client is None: try: import anthropic + kwargs: dict[str, object] = {"api_key": self._api_key} if self._base_url and self._base_url != self.default_base_url: kwargs["base_url"] = self._base_url @@ -442,4 +443,3 @@ def _convert_messages_to_anthropic( out.append({"role": msg.role, "content": msg.content}) return system_content, out - diff --git a/bmlib/llm/providers/base.py b/bmlib/llm/providers/base.py index d2c4628..5725cb5 100644 --- a/bmlib/llm/providers/base.py +++ b/bmlib/llm/providers/base.py @@ -140,9 +140,7 @@ def embed( :class:`NotImplementedError`; providers that support embeddings (e.g. Ollama) override this method. """ - raise NotImplementedError( - f"{self.PROVIDER_NAME} does not support embeddings" - ) + raise NotImplementedError(f"{self.PROVIDER_NAME} does not support embeddings") @abstractmethod def list_models(self, force_refresh: bool = False) -> list[ModelMetadata]: ... @@ -165,10 +163,9 @@ def calculate_cost( output_tokens: int, ) -> float: pricing = self.get_model_pricing(model) - return ( - (input_tokens / 1_000_000) * pricing.input_cost - + (output_tokens / 1_000_000) * pricing.output_cost - ) + return (input_tokens / 1_000_000) * pricing.input_cost + ( + output_tokens / 1_000_000 + ) * pricing.output_cost # --- Utility --- diff --git a/bmlib/llm/providers/ollama.py b/bmlib/llm/providers/ollama.py index 19220d9..82e8a7c 100644 --- a/bmlib/llm/providers/ollama.py +++ b/bmlib/llm/providers/ollama.py @@ -83,9 +83,7 @@ def __init__( base_url: str | None = None, **kwargs: object, ) -> None: - resolved_base_url = base_url or os.environ.get( - "OLLAMA_HOST", "http://localhost:11434" - ) + resolved_base_url = base_url or os.environ.get("OLLAMA_HOST", "http://localhost:11434") super().__init__(api_key=None, base_url=resolved_base_url, **kwargs) self._model_info_cache: dict[str, ModelMetadata] = {} @@ -123,11 +121,10 @@ def _get_client(self) -> Any: if self._client is None: try: import ollama + self._client = ollama.Client(host=self._base_url) except ImportError: - raise ImportError( - "ollama package not installed. Install with: pip install ollama" - ) + raise ImportError("ollama package not installed. Install with: pip install ollama") return self._client # --- Core operations --- @@ -217,22 +214,24 @@ def chat( if isinstance(args, str): try: import json as _json + args = _json.loads(args) except (ValueError, TypeError): args = {"_raw": args} if not isinstance(args, dict): args = {} - tool_calls.append( - LLMToolCall(id=str(call_id), name=str(name), arguments=args) - ) + tool_calls.append(LLMToolCall(id=str(call_id), name=str(name), arguments=args)) + # Use ``is None`` rather than truthiness: a real count of 0 (e.g. a + # fully cached prompt, or an empty/tool-only completion) is valid and + # must not be replaced by an estimate. + prompt_eval_count = _safe_get(response, "prompt_eval_count") + eval_count = _safe_get(response, "eval_count") input_tokens: int = ( - _safe_get(response, "prompt_eval_count") - or self._estimate_tokens(messages) + prompt_eval_count if prompt_eval_count is not None else self._estimate_tokens(messages) ) output_tokens: int = ( - _safe_get(response, "eval_count") - or len(content) // CHARS_PER_TOKEN_ESTIMATE + eval_count if eval_count is not None else len(content) // CHARS_PER_TOKEN_ESTIMATE ) return LLMResponse( @@ -311,9 +310,7 @@ def _get_model_info(self, model_name: str) -> ModelMetadata: context_window = _extract_context_window(info) details = _safe_get(info, "details") or {} parameter_size = _safe_get(details, "parameter_size", "") - display_name = ( - f"{model_name} ({parameter_size})" if parameter_size else model_name - ) + display_name = f"{model_name} ({parameter_size})" if parameter_size else model_name metadata = ModelMetadata( model_id=model_name, @@ -469,6 +466,7 @@ def _convert_messages_to_ollama(messages: list[LLMMessage]) -> list[dict[str, An # Pure helpers # --------------------------------------------------------------------------- + def _extract_context_window(info: Any) -> int: """Extract context window size from an ``ollama.show()`` response. diff --git a/bmlib/llm/providers/openai_compat.py b/bmlib/llm/providers/openai_compat.py index 6e6b7a9..d109913 100644 --- a/bmlib/llm/providers/openai_compat.py +++ b/bmlib/llm/providers/openai_compat.py @@ -120,9 +120,7 @@ def _get_client(self) -> Any: try: from openai import OpenAI except ImportError: - raise ImportError( - "openai package not installed. Install with: pip install openai" - ) + raise ImportError("openai package not installed. Install with: pip install openai") self._client = OpenAI( api_key=self._api_key or "unused", base_url=self._base_url, @@ -131,6 +129,15 @@ def _get_client(self) -> Any: # --- Chat --- + def _is_reasoning_model(self, model: str) -> bool: + """Whether *model* is a reasoning model with restricted parameters. + + Reasoning models (e.g. OpenAI's o-series) use ``max_completion_tokens`` + instead of ``max_tokens`` and reject non-default ``temperature``. + Overridden by providers that expose such models; defaults to ``False``. + """ + return False + def chat( self, messages: list[LLMMessage], @@ -152,11 +159,17 @@ def chat( request_kwargs: dict[str, object] = { "model": model, "messages": openai_messages, - "max_tokens": max_tokens, - "temperature": temperature, } - if top_p is not None: - request_kwargs["top_p"] = top_p + # Reasoning models (e.g. OpenAI o1/o3) reject ``max_tokens`` (they + # require ``max_completion_tokens``) and only accept the default + # temperature. Branch on a provider-overridable hook. + if self._is_reasoning_model(model): + request_kwargs["max_completion_tokens"] = max_tokens + else: + request_kwargs["max_tokens"] = max_tokens + request_kwargs["temperature"] = temperature + if top_p is not None: + request_kwargs["top_p"] = top_p if json_mode: request_kwargs["response_format"] = {"type": "json_object"} @@ -257,9 +270,7 @@ def list_models(self, force_refresh: bool = False) -> list[ModelMetadata]: self._cache_timestamp = time.time() return models except Exception as e: - logger.warning( - "Failed to fetch models from %s API: %s", self.DISPLAY_NAME, e - ) + logger.warning("Failed to fetch models from %s API: %s", self.DISPLAY_NAME, e) return list(self.FALLBACK_MODELS) diff --git a/bmlib/llm/providers/openai_provider.py b/bmlib/llm/providers/openai_provider.py index 83c3929..1ea050e 100644 --- a/bmlib/llm/providers/openai_provider.py +++ b/bmlib/llm/providers/openai_provider.py @@ -18,6 +18,8 @@ from __future__ import annotations +import re + from bmlib.llm.providers.base import ModelMetadata, ModelPricing, ProviderCapabilities from bmlib.llm.providers.openai_compat import OpenAICompatibleProvider @@ -70,3 +72,10 @@ class OpenAIProvider(OpenAICompatibleProvider): pricing=ModelPricing(input_cost=1.10, output_cost=4.40), ), ] + + def _is_reasoning_model(self, model: str) -> bool: + """OpenAI o-series (o1/o3/o4...) are reasoning models.""" + name = (model or "").lower() + # Match the o-series families (o1, o1-mini, o3, o3-mini, o4-mini, ...) + # without misfiring on names that merely start with the letter "o". + return bool(re.match(r"o\d", name)) diff --git a/bmlib/llm/token_tracker.py b/bmlib/llm/token_tracker.py index 84eb193..23c5e73 100644 --- a/bmlib/llm/token_tracker.py +++ b/bmlib/llm/token_tracker.py @@ -98,7 +98,10 @@ def record_usage( logger.debug( "Recorded usage: %s, in=%d, out=%d, cost=$%.6f", - model, input_tokens, output_tokens, cost, + model, + input_tokens, + output_tokens, + cost, ) def get_summary(self) -> TokenUsageSummary: diff --git a/bmlib/llm/utils.py b/bmlib/llm/utils.py index c171c47..a209c8d 100644 --- a/bmlib/llm/utils.py +++ b/bmlib/llm/utils.py @@ -22,15 +22,47 @@ import re +def _iter_balanced_objects(text: str): + """Yield substrings of *text* that are balanced ``{...}`` blocks. + + Brace counting respects string literals (and escaped quotes) so braces + inside JSON string values do not affect nesting. Each top-level object is + yielded in order, allowing the caller to pick the first that parses. + """ + depth = 0 + start = -1 + in_str = False + escape = False + for i, ch in enumerate(text): + if in_str: + if escape: + escape = False + elif ch == "\\": + escape = True + elif ch == '"': + in_str = False + continue + if ch == '"': + in_str = True + elif ch == "{": + if depth == 0: + start = i + depth += 1 + elif ch == "}": + if depth > 0: + depth -= 1 + if depth == 0 and start != -1: + yield text[start : i + 1] + + def extract_json(text: str) -> str: """Extract JSON from text that may contain markdown code blocks. - Tries code-block extraction first, then bare ``{...}`` matching. - Returns the original *text* unchanged if no JSON can be found. + Tries code-block extraction first, then scans for the first balanced + ``{...}`` object that parses as JSON. Returns the original *text* + unchanged if no JSON can be found. """ - code_block_match = re.search( - r"```(?:json)?\s*\n?(.*?)\n?```", text, re.DOTALL - ) + code_block_match = re.search(r"```(?:json)?\s*\n?(.*?)\n?```", text, re.DOTALL) if code_block_match: candidate = code_block_match.group(1).strip() try: @@ -39,13 +71,14 @@ def extract_json(text: str) -> str: except json.JSONDecodeError: pass - brace_match = re.search(r"\{.*\}", text, re.DOTALL) - if brace_match: - candidate = brace_match.group(0) + # Scan for the first balanced object that actually parses. A greedy + # ``\{.*\}`` would span from the first "{" to the last "}", swallowing + # prose between two separate objects and failing to parse. + for candidate in _iter_balanced_objects(text): try: json.loads(candidate) return candidate except json.JSONDecodeError: - pass + continue return text diff --git a/bmlib/publications/fetchers/biorxiv.py b/bmlib/publications/fetchers/biorxiv.py index a9fc676..0e00b89 100644 --- a/bmlib/publications/fetchers/biorxiv.py +++ b/bmlib/publications/fetchers/biorxiv.py @@ -29,7 +29,7 @@ from typing import Any from bmlib.fulltext.models import FullTextSourceEntry -from bmlib.publications.models import FetchResult, FetchedRecord, SyncProgress +from bmlib.publications.models import FetchedRecord, FetchResult, SyncProgress # --------------------------------------------------------------------------- # Constants @@ -54,19 +54,31 @@ def _normalize(raw: dict[str, Any], server: str) -> FetchedRecord: # Build full-text sources fulltext_sources: list[FullTextSourceEntry] = [] - # PDF URL derived from DOI + # PDF URL derived from DOI. Use the record's actual version rather than a + # hard-coded "v1", which 404s / points to the wrong revision for v2+. if doi: - pdf_url = f"https://www.{server}.org/content/{doi}v1.full.pdf" - fulltext_sources.append(FullTextSourceEntry( - url=pdf_url, format="pdf", source=server, open_access=True, - )) + version = str(raw.get("version") or "1").strip() or "1" + pdf_url = f"https://www.{server}.org/content/{doi}v{version}.full.pdf" + fulltext_sources.append( + FullTextSourceEntry( + url=pdf_url, + format="pdf", + source=server, + open_access=True, + ) + ) # JATS XML URL from the record jatsxml = raw.get("jatsxml", "") if jatsxml: - fulltext_sources.append(FullTextSourceEntry( - url=jatsxml, format="xml", source=server, open_access=True, - )) + fulltext_sources.append( + FullTextSourceEntry( + url=jatsxml, + format="xml", + source=server, + open_access=True, + ) + ) return FetchedRecord( title=raw.get("title", ""), diff --git a/bmlib/publications/fetchers/openalex.py b/bmlib/publications/fetchers/openalex.py index 5b1655d..bd51df0 100644 --- a/bmlib/publications/fetchers/openalex.py +++ b/bmlib/publications/fetchers/openalex.py @@ -29,7 +29,7 @@ from typing import Any from bmlib.fulltext.models import FullTextSourceEntry -from bmlib.publications.models import FetchResult, FetchedRecord, SyncProgress +from bmlib.publications.models import FetchedRecord, FetchResult, SyncProgress # --------------------------------------------------------------------------- # Constants @@ -140,17 +140,27 @@ def _normalize(raw: dict[str, Any]) -> FetchedRecord: landing_url = location.get("landing_page_url") if landing_url: - fulltext_sources.append(FullTextSourceEntry( - url=landing_url, format="html", source=loc_source, - open_access=loc_is_oa, version=version, - )) + fulltext_sources.append( + FullTextSourceEntry( + url=landing_url, + format="html", + source=loc_source, + open_access=loc_is_oa, + version=version, + ) + ) pdf_url = location.get("pdf_url") if pdf_url: - fulltext_sources.append(FullTextSourceEntry( - url=pdf_url, format="pdf", source=loc_source, - open_access=loc_is_oa, version=version, - )) + fulltext_sources.append( + FullTextSourceEntry( + url=pdf_url, + format="pdf", + source=loc_source, + open_access=loc_is_oa, + version=version, + ) + ) return FetchedRecord( title=raw.get("title") or "", diff --git a/bmlib/publications/fetchers/pubmed.py b/bmlib/publications/fetchers/pubmed.py index f5c890d..01d8299 100644 --- a/bmlib/publications/fetchers/pubmed.py +++ b/bmlib/publications/fetchers/pubmed.py @@ -31,7 +31,7 @@ from typing import Any from bmlib.fulltext.models import FullTextSourceEntry -from bmlib.publications.models import FetchResult, FetchedRecord, SyncProgress +from bmlib.publications.models import FetchedRecord, FetchResult, SyncProgress logger = logging.getLogger(__name__) @@ -100,8 +100,17 @@ def _parse_pubdate(pubdate_el: ET.Element | None) -> str | None: if month_text is None: return year - # Convert text month to numeric - month = _MONTH_MAP.get(month_text.lower().strip()[:3], month_text.zfill(2)) + # Convert text month to numeric. Accept known abbreviations or numeric + # months; for anything else (e.g. a season like "Winter"), drop the month + # rather than emit an invalid date such as "2024-Winter". + month_key = month_text.lower().strip()[:3] + stripped = month_text.strip() + if month_key in _MONTH_MAP: + month = _MONTH_MAP[month_key] + elif stripped.isdigit(): + month = stripped.zfill(2) + else: + return year day_text = _text(pubdate_el.find("Day")) if day_text is None: @@ -184,15 +193,23 @@ def _parse_article_xml(article_el: ET.Element) -> FetchedRecord: # Fulltext sources fulltext_sources: list[FullTextSourceEntry] = [] if pmc_id: - fulltext_sources.append(FullTextSourceEntry( - url=f"{PMC_BASE_URL}{pmc_id}/", source="pmc", format="html", - open_access=True, - )) + fulltext_sources.append( + FullTextSourceEntry( + url=f"{PMC_BASE_URL}{pmc_id}/", + source="pmc", + format="html", + open_access=True, + ) + ) if doi: - fulltext_sources.append(FullTextSourceEntry( - url=f"{DOI_BASE_URL}{doi}", source="publisher", format="html", - open_access=False, - )) + fulltext_sources.append( + FullTextSourceEntry( + url=f"{DOI_BASE_URL}{doi}", + source="publisher", + format="html", + open_access=False, + ) + ) return FetchedRecord( title=title or "", diff --git a/bmlib/publications/fetchers/registry.py b/bmlib/publications/fetchers/registry.py index 9ce45b3..97cd006 100644 --- a/bmlib/publications/fetchers/registry.py +++ b/bmlib/publications/fetchers/registry.py @@ -60,9 +60,7 @@ def get_source(name: str) -> tuple[SourceDescriptor, Callable[..., Any]]: _ensure_builtins() entry = _REGISTRY.get(name) if entry is None: - raise ValueError( - f"Unknown source {name!r}. Available: {sorted(_REGISTRY.keys())}" - ) + raise ValueError(f"Unknown source {name!r}. Available: {sorted(_REGISTRY.keys())}") return entry @@ -118,8 +116,12 @@ def _register_builtins() -> None: ], ), lambda client, target_date, *, on_record, on_progress=None, **config: fetch_biorxiv( - client, target_date, on_record=on_record, on_progress=on_progress, - server="biorxiv", api_key=config.get("api_key"), + client, + target_date, + on_record=on_record, + on_progress=on_progress, + server="biorxiv", + api_key=config.get("api_key"), ), ) @@ -133,8 +135,12 @@ def _register_builtins() -> None: ], ), lambda client, target_date, *, on_record, on_progress=None, **config: fetch_biorxiv( - client, target_date, on_record=on_record, on_progress=on_progress, - server="medrxiv", api_key=config.get("api_key"), + client, + target_date, + on_record=on_record, + on_progress=on_progress, + server="medrxiv", + api_key=config.get("api_key"), ), ) diff --git a/bmlib/publications/sync.py b/bmlib/publications/sync.py index 0b7a3ae..096b6fb 100644 --- a/bmlib/publications/sync.py +++ b/bmlib/publications/sync.py @@ -162,13 +162,27 @@ def _record_to_fulltext_sources(record: FetchedRecord) -> list[FullTextSource] | return None result = [] for fts in record.fulltext_sources: + # Fetchers populate ``fulltext_sources`` with ``FullTextSourceEntry`` + # dataclass instances; accept plain dicts too for robustness. + if isinstance(fts, dict): + url = fts.get("url") + source = fts.get("source", "unknown") + fmt = fts.get("format", "html") + version = fts.get("version") + else: + url = getattr(fts, "url", None) + source = getattr(fts, "source", "unknown") + fmt = getattr(fts, "format", "html") + version = getattr(fts, "version", None) + if not url: + continue result.append( FullTextSource( publication_id=0, # will be set by store_publication - source=fts.get("source", "unknown"), - url=fts["url"], - format=fts.get("format", "html"), - version=fts.get("version"), + source=source, + url=url, + format=fmt, + version=version, ) ) return result if result else None @@ -288,9 +302,7 @@ def sync( if _fetcher_override is None: import httpx - user_agent_email = ( - resolved_configs.get("openalex", {}).get("email", email) or "unknown" - ) + user_agent_email = resolved_configs.get("openalex", {}).get("email", email) or "unknown" client = httpx.Client( timeout=_HTTP_TIMEOUT_SECONDS, headers={"User-Agent": f"bmlib/0.1 (mailto:{user_agent_email})"}, @@ -351,7 +363,8 @@ def handle_record(record: FetchedRecord) -> None: on_record(record) fetch_result = fetcher( - client, day, + client, + day, on_record=handle_record, on_progress=on_progress, **src_config, diff --git a/bmlib/quality/data_models.py b/bmlib/quality/data_models.py index aa4d3f5..8f7e8c4 100644 --- a/bmlib/quality/data_models.py +++ b/bmlib/quality/data_models.py @@ -31,6 +31,7 @@ # Study design # --------------------------------------------------------------------------- + class StudyDesign(Enum): SYSTEMATIC_REVIEW = "systematic_review" META_ANALYSIS = "meta_analysis" @@ -84,15 +85,17 @@ class StudyDesign(Enum): # Quality tiers (Oxford CEBM–inspired) # --------------------------------------------------------------------------- + @total_ordering class QualityTier(Enum): """Evidence quality tier. Higher value = stronger evidence.""" + UNCLASSIFIED = 0 - TIER_1_ANECDOTAL = 1 # case reports, editorials, letters - TIER_2_OBSERVATIONAL = 2 # cross-sectional, case-control - TIER_3_CONTROLLED = 3 # cohort studies - TIER_4_EXPERIMENTAL = 4 # RCTs - TIER_5_SYNTHESIS = 5 # systematic reviews, meta-analyses + TIER_1_ANECDOTAL = 1 # case reports, editorials, letters + TIER_2_OBSERVATIONAL = 2 # cross-sectional, case-control + TIER_3_CONTROLLED = 3 # cohort studies + TIER_4_EXPERIMENTAL = 4 # RCTs + TIER_5_SYNTHESIS = 5 # systematic reviews, meta-analyses def __lt__(self, other): if self.__class__ is other.__class__: @@ -143,10 +146,12 @@ def __lt__(self, other): # Bias risk (Cochrane RoB) # --------------------------------------------------------------------------- + @dataclass class BiasRisk: """Cochrane Risk-of-Bias across five domains.""" - selection: str = "unclear" # "low", "unclear", "high" + + selection: str = "unclear" # "low", "unclear", "high" performance: str = "unclear" detection: str = "unclear" attrition: str = "unclear" @@ -164,10 +169,15 @@ def to_dict(self) -> dict[str, str]: @classmethod def from_dict(cls, data: dict) -> BiasRisk: valid = ("low", "unclear", "high") - def v(k): return data.get(k, "unclear") if data.get(k) in valid else "unclear" + + def v(k): + return data.get(k, "unclear") if data.get(k) in valid else "unclear" + return cls( - selection=v("selection"), performance=v("performance"), - detection=v("detection"), attrition=v("attrition"), + selection=v("selection"), + performance=v("performance"), + detection=v("detection"), + attrition=v("attrition"), reporting=v("reporting"), ) @@ -176,23 +186,24 @@ def v(k): return data.get(k, "unclear") if data.get(k) in valid else "unclear" # Quality assessment result # --------------------------------------------------------------------------- + @dataclass class QualityAssessment: """Result from any tier of the quality pipeline.""" - assessment_tier: int = 0 # 0=unclassified, 1=metadata, 2=haiku, 3=sonnet + assessment_tier: int = 0 # 0=unclassified, 1=metadata, 2=haiku, 3=sonnet extraction_method: str = "none" study_design: StudyDesign = StudyDesign.UNKNOWN quality_tier: QualityTier = QualityTier.UNCLASSIFIED - quality_score: float = 0.0 # 0–10 - evidence_level: str | None = None # Oxford CEBM level + quality_score: float = 0.0 # 0–10 + evidence_level: str | None = None # Oxford CEBM level is_randomized: bool | None = None is_controlled: bool | None = None - is_blinded: str | None = None # none / single / double / triple + is_blinded: str | None = None # none / single / double / triple is_prospective: bool | None = None is_multicenter: bool | None = None sample_size: int | None = None - confidence: float = 0.0 # 0–1 + confidence: float = 0.0 # 0–1 bias_risk: BiasRisk | None = None strengths: list[str] = field(default_factory=list) limitations: list[str] = field(default_factory=list) @@ -315,9 +326,11 @@ def from_dict(cls, data: dict[str, Any]) -> QualityAssessment: # Quality filter (user preferences) # --------------------------------------------------------------------------- + @dataclass class QualityFilter: """User-configurable quality filter thresholds.""" + min_tier: QualityTier | None = None require_randomization: bool = False require_blinding: bool = False diff --git a/bmlib/quality/metadata_filter.py b/bmlib/quality/metadata_filter.py index 2234698..a96cd17 100644 --- a/bmlib/quality/metadata_filter.py +++ b/bmlib/quality/metadata_filter.py @@ -48,7 +48,12 @@ "Clinical Trial, Phase IV": StudyDesign.RCT, "Pragmatic Clinical Trial": StudyDesign.RCT, "Equivalence Trial": StudyDesign.RCT, - "Multicenter Study": StudyDesign.RCT, + # NOTE: "Multicenter Study" and "Comparative Study" are deliberately NOT + # mapped. They are organisational / generic attributes, not study designs + # (a multicenter or comparative study can be an RCT, cohort, etc.), so + # mapping them to a specific design misclassified papers with high + # confidence. Records carrying only such tags fall through to LLM + # classification instead. "Observational Study": StudyDesign.COHORT_PROSPECTIVE, "Cohort Study": StudyDesign.COHORT_PROSPECTIVE, "Longitudinal Study": StudyDesign.COHORT_PROSPECTIVE, @@ -58,7 +63,6 @@ "Cross-Sectional Study": StudyDesign.CROSS_SECTIONAL, "Twin Study": StudyDesign.CROSS_SECTIONAL, "Validation Study": StudyDesign.CROSS_SECTIONAL, - "Comparative Study": StudyDesign.CROSS_SECTIONAL, "Case Reports": StudyDesign.CASE_REPORT, "Practice Guideline": StudyDesign.GUIDELINE, "Guideline": StudyDesign.GUIDELINE, @@ -83,11 +87,14 @@ "Clinical Trial, Phase II", "Clinical Trial, Phase I", "Clinical Trial", - "Case-Control Study", + # Cohort designs outrank case-control in the evidence hierarchy, so they + # must come first: a paper tagged with both should resolve to the stronger + # (cohort) design, not be downgraded to case-control. "Cohort Study", "Longitudinal Study", "Prospective Study", "Retrospective Study", + "Case-Control Study", "Cross-Sectional Study", "Observational Study", "Case Reports", @@ -105,9 +112,7 @@ def _normalize_type(s: str) -> str: # Build a case-insensitive lookup: normalized form → canonical PubMed key -_NORMALIZED_LOOKUP: dict[str, str] = { - _normalize_type(k): k for k in PUBMED_TYPE_TO_DESIGN -} +_NORMALIZED_LOOKUP: dict[str, str] = {_normalize_type(k): k for k in PUBMED_TYPE_TO_DESIGN} def classify_from_metadata( @@ -152,7 +157,5 @@ def classify_from_metadata( confidence=METADATA_HIGH_CONFIDENCE * 0.8, ) - logger.debug( - "No metadata classification match for types: %s", list(publication_types) - ) + logger.debug("No metadata classification match for types: %s", list(publication_types)) return QualityAssessment.unclassified() diff --git a/bmlib/templates/engine.py b/bmlib/templates/engine.py index 8dadf12..e015e97 100644 --- a/bmlib/templates/engine.py +++ b/bmlib/templates/engine.py @@ -47,7 +47,9 @@ def __init__( self.default_dir = default_dir def get_source( - self, environment: Environment, template: str, + self, + environment: Environment, + template: str, ) -> tuple[str, str, callable]: for directory in (self.user_dir, self.default_dir): if directory is None: diff --git a/bmlib/transparency/analyzer.py b/bmlib/transparency/analyzer.py index 18405cf..ee235a2 100644 --- a/bmlib/transparency/analyzer.py +++ b/bmlib/transparency/analyzer.py @@ -40,9 +40,18 @@ # ---- Known pharma / industry funder keywords ---- _INDUSTRY_KEYWORDS = [ - "pharma", "biotech", "therapeutics", "inc.", "corp.", "ltd.", - "gmbh", "laboratories", "employee of", "speaker fee", - "consultant for", "advisory board", + "pharma", + "biotech", + "therapeutics", + "inc.", + "corp.", + "ltd.", + "gmbh", + "laboratories", + "employee of", + "speaker fee", + "consultant for", + "advisory board", ] # ---- Rate limiting ---- @@ -68,9 +77,12 @@ # ---- COI detection patterns ---- _COI_PATTERNS = [ - "conflict of interest", "competing interest", - "no conflict", "nothing to disclose", - "declare no", "financial disclosure", + "conflict of interest", + "competing interest", + "no conflict", + "nothing to disclose", + "declare no", + "financial disclosure", ] # ---- Data availability patterns ---- @@ -137,9 +149,10 @@ def analyze( industry_funding = False industry_confidence = 0.0 data_level = "unknown" - coi_disclosed = False + coi_disclosed: bool | None = None trial_registered = False results_compliant = False + full_text_analyzed = False with httpx.Client( timeout=_HTTP_TIMEOUT_SECONDS, @@ -147,18 +160,20 @@ def analyze( ) as client: # --- CrossRef (funder info) --- if doi: - score, industry_funding, industry_confidence, indicators = ( - self._check_crossref( - client, doi, score, industry_funding, - industry_confidence, indicators, - ) + score, industry_funding, industry_confidence, indicators = self._check_crossref( + client, + doi, + score, + industry_funding, + industry_confidence, + indicators, ) - # --- EuropePMC (abstract, COI) --- + # --- EuropePMC (full text / abstract, COI, data availability) --- epmc = self._fetch_europepmc(client, pmid, doi) if epmc: - coi_disclosed, data_level, score, indicators = ( - self._check_europepmc(epmc, score, indicators) + coi_disclosed, data_level, score, indicators, full_text_analyzed = ( + self._check_europepmc(client, epmc, score, indicators) ) # --- OpenAlex (additional metadata) --- @@ -169,7 +184,11 @@ def analyze( if doi or pmid: trial_registered, results_compliant, score, indicators = ( self._check_trial_registration( - client, pmid, doi, score, indicators, + client, + pmid, + doi, + score, + indicators, ) ) @@ -194,10 +213,9 @@ def analyze( trial_registered=trial_registered, trial_results_compliant=results_compliant, risk_indicators=indicators, + full_text_analyzed=full_text_analyzed, tier_downgrade_applied=( - self.settings.tier_downgrade_amount - if risk_level == TransparencyRisk.HIGH - else 0 + self.settings.tier_downgrade_amount if risk_level == TransparencyRisk.HIGH else 0 ), ) @@ -222,18 +240,17 @@ def _check_crossref( name = (funder.get("name") or "").lower() if any(kw in name for kw in _INDUSTRY_KEYWORDS): industry_funding = True - industry_confidence = max( - industry_confidence, DEFAULT_INDUSTRY_CONFIDENCE - ) - indicators.append( - f"Industry funder: {funder.get('name')}" - ) + industry_confidence = max(industry_confidence, DEFAULT_INDUSTRY_CONFIDENCE) + indicators.append(f"Industry funder: {funder.get('name')}") else: indicators.append("No funder information in CrossRef") return score, industry_funding, industry_confidence, indicators def _fetch_europepmc( - self, client: Any, pmid: str | None, doi: str | None, + self, + client: Any, + pmid: str | None, + doi: str | None, ) -> dict | None: """Fetch a paper record from EuropePMC.""" if doi: @@ -244,32 +261,64 @@ def _fetch_europepmc( def _check_europepmc( self, + client: Any, epmc: dict, score: int, indicators: list[str], - ) -> tuple[bool, str, int, list[str]]: - """Extract COI and data-availability signals from EuropePMC.""" - coi_disclosed = False + ) -> tuple[bool | None, str, int, list[str], bool]: + """Extract COI and data-availability signals from EuropePMC. + + COI and data-availability statements live in a paper's full text, not + its abstract. We therefore fetch the full text from EuropePMC when it + is available (open-access articles) and scan that; we fall back to the + abstract only when full text cannot be retrieved. + + Returns ``(coi_disclosed, data_level, score, indicators, + full_text_analyzed)`` where ``coi_disclosed`` is tri-state: + ``True`` (statement found), ``False`` (full text scanned, none found), + or ``None`` (undeterminable — full text unavailable and no abstract + signal). + """ + coi_disclosed: bool | None = None data_level = "unknown" + full_text_analyzed = False result_list = epmc.get("resultList", {}).get("result", []) if not result_list: - return coi_disclosed, data_level, score, indicators - - abstract_text = (result_list[0].get("abstractText") or "").lower() - - # COI detection - for pat in _COI_PATTERNS: - if pat in abstract_text: - coi_disclosed = True - score += SCORE_COI_DISCLOSED - break - if not coi_disclosed: - indicators.append("No COI disclosure found") + return coi_disclosed, data_level, score, indicators, full_text_analyzed + + record = result_list[0] + abstract_text = (record.get("abstractText") or "").lower() + + # Prefer full text — COI / data-availability statements are not in the + # abstract. EuropePMC serves full text for open-access records. + search_text = abstract_text + if record.get("inEPMC") == "Y": + full_text = self._fetch_europepmc_fulltext( + client, + record.get("source"), + record.get("pmcid") or record.get("id"), + ) + if full_text: + search_text = full_text.lower() + full_text_analyzed = True + + # COI detection (a COI/disclosure statement counts as "disclosed", + # including a statement that there is nothing to declare). + if any(pat in search_text for pat in _COI_PATTERNS): + coi_disclosed = True + score += SCORE_COI_DISCLOSED + elif full_text_analyzed: + # Full text inspected and no COI statement found -> explicitly absent. + coi_disclosed = False + indicators.append("No COI disclosure found in full text") + else: + # Could not inspect full text; status is genuinely unknown. + indicators.append("COI disclosure status unknown (full text unavailable)") # Data availability for pattern, level in _DATA_PATTERNS.items(): - if pattern in abstract_text: + if pattern in search_text: data_level = level break if data_level == "full_open": @@ -279,10 +328,33 @@ def _check_europepmc( elif data_level == "not_available": indicators.append("Data explicitly not available") - return coi_disclosed, data_level, score, indicators + return coi_disclosed, data_level, score, indicators, full_text_analyzed + + def _fetch_europepmc_fulltext( + self, + client: Any, + source: str | None, + ext_id: str | None, + ) -> str | None: + """Fetch full-text XML for an open-access EuropePMC record.""" + if not source or not ext_id: + return None + self._rate_limit() + try: + resp = client.get( + f"https://www.ebi.ac.uk/europepmc/webservices/rest/{source}/{ext_id}/fullTextXML" + ) + if resp.status_code == 200: + return resp.text + except Exception as e: + logger.debug("EuropePMC full-text fetch failed for %s/%s: %s", source, ext_id, e) + return None def _check_openalex( - self, client: Any, doi: str, score: int, + self, + client: Any, + doi: str, + score: int, ) -> int: """Check open-access status and citation count via OpenAlex.""" oa = self._query_openalex(client, doi) @@ -372,7 +444,10 @@ def _query_openalex(self, client: Any, doi: str) -> dict | None: return None def _find_trial_ids( - self, client: Any, pmid: str | None, doi: str | None, + self, + client: Any, + pmid: str | None, + doi: str | None, ) -> list[str]: """Look for clinical trial IDs linked to this paper.""" ids: list[str] = [] @@ -389,16 +464,25 @@ def _find_trial_ids( return list(set(ids)) def _check_trial_results(self, client: Any, nct_id: str) -> bool: - """Check if a ClinicalTrials.gov trial has posted results.""" + """Check if a ClinicalTrials.gov trial has posted results. + + Uses the v2 API's top-level ``hasResults`` boolean. The previous + implementation requested a ``ResultsSection`` field but read a + ``resultsSection`` key, so it under-detected posted results. + """ self._rate_limit() try: resp = client.get( f"https://clinicaltrials.gov/api/v2/studies/{nct_id}", - params={"fields": "ResultsSection"}, + params={"fields": "hasResults"}, ) if resp.status_code == 200: data = resp.json() - return bool(data.get("resultsSection")) + has_results = data.get("hasResults") + if has_results is None: + # Fall back to presence of a results section in the payload. + has_results = bool(data.get("resultsSection")) + return bool(has_results) except Exception as e: logger.debug("ClinicalTrials.gov query failed for %s: %s", nct_id, e) return False diff --git a/bmlib/transparency/models.py b/bmlib/transparency/models.py index c27843b..b5b7501 100644 --- a/bmlib/transparency/models.py +++ b/bmlib/transparency/models.py @@ -29,6 +29,7 @@ class TransparencyRisk(Enum): """Risk level based on transparency analysis.""" + LOW = "low" MEDIUM = "medium" HIGH = "high" @@ -38,12 +39,13 @@ class TransparencyRisk(Enum): @dataclass class TransparencySettings: """User-configurable transparency thresholds.""" + enabled: bool = True - score_threshold: int = 40 # Below this -> HIGH risk + score_threshold: int = 40 # Below this -> HIGH risk industry_funding_triggers_downgrade: bool = True missing_coi_triggers_downgrade: bool = True tier_downgrade_amount: int = 1 - filtering_enabled: bool = False # Whether to exclude high-risk papers + filtering_enabled: bool = False # Whether to exclude high-risk papers max_concurrent_analyses: int = 3 cache_results: bool = True @@ -51,14 +53,15 @@ class TransparencySettings: @dataclass class TransparencyResult: """Result of a transparency analysis for a single document.""" + document_id: str - transparency_score: int # 0-100 + transparency_score: int # 0-100 risk_level: TransparencyRisk industry_funding_detected: bool = False industry_funding_confidence: float = 0.0 data_availability_level: str = "unknown" - coi_disclosed: bool = True + coi_disclosed: bool | None = True trial_registered: bool = False trial_results_compliant: bool = False outcome_switching_detected: bool = False @@ -120,7 +123,7 @@ def calculate_risk_level( score: int, industry_funding: bool, data_availability: str, - coi_disclosed: bool, + coi_disclosed: bool | None, settings: TransparencySettings, ) -> TransparencyRisk: """Determine risk level from transparency metrics. @@ -129,6 +132,12 @@ def calculate_risk_level( - HIGH: score < threshold OR (industry + restricted data) OR missing COI - MEDIUM: score <= 70 OR industry funding present - LOW: score > 70 and transparent + + ``coi_disclosed`` is tri-state: ``True`` (a COI statement was found), + ``False`` (full text was inspected and no COI statement exists), or + ``None`` (could not be determined — e.g. full text unavailable). Only an + *explicit* ``False`` triggers the missing-COI downgrade; ``None`` does not, + to avoid penalising papers merely because their COI status is unknown. """ if score < settings.score_threshold: return TransparencyRisk.HIGH @@ -138,7 +147,7 @@ def calculate_risk_level( if industry_funding and restricted: return TransparencyRisk.HIGH - if settings.missing_coi_triggers_downgrade and not coi_disclosed: + if settings.missing_coi_triggers_downgrade and coi_disclosed is False: return TransparencyRisk.HIGH if score <= MEDIUM_RISK_SCORE_THRESHOLD: diff --git a/pyproject.toml b/pyproject.toml index b15eef3..e8eabba 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,5 +42,11 @@ line-length = 100 [tool.ruff.lint] select = ["E", "F", "I", "N", "W", "UP"] +[tool.ruff.lint.per-file-ignores] +# xml.sax ContentHandler requires camelCase override method names. +"bmlib/fulltext/jats_parser.py" = ["N802"] +# The `StubProvider` pytest fixture returns a class, so PascalCase is intentional. +"tests/test_openai_compat.py" = ["N802", "N803"] + [tool.pytest.ini_options] testpaths = ["tests"] diff --git a/scripts/smoke_test_tool_calling.py b/scripts/smoke_test_tool_calling.py index 21ec8a0..82a5359 100644 --- a/scripts/smoke_test_tool_calling.py +++ b/scripts/smoke_test_tool_calling.py @@ -21,6 +21,7 @@ Exits 0 on success, 1 on failure, 2 on configuration problem. """ + from __future__ import annotations import json @@ -35,7 +36,6 @@ LLMToolDefinition, ) - ADD_TOOL = LLMToolDefinition( name="add", description=( @@ -147,10 +147,7 @@ def run_test(provider_name: str) -> int: expected_args = {1247, 3856} actual_args = set(call.arguments.values()) if call.arguments else set() if actual_args != expected_args: - print( - f"FAIL: tool call arguments {call.arguments!r} " - f"do not match expected {expected_args}" - ) + print(f"FAIL: tool call arguments {call.arguments!r} do not match expected {expected_args}") return 1 print(f"[dispatch] running add({call.arguments})") @@ -198,9 +195,7 @@ def run_test(provider_name: str) -> int: # the content before matching so "5,103" / "5 103" / "5103" all # compare equal. expected_sum = 5103 # 1247 + 3856 - content_stripped = ( - resp2.content.replace(",", "").replace(" ", "").replace("\u00a0", "") - ) + content_stripped = resp2.content.replace(",", "").replace(" ", "").replace("\u00a0", "") if str(expected_sum) not in content_stripped: print( f"FAIL: final response does not contain the expected sum " diff --git a/tests/test_agents.py b/tests/test_agents.py index cdf0a63..92e79f8 100644 --- a/tests/test_agents.py +++ b/tests/test_agents.py @@ -50,6 +50,19 @@ def test_invalid_json_raises(self): with pytest.raises(ValueError, match="Could not parse JSON"): BaseAgent.parse_json("not json at all") + def test_json_with_braces_in_string_value(self): + # Braces inside a string value must not break brace balancing. + text = 'Result: {"expr": "f(x) = {x}", "ok": true} done.' + result = BaseAgent.parse_json(text) + assert result["expr"] == "f(x) = {x}" + assert result["ok"] is True + + def test_picks_first_valid_object_among_prose(self): + # A broken brace pair before a valid object must not swallow it. + text = 'noise {not valid} more prose {"good": 1} trailing' + result = BaseAgent.parse_json(text) + assert result == {"good": 1} + def test_message_helpers(self): sys = BaseAgent.system_msg("sys") usr = BaseAgent.user_msg("usr") @@ -151,6 +164,7 @@ def test_empty_response_logs_warning(self, mock_sleep, caplog): ] import logging + with caplog.at_level(logging.WARNING, logger="bmlib.agents.base"): agent.chat_json([agent.user_msg("test")]) @@ -166,6 +180,7 @@ def test_unparseable_response_logs_error_with_content(self, mock_sleep, caplog): ] import logging + with caplog.at_level(logging.ERROR, logger="bmlib.agents.base"): agent.chat_json([agent.user_msg("test")]) diff --git a/tests/test_fulltext_cache.py b/tests/test_fulltext_cache.py index 776c563..4e985f9 100644 --- a/tests/test_fulltext_cache.py +++ b/tests/test_fulltext_cache.py @@ -16,8 +16,6 @@ """Tests for bmlib.fulltext.cache.""" -from pathlib import Path - from bmlib.fulltext.cache import FullTextCache PDF_MAGIC = b"%PDF-1.4 fake content" diff --git a/tests/test_fulltext_models.py b/tests/test_fulltext_models.py index bedaab9..680888b 100644 --- a/tests/test_fulltext_models.py +++ b/tests/test_fulltext_models.py @@ -18,13 +18,10 @@ from bmlib.fulltext.models import ( FullTextResult, - JATSAbstractSection, JATSArticle, JATSAuthorInfo, JATSBodySection, - JATSFigureInfo, JATSReferenceInfo, - JATSTableInfo, ) diff --git a/tests/test_fulltext_service.py b/tests/test_fulltext_service.py index ece7014..bc9420c 100644 --- a/tests/test_fulltext_service.py +++ b/tests/test_fulltext_service.py @@ -58,7 +58,8 @@ def test_404_falls_through(self): service = FullTextService(email="test@example.com") # PMC XML 404 -> search (no PDF) -> Unpaywall 404 -> DOI fallback with patch.object( - service, "_http_get", + service, + "_http_get", side_effect=[mock_404, mock_search_no_pdf, mock_unpaywall_404], ): result = service.fetch_fulltext(pmc_id="PMC123", doi="10.1/test", pmid="456") @@ -76,9 +77,7 @@ def test_discovers_pmc_id_from_doi(self): mock_search = MagicMock() mock_search.status_code = 200 mock_search.json.return_value = { - "resultList": { - "result": [{"pmcid": "PMC999", "inEPMC": "Y", "doi": "10.1/test"}] - } + "resultList": {"result": [{"pmcid": "PMC999", "inEPMC": "Y", "doi": "10.1/test"}]} } # Fulltext XML response @@ -100,9 +99,7 @@ def test_discovers_pmc_id_from_pmid(self): mock_search = MagicMock() mock_search.status_code = 200 mock_search.json.return_value = { - "resultList": { - "result": [{"pmcid": "PMC888", "inEPMC": "Y"}] - } + "resultList": {"result": [{"pmcid": "PMC888", "inEPMC": "Y"}]} } mock_xml = MagicMock() mock_xml.status_code = 200 @@ -119,16 +116,16 @@ def test_not_in_epmc_falls_through(self): mock_search = MagicMock() mock_search.status_code = 200 mock_search.json.return_value = { - "resultList": { - "result": [{"pmcid": None, "inEPMC": "N", "doi": "10.1/test"}] - } + "resultList": {"result": [{"pmcid": None, "inEPMC": "N", "doi": "10.1/test"}]} } mock_unpaywall_404 = MagicMock() mock_unpaywall_404.status_code = 404 service = FullTextService(email="test@example.com") with patch.object( - service, "_http_get", side_effect=[mock_search, mock_unpaywall_404], + service, + "_http_get", + side_effect=[mock_search, mock_unpaywall_404], ): result = service.fetch_fulltext(pmc_id=None, doi="10.1/test", pmid="") @@ -162,7 +159,8 @@ def test_success(self): service = FullTextService(email="test@example.com") with patch.object( - service, "_http_get", + service, + "_http_get", side_effect=[mock_pmc_404, mock_search_no_pdf, mock_unpaywall], ): result = service.fetch_fulltext(pmc_id="PMC123", doi="10.1/test", pmid="456") @@ -182,7 +180,9 @@ def test_no_pmc_no_unpaywall(self): service = FullTextService(email="test@example.com") with patch.object( - service, "_http_get", side_effect=[mock_search_empty, mock_unpaywall_404], + service, + "_http_get", + side_effect=[mock_search_empty, mock_unpaywall_404], ): result = service.fetch_fulltext(pmc_id=None, doi="10.1/test", pmid="456") assert result.source == "doi" @@ -203,7 +203,9 @@ def test_jats_xml_source_tried_first(self): mock_response.content = xml_data sources = [ - FullTextSourceEntry(url="https://medrxiv.org/paper.pdf", format="pdf", source="medrxiv"), + FullTextSourceEntry( + url="https://medrxiv.org/paper.pdf", format="pdf", source="medrxiv" + ), FullTextSourceEntry(url="https://medrxiv.org/jats.xml", format="xml", source="medrxiv"), ] @@ -222,7 +224,9 @@ def test_xml_fails_falls_to_pdf(self): sources = [ FullTextSourceEntry(url="https://biorxiv.org/jats.xml", format="xml", source="biorxiv"), - FullTextSourceEntry(url="https://biorxiv.org/paper.pdf", format="pdf", source="biorxiv"), + FullTextSourceEntry( + url="https://biorxiv.org/paper.pdf", format="pdf", source="biorxiv" + ), ] service = FullTextService(email="test@example.com") @@ -247,7 +251,8 @@ def test_all_known_fail_falls_to_europepmc(self): service = FullTextService(email="test@example.com") with patch.object(service, "_http_get", side_effect=[mock_fail, mock_epmc]): result = service.fetch_fulltext( - fulltext_sources=sources, pmc_id="PMC123", + fulltext_sources=sources, + pmc_id="PMC123", ) assert result.source == "europepmc" @@ -268,7 +273,9 @@ def test_no_sources_backwards_compatible(self): def test_html_source_returns_web_url(self): """HTML sources should be returned as web_url.""" sources = [ - FullTextSourceEntry(url="https://pmc.ncbi.nlm.nih.gov/PMC123/", format="html", source="pmc"), + FullTextSourceEntry( + url="https://pmc.ncbi.nlm.nih.gov/PMC123/", format="html", source="pmc" + ), ] service = FullTextService(email="test@example.com") @@ -281,8 +288,11 @@ def test_html_source_returns_web_url(self): class TestFullTextSourceEntry: def test_to_dict_and_from_dict(self): entry = FullTextSourceEntry( - url="https://example.com/paper.pdf", format="pdf", - source="biorxiv", open_access=True, version="preprint", + url="https://example.com/paper.pdf", + format="pdf", + source="biorxiv", + open_access=True, + version="preprint", ) d = entry.to_dict() assert d == { @@ -316,7 +326,8 @@ def test_no_identifiers_message(self): class TestSanitizeIdentifier: def test_doi_sanitized(self): - assert _sanitize_identifier("10.1234/test.paper-1") == "10.1234_test.paper-1" + # Readable prefix is preserved (a collision-proof hash is appended). + assert _sanitize_identifier("10.1234/test.paper-1").startswith("10.1234_test.paper-1_") def test_slashes_replaced(self): result = _sanitize_identifier("10.1101/2024.01.15.123456") @@ -324,7 +335,16 @@ def test_slashes_replaced(self): def test_safe_chars_preserved(self): result = _sanitize_identifier("simple_name-1.0") - assert result == "simple_name-1.0" + assert result.startswith("simple_name-1.0_") + + def test_distinct_identifiers_do_not_collide(self): + # Two DOIs that sanitise to the same prefix must map to different keys. + a = _sanitize_identifier("10.1/a:b") + b = _sanitize_identifier("10.1/a/b") + assert a != b + + def test_deterministic(self): + assert _sanitize_identifier("10.1/a:b") == _sanitize_identifier("10.1/a:b") class TestCacheIntegration: @@ -335,12 +355,13 @@ class TestCacheIntegration: def test_cached_html_returned_without_network(self, tmp_path): """If HTML is in the disk cache, return it immediately.""" cache = FullTextCache(cache_dir=tmp_path) - cache.save_html("

Cached

", "10.1234_test") + cache.save_html("

Cached

", _sanitize_identifier("10.1234/test")) service = FullTextService(email="test@example.com", cache=cache) with patch.object(service, "_http_get") as mock_get: result = service.fetch_fulltext( - doi="10.1234/test", identifier="10.1234/test", + doi="10.1234/test", + identifier="10.1234/test", ) mock_get.assert_not_called() @@ -350,12 +371,13 @@ def test_cached_html_returned_without_network(self, tmp_path): def test_cached_pdf_returned_without_network(self, tmp_path): """If PDF is in the disk cache, return file_path immediately.""" cache = FullTextCache(cache_dir=tmp_path) - cache.save_pdf(self.PDF_MAGIC, "10.1234_test") + cache.save_pdf(self.PDF_MAGIC, _sanitize_identifier("10.1234/test")) service = FullTextService(email="test@example.com", cache=cache) with patch.object(service, "_http_get") as mock_get: result = service.fetch_fulltext( - doi="10.1234/test", identifier="10.1234/test", + doi="10.1234/test", + identifier="10.1234/test", ) mock_get.assert_not_called() @@ -374,11 +396,12 @@ def test_fetched_jats_html_saved_to_cache(self, tmp_path): service = FullTextService(email="test@example.com", cache=cache) with patch.object(service, "_http_get", return_value=mock_response): result = service.fetch_fulltext( - pmc_id="PMC123", identifier="10.1234/test", + pmc_id="PMC123", + identifier="10.1234/test", ) assert result.source == "europepmc" - cached_html = cache.get_html("10.1234_test") + cached_html = cache.get_html(_sanitize_identifier("10.1234/test")) assert cached_html is not None assert "

" in cached_html @@ -404,11 +427,13 @@ def test_pdf_downloaded_and_cached(self, tmp_path): cache = FullTextCache(cache_dir=tmp_path) service = FullTextService(email="test@example.com", cache=cache) with patch.object( - service, "_http_get", + service, + "_http_get", side_effect=[mock_search_empty, mock_unpaywall, mock_pdf], ): result = service.fetch_fulltext( - doi="10.1234/test", identifier="10.1234/test", + doi="10.1234/test", + identifier="10.1234/test", ) assert result.source == "unpaywall" @@ -438,11 +463,13 @@ def test_invalid_pdf_rejected_keeps_url(self, tmp_path): cache = FullTextCache(cache_dir=tmp_path) service = FullTextService(email="test@example.com", cache=cache) with patch.object( - service, "_http_get", + service, + "_http_get", side_effect=[mock_search_empty, mock_unpaywall, mock_pdf], ): result = service.fetch_fulltext( - doi="10.1234/test", identifier="10.1234/test", + doi="10.1234/test", + identifier="10.1234/test", ) assert result.source == "unpaywall" @@ -474,7 +501,9 @@ def test_known_source_xml_cached(self, tmp_path): sources = [ FullTextSourceEntry( - url="https://medrxiv.org/jats.xml", format="xml", source="medrxiv", + url="https://medrxiv.org/jats.xml", + format="xml", + source="medrxiv", ), ] @@ -482,11 +511,12 @@ def test_known_source_xml_cached(self, tmp_path): service = FullTextService(email="test@example.com", cache=cache) with patch.object(service, "_http_get", return_value=mock_response): result = service.fetch_fulltext( - fulltext_sources=sources, identifier="10.1234/test", + fulltext_sources=sources, + identifier="10.1234/test", ) assert result.source == "medrxiv" - assert cache.get_html("10.1234_test") is not None + assert cache.get_html(_sanitize_identifier("10.1234/test")) is not None def test_known_source_pdf_downloaded_and_cached(self, tmp_path): """PDF from known sources is downloaded and cached.""" @@ -496,7 +526,9 @@ def test_known_source_pdf_downloaded_and_cached(self, tmp_path): sources = [ FullTextSourceEntry( - url="https://medrxiv.org/paper.pdf", format="pdf", source="medrxiv", + url="https://medrxiv.org/paper.pdf", + format="pdf", + source="medrxiv", ), ] @@ -504,7 +536,8 @@ def test_known_source_pdf_downloaded_and_cached(self, tmp_path): service = FullTextService(email="test@example.com", cache=cache) with patch.object(service, "_http_get", return_value=mock_pdf): result = service.fetch_fulltext( - fulltext_sources=sources, identifier="10.1234/test", + fulltext_sources=sources, + identifier="10.1234/test", ) assert result.source == "medrxiv" diff --git a/tests/test_jats_parser.py b/tests/test_jats_parser.py index 41477f1..c2d1eef 100644 --- a/tests/test_jats_parser.py +++ b/tests/test_jats_parser.py @@ -51,7 +51,55 @@ def test_parse_identifiers(self): assert article.doi != "" +class TestTableBuilderHeaderClassification: + def test_row_label_th_not_treated_as_header(self): + # A table without / whose first row is a data row with a + # leading row-label must NOT be misclassified as a header row. + from bmlib.fulltext.jats_parser import _TableBuilder + + b = _TableBuilder() + b.start_row() + b.start_cell(is_header=True) + b.append_cell_text("Gene") + b.end_cell() + b.start_cell(is_header=False) + b.append_cell_text("1.2") + b.end_cell() + b.end_row() + + assert b.header_rows == [] + assert len(b.body_rows) == 1 + + def test_all_th_row_is_header(self): + from bmlib.fulltext.jats_parser import _TableBuilder + + b = _TableBuilder() + b.start_row() + for text in ("Gene", "Value"): + b.start_cell(is_header=True) + b.append_cell_text(text) + b.end_cell() + b.end_row() + + assert len(b.header_rows) == 1 + assert b.body_rows == [] + + class TestJATSParserAbstract: + def test_titled_section_without_body_preserved(self): + # A structured-abstract subsection that has a title but no

body + # must still be emitted, not silently dropped. + xml = ( + b"

" + b"Background

Some text.

" + b"Conclusions" + b"
" + ) + article = JATSParser(xml).parse() + titles = [s.title for s in article.abstract_sections] + assert "Background" in titles + assert "Conclusions" in titles + def test_structured_abstract(self): data = _load_fixture("sample_article.xml") article = JATSParser(data).parse() diff --git a/tests/test_llm.py b/tests/test_llm.py index e145fa9..5065606 100644 --- a/tests/test_llm.py +++ b/tests/test_llm.py @@ -72,6 +72,7 @@ def test_recent_records(self): class TestProviderRegistry: def test_list_providers_includes_builtins(self): from bmlib.llm.providers import list_providers + # Even without the actual packages installed, the registry # should at least attempt to register them. If neither # anthropic nor ollama is installed, the list may be empty — @@ -83,5 +84,61 @@ def test_unknown_provider_raises(self): import pytest from bmlib.llm.providers import get_provider + with pytest.raises(ValueError, match="Unknown provider"): get_provider("nonexistent_provider_xyz") + + +class TestOllamaTokenAccounting: + """Real token counts of 0 must not be replaced by estimates.""" + + class _FakeOllamaClient: + def __init__(self, response): + self._response = response + + def chat(self, **kwargs): + return self._response + + def _make_provider(self, response): + from bmlib.llm.providers.ollama import OllamaProvider + + provider = OllamaProvider() + provider._client = self._FakeOllamaClient(response) + return provider + + def test_zero_counts_preserved(self): + response = { + "message": {"content": "hi"}, + "prompt_eval_count": 0, + "eval_count": 0, + } + provider = self._make_provider(response) + resp = provider.chat([LLMMessage(role="user", content="hello")]) + assert resp.input_tokens == 0 + assert resp.output_tokens == 0 + + def test_missing_counts_estimated(self): + response = {"message": {"content": "hi"}} # no counts at all + provider = self._make_provider(response) + resp = provider.chat([LLMMessage(role="user", content="hello world")]) + # Falls back to an estimate (> 0) when the field is absent. + assert resp.input_tokens > 0 + + +class TestModelStringParsing: + def test_default_provider_normalised_to_lowercase(self): + from bmlib.llm.client import LLMClient + + client = LLMClient(default_provider="Anthropic") + assert client.default_provider == "anthropic" + # A bare model name routes to the (normalised) default provider. + provider, _model = client._parse_model_string(None) + assert provider == "anthropic" + + def test_colon_form_lowercases_provider(self): + from bmlib.llm.client import LLMClient + + client = LLMClient() + provider, model = client._parse_model_string("Anthropic:claude-x") + assert provider == "anthropic" + assert model == "claude-x" diff --git a/tests/test_llm_tools.py b/tests/test_llm_tools.py index cf40597..81c3fbd 100644 --- a/tests/test_llm_tools.py +++ b/tests/test_llm_tools.py @@ -38,7 +38,6 @@ LLMToolDefinition, ) - # =========================================================================== # 1. DATA TYPE TESTS # =========================================================================== @@ -227,9 +226,7 @@ def test_messages_assistant_with_tool_calls_emits_tool_use_blocks(self): LLMMessage( role="assistant", content="Let me add those.", - tool_calls=[ - LLMToolCall(id="toolu_01", name="add", arguments={"a": 2, "b": 3}) - ], + tool_calls=[LLMToolCall(id="toolu_01", name="add", arguments={"a": 2, "b": 3})], ), ] _, out = _convert_messages_to_anthropic(msgs) @@ -347,9 +344,7 @@ def test_messages_tool_role_with_id(self): msgs = [LLMMessage(role="tool", content="5", tool_call_id="call_0")] out = _convert_messages_to_ollama(msgs) - assert out == [ - {"role": "tool", "content": "5", "tool_call_id": "call_0"} - ] + assert out == [{"role": "tool", "content": "5", "tool_call_id": "call_0"}] def test_messages_tool_role_without_id(self): from bmlib.llm.providers.ollama import _convert_messages_to_ollama @@ -368,11 +363,7 @@ def test_messages_assistant_with_tool_calls(self): LLMMessage( role="assistant", content="", - tool_calls=[ - LLMToolCall( - id="call_0", name="add", arguments={"a": 2, "b": 3} - ) - ], + tool_calls=[LLMToolCall(id="call_0", name="add", arguments={"a": 2, "b": 3})], ), ] out = _convert_messages_to_ollama(msgs) @@ -457,9 +448,7 @@ def test_messages_tool_role_requires_tool_call_id(self): msgs = [LLMMessage(role="tool", content="5", tool_call_id="call_0")] out = _convert_messages_to_openai(msgs) - assert out == [ - {"role": "tool", "content": "5", "tool_call_id": "call_0"} - ] + assert out == [{"role": "tool", "content": "5", "tool_call_id": "call_0"}] def test_messages_assistant_tool_calls_arguments_serialised_to_json_string(self): from bmlib.llm.providers.openai_compat import _convert_messages_to_openai @@ -468,11 +457,7 @@ def test_messages_assistant_tool_calls_arguments_serialised_to_json_string(self) LLMMessage( role="assistant", content="", - tool_calls=[ - LLMToolCall( - id="call_0", name="add", arguments={"a": 2, "b": 3} - ) - ], + tool_calls=[LLMToolCall(id="call_0", name="add", arguments={"a": 2, "b": 3})], ), ] out = _convert_messages_to_openai(msgs) @@ -488,9 +473,7 @@ def test_messages_assistant_only_tool_calls_has_none_content(self): LLMMessage( role="assistant", content="", - tool_calls=[ - LLMToolCall(id="x", name="ping", arguments={}) - ], + tool_calls=[LLMToolCall(id="x", name="ping", arguments={})], ), ] out = _convert_messages_to_openai(msgs) @@ -575,9 +558,7 @@ def test_chat_parses_tool_use_blocks_into_tool_calls(self): tool_name="add", tool_input={"a": 5, "b": 7} ) - tool = LLMToolDefinition( - name="add", description="Add", parameters={} - ) + tool = LLMToolDefinition(name="add", description="Add", parameters={}) msgs = [LLMMessage(role="user", content="Add 5 and 7")] result = p.chat(msgs, model="claude-sonnet-4-20250514", tools=[tool]) diff --git a/tests/test_migrations.py b/tests/test_migrations.py index fe6a945..6b6f4cf 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -21,8 +21,8 @@ from bmlib.db import ( connect_sqlite, create_tables, - table_exists, fetch_scalar, + table_exists, ) from bmlib.db.migrations import Migration, get_applied_versions, run_migrations @@ -77,6 +77,51 @@ def test_empty_migration_list(self): assert table_exists(conn, "schema_version") +class TestSplitSqlStatements: + def test_splits_respecting_comments_and_strings(self): + from bmlib.db.operations import _split_sql_statements + + script = ( + "CREATE TABLE a (id INTEGER); -- trailing; comment\n" + "CREATE INDEX i ON a(id);\n" + "/* block; comment */ INSERT INTO a VALUES (1);\n" + "SELECT ';' AS x;" + ) + out = _split_sql_statements(script) + assert out == [ + "CREATE TABLE a (id INTEGER)", + "CREATE INDEX i ON a(id)", + "INSERT INTO a VALUES (1)", + "SELECT ';' AS x", + ] + + def test_handles_escaped_quotes(self): + from bmlib.db.operations import _split_sql_statements + + out = _split_sql_statements("SELECT 'it''s; ok' AS v; SELECT 2;") + assert out == ["SELECT 'it''s; ok' AS v", "SELECT 2"] + + +class TestMigrationAtomicity: + def test_failed_migration_rolls_back_ddl(self): + """If a migration raises after creating a table, the DDL must roll back + and the version must not be recorded (no half-applied migration).""" + import pytest + + def _bad(conn): + create_tables(conn, "CREATE TABLE t_bad (id INTEGER PRIMARY KEY);") + raise RuntimeError("boom after DDL") + + conn = _mem() + with pytest.raises(RuntimeError, match="boom after DDL"): + run_migrations(conn, [Migration(1, "bad", _bad)]) + + # The table created inside the migration must have been rolled back. + assert not table_exists(conn, "t_bad") + # And the migration must not be marked as applied. + assert get_applied_versions(conn) == set() + + class TestGetAppliedVersions: def test_empty_on_fresh_db(self): conn = _mem() @@ -104,7 +149,5 @@ class TestVersionTableTracking: def test_version_names_recorded(self): conn = _mem() run_migrations(conn, SAMPLE_MIGRATIONS) - name = fetch_scalar( - conn, "SELECT name FROM schema_version WHERE version = ?", (1,) - ) + name = fetch_scalar(conn, "SELECT name FROM schema_version WHERE version = ?", (1,)) assert name == "create_t1" diff --git a/tests/test_openai_compat.py b/tests/test_openai_compat.py index cefab6b..f35338e 100644 --- a/tests/test_openai_compat.py +++ b/tests/test_openai_compat.py @@ -10,12 +10,12 @@ from __future__ import annotations -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock import pytest from bmlib.llm.data_types import LLMMessage -from bmlib.llm.providers.base import ModelMetadata, ModelPricing, ProviderCapabilities +from bmlib.llm.providers.base import ModelMetadata, ModelPricing class _StubProvider: @@ -97,6 +97,40 @@ def test_chat_routes_to_openai_sdk(self, StubProvider): call_kwargs = mock_client.chat.completions.create.call_args assert call_kwargs.kwargs["model"] == "stub-model" + def test_reasoning_model_uses_max_completion_tokens(self, StubProvider): + from bmlib.llm.providers.openai_compat import OpenAICompatibleProvider + + class _Reasoning(_StubProvider, OpenAICompatibleProvider): + def _is_reasoning_model(self, model): + return True + + p = _Reasoning(api_key="k") + mock_response = MagicMock() + mock_response.choices = [MagicMock()] + mock_response.choices[0].message.content = "ok" + mock_response.choices[0].message.tool_calls = None + mock_response.usage.prompt_tokens = 1 + mock_response.usage.completion_tokens = 1 + mock_response.choices[0].finish_reason = "stop" + mock_client = MagicMock() + mock_client.chat.completions.create.return_value = mock_response + p._client = mock_client + + p.chat([LLMMessage(role="user", content="hi")], temperature=0.7, max_tokens=99) + + kwargs = mock_client.chat.completions.create.call_args.kwargs + assert kwargs["max_completion_tokens"] == 99 + assert "max_tokens" not in kwargs + assert "temperature" not in kwargs + + def test_openai_provider_detects_reasoning_models(self): + from bmlib.llm.providers.openai_provider import OpenAIProvider + + p = OpenAIProvider(api_key="k") + assert p._is_reasoning_model("o1") is True + assert p._is_reasoning_model("o3-mini") is True + assert p._is_reasoning_model("gpt-4o") is False + def test_chat_separates_system_message(self, StubProvider): p = StubProvider(api_key="test-key") diff --git a/tests/test_publications.py b/tests/test_publications.py index f2f312d..94a4908 100644 --- a/tests/test_publications.py +++ b/tests/test_publications.py @@ -28,6 +28,7 @@ from bmlib.publications.fetchers.biorxiv import PAGE_SIZE, _normalize, fetch_biorxiv from bmlib.publications.models import ( DownloadDay, + FetchedRecord, FetchResult, FullTextSource, Publication, @@ -41,7 +42,6 @@ get_publication_by_pmid, store_publication, ) -from bmlib.publications.models import FetchedRecord from bmlib.publications.sync import _record_to_fulltext_sources # --------------------------------------------------------------------------- @@ -628,6 +628,14 @@ def test_normalize_builds_fulltext_sources(self): assert xml.format == "xml" assert "source.xml" in xml.url + def test_normalize_uses_record_version_for_pdf_url(self): + # A revised preprint (v2) must produce a v2 PDF URL, not a stale v1. + raw = _sample_record(doi="10.1101/2024.01.01.000001") + raw["version"] = 2 + result = _normalize(raw, "biorxiv") + pdf = result.fulltext_sources[0] + assert pdf.url == ("https://www.biorxiv.org/content/10.1101/2024.01.01.000001v2.full.pdf") + def test_normalize_sets_source(self): raw = _sample_record() result = _normalize(raw, "medrxiv") diff --git a/tests/test_pubmed_fetcher.py b/tests/test_pubmed_fetcher.py index e7bcc9d..1ac80d3 100644 --- a/tests/test_pubmed_fetcher.py +++ b/tests/test_pubmed_fetcher.py @@ -210,6 +210,33 @@ def test_numeric_month(self): assert isinstance(result, FetchedRecord) assert result.publication_date == "2024-03-05" + def test_season_month_falls_back_to_year(self): + """A non-numeric, non-month value (e.g. a season) must not produce an + invalid date like '2024-Winter' — fall back to year only.""" + xml = """\ + + + 22222222 +
+ Season month test + + Test Journal + + + 2024 + Winter + + + +
+
+ +
+ """ + el = ET.fromstring(xml) + result = _parse_article_xml(el) + assert result.publication_date == "2024" + def test_medline_date_fallback(self): """When Year is missing, MedlineDate is used as fallback (first 4 chars).""" xml = """\ diff --git a/tests/test_quality.py b/tests/test_quality.py index f3ebc17..f0f4f58 100644 --- a/tests/test_quality.py +++ b/tests/test_quality.py @@ -44,8 +44,13 @@ def test_ordering(self): class TestBiasRisk: def test_roundtrip(self): - br = BiasRisk(selection="low", performance="high", detection="unclear", - attrition="low", reporting="high") + br = BiasRisk( + selection="low", + performance="high", + detection="unclear", + attrition="low", + reporting="high", + ) d = br.to_dict() br2 = BiasRisk.from_dict(d) assert br2.selection == "low" @@ -71,7 +76,9 @@ def test_from_metadata(self): def test_from_classification(self): a = QualityAssessment.from_classification( - StudyDesign.COHORT_PROSPECTIVE, confidence=0.75, sample_size=500, + StudyDesign.COHORT_PROSPECTIVE, + confidence=0.75, + sample_size=500, ) assert a.quality_tier == QualityTier.TIER_3_CONTROLLED assert a.assessment_tier == 2 @@ -98,9 +105,13 @@ def test_serialisation_roundtrip(self): quality_tier=QualityTier.TIER_4_EXPERIMENTAL, quality_score=8.0, confidence=0.85, - bias_risk=BiasRisk(selection="low", performance="low", - detection="unclear", attrition="low", - reporting="low"), + bias_risk=BiasRisk( + selection="low", + performance="low", + detection="unclear", + attrition="low", + reporting="low", + ), strengths=["Large sample"], limitations=["Single center"], ) @@ -144,13 +155,35 @@ def test_hyphenated_type(self): def test_mixed_case_with_noise(self): # EuropePMC categories include journal name as noise - result = classify_from_metadata([ - "European Radiology", "Systematic Review", "Meta-Analysis", - ]) + result = classify_from_metadata( + [ + "European Radiology", + "Systematic Review", + "Meta-Analysis", + ] + ) assert result.study_design == StudyDesign.SYSTEMATIC_REVIEW def test_lowercase_from_categories(self): - result = classify_from_metadata([ - "some journal", "randomized controlled trial", - ]) + result = classify_from_metadata( + [ + "some journal", + "randomized controlled trial", + ] + ) assert result.study_design == StudyDesign.RCT + + def test_cohort_outranks_case_control(self): + # A paper tagged with both should resolve to the stronger cohort design. + result = classify_from_metadata(["Case-Control Study", "Cohort Study"]) + assert result.study_design == StudyDesign.COHORT_PROSPECTIVE + + def test_multicenter_study_not_classified_as_rct(self): + # "Multicenter Study" is an organisational attribute, not a design. + result = classify_from_metadata(["Multicenter Study"]) + assert result.quality_tier == QualityTier.UNCLASSIFIED + + def test_comparative_study_not_classified(self): + # "Comparative Study" is a generic tag, not a specific design. + result = classify_from_metadata(["Comparative Study"]) + assert result.quality_tier == QualityTier.UNCLASSIFIED diff --git a/tests/test_sync.py b/tests/test_sync.py index d57f5ab..30cebba 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -21,6 +21,7 @@ from datetime import UTC, date, datetime, timedelta from bmlib.db import connect_sqlite, execute +from bmlib.fulltext.models import FullTextSourceEntry from bmlib.publications.fetchers import ALL_SOURCES from bmlib.publications.models import FetchedRecord, FetchResult from bmlib.publications.schema import ensure_schema @@ -90,7 +91,7 @@ def _sample_raw_record(doi="10.1234/test.001", title="Test Paper", source="pubme license=None, source=source, fulltext_sources=[ - {"url": f"https://example.com/{doi}.pdf", "format": "pdf", "source": source}, + FullTextSourceEntry(url=f"https://example.com/{doi}.pdf", format="pdf", source=source), ], ) @@ -200,6 +201,19 @@ def test_sync_stores_records(self): assert pub2 is not None assert pub2.title == "Sync Paper 2" + # Verify the full-text source (a FullTextSourceEntry dataclass) was + # extracted and stored, not silently dropped. + from bmlib.db import fetch_all + + fts_rows = fetch_all( + conn, + "SELECT * FROM fulltext_sources WHERE publication_id = ?", + (pub1.id,), + ) + assert len(fts_rows) == 1 + assert fts_rows[0]["url"] == "https://example.com/10.1234/sync.001.pdf" + assert fts_rows[0]["format"] == "pdf" + # Verify download_days was updated from bmlib.db import fetch_one diff --git a/tests/test_templates.py b/tests/test_templates.py index 8af31ee..d978f76 100644 --- a/tests/test_templates.py +++ b/tests/test_templates.py @@ -77,9 +77,7 @@ def test_has_template(tmp_path): def test_jinja_conditionals(tmp_path): d = tmp_path / "d" d.mkdir() - (d / "cond.txt").write_text( - "{% if include_methods %}Methods: {{ methods }}{% endif %}" - ) + (d / "cond.txt").write_text("{% if include_methods %}Methods: {{ methods }}{% endif %}") engine = TemplateEngine(default_dir=d) assert engine.render("cond.txt", include_methods=True, methods="RCT") == "Methods: RCT" @@ -89,9 +87,7 @@ def test_jinja_conditionals(tmp_path): def test_jinja_loops(tmp_path): d = tmp_path / "d" d.mkdir() - (d / "loop.txt").write_text( - "{% for item in items %}- {{ item }}\n{% endfor %}" - ) + (d / "loop.txt").write_text("{% for item in items %}- {{ item }}\n{% endfor %}") engine = TemplateEngine(default_dir=d) result = engine.render("loop.txt", items=["a", "b", "c"]) diff --git a/tests/test_transparency.py b/tests/test_transparency.py index e9d2137..124056b 100644 --- a/tests/test_transparency.py +++ b/tests/test_transparency.py @@ -18,6 +18,7 @@ from __future__ import annotations +from bmlib.transparency.analyzer import TransparencyAnalyzer from bmlib.transparency.models import ( TransparencyResult, TransparencyRisk, @@ -26,56 +27,190 @@ ) +class _FakeResponse: + def __init__(self, status_code=200, json_data=None, text=""): + self.status_code = status_code + self._json = json_data or {} + self.text = text + + def json(self): + return self._json + + +class _FakeFullTextClient: + """A fake httpx client that serves a single full-text XML body.""" + + def __init__(self, full_text: str | None): + self._full_text = full_text + + def get(self, url, **kwargs): + if url.endswith("/fullTextXML"): + if self._full_text is None: + return _FakeResponse(status_code=404, text="") + return _FakeResponse(status_code=200, text=self._full_text) + return _FakeResponse(status_code=404) + + class TestTransparencyRisk: def test_high_risk_low_score(self): settings = TransparencySettings(score_threshold=40) risk = calculate_risk_level( - score=20, industry_funding=False, data_availability="full_open", - coi_disclosed=True, settings=settings, + score=20, + industry_funding=False, + data_availability="full_open", + coi_disclosed=True, + settings=settings, ) assert risk == TransparencyRisk.HIGH def test_high_risk_industry_restricted(self): settings = TransparencySettings(industry_funding_triggers_downgrade=True) risk = calculate_risk_level( - score=60, industry_funding=True, data_availability="restricted", - coi_disclosed=True, settings=settings, + score=60, + industry_funding=True, + data_availability="restricted", + coi_disclosed=True, + settings=settings, ) assert risk == TransparencyRisk.HIGH def test_high_risk_missing_coi(self): settings = TransparencySettings(missing_coi_triggers_downgrade=True) risk = calculate_risk_level( - score=80, industry_funding=False, data_availability="full_open", - coi_disclosed=False, settings=settings, + score=80, + industry_funding=False, + data_availability="full_open", + coi_disclosed=False, + settings=settings, ) assert risk == TransparencyRisk.HIGH + def test_unknown_coi_does_not_trigger_downgrade(self): + """coi_disclosed=None (undeterminable) must NOT force HIGH risk.""" + settings = TransparencySettings(missing_coi_triggers_downgrade=True) + risk = calculate_risk_level( + score=80, + industry_funding=False, + data_availability="full_open", + coi_disclosed=None, + settings=settings, + ) + assert risk == TransparencyRisk.LOW + def test_medium_risk_borderline(self): settings = TransparencySettings() risk = calculate_risk_level( - score=60, industry_funding=False, data_availability="full_open", - coi_disclosed=True, settings=settings, + score=60, + industry_funding=False, + data_availability="full_open", + coi_disclosed=True, + settings=settings, ) assert risk == TransparencyRisk.MEDIUM def test_medium_risk_industry(self): settings = TransparencySettings() risk = calculate_risk_level( - score=80, industry_funding=True, data_availability="full_open", - coi_disclosed=True, settings=settings, + score=80, + industry_funding=True, + data_availability="full_open", + coi_disclosed=True, + settings=settings, ) assert risk == TransparencyRisk.MEDIUM def test_low_risk(self): settings = TransparencySettings() risk = calculate_risk_level( - score=85, industry_funding=False, data_availability="full_open", - coi_disclosed=True, settings=settings, + score=85, + industry_funding=False, + data_availability="full_open", + coi_disclosed=True, + settings=settings, ) assert risk == TransparencyRisk.LOW +class TestCheckEuropePMC: + """Tests that COI/data-availability are read from full text, not abstract.""" + + def _epmc(self, in_epmc="Y", abstract=""): + return { + "resultList": { + "result": [ + { + "abstractText": abstract, + "inEPMC": in_epmc, + "source": "PMC", + "pmcid": "PMC123", + } + ] + } + } + + def test_coi_detected_in_full_text(self): + analyzer = TransparencyAnalyzer() + client = _FakeFullTextClient( + "
The authors declare no conflict of interest.
" + ) + coi, _level, score, _ind, ft = analyzer._check_europepmc( + client, self._epmc(), score=0, indicators=[] + ) + assert coi is True + assert ft is True + assert score == 10 # SCORE_COI_DISCLOSED + + def test_coi_absent_in_full_text(self): + analyzer = TransparencyAnalyzer() + client = _FakeFullTextClient("
No disclosure section here.
") + coi, _level, _score, _ind, ft = analyzer._check_europepmc( + client, self._epmc(), score=0, indicators=[] + ) + assert coi is False # full text scanned, explicitly absent + assert ft is True + + def test_coi_unknown_when_no_full_text(self): + analyzer = TransparencyAnalyzer() + # inEPMC == "N" so no full text is fetched, abstract has no COI signal. + client = _FakeFullTextClient(None) + coi, _level, _score, _ind, ft = analyzer._check_europepmc( + client, self._epmc(in_epmc="N"), score=0, indicators=[] + ) + assert coi is None # undeterminable, not "absent" + assert ft is False + + +class TestCheckTrialResults: + """Tests that posted-results detection reads the correct v2 API field.""" + + def test_has_results_true(self): + analyzer = TransparencyAnalyzer() + + class _Client: + def get(self, url, **kwargs): + return _FakeResponse(status_code=200, json_data={"hasResults": True}) + + assert analyzer._check_trial_results(_Client(), "NCT12345678") is True + + def test_has_results_false(self): + analyzer = TransparencyAnalyzer() + + class _Client: + def get(self, url, **kwargs): + return _FakeResponse(status_code=200, json_data={"hasResults": False}) + + assert analyzer._check_trial_results(_Client(), "NCT12345678") is False + + def test_results_section_fallback(self): + analyzer = TransparencyAnalyzer() + + class _Client: + def get(self, url, **kwargs): + return _FakeResponse(status_code=200, json_data={"resultsSection": {"x": 1}}) + + assert analyzer._check_trial_results(_Client(), "NCT12345678") is True + + class TestTransparencyResult: def test_roundtrip(self): result = TransparencyResult(