fix: harden serve/web/CLI/i18n from repo-health findings (#105, #96, #102, #99, #106) - #108
Conversation
- serve: read MEDCHECK_HOST/MEDCHECK_PORT via Typer envvar so the Docker image's ENV MEDCHECK_HOST=0.0.0.0 takes effect (container was unreachable) (#105) - web: POST /api/analyze returns 501 (not 200) for the unimplemented stub so clients/health checks can detect it (#96) - vision_analysis: bound load_anatomy_instructions cache with lru_cache(maxsize=64) instead of unbounded @cache to cap memory from request-supplied anatomy (#102) - cli: validate --report and --lang, failing fast on unknown values instead of silently defaulting to JSON (#99) - i18n: confine _load_catalog lang to a safe pattern before building a path (defense-in-depth against traversal via unvalidated --lang) (#106) Adds regression tests for each. Full suite + mypy + ruff green. https://claude.ai/code/session_01KYcUxCGAHaTPrRHTzJCWQz
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR hardens the medcheck application across four areas: i18n language codes are validated with a regex before file path construction to prevent traversal; the ChangesSecurity, Validation, and Stub Corrections
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/medcheck/web/app.py`:
- Around line 75-82: The AnalyzeRequest class is restricting the language field
to only en and de, which causes incoming requests with fr or es language codes
to fail validation with a 422 error before reaching the analyze endpoint
function. Update the language field validation in the AnalyzeRequest class to
include fr and es as supported values, ensuring requests with these newly
supported locales pass validation and reach the 501 stub endpoint rather than
being rejected during request parsing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ddd30cf9-b9a7-447e-870b-20c66a7dca24
📒 Files selected for processing (8)
CHANGELOG.mdsrc/medcheck/i18n/loader.pysrc/medcheck/main.pysrc/medcheck/pipeline/vision_analysis.pysrc/medcheck/web/app.pytests/unit/test_cli.pytests/unit/test_i18n.pytests/unit/test_web.py
The CLI now accepts en/de/fr/es and the i18n catalogs ship those locales, but AnalyzeRequest restricted language to ^(en|de)$ — so fr/es requests were rejected with 422 before reaching the endpoint. Align the schema pattern and add tests. Addresses CodeRabbit review on #108.
Bundles five clearly-scoped fixes from the latest repo-health-agent batch. Each is low-risk, self-contained, and ships with a regression test.
Changes
servenow honoursMEDCHECK_HOST/MEDCHECK_PORTvia Typerenvvar=. The Docker image setsENV MEDCHECK_HOST=0.0.0.0and runsmedcheck servewith no--host, but the hardcoded127.0.0.1default meant the container bound to loopback and was unreachable from the host. Now the env vars take effect when the flag is omitted.POST /api/analyzereturns501 Not Implementedinstead of200 OKfor the not-yet-wired stub, so clients, health checks, and CI can detect that no analysis ran.load_anatomy_instructionsuses@lru_cache(maxsize=64)instead of unbounded@cache. The/api/analyzebody can supply arbitraryanatomystrings; an unbounded cache grows without limit.analyzevalidates--report/--langand fails fast with a clear error instead of silently falling through to a JSON report on e.g.--report xml._load_catalog()confineslangto a safe pattern before building a file path (defense-in-depth: the loader is reachable from the CLI with an unvalidated--lang).Tests
test_serve_honors_host_port_env— patchesuvicorn.run, asserts host/port come from envtest_analyze_open_when_no_api_key_configured/test_analyze_requires_key_when_configured— updated to expect501test_analyze_rejects_invalid_report_format/test_analyze_rejects_invalid_languagetest_i18n_rejects_path_traversal_langFull suite (130 tests) +
mypy --strict+ruff check/ruff formatall green locally.Not included (deliberately)
actions/checkout@v6, so it resolves and works; downgrading would be a needless change. Recommend closing.--dob), [repo-health] Medium: bandit B104 globally suppressed — security scan blind to all bind-to-all-interfaces violations #91 (bandit B104 scope) — real, but each needs a design decision (fallback policy / de-identification flag / flag removal / scan scope). Better handled as separate, considered PRs.Closes #105, #96, #102, #99, #106
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
fr,es) via the--langoption and/api/analyzerequest schema.servenow readsMEDCHECK_HOSTandMEDCHECK_PORTenvironment variables when--host/--portaren’t provided.Bug Fixes
POST /api/analyzenow returns 501 Not Implemented (instead of a success response).