From 2c65d6f94c4090321bf0d5c68371b647602bcb92 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:44:55 +0000 Subject: [PATCH 01/13] Fix silent loss of full-text sources during sync _record_to_fulltext_sources treated each entry as a dict (.get()/[]), but fetchers populate FetchedRecord.fulltext_sources with FullTextSourceEntry dataclass instances. Every record carrying a full-text source raised AttributeError inside handle_record, was caught, counted as failed, and the publication was never stored. Use attribute access (with dict fallback for robustness) and fix the test fixture to use the real FullTextSourceEntry type so the regression is actually exercised. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/publications/sync.py | 29 +++++++++++++++++++++-------- tests/test_sync.py | 16 +++++++++++++++- 2 files changed, 36 insertions(+), 9 deletions(-) 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/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 From ad80a5f38a254d214f8daca5502ba8417f689b05 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:48:12 +0000 Subject: [PATCH 02/13] Fix transparency COI detection and trial-results check COI: COI/data-availability statements live in full text, not the abstract, so reading them from abstractText produced near-universal false negatives that drove almost every paper to HIGH risk via the missing-COI downgrade. Now fetch EuropePMC full text when available and scan that, and make coi_disclosed tri-state (True/False/None). Only an explicit False (full text inspected, no statement) triggers the downgrade; None (undeterminable) no longer penalises the paper. Trial results: the ClinicalTrials.gov v2 check requested a 'ResultsSection' field but read a 'resultsSection' key, under-detecting posted results. Use the top-level 'hasResults' boolean with a resultsSection fallback. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/transparency/analyzer.py | 180 ++++++++++++++++++++++++--------- bmlib/transparency/models.py | 21 ++-- tests/test_transparency.py | 159 ++++++++++++++++++++++++++--- 3 files changed, 294 insertions(+), 66 deletions(-) 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/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( From 44a061d7c24e5c2bd57426d8367d6cfbebe69260 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:49:19 +0000 Subject: [PATCH 03/13] Fix Ollama token accounting clobbering legitimate zero counts input/output token counts used 'value or estimate', so a real count of 0 (fully cached prompt, or empty/tool-only completion) was discarded and replaced by a fabricated char-based estimate, corrupting usage/cost accounting. Use 'is None' to only estimate when the field is absent. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/llm/providers/ollama.py | 30 +++++++++++++-------------- tests/test_llm.py | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 16 deletions(-) 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/tests/test_llm.py b/tests/test_llm.py index e145fa9..eeae547 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,42 @@ 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 From 6c5fbbf95f096fde3c3ecf4e600e1c3e31769a8c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:50:35 +0000 Subject: [PATCH 04/13] Fix study-design misclassification in metadata filter - Reorder TYPE_PRIORITY so cohort designs precede case-control. A paper tagged with both was resolved to the lower-evidence case-control design (Tier 2) instead of the stronger cohort design (Tier 3). - Drop 'Multicenter Study' -> RCT and 'Comparative Study' -> cross-sectional mappings. These are organisational/generic attributes, not study designs; mapping them misclassified observational papers (e.g. multicenter cohorts) as RCTs with high confidence. Such records now fall through to LLM classification. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/quality/metadata_filter.py | 21 +++++++----- tests/test_quality.py | 57 +++++++++++++++++++++++++------- 2 files changed, 57 insertions(+), 21 deletions(-) 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/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 From f2418e1d148fc7203cf47f78d5186e5dff0a281a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:52:52 +0000 Subject: [PATCH 05/13] Make SQLite migrations atomic by avoiding executescript sqlite3.executescript() issues an implicit COMMIT before running, which committed the BEGIN opened by transaction() mid-migration. A migration whose DDL succeeded but whose version-insert (or later DDL) failed left a half-applied schema that could not roll back and re-ran next time. create_tables now executes statements individually via a cursor (SQLite supports transactional DDL), so schema changes stay inside the active transaction and roll back cleanly on failure. A small comment/-string- aware splitter handles the multi-statement schema strings; standalone callers still commit when no transaction is active. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/db/operations.py | 74 +++++++++++++++++++++++++++++++++++++--- tests/test_migrations.py | 51 ++++++++++++++++++++++++--- 2 files changed, 117 insertions(+), 8 deletions(-) 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/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" From 54a0be90633da4965407a7e0c10f7ca4724b487c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:55:29 +0000 Subject: [PATCH 06/13] Fix full-text cache key collisions and result mislabels - _sanitize_identifier mapped every non-[\w.\-] char to '_', so distinct identifiers (e.g. '10.1/a:b' and '10.1/a/b') collided to the same cache file and could serve the wrong article's full text. Append a short SHA-1 of the raw identifier so keys are collision-free (readable prefix kept). - The PubMed URL fallback returned source='doi' (with a PubMed URL); label it source='pubmed' and document the value. - URL-encode the Unpaywall contact email so '+'/'&' in an address don't corrupt the query string. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/fulltext/models.py | 2 +- bmlib/fulltext/service.py | 18 ++++++++++++++---- tests/test_fulltext_service.py | 24 ++++++++++++++++++------ 3 files changed, 33 insertions(+), 11 deletions(-) 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..51bea43 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: @@ -202,7 +211,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") @@ -358,7 +367,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/tests/test_fulltext_service.py b/tests/test_fulltext_service.py index ece7014..85a9fe3 100644 --- a/tests/test_fulltext_service.py +++ b/tests/test_fulltext_service.py @@ -316,7 +316,10 @@ 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 +327,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,7 +347,7 @@ 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: @@ -350,7 +362,7 @@ 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: @@ -378,7 +390,7 @@ def test_fetched_jats_html_saved_to_cache(self, tmp_path): ) 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 @@ -486,7 +498,7 @@ def test_known_source_xml_cached(self, tmp_path): ) 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.""" From 84df67ae8126d479918cf9b6a390580349a30dd7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:57:56 +0000 Subject: [PATCH 07/13] Fix JATS parser: table headers, dropped abstract sections, captions - Table header heuristic now requires *all* cells in a row to be header cells before treating a header-less table's first row as a header. Previously a normal data row that merely started with a single row-label was misclassified into . - Structured-abstract subsections with a title but no

body (and title-only abstracts) are no longer silently dropped on flush. - Multi-paragraph figure/table captions are now joined with a space instead of being glued together with no separator. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/fulltext/jats_parser.py | 32 +++++++++++++++++++---- tests/test_jats_parser.py | 48 +++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/bmlib/fulltext/jats_parser.py b/bmlib/fulltext/jats_parser.py index f0b57dd..203d84f 100644 --- a/bmlib/fulltext/jats_parser.py +++ b/bmlib/fulltext/jats_parser.py @@ -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,13 +133,22 @@ 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: + # 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 ( - self.current_row_has_header_cells - and not self.in_body - and not self.header_rows + all_header_cells and not self.in_body and not self.header_rows ): self.header_rows.append(self.current_row) else: @@ -145,13 +156,17 @@ def end_row(self) -> None: 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: @@ -573,7 +588,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,7 +599,7 @@ 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( @@ -599,8 +617,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": 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() From 4b3f5dca09d3e870f8d24bee654527d98827b1f5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 22:58:52 +0000 Subject: [PATCH 08/13] Normalise LLM provider casing in client routing Provider registry keys are lowercase, but a mixed-case default_provider (or a 'Provider:model' string) was passed through un-normalised in the no-colon branch of _parse_model_string and in get_model_metadata. This desynced routing, per-model cost aggregation, and the tool-capability allowlist (e.g. 'Anthropic' would wrongly fail the tool check). Lowercase default_provider once in __init__ and normalise the parsed provider. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/llm/client.py | 8 +++++--- tests/test_llm.py | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/bmlib/llm/client.py b/bmlib/llm/client.py index 1f46ef0..7d9ba78 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 @@ -292,7 +294,7 @@ def get_model_metadata( """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) diff --git a/tests/test_llm.py b/tests/test_llm.py index eeae547..5065606 100644 --- a/tests/test_llm.py +++ b/tests/test_llm.py @@ -123,3 +123,22 @@ def test_missing_counts_estimated(self): 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" From 6dd4a818728627814b6917deb2a7671b888e0338 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 23:00:24 +0000 Subject: [PATCH 09/13] Fix bioRxiv PDF version and PubMed non-numeric month dates - bioRxiv: derive the PDF URL from the record's actual 'version' field instead of a hard-coded 'v1', which 404'd or pointed at the wrong revision for v2+ preprints. - PubMed: a non-numeric, non-abbreviation Month (e.g. a season like 'Winter') was emitted verbatim, producing invalid dates like '2024-Winter'. Accept only known abbreviations or numeric months; otherwise fall back to year-only. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/publications/fetchers/biorxiv.py | 30 ++++++++++++++------ bmlib/publications/fetchers/pubmed.py | 39 ++++++++++++++++++-------- tests/test_publications.py | 10 ++++++- tests/test_pubmed_fetcher.py | 27 ++++++++++++++++++ 4 files changed, 85 insertions(+), 21 deletions(-) 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/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/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 = """\ From 2e732cbf8d54037cbb526bb5366ab891e49ecf8c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 23:01:49 +0000 Subject: [PATCH 10/13] Make JSON extraction robust to multiple objects and braces in strings extract_json (and BaseAgent.parse_json) used a greedy '\{.*\}' that spanned from the first '{' to the last '}', swallowing prose between two objects and failing to parse. Replace with balanced-brace scanning that respects string literals and returns the first object that parses; parse_json now reuses extract_json instead of duplicating the regex. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/agents/base.py | 38 +++++++++++++++------------------ bmlib/llm/utils.py | 51 ++++++++++++++++++++++++++++++++++++-------- tests/test_agents.py | 15 +++++++++++++ 3 files changed, 74 insertions(+), 30 deletions(-) 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/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/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")]) From ab9cfef2f002bf3f21e60f65544f67a6e4f6cc87 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 23:03:43 +0000 Subject: [PATCH 11/13] Use max_completion_tokens for OpenAI reasoning models OpenAI o-series reasoning models (o1, o3-mini, ...) reject 'max_tokens' (they require 'max_completion_tokens') and non-default 'temperature', so calls to advertised models like o1/o3-mini failed with a 400. Add an overridable _is_reasoning_model hook (default False) and branch the request parameters; OpenAIProvider detects the o-series by name. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/llm/providers/openai_compat.py | 23 ++++++++++++++--- bmlib/llm/providers/openai_provider.py | 9 +++++++ tests/test_openai_compat.py | 34 ++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/bmlib/llm/providers/openai_compat.py b/bmlib/llm/providers/openai_compat.py index 6e6b7a9..072a843 100644 --- a/bmlib/llm/providers/openai_compat.py +++ b/bmlib/llm/providers/openai_compat.py @@ -131,6 +131,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 +161,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"} 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/tests/test_openai_compat.py b/tests/test_openai_compat.py index cefab6b..ef8fbc7 100644 --- a/tests/test_openai_compat.py +++ b/tests/test_openai_compat.py @@ -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") From 91760d595806765b29edf3442492d5fe9636fd90 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 23:13:48 +0000 Subject: [PATCH 12/13] Make repo lint- and format-clean for CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prerequisite for enforcing ruff in CI. Mechanical cleanup only — no behavioural changes: - ruff --fix: import sorting (I001), unused imports (F401), collections.abc.Callable (UP035), unquoted annotations (UP037) - Move jats_parser model import above the module logger (E402) - Hoist _TOOL_CAPABLE_PROVIDERS to module level (N806) - Wrap over-length test lines (E501) - ruff format across the repo - per-file-ignores for two unavoidable cases: xml.sax ContentHandler override method names (N802) and the StubProvider pytest fixture that returns a class (N802/N803) Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- bmlib/db/__init__.py | 2 +- bmlib/db/connection.py | 4 +- bmlib/db/migrations.py | 7 +- bmlib/fulltext/jats_parser.py | 30 ++++----- bmlib/fulltext/service.py | 28 +++++--- bmlib/llm/client.py | 36 +++++----- bmlib/llm/data_types.py | 2 +- bmlib/llm/providers/__init__.py | 10 ++- bmlib/llm/providers/anthropic.py | 2 +- bmlib/llm/providers/base.py | 11 ++-- bmlib/llm/providers/openai_compat.py | 8 +-- bmlib/llm/token_tracker.py | 5 +- bmlib/publications/fetchers/openalex.py | 28 +++++--- bmlib/publications/fetchers/registry.py | 20 ++++-- bmlib/quality/data_models.py | 41 ++++++++---- bmlib/templates/engine.py | 4 +- pyproject.toml | 6 ++ scripts/smoke_test_tool_calling.py | 11 +--- tests/test_fulltext_cache.py | 2 - tests/test_fulltext_models.py | 3 - tests/test_fulltext_service.py | 87 +++++++++++++++---------- tests/test_llm_tools.py | 33 ++-------- tests/test_openai_compat.py | 4 +- tests/test_templates.py | 8 +-- 24 files changed, 211 insertions(+), 181 deletions(-) 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/fulltext/jats_parser.py b/bmlib/fulltext/jats_parser.py index 203d84f..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 @@ -147,9 +147,7 @@ def end_row(self) -> None: 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 - ): + 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) @@ -475,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": @@ -571,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 @@ -602,9 +598,7 @@ def endElement(self, name: str) -> None: 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 @@ -942,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/service.py b/bmlib/fulltext/service.py index 51bea43..5aa8101 100644 --- a/bmlib/fulltext/service.py +++ b/bmlib/fulltext/service.py @@ -159,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) @@ -173,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) @@ -228,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: @@ -248,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 @@ -278,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. @@ -313,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. diff --git a/bmlib/llm/client.py b/bmlib/llm/client.py index 7d9ba78..6e3ee80 100644 --- a/bmlib/llm/client.py +++ b/bmlib/llm/client.py @@ -192,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, @@ -248,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: @@ -269,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: @@ -289,7 +289,9 @@ 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: @@ -323,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. @@ -337,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/openai_compat.py b/bmlib/llm/providers/openai_compat.py index 072a843..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, @@ -272,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/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/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/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/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/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/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_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 85a9fe3..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 == { @@ -317,9 +327,7 @@ def test_no_identifiers_message(self): class TestSanitizeIdentifier: def test_doi_sanitized(self): # Readable prefix is preserved (a collision-proof hash is appended). - assert _sanitize_identifier("10.1234/test.paper-1").startswith( - "10.1234_test.paper-1_" - ) + 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") @@ -352,7 +360,8 @@ def test_cached_html_returned_without_network(self, tmp_path): 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() @@ -367,7 +376,8 @@ def test_cached_pdf_returned_without_network(self, tmp_path): 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() @@ -386,7 +396,8 @@ 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" @@ -416,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" @@ -450,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" @@ -486,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", ), ] @@ -494,7 +511,8 @@ 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" @@ -508,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", ), ] @@ -516,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_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_openai_compat.py b/tests/test_openai_compat.py index ef8fbc7..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: 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"]) From 9c87ba5e27419a201eee7444f9c95cc79845c37f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 16 Jun 2026 23:13:48 +0000 Subject: [PATCH 13/13] Add GitHub Actions CI workflow Runs on pushes to main and all pull requests: - lint job: ruff check + ruff format --check - test job: pytest across Python 3.11, 3.12, and 3.13 with all extras Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV --- .github/workflows/ci.yml | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/ci.yml 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