feat(gates): add BUILD validation gate for config errors - #303
Conversation
Adds a new BUILD quality gate that validates pyproject.toml and package.json before running slower gates (tests, coverage, review). Detects TOML syntax errors, hatchling misconfigurations, and npm dependency resolution failures with clear, actionable error messages.
WalkthroughA new "build" quality gate was added: it's registered in gate rules and tool mappings, implemented to validate Python (pyproject.toml) and Node (package.json) build configs, integrated into gate orchestration, and covered by new tests and CI workflow updates. Changes
Sequence DiagramsequenceDiagram
actor TaskExec as Task Executor
participant QG as QualityGates
participant FS as File System
participant PB as Python Build Tool
participant NB as Node Build Tool
participant DB as Database
participant EP as Error Parser
TaskExec->>QG: run_all_gates(task)
QG->>QG: determine applicable gates
alt BUILD applicable
QG->>FS: check for pyproject.toml
alt pyproject.toml found
QG->>PB: run uv sync --no-install-project
alt uv not available
PB-->>QG: FileNotFoundError
QG->>PB: run pip install -e . --dry-run
end
PB-->>QG: returncode, output
QG->>EP: _extract_build_error_summary(output, "pyproject.toml")
EP-->>QG: summary
end
QG->>FS: check for package.json
alt package.json found
QG->>NB: npm install --dry-run --ignore-scripts
NB-->>QG: returncode, output
QG->>EP: _extract_build_error_summary(output, "package.json")
EP-->>QG: summary
end
alt any build failed
QG->>DB: persist QualityGateFailure (HIGH)
QG->>DB: create quality blocker
end
end
QG-->>TaskExec: QualityGateResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add BUILD validation gate and run it in
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| execution_time = (datetime.now(timezone.utc) - start_time).total_seconds() | ||
|
|
||
| status = "passed" if len(failures) == 0 else "failed" |
There was a problem hiding this comment.
The # type: ignore suppresses the type checker, but if task.id is None, Pydantic will raise ValidationError at runtime. Consider adding a guard (e.g., if task.id is None: raise ValueError(...)) or using a default like task.id or 0.
- status = "passed" if len(failures) == 0 else "failed"
+ status = "passed" if len(failures) == 0 else "failed"
+ if task.id is None:
+ raise ValueError("Cannot run build gate on task without an ID")🚀 Want me to fix this? Reply ex: "fix it for me".
Code Review: BUILD Validation GateOverall AssessmentThis is a well-structured PR that adds a much-needed BUILD validation gate. The implementation follows existing patterns and includes good test coverage. However, there are a few areas that could be improved before merging. ✅ Strengths
Commands: General Options: --no-cache-dir Disable the cache. --disable-pip-version-check Don't periodically check PyPI to determine whether a new version of pip is available for download. Implied with --no-index. --no-color Suppress colored output. --no-python-version-warning Silence deprecation warnings for upcoming unsupported Pythons. --use-feature Enable new functionality, that may be backward incompatible. --use-deprecated Enable deprecated functionality, that will be removed in the future. fallback is excellent for environments without installed 3. Comprehensive Testing - 14 new tests covering build gate logic and rule applicability 4. Safe Command Flags - Using and prevents side effects 5. Consistent Error Handling - Proper and handling 🔶 Issues to Address1. Missing Node.js Fallback (Priority: Medium)In , lacks fallback for Node.js builds like it does for Python ( → Commands: General Options: --no-cache-dir Disable the cache. --disable-pip-version-check Don't periodically check PyPI to determine whether a new version of pip is available for download. Implied with --no-index. --no-color Suppress colored output. --no-python-version-warning Silence deprecation warnings for upcoming unsupported Pythons. --use-feature Enable new functionality, that may be backward incompatible. --use-deprecated Enable deprecated functionality, that will be removed in the future.): Suggestion: Consider adding yarn install v1.22.22 Commands: General Options: --no-cache-dir Disable the cache. --disable-pip-version-check Don't periodically check PyPI to determine whether a new version of pip is available for download. Implied with --no-index. --no-color Suppress colored output. --no-python-version-warning Silence deprecation warnings for upcoming unsupported Pythons. --use-feature Enable new functionality, that may be backward incompatible. --use-deprecated Enable deprecated functionality, that will be removed in the future. pattern. If not, update the docstring to document this design decision. 2. Build Gate Not Checking Task Files (Priority: Medium)Unlike other gates (e.g., at line 150-151), checks for config files existence but doesn't validate against task files: This means a task that only changes documentation could fail the build gate if it happens to be in a repo with a broken . Suggestion: Align with existing gates by adding task file detection: 3. TESTING Category Skip Logic (Priority: Low - Design Decision)The PR skips BUILD for TESTING tasks (line 101 in quality_gate_rules.py): However, testing code that mocks or integrates with the actual module might catch build errors (e.g., incorrect dependency in causing import failures). Question: Should TESTING tasks run the BUILD gate? Consider adding an optional configuration flag to enable it for testing-heavy repos. 4. Test Coverage Gap (Priority: Low)In (line ~1190 in test_quality_gates.py), the test passes when no config files exist but doesn't verify the correct behavior: Suggestion: Verify that no build command was executed when config files are absent: 5. Minor: Inconsistent File Extension Checking (Priority: Very Low)In , the skip reason for DESIGN tasks (line 79) says "Design tasks do not produce lintable code," but the BUILD skip reason (line 79) says "Design tasks do not produce buildable artifacts." This is fine, but the DOC skip reason (line 91) is more nuanced: "Documentation tasks are reviewed through linting." The consistency could be improved, but this is purely cosmetic and doesn't affect functionality. 📋 Test Results VerifiedBased on PR description:
RecommendationApprove with minor changes: The implementation is solid and well-tested. Addressing issues #1 and #2 would make it more robust and consistent with existing gates. Issues #3, #4, and #5 are optional improvements for future iterations. |
Code Review: BUILD Validation GateOverall AssessmentThis is a well-structured PR that adds a much-needed BUILD validation gate. The implementation follows existing patterns and includes good test coverage. However, there are a few areas that could be improved before merging. ✅ Strengths
🔶 Issues to Address1. Missing Node.js Fallback (Priority: Medium)In The current code only checks if npm exists and returns a skip message if not found. For Python builds, there's a fallback to pip. Consider adding yarn as a fallback for Node.js builds to maintain consistency. Suggestion: Consider adding 2. Build Gate Not Checking Task Files (Priority: Medium)Unlike other gates (e.g., has_python = (self.project_root / "pyproject.toml").exists()
has_node = (self.project_root / "package.json").exists()This means a task that only changes documentation could fail the build gate if it happens to be in a repo with a broken Suggestion: Align with existing gates by adding task file detection to ensure the gate only runs when relevant files are modified. 3. TESTING Category Skip Logic (Priority: Low - Design Decision)The PR skips BUILD for TESTING tasks. However, testing code that mocks or integrates with actual modules might catch build errors (e.g., incorrect dependency in Question: Should TESTING tasks run BUILD gate? Consider adding an optional configuration flag to enable it for testing-heavy repos. 4. Test Coverage Gap (Priority: Low)In Suggestion: Verify that no build command was executed when config files are absent by asserting subprocess.run was not called. 5. Minor: Inconsistent Skip Reason Wording (Priority: Very Low)Skip reasons use slightly different phrasing across categories. This is purely cosmetic and doesn't affect functionality. 📋 Test Results VerifiedBased on PR description:
RecommendationApprove with minor changes: The implementation is solid and well-tested. Addressing issues #1 and #2 would make it more robust and consistent with existing gates. Issues #3, #4, and #5 are optional improvements for future iterations. |
|
Author identity unknown *** Please tell me who you are. Run git config --global user.email "you@example.com" to set your account's default identity. fatal: empty ident name (for runner@runnervmymu0l.t23bbcoo3atehcqvjow3yojseh.ex.internal.cloudapp.net) not allowed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/lib/quality_gate_tool.py (1)
33-44: Add BUILD handling in_run_specific_gates(currently KeyError).
VALID_CHECKSnow accepts"build", but_run_specific_gatesdoesn’t map it, sochecks=["build"]will crash at runtime.🛠️ Proposed fix
- checks: Optional list of specific checks to run. If None, runs all gates. - Valid values: ["tests", "types", "coverage", "review", "linting"] + checks: Optional list of specific checks to run. If None, runs all gates. + Valid values: ["build", "tests", "types", "coverage", "review", "linting"]- gate_method = { - "tests": quality_gates.run_tests_gate, - "types": quality_gates.run_type_check_gate, - "coverage": quality_gates.run_coverage_gate, - "review": quality_gates.run_review_gate, - "linting": quality_gates.run_linting_gate, - }[check] + gate_method = { + "build": quality_gates.run_build_gate, + "tests": quality_gates.run_tests_gate, + "types": quality_gates.run_type_check_gate, + "coverage": quality_gates.run_coverage_gate, + "review": quality_gates.run_review_gate, + "linting": quality_gates.run_linting_gate, + }[check]
🤖 Fix all issues with AI agents
In `@codeframe/lib/quality_gates.py`:
- Around line 791-799: Update the configuration guidance text in
_get_category_guidance to include BUILD as an applicable gate for configuration
tasks (in addition to linting/type-check), so blocker messages are accurate now
that QualityGateType.BUILD is checked in run_build_gate; locate the
_get_category_guidance function in quality_gates.py and add BUILD
(QualityGateType.BUILD or its value) to the list or sentence that currently only
mentions linting/type-check, and ensure any returned guidance strings and
skip-message logic reference BUILD where appropriate.
Code Review: BUILD Validation GateOverall AssessmentThis is a well-structured PR that adds a much-needed BUILD validation gate. The implementation follows existing patterns and includes good test coverage. However, there are a few issues that should be addressed before merging. ✅ Strengths
🔶 Issues to Address1. Missing
|
|
Code review posted to PR #303 focusing on the KeyError bug in |
- Add missing TimeoutExpired catch in pip fallback path - Add "build" mapping to _run_specific_gates to prevent KeyError - Update _get_category_guidance configuration text to mention BUILD
- Add concurrency group to cancel duplicate runs on same PR - Remove credential-clearing step that wiped git user identity - Remove raw PR body/title interpolation that stripped parenthesized file names via shell interpretation; use gh pr view instead - Remove pyproject.toml from paths-ignore so config PRs get reviewed - Quote shell variable assignments to prevent word splitting
Code Review SummaryThis is a well-implemented addition of a BUILD validation gate. The code follows existing patterns and has good test coverage. ✅ All Issues AddressedThe previous reviews identified several issues, and they have all been resolved:
Design Observations (Not Issues, Just Notes)The build gate checks for config file existence but doesn't validate against task files like other gates do. This appears intentional and sensible:
The decision to run BUILD even for documentation-only tasks (if config exists) is sound - a broken pyproject.toml would eventually cause problems anyway. ✅ Test Coverage Excellent14 new tests provide comprehensive coverage:
All 298 lib tests pass, and Ruff lint passes. 🎯 RecommendationAPPROVE - This PR is ready to merge. All previously identified issues have been resolved, the implementation is solid, and test coverage is comprehensive. |
|
Based on my review of PR #303, I can see that the author has already addressed the main issues identified in prior reviews through commit There is one minor documentation issue that wasn't covered in previous reviews: Outdated docstring ( Valid values: ["tests", "types", "coverage", "review", "linting"]Should include Valid values: ["build", "tests", "types", "coverage", "review", "linting"]Overall Assessment: ✅ Approve The implementation:
The only remaining item is updating the docstring to accurately reflect the available checks. This is a documentation-only issue and doesn't affect functionality. |
|
Review comment posted to PR #303. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/lib/quality_gate_tool.py (1)
62-63: Docstring is outdated - missing "build" in valid checks list.The docstring at line 63 still shows the old list without "build":
Valid values: ["tests", "types", "coverage", "review", "linting"]This should be updated to include "build" to match
VALID_CHECKS.📝 Suggested fix
- Valid values: ["tests", "types", "coverage", "review", "linting"] + Valid values: ["build", "tests", "types", "coverage", "review", "linting"]
🧹 Nitpick comments (1)
codeframe/lib/quality_gates.py (1)
800-809: Comment numbering is inconsistent after inserting build gate.Line 800 comments "3. Type check gate" and line 809 comments "3. Skip detection gate" - both say "3." The numbering should be updated to reflect the new order with build gate at position 2.
📝 Suggested fix
- # 3. Type check gate (fast) + # 3. Type check gate (fast) if QualityGateType.TYPE_CHECK in applicable_gates: ... - # 3. Skip detection gate (fast, scans for test skips) + # 4. Skip detection gate (fast, scans for test skips) if QualityGateType.SKIP_DETECTION in applicable_gates:Continue renumbering: Test gate → 5, Coverage gate → 6, Review gate → 7.
1 participant
Summary
BUILDquality gate that validatespyproject.tomlandpackage.jsonconfiguration before running slower gatesChanges
codeframe/core/models.py: AddedBUILDenum value toQualityGateTypecodeframe/lib/quality_gates.py: Implementedrun_build_gate(),_run_python_build()(uv with pip fallback),_run_node_build(),_extract_build_error_summary(), and integrated intorun_all_gates()orchestratorcodeframe/lib/quality_gate_rules.py: Added BUILD to CODE_IMPLEMENTATION, CONFIGURATION, REFACTORING, MIXED categories with skip reasons for DESIGN, DOCUMENTATION, TESTINGcodeframe/lib/quality_gate_tool.py: Added "build" to valid checks mappingTest plan
TestBuildGateclass (valid/invalid python, valid/invalid node, no config, hatchling detection, uv→pip fallback)TestQualityGateRulesfor BUILD gate applicability per categorySummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.