feat(ci): bifurcate CI into hosted and self-hosted jobs #330
Conversation
Splits the monolithic unit_tests_hosted job into parallel lint, security, and docs jobs on GitHub-hosted runners. Moves the main build and test execution to the self-hosted runner to leverage caching and hardware capabilities.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 |
||||||||||||||||||||||||||||||
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan 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 |
||||||||||||||||||||||||||||||||
CI Feedback 🧐(Feedback updated until commit 7a0ca5e)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Code Suggestions ✨No code suggestions found for the PR. |
|
| Severity | Issue | Location |
|---|---|---|
| CRITICAL | Missing ALSA libraries in lint job | .github/workflows/ci.yml:139 |
| WARNING | Missing system deps in docs job | .github/workflows/ci.yml:151 |
| WARNING | Silent security tool installation failure | .github/workflows/ci.yml:118 |
| WARNING | Documentation build should fail CI | .github/workflows/ci.yml:401 |
| WARNING | Trailing whitespace | .github/workflows/ci.yml:379, 387 |
Recommendation: Fix critical ALSA dependency issue in lint job before merge. Other issues are non-blocking but should be addressed.
Review Details (1 file)
Files: .github/workflows/ci.yml
Progress Since Last Review:
- ✅ RESOLVED: Unit tests moved to GitHub-hosted
build_and_unit_testsjob (addresses architectural concern) ⚠️ PARTIALLY RESOLVED: ALSA dependencies added tobuild_and_unit_testsbut missing fromlintanddocsjobs
Critical Issues:
-
ALSA in lint job: The
lintjob runscargo clippywhich compiles code and needs ALSA libraries. Currently fails on ubuntu-latest. -
System deps in docs: Missing pkg-config and libasound2-dev may cause doc build failures.
-
Security tools: Installation uses
|| truemasking failures. -
Documentation policy: Should fail CI as error, not warning.
-
Formatting: Trailing whitespace on multiple lines.
There was a problem hiding this comment.
Pull request overview
This PR implements a CI bifurcation strategy that splits the monolithic CI workflow into parallel hosted jobs and a consolidated self-hosted job for better resource utilization and faster feedback.
Key Changes:
- Split lint, security, and documentation checks into parallel jobs on GitHub-hosted runners
- Consolidate build and test operations into a single self-hosted job with hardware integration tests
- Rename jobs for clarity (
security_audit→security,text_injection_tests→build_and_test)
| echo "- docs: ${{ needs.docs.result }}" >> report.md | ||
| echo "- build_and_test: ${{ needs.build_and_test.result }}" >> report.md | ||
| echo "- moonshine_check: ${{ needs.moonshine_check.result }} (optional)" >> report.md | ||
There was a problem hiding this comment.
Trailing whitespace detected on this line. Remove the trailing spaces for cleaner code formatting.
| if [[ "${{ needs.docs.result }}" != "success" ]]; then echo "::warning::Documentation build failed."; fi | ||
| if [[ "${{ needs.build_and_test.result }}" != "success" ]]; then echo "::error::Build and Test failed."; exit 1; fi | ||
| if [[ "${{ needs.moonshine_check.result }}" != "success" ]]; then echo "::warning::Moonshine check failed (optional)."; fi | ||
There was a problem hiding this comment.
Trailing whitespace detected on this line. Remove the trailing spaces for cleaner code formatting.
| - name: Run clippy | ||
| if: matrix.rust-version == 'stable' | ||
| run: cargo clippy --all-targets --locked |
There was a problem hiding this comment.
The lint job runs cargo clippy --all-targets --locked without installing system dependencies. The workspace includes crates that depend on system libraries (e.g., coldvox-audio uses cpal which requires ALSA development libraries on Linux, coldvox-text-injection may require GTK headers via pkg-config). While clippy with default features might work if these dependencies are optional, consider adding necessary system dependencies (pkg-config, libasound2-dev) or running clippy with specific feature flags to ensure reliable builds on Ubuntu runners.
| - name: Build documentation | ||
| if: matrix.rust-version == 'stable' | ||
| run: cargo doc --workspace --no-deps --locked |
There was a problem hiding this comment.
The docs job runs cargo doc --workspace without installing system dependencies. The workspace includes crates that may require system libraries to compile (e.g., coldvox-audio uses cpal which requires ALSA development libraries). While documentation building with default features might work if these dependencies are optional, consider adding necessary system dependencies (pkg-config, libasound2-dev) to ensure reliable doc builds on Ubuntu runners.
.github/workflows/ci.yml
Outdated
| - name: Run Workspace Unit Tests | ||
| run: | | ||
| echo "=== Running Workspace Unit Tests ===" | ||
| cargo test --workspace --locked |
There was a problem hiding this comment.
The Golden Master pipeline test that was previously executed in the unit_tests_hosted job is not present in the new build_and_test job. The original workflow included a specific step to run cargo test -p coldvox-app --test golden_master -- --nocapture with Python dependencies (faster-whisper). Since the build_and_test job now has access to WHISPER_MODEL_PATH and WHISPER_MODEL_SIZE environment variables and runs on the self-hosted runner, this test should be included to maintain test coverage.
.github/workflows/ci.yml
Outdated
| - name: Run Workspace Unit Tests | ||
| run: | | ||
| echo "=== Running Workspace Unit Tests ===" | ||
| cargo test --workspace --locked |
There was a problem hiding this comment.
The "Run Workspace Unit Tests" step does not include validation of the WHISPER_MODEL_PATH and WHISPER_MODEL_SIZE environment variables before executing tests. The original unit_tests_hosted job validated these variables were set and accessible. Consider adding validation similar to the original implementation to ensure tests fail early with clear error messages if the Whisper dependencies are not properly configured.
The laptop has WEAK hardware but a LIVE display. GitHub-hosted runners have POWERFUL hardware but no display. Correct split: - GitHub-hosted: lint, security, docs, build, unit tests - Self-hosted: ONLY hardware-dependent tests (real-injection, audio) This minimizes work on the weak laptop while leveraging its unique capability (live display session).
User description
Implements the CI bifurcation plan described in Proposal B and Issue #329.
Changes
unit_tests_hosted: Replaced the monolithic job with parallellint,security, anddocsjobs running onubuntu-latest.text_injection_teststobuild_and_test: This job now runs on the self-hosted runner and handles:cargo test --workspace).real-injection-testsfeature).ci_success: Now depends on the new parallel jobs.Benefits
Closes #329
PR Type
Enhancement
Description
Split monolithic
unit_tests_hostedinto three parallel jobs:lint,security, anddocsRenamed
text_injection_teststobuild_and_testwith expanded responsibilitiesMoved build, workspace tests, and hardware integration tests to self-hosted runner
Updated
ci_successjob to depend on new parallel jobs with refined error handlingDiagram Walkthrough
File Walkthrough
ci.yml
Restructure CI workflow into parallel and self-hosted jobs.github/workflows/ci.yml
security_auditjob tosecurityfor consistencyunit_tests_hostedinto three independent parallel jobs:lint(fmt + clippy),
security(audit + deny), anddocs(documentationbuild)
text_injection_teststobuild_and_testand added workspaceunit tests execution
WHISPER_MODEL_PATHandWHISPER_MODEL_SIZEenvironment variablesto
build_and_testjobci_successjob dependencies and CI report generation toreference new job names with refined error handling logic