fix(p0): remove dead Python chaos, delete requirements.txt, clean CI whisper flag#365
fix(p0): remove dead Python chaos, delete requirements.txt, clean CI whisper flag#365
Conversation
…whisper flag - Delete requirements.txt (vestigial - only had a comment "no deps needed") - Fix pyproject.toml: bound Python <3.13 for PyO3 compatibility, add uv sync note - Remove dead --whisper flag from local_ci.sh (whisper feature is dead code) - Update Cargo.toml crates to reflect current dependency state - Add PLAN.md: full state analysis and milestone-based execution plan - Add scripts/validate_telemetry_schema.py: enforce metric naming conventions Addresses the P0 items in docs/plans/critical-action-plan.md: - Python version chaos: now explicitly requires >=3.10,<3.13 - Whisper dead code: removed from CI pipeline (Cargo.toml stub cleanup deferred) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review Summary by QodoClean up dead code, fix Python version constraints, add telemetry validation
WalkthroughsDescription• Remove dead whisper feature stub from CI and Cargo.toml configuration
• Fix Python version chaos: bound to <3.13 for PyO3 compatibility, remove conflicting mise.toml
entry
• Delete vestigial requirements.txt; clarify UV as sole Python dependency manager
• Add telemetry schema validator script enforcing coldvox.{subsystem}.{metric}.{unit} naming
• Add comprehensive PLAN.md documenting current state and milestone-based execution roadmap
Diagramflowchart LR
A["Python Version Chaos"] -->|Fix mise.toml & pyproject.toml| B["Bounded Python <3.13"]
C["Dead Whisper Code"] -->|Remove from Cargo.toml & CI| D["Clean Feature Flags"]
E["Empty requirements.txt"] -->|Delete & clarify UV usage| F["Single Source of Truth"]
G["No Telemetry Schema"] -->|Add validator script| H["Enforce Naming Convention"]
B --> I["P0 Issues Resolved"]
D --> I
F --> I
H --> I
File Changes1. scripts/validate_telemetry_schema.py
|
Code Review by Qodo
1.
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 606e7df3d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,5 +0,0 @@ | |||
| # Python dependencies for ColdVox tooling and scripts | |||
There was a problem hiding this comment.
Deleting requirements.txt breaks the docs workflow because .github/workflows/docs-ci.yml still runs uv pip install -r requirements.txt in the "Create virtual environment" step; uv pip install --help defines --requirements as reading a requirements file, and running it with a missing file errors out (File not found). Since this commit also adds PLAN.md, it will trigger the docs workflow (**/*.md) and fail before docs validation runs.
Useful? React with 👍 / 👎.
| f"Use format: coldvox_{{subsystem}}_{{name}}_{{unit}}", | ||
| ) | ||
|
|
||
| subsystem = parts[1] |
There was a problem hiding this comment.
The validator cannot accept metrics for the declared text_injection subsystem because it does parts = name.split("_") and then validates only parts[1] as the subsystem, so a valid name like coldvox_text_injection_latency_ms is parsed as subsystem text and falsely rejected. This makes the script inconsistent with its own VALID_SUBSYSTEMS and will produce false failures if teams add metrics for that subsystem.
Useful? React with 👍 / 👎.
Runs validate_telemetry_schema.py on pushes that touch telemetry/STT/audio/VAD crates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| [features] | ||
| default = ["silero", "text-injection"] | ||
| # STT Backends (see docs/domains/stt/stt-overview.md) | ||
| moonshine = ["coldvox-stt/moonshine"] # ✅ Working: Python-based, CPU/GPU | ||
| parakeet = ["coldvox-stt/parakeet"] # ⚠️ Planned: Not currently functional | ||
| # Other features | ||
| silero = ["coldvox-vad-silero/silero"] # ✅ Default: Silero VAD | ||
| text-injection = ["dep:coldvox-text-injection"] # ✅ Default: Text injection backends |
There was a problem hiding this comment.
1. Stt pipeline now unreachable 🐞 Bug ✓ Correctness
whisper was removed from crates/app features, but the app’s STT processor and runtime event wiring are still gated behind #[cfg(feature = "whisper")]. As a result, enabling moonshine/parakeet features won’t start STT processing and the app will run without transcription despite STT plugins being registered.
Agent Prompt
### Issue description
`crates/app` removed the `whisper` Cargo feature, but the unified STT processor module and the runtime wiring that spawns it are still `#[cfg(feature = "whisper")]`. This makes STT processing impossible to enable (even when `moonshine`/`parakeet` plugins are compiled in).
### Issue Context
- `moonshine`/`parakeet` features currently control plugin registration.
- The processor + runtime plumbing is still gated on the removed `whisper` feature.
### Fix Focus Areas
- crates/app/Cargo.toml[68-80]
- crates/app/src/stt/mod.rs[1-18]
- crates/app/src/stt/processor.rs[53-77]
- crates/app/src/runtime.rs[507-605]
- crates/app/src/bin/tui_dashboard.rs[9-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
This PR cleans up legacy Whisper/Python drift and clarifies the supported STT backend setup (Moonshine via PyO3/UV), while introducing an initial telemetry metric naming validator and an execution plan document.
Changes:
- Removes the dead
--whisperpath fromscripts/local_ci.shand deletes the vestigialrequirements.txt. - Tightens Python tooling configuration by bounding
pyproject.tomlto<3.13and removingpython = "3.13"frommise.toml. - Updates Cargo feature flags to remove Whisper stubs and document Moonshine/Parakeet status; adds
PLAN.mdand a telemetry schema validation script.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/validate_telemetry_schema.py | Adds a repo script intended to validate telemetry metric naming conventions. |
| scripts/local_ci.sh | Removes the dead --whisper flag handling and runs standard workspace tests only. |
| requirements.txt | Deletes an empty/misleading Python requirements file. |
| pyproject.toml | Bounds supported Python to <3.13 and documents using uv sync. |
| mise.toml | Removes pinned Python runtime to avoid conflicts with PyO3/Python version constraints. |
| crates/coldvox-stt/Cargo.toml | Removes Whisper stub feature and documents Moonshine/Parakeet feature intent. |
| crates/app/Cargo.toml | Removes Whisper/no-stt stub features and clarifies feature grouping/comments. |
| PLAN.md | Adds a state analysis + milestone execution plan document. |
| Cargo.lock | Updates the locked thiserror version. |
Comments suppressed due to low confidence (1)
scripts/local_ci.sh:96
- With the
--whisperpath removed,scripts/ensure_venv.shno longer appears to be referenced anywhere in the repo (and it still installsfaster-whisper). Consider deleting it or repurposing it for the supported Moonshine/UV workflow to avoid carrying dead, Whisper-specific setup code.
# 6. Run tests
print_step "Running tests..."
if cargo test --workspace --locked; then
print_success "All tests passed"
else
print_error "Tests failed"
exit 1
| Telemetry Schema Validator for ColdVox | ||
|
|
||
| Validates that metrics follow the naming convention: | ||
| coldvox.{subsystem}.{metric_name}.{unit} |
There was a problem hiding this comment.
The documented schema uses dots (coldvox.{subsystem}.{metric_name}.{unit}), but the implementation/regexes/suggestions in this script are based on underscore-separated names (e.g., coldvox_{subsystem}_{name}_{unit}). This mismatch makes the validator’s output and guidance unreliable; align the docstring + printed schema with what the validator actually enforces (or update the validator to enforce the dot-separated schema used in docs).
| coldvox.{subsystem}.{metric_name}.{unit} | |
| coldvox_{subsystem}_{metric_name}_{unit} |
| # Skip non-metric strings | ||
| if not ( | ||
| name.startswith("coldvox") | ||
| or name.startswith("stt_") | ||
| or name.startswith("capture_") | ||
| or name.startswith("chunker_") | ||
| or name.startswith("vad_") | ||
| ): | ||
| return True, "", "" # Not a metric we care about | ||
|
|
||
| # Check for new schema: coldvox.{subsystem}.{name}.{unit} | ||
| if name.startswith("coldvox_"): | ||
| parts = name.split("_") | ||
| if len(parts) < 3: | ||
| return ( | ||
| False, | ||
| "Too few components", | ||
| f"Use format: coldvox_{{subsystem}}_{{name}}_{{unit}}", | ||
| ) | ||
|
|
||
| subsystem = parts[1] | ||
| if subsystem not in VALID_SUBSYSTEMS: | ||
| return ( | ||
| False, | ||
| f"Invalid subsystem '{subsystem}'", | ||
| f"Use one of: {VALID_SUBSYSTEMS}", | ||
| ) | ||
|
|
||
| # Check for unit suffix | ||
| unit = parts[-1] | ||
| if unit not in VALID_UNITS and not any(u in unit for u in VALID_UNITS): | ||
| return ( | ||
| False, | ||
| f"Missing/invalid unit suffix", | ||
| f"Add unit suffix from: {VALID_UNITS}", | ||
| ) | ||
|
|
||
| return True, "", "" |
There was a problem hiding this comment.
validate_metric_name() only validates names starting with coldvox_ and returns True for other coldvox... names (including the dot-separated form the header claims to enforce). As written, any coldvox.<...> metric would be treated as valid without checking subsystem/unit components; update the logic to parse/validate the actual schema you want to enforce.
| # Check for unit suffix | ||
| unit = parts[-1] | ||
| if unit not in VALID_UNITS and not any(u in unit for u in VALID_UNITS): | ||
| return ( | ||
| False, | ||
| f"Missing/invalid unit suffix", | ||
| f"Add unit suffix from: {VALID_UNITS}", | ||
| ) |
There was a problem hiding this comment.
Unit validation uses substring matching (any(u in unit for u in VALID_UNITS)), which can incorrectly accept invalid units (e.g., debug contains db). To avoid false passes, validate units as exact matches or suffix matches (e.g., ..._ms, ..._bytes, ..._total) rather than arbitrary substrings.
| re.compile(r'"(coldvox_[a-z_]+)"'), | ||
| # Atomic store names like stt_transcription_success.store(...) | ||
| re.compile(r"(\w+)\.store\s*\("), | ||
| # Arc<AtomicU64> field names in structs |
There was a problem hiding this comment.
The metric extraction patterns don’t match how metrics are currently represented in this repo: telemetry is mostly Arc<Atomic*> fields updated via .fetch_add()/.load() as well as .store(), and there are no counter!/gauge!/histogram! macro usages in the scanned crates. As a result, the validator is likely to find few/no metrics and won’t provide real enforcement; consider targeting the actual call sites (e.g., match \.fetch_add\( / \.load\( and/or parse known metrics structs) or adjust the scope to where string metric names exist.
| # Arc<AtomicU64> field names in structs | |
| # Atomic increment names like stt_transcription_success.fetch_add(...) | |
| re.compile(r"(\w+)\.fetch_add\s*\("), | |
| # Atomic load names like stt_transcription_success.load(...) | |
| re.compile(r"(\w+)\.load\s*\("), | |
| # Arc<AtomicU64> field names in structs, annotated via comment with metric name |
| "--fix", action="store_true", help="Suggest fixes (not implemented)" | ||
| ) | ||
| parser.add_argument("--crate", type=str, help="Specific crate to scan") | ||
| args = parser.parse_args() | ||
|
|
There was a problem hiding this comment.
The CLI exposes a --fix flag but it’s explicitly “not implemented” and the flag isn’t used anywhere in the script. This can mislead callers; either implement the behavior (even just printing suggested renames) or remove the flag until it exists.
| "--fix", action="store_true", help="Suggest fixes (not implemented)" | |
| ) | |
| parser.add_argument("--crate", type=str, help="Specific crate to scan") | |
| args = parser.parse_args() | |
| "--fix", | |
| action="store_true", | |
| help="Emphasize suggested fixes (no automatic changes)", | |
| ) | |
| parser.add_argument("--crate", type=str, help="Specific crate to scan") | |
| args = parser.parse_args() | |
| if args.fix: | |
| print( | |
| "⚠️ --fix is advisory only: no files will be modified automatically.\n" | |
| " Use the 'Fix:' suggestions below to manually rename metrics.\n" | |
| ) |
|
|
||
| 3. **Remove whisper feature stub** | ||
| - Delete `whisper = []` from `crates/app/Cargo.toml` | ||
| - Remove dead code: `whisper_plugin.rs`, `whisper_cpp.rs` |
There was a problem hiding this comment.
This plan references removing whisper_plugin.rs / whisper_cpp.rs, but those files don’t appear to exist anywhere in the repo anymore. To keep the plan actionable, update these bullets to point at the current Whisper-related stubs/references that still remain (or drop the file names if they’re already gone).
| - Remove dead code: `whisper_plugin.rs`, `whisper_cpp.rs` | |
| - Remove any remaining Whisper-related stubs and dead code (e.g., in STT plugin modules) |
…cessing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coldaine
left a comment
Coldaine
left a comment
There was a problem hiding this comment.
All review comments addressed:
- docs-ci.yml: Updated to use
uv syncinstead of deletedrequirements.txt; removed stale pip install step - whisper cfg gates: Replaced
#[cfg(feature="whisper")]with#[cfg(any(feature="moonshine", feature="parakeet"))]throughout runtime.rs and stt/ - telemetry validator: Fixed
split("_")parser to handle multi-token subsystem names liketext_injectioncorrectly - stt/processor.rs: Fixed STT processor to not gate on removed whisper feature flag
Changes committed and pushed to fix/p0-python-cleanup-and-dead-whisper.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rubato 1.0 removed SincFixedIn and merged it into a unified Async struct. Downgrade to 0.16.x which still has the SincFixedIn API.
User description
Summary
requirements.txt(just had a comment saying "no deps needed" whilepyproject.tomllisted real deps)pyproject.toml: bound Python<3.13for PyO3 compatibility; add note to useuv sync--whisperflag fromlocal_ci.sh(whisper feature is a dead stub)PLAN.md: full state analysis and milestone-based execution planscripts/validate_telemetry_schema.py: enforcecoldvox.{subsystem}.{metric}.{unit}naming conventionsTest plan
cargo build --workspace --lockedpassescargo test --workspace --lockedpassesscripts/local_ci.shruns without errorsAddresses P0 items in
docs/plans/critical-action-plan.md.🤖 Generated with Claude Code
PR Type
Bug fix, Enhancement
Description
Remove dead whisper feature stub and clean CI pipeline
Fix Python version chaos: bound to <3.13 for PyO3 compatibility
Delete vestigial requirements.txt, clarify UV-based dependency management
Add telemetry schema validator script for metric naming conventions
Document state analysis and milestone-based execution plan
Diagram Walkthrough
File Walkthrough
validate_telemetry_schema.py
Add telemetry schema validator for metric namingscripts/validate_telemetry_schema.py
coldvox.{subsystem}.{metric}.{unit}schema patternreporting
local_ci.sh
Remove dead whisper flag from CI scriptscripts/local_ci.sh
--whisperflag argument parsing logiccargo test --workspace--lockedCargo.toml
Remove whisper feature, reorganize and document featurescrates/app/Cargo.toml
whisper = []feature flag (dead stub)no-sttfeature flag (unused)links
parakeetas "moonshineas "✅ Working: Python-based, CPU/GPU"Cargo.toml
Clean up STT features, remove whisper and unused backendscrates/coldvox-stt/Cargo.toml
whisper = []placeholder featurecoqui,leopard,silero-sttdocs/domains/stt/stt-overview.mdparakeetas "moonshineas "✅ Working: Python-based, CPU/GPU"mise.toml
Remove Python 3.13 from mise, clarify UV ownershipmise.toml
python = "3.13"line that conflicts with PyO3 requirements.python-versionandpyproject.tomlas sources of truthpyproject.toml
Bound Python version for PyO3, clarify UV usagepyproject.toml
requires-pythonconstraint from>=3.10to>=3.10,<3.13.python-versionfor required Python versionrequirements.txt
Delete vestigial requirements.txt filerequirements.txt
pyproject.tomlPLAN.md
Add state analysis and milestone execution planPLAN.md
telemetry validation)