Repository navigation
fix(doctor): resolve models/ from repo root, not config_path.parent - #144
Merged
Merged
Conversation
dotty_doctor's SenseVoice/Piper checks anchored `root = config_path.parent`. In a standard deploy the config is at `<root>/data/.config.yaml` while models are bind-mounted from `<root>/models/` (./models/... in docker-compose), so the checks looked under `<root>/data/models/...` and FALSE-FAILed a healthy install — `make doctor` exited non-zero on a correct standard deploy. Add `_project_root()`: if config sits under a `data/` dir, step up to the repo root; prefer whichever candidate actually contains `models/`, else fall back to the most likely root so the "missing" message still points at `<root>/models`. Both model checks now use it. Verified locally: the checks resolve to `<repo>/models/...` (legacy root-config layout still works too). Tests: new tests/test_dotty_doctor_models.py — standard vs legacy layout, None config, missing-models path, and end-to-end Piper/SenseVoice checks in the data/ layout (SenseVoice size floors monkeypatched small). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes scripts/dotty_doctor.py model-path checks so they correctly resolve models/ from the project root in the standard deployment layout where config lives at <root>/data/.config.yaml (instead of incorrectly looking under <root>/data/models/...). It adds targeted regression tests covering both standard and legacy layouts.
Changes:
- Add
_project_root()helper to resolve the correct base directory formodels/based onconfig_path. - Update
check_models_sensevoiceandcheck_models_piperto use_project_root()instead ofconfig_path.parent. - Add new unit tests validating root resolution and end-to-end pass/fail behavior for both Piper and SenseVoice checks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
scripts/dotty_doctor.py |
Introduces _project_root() and switches model checks to resolve models/ from the correct repo root in standard installs. |
tests/test_dotty_doctor_models.py |
Adds regression tests for standard vs legacy config layouts and verifies model checks point at <root>/models/.... |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Audit finding (HIGH, confirmed 2/3). Independent of #142/#143 — off
main.Bug
dotty_doctor'scheck_models_sensevoice/check_models_piperanchoredroot = config_path.parent. In a standard deploy the config is at<root>/data/.config.yaml, but models are bind-mounted from<root>/models/(./models/...in docker-compose). So the checks looked under<root>/data/models/...and FALSE-FAILed a healthy install —make doctorexits non-zero on a correct standard deploy.Fix
New
_project_root(): if the config sits under adata/dir, step up to the repo root; prefer whichever candidate actually containsmodels/, else fall back to the most likely root so the "missing" message still points at<root>/models(not<root>/data/models). Both model checks use it. The legacy root-level.config.yamllayout still resolves correctly.Verified locally: the checks now resolve to
<repo>/models/....Tests
New
tests/test_dotty_doctor_models.py: standard (data/) vs legacy layout,Noneconfig, missing-models path resolution, and end-to-end Piper + SenseVoice checks in thedata/layout (SenseVoice size floors monkeypatched small so no 200 MB fixture). Fulltests/suite passes; ruff clean.No deploy:
dotty_doctor.pyis a diagnostic CLI, not bind-mounted into any container.🤖 Generated with Claude Code