fix: honour MEDCHECK_LLM_PROVIDER, validate LLM findings, correct model IDs - #75
Merged
Conversation
Addresses three previously unaddressed issues: - #43: build_prompt() now loads detailed anatomy templates from prompts/anatomy/*.txt (knee/shoulder/spine), with cached lookup and fallback to built-in hints. The shipped templates were dead code before. Adds an abdomen hint so all README-advertised regions are covered. - #44: add `medcheck providers` and `medcheck models` discovery commands. - #42: fix README "Report Bug" link (bug_report.md -> bug_report.yml). Updates CHANGELOG and README; adds unit tests for the new loader and CLI commands (72 passing). https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
…versal - Add missing blank line so `ruff format --check` passes (lint CI was red). - Slug-validate the anatomy region before building a filesystem path in load_anatomy_instructions(), preventing path traversal (CodeRabbit review). - Add regression test for the traversal guard. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
…dd offline provider stub Addresses three High-severity issues: - #26: web server now binds 127.0.0.1 by default (was 0.0.0.0); add optional X-API-Key auth on /api endpoints via MEDCHECK_API_KEY, and warn when binding to a non-loopback host without a key configured. - #28: validate the easyRadiology linkToERI download URL (HTTPS + host allowlist) and disable redirects to prevent SSRF to internal/metadata hosts. - #36: add LocalLLMProvider stub so the advertised "local" offline fallback degrades gracefully (check_available()=False, actionable NotImplementedError) instead of crashing with "No LLM provider available"; register it in the vision router and `medcheck models`. Updates README, .env.example, CHANGELOG; adds unit tests for all three (89 passing, coverage 66%). https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
…validation Addresses four Medium/High-severity privacy issues: - #27: gate external (cloud) LLM transmission behind explicit consent (--allow-cloud-llm flag, MEDCHECK_ALLOW_EXTERNAL_LLM env, interactive prompt); vision_analysis raises instead of silently sending PHI. - #29: stop logging the raw patient name to stdout — log a non-reversible hash of the patient ID instead. - #30: stop echoing portal access codes into ValueError messages. - #33: add a Pydantic AnalyzeRequest schema with validation for POST /api/analyze (auth was already added in #46). Docs: SECURITY.md "Handling of Patient Data" section, README + .env.example. Tests: +5 (consent gate, credential redaction, request validation); 93 passing. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
- ingest: stop logging study description (free-text DICOM field may contain PHI) - vision_analysis: honour explicit LLM provider preference (wires up --model) instead of hardcoding "claude"; default to on-device "local" without consent so offline execution stays reachable - docs: soften compliance/anonymity wording (drop "(HIPAA/GDPR)" assertion and "non-reversible" claim; describe pseudonymisation accurately) 94 tests passing. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
Consent (--allow-cloud-llm / MEDCHECK_ALLOW_EXTERNAL_LLM) now only permits falling back to an external provider; it no longer flips the implicit default from on-device 'local' to 'claude'. Addresses CodeRabbit review on #47. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
…larity Addresses four Medium-severity reliability/supply-chain issues: - #38: add configurable timeout (MEDCHECK_LLM_TIMEOUT) + retry with exponential backoff (MEDCHECK_LLM_RETRIES) to all three LLM providers via a shared call_with_retries helper; transient failures no longer crash the pipeline and surface as a clear LLMProviderError. - #39: run Docker containers as a non-root 'medcheck' user. - #37: vendor htmx locally (src/medcheck/web/static/htmx.min.js) with an SRI hash instead of loading from the unpkg CDN; air-gapped friendly. - #34: clarify the misleading bandit nosec on the Crypto import (pycryptodome, not pycrypto; B413 can't distinguish the shared namespace) and add an AES-CBC round-trip test that verifies the decryption path. 102 tests passing; coverage 67%. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
- base: only retry transient errors (timeouts/connection/429/5xx), fail fast on permanent ones (auth/4xx); add is_transient_error() helper - claude/openai: pass max_retries=0 so call_with_retries is the sole retry controller (avoids SDK retry stacking) - remove unused `# noqa: BLE001` directive (RUF100 lint failure) - add fake-SDK provider tests (claude/openai/gemini happy path + missing key), lifting LLM provider patch coverage to ~100% (fixes codecov/patch) - web test now also asserts the htmx SRI integrity + crossorigin attributes 113 tests passing. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
…rror) Coerce the attempt count to >=1 so the loop always runs once and failures wrap as LLMProviderError instead of raising UnboundLocalError. Addresses CodeRabbit review on #48. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
authenticate() required a non-empty dob, implying DOB verification — but dob is never sent to the portal or checked by this client. Gating on it created a false security boundary. Authentication now relies on the access code only; dob remains accepted but is documented as not verified. Updates README + .env.example to stop overstating DOB verification. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
Closes #10, #11, #12. Adds detailed MRI analysis templates (modelled on knee.txt) that the prompt loader picks up automatically, and extends detect_anatomy() with hip/ankle/wrist keyword patterns (incl. German terms). Removes the over-broad "gelenk" keyword from the knee pattern so it no longer mis-matches Handgelenk (wrist) / Sprunggelenk (ankle); "Knie" still matches knee. 115 tests passing. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
Verified (against primary sources) survey of open-source resources MedCheck can adopt, mapped to the open feature issues: - local medical VLMs (#18): recommend Lingshu-7B (MIT, MRI-capable) over the research-only LLaVA-Med - DICOMDIR/CD handling (#25): pydicom FileSet + pylibjpeg/gdcm - DICOMweb providers (#13 Orthanc, #14 Google Cloud Healthcare) - #15 re-scope: OHIF is a viewer, not a data provider - ML frameworks (MONAI/TorchIO), reporting standards (FHIR/DICOM SR), datasets Includes license compatibility analysis for Apache-2.0 and flags research-only / non-commercial weights. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
The open-source options survey was working material for planning; it does not belong in the public repository. Findings are being applied directly as code. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
…el IDs Three correctness bugs from the repo-health sweep: - #73: the `analyze` command now falls back to Settings.default_llm_provider (MEDCHECK_LLM_PROVIDER) when --model is omitted, instead of silently using the offline `local` provider (which raises NotImplementedError). - #71: parse_llm_response now defensively coerces each LLM-returned structure (known fields only, type coercion, confidence clamped to [0,1], empty entries dropped) instead of StructureFinding(**s), which crashed on unexpected keys and rendered fabricated high-confidence findings verbatim. - #68/#69: default Claude model ID corrected claude-opus-4-7 -> claude-opus-4-8 and made overridable via MEDCHECK_{CLAUDE,OPENAI,GEMINI}_MODEL; updated all README/docs/web-UI references. 130 tests passing. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three correctness bugs from the repo-health sweep (#68, #71, #73), plus the doc half of #69.
Closes
MEDCHECK_LLM_PROVIDERsilently ignoredStructureFinding(**s)deserializes raw LLM JSON with no validationclaude.pyhardcodes invalid model IDclaude-opus-4-7Changes
#73 — honour the configured LLM provider
analyzeonly readSettings()for the consent flag;ctx.llm_providerwas set straight from the--modelflag (defaultNone), soVisionAnalysisStepfell back to the offlinelocalprovider — which raisesNotImplementedError. Now:ctx.llm_provider = model or settings.default_llm_provider.#71 — defensive LLM finding deserialization
parse_llm_responsepreviously didStructureFinding(**s)on raw LLM JSON — an unexpected key crashed the pipeline, and fabricated/hallucinated entries rendered verbatim with high confidence. New_coerce_structure_finding()accepts only known fields, coerces types, clampsconfidenceto [0, 1], drops empty entries, and tolerates a non-liststructures. (StructureFindingis a dataclass, not Pydantic, so a focused coercer is lighter than converting the model and rippling through report.py.)#68 / #69 — model IDs
Default Claude model corrected
claude-opus-4-7→claude-opus-4-8, and all three providers now readMEDCHECK_{CLAUDE,OPENAI,GEMINI}_MODELso IDs aren't hardcoded. Updated every4.7reference across README,docs/models.md,docs/workflows.md, and the web UI dropdown. (The SECURITY.md anchor flagged in #69 already resolves correctly since PR #47 added that heading.)Validation
ruff✅ ·ruff format✅ ·mypy✅https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
Generated by Claude Code
Summary by CodeRabbit
Changed
MEDCHECK_CLAUDE_MODEL,MEDCHECK_OPENAI_MODEL,MEDCHECK_GEMINI_MODEL)Fixed