Skip to content

Fix correctness bugs found in code review#4

Merged
hherb merged 13 commits into
mainfrom
claude/nifty-mendel-eyb9lt
Jun 16, 2026
Merged

Fix correctness bugs found in code review#4
hherb merged 13 commits into
mainfrom
claude/nifty-mendel-eyb9lt

Conversation

@hherb

@hherb hherb commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary

A thorough code review surfaced a number of genuine correctness bugs — cases where the library would silently produce wrong or unintended results. This PR fixes each one, with regression tests, across 11 focused commits. All 336 tests pass (was 308; +28 new). No new lint errors are introduced.

Fixes

High severity (silent data loss / systematically wrong results)

  • Full-text sources silently dropped during sync (publications/sync.py). _record_to_fulltext_sources accessed each entry as a dict (.get()/[]), but fetchers populate FetchedRecord.fulltext_sources with FullTextSourceEntry dataclasses. Every record carrying a full-text source raised inside the swallowed except, was counted as failed, and the publication was never stored. The bug was masked by a test fixture that used plain dicts — now fixed to use the real type.
  • Transparency COI detection read the wrong field (transparency/). 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 fetches EuropePMC full text when available and scans that; coi_disclosed is tri-state (True/False/None) and only an explicit False triggers the downgrade.
  • Ollama token accounting clobbered legitimate zero counts (llm/providers/ollama.py). value or estimate replaced a real 0 (cached prompt / empty completion) with a fabricated estimate, corrupting usage/cost. Uses is None now.

Medium severity (wrong classification / parsing / atomicity)

  • Study-design misclassification (quality/metadata_filter.py). Re-ordered priority so cohort designs outrank case-control (a paper tagged both was downgraded to the weaker design); dropped Multicenter Study → RCT and Comparative Study → cross-sectional mappings (organisational/generic attributes, not designs — they misclassified observational papers as RCTs with high confidence).
  • ClinicalTrials.gov results check (transparency/analyzer.py). Requested a ResultsSection field but read a resultsSection key, under-detecting posted results. Uses the v2 hasResults boolean with a fallback.
  • SQLite migrations were not atomic (db/operations.py). executescript() issues an implicit COMMIT, committing the migration's BEGIN early; a later failure left a half-applied, unrecorded schema. Statements are now executed individually (SQLite supports transactional DDL) so failures roll back cleanly.
  • JATS parser (fulltext/jats_parser.py): header-less tables no longer misclassify a data row with a leading <th> row-label as a header; titled-but-empty abstract subsections are no longer dropped; multi-paragraph captions are joined with a space.

Low severity

  • Full-text cache key collisions (fulltext/service.py): distinct identifiers could sanitise to the same filename and serve the wrong article's text — now append a short hash. Also: PubMed fallback mislabelled source="doi""pubmed"; URL-encode the Unpaywall email.
  • LLM provider casing (llm/client.py): a mixed-case default_provider/Provider:model desynced routing, cost aggregation, and the tool-capability allowlist — normalised to lowercase.
  • bioRxiv PDF version hard-coded v1 → uses the record's actual version; PubMed non-numeric months (e.g. Winter) no longer produce invalid dates like 2024-Winter.
  • JSON extraction (llm/utils.py, agents/base.py): greedy \{.*\} spanned first-to-last brace, swallowing prose between objects — replaced with balanced-brace scanning that respects string literals and returns the first parseable object.
  • OpenAI reasoning models (llm/providers/openai_compat.py): o1/o3 reject max_tokens/temperature — now use max_completion_tokens via an overridable hook.

Investigated, not a bug

  • A reported missing case_series value in the classifier prompts was a false positive — both prompts and the score/tier mappings already support it. No change.

Testing

  • pytest tests/ -v → 336 passed.
  • Each fix ships a regression test that fails against the old behaviour.

🤖 Generated with Claude Code


Generated by Claude Code

claude added 13 commits June 16, 2026 22:44
_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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
- _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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
- 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 <th>
  row-label was misclassified into <thead>.
- Structured-abstract subsections with a title but no <p> 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNBmZ9GTQw1Esy9oU7vXNV
@hherb hherb merged commit 65f9f20 into main Jun 16, 2026
4 checks passed
@hherb hherb mentioned this pull request Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants