Skip to content

Commit 23cd35f

Browse files
frankbriaTest User
andauthored
feat(gates): add BUILD validation gate for config errors (#303)
* feat(gates): add BUILD validation gate to catch config errors early 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. * fix: address PR review feedback for BUILD gate - 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 * fix(ci): fix opencode-review workflow reliability issues - 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 --------- Co-authored-by: Test User <test@example.com>
1 parent 499f56d commit 23cd35f

7 files changed

Lines changed: 383 additions & 45 deletions

File tree

.github/workflows/opencode-review.yml

Lines changed: 11 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,17 @@ name: OpenCode PR Review
33
on:
44
pull_request:
55
types: [opened, synchronize]
6-
# Skip review for documentation and config-only changes
6+
# Skip review for documentation-only changes
77
# Exclude this workflow file to prevent self-triggering loops
88
paths-ignore:
99
- "**/*.md"
1010
- ".github/workflows/opencode-review.yml"
1111
- ".gitignore"
12-
- "pyproject.toml"
12+
13+
# Cancel in-progress runs for the same PR to avoid duplicate reviews
14+
concurrency:
15+
group: opencode-review-${{ github.event.pull_request.number }}
16+
cancel-in-progress: true
1317

1418
jobs:
1519
opencode-review:
@@ -25,10 +29,10 @@ jobs:
2529
- name: Calculate total changes
2630
id: calc
2731
run: |
28-
additions=${{ github.event.pull_request.additions }}
29-
deletions=${{ github.event.pull_request.deletions }}
32+
additions="${{ github.event.pull_request.additions }}"
33+
deletions="${{ github.event.pull_request.deletions }}"
3034
total=$((additions + deletions))
31-
echo "total=$total" >> $GITHUB_OUTPUT
35+
echo "total=$total" >> "$GITHUB_OUTPUT"
3236
3337
- name: Checkout repository
3438
# Only review substantial changes (5+ files OR 20+ lines changed)
@@ -40,31 +44,6 @@ jobs:
4044
fetch-depth: 1
4145
persist-credentials: false
4246

43-
- name: Clear git credentials to avoid duplicate auth
44-
if: |
45-
github.event.pull_request.changed_files >= 5 ||
46-
steps.calc.outputs.total >= 20
47-
run: |
48-
# Clear all GitHub-related git config to prevent auth conflicts
49-
git config --global --unset-all http.https://github.com/.extraheader || true
50-
git config --local --unset-all http.https://github.com/.extraheader || true
51-
git config --global --unset-all credential.helper || true
52-
git config --local --unset-all credential.helper || true
53-
git config --global --unset-all credential."https://github.com".helper || true
54-
git config --local --unset-all credential."https://github.com".helper || true
55-
# Remove any credential URLs
56-
git config --global --unset-all credential.url || true
57-
git config --local --unset-all credential.url || true
58-
# Clear any includeIf configs that might add credentials
59-
# Note: git config doesn't support wildcards, so we iterate over matching keys
60-
# Use case-insensitive grep to catch both "includeIf" and "includeif"
61-
for key in $(git config --global --list --name-only 2>/dev/null | grep -i "^includeif\." || true); do
62-
git config --global --unset "$key" || true
63-
done
64-
for key in $(git config --local --list --name-only 2>/dev/null | grep -i "^includeif\." || true); do
65-
git config --local --unset "$key" || true
66-
done
67-
6847
- name: Run OpenCode PR Review
6948
# Only review substantial changes (5+ files OR 20+ lines changed)
7049
if: |
@@ -74,18 +53,14 @@ jobs:
7453
env:
7554
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
7655
ZHIPU_API_KEY: ${{ secrets.ZHIPU_API_KEY }}
77-
# Pass PR context as environment variables for the review
78-
PR_NUMBER: ${{ github.event.pull_request.number }}
79-
PR_TITLE: ${{ github.event.pull_request.title }}
80-
PR_BODY: ${{ github.event.pull_request.body }}
81-
REPO_NAME: ${{ github.repository }}
8256
with:
8357
model: zai-coding-plan/glm-4.7
8458
use_github_token: true
8559
prompt: |
8660
You are reviewing PR #${{ github.event.pull_request.number }} in repository ${{ github.repository }}.
8761
88-
PR TITLE: ${{ github.event.pull_request.title }}
62+
Use `gh pr view ${{ github.event.pull_request.number }}` to read the PR title and description.
63+
Use `gh pr diff ${{ github.event.pull_request.number }}` to read the changed files.
8964
9065
Please review this pull request and provide feedback on:
9166
- Code quality and best practices

codeframe/core/models.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ class ReviewCategory(str, Enum):
158158
class QualityGateType(str, Enum):
159159
"""Types of quality gates (Sprint 10)."""
160160

161+
BUILD = "build"
161162
TESTS = "tests"
162163
TYPE_CHECK = "type_check"
163164
COVERAGE = "coverage"

codeframe/lib/quality_gate_rules.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
# Based on the Quality Gate Applicability Matrix
3030
_GATE_RULES: dict[TaskCategory, List[QualityGateType]] = {
3131
TaskCategory.CODE_IMPLEMENTATION: [
32+
QualityGateType.BUILD,
3233
QualityGateType.TESTS,
3334
QualityGateType.COVERAGE,
3435
QualityGateType.TYPE_CHECK,
@@ -43,6 +44,7 @@
4344
QualityGateType.LINTING,
4445
],
4546
TaskCategory.CONFIGURATION: [
47+
QualityGateType.BUILD,
4648
QualityGateType.TYPE_CHECK,
4749
QualityGateType.LINTING,
4850
],
@@ -52,6 +54,7 @@
5254
QualityGateType.SKIP_DETECTION,
5355
],
5456
TaskCategory.REFACTORING: [
57+
QualityGateType.BUILD,
5558
QualityGateType.TESTS,
5659
QualityGateType.COVERAGE,
5760
QualityGateType.TYPE_CHECK,
@@ -60,6 +63,7 @@
6063
QualityGateType.SKIP_DETECTION,
6164
],
6265
TaskCategory.MIXED: [
66+
QualityGateType.BUILD,
6367
QualityGateType.TESTS,
6468
QualityGateType.COVERAGE,
6569
QualityGateType.TYPE_CHECK,
@@ -72,13 +76,15 @@
7276
# Skip reasons for each category/gate combination
7377
_SKIP_REASONS: dict[TaskCategory, dict[QualityGateType, str]] = {
7478
TaskCategory.DESIGN: {
79+
QualityGateType.BUILD: "Design tasks do not produce buildable artifacts",
7580
QualityGateType.TESTS: "Design tasks do not produce executable code to test",
7681
QualityGateType.COVERAGE: "Design tasks do not produce code requiring coverage",
7782
QualityGateType.TYPE_CHECK: "Design tasks do not produce typed code",
7883
QualityGateType.LINTING: "Design tasks may not produce lintable code",
7984
QualityGateType.SKIP_DETECTION: "Design tasks do not include test files",
8085
},
8186
TaskCategory.DOCUMENTATION: {
87+
QualityGateType.BUILD: "Documentation tasks do not require build validation",
8288
QualityGateType.TESTS: "Documentation tasks do not produce executable code to test",
8389
QualityGateType.COVERAGE: "Documentation tasks do not produce code requiring coverage",
8490
QualityGateType.TYPE_CHECK: "Documentation tasks do not produce typed code",
@@ -92,6 +98,7 @@
9298
QualityGateType.SKIP_DETECTION: "Configuration tasks do not include test files",
9399
},
94100
TaskCategory.TESTING: {
101+
QualityGateType.BUILD: "Testing tasks typically don't modify build configuration",
95102
QualityGateType.TYPE_CHECK: "Test code may use dynamic patterns that fail type checks",
96103
QualityGateType.LINTING: "Test code may use patterns that trigger linting warnings",
97104
QualityGateType.CODE_REVIEW: "Test code is validated through execution rather than review",

codeframe/lib/quality_gate_tool.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,11 @@
3131
logger = logging.getLogger(__name__)
3232

3333
# Valid quality gate check names
34-
VALID_CHECKS = ["tests", "types", "coverage", "review", "linting"]
34+
VALID_CHECKS = ["build", "tests", "types", "coverage", "review", "linting"]
3535

3636
# Mapping from check names to QualityGateType enum
3737
CHECK_NAME_TO_GATE = {
38+
"build": QualityGateType.BUILD,
3839
"tests": QualityGateType.TESTS,
3940
"types": QualityGateType.TYPE_CHECK,
4041
"coverage": QualityGateType.COVERAGE,
@@ -230,6 +231,7 @@ async def _run_specific_gates(
230231
# Run each requested gate
231232
for check in checks:
232233
gate_method = {
234+
"build": quality_gates.run_build_gate,
233235
"tests": quality_gates.run_tests_gate,
234236
"types": quality_gates.run_type_check_gate,
235237
"coverage": quality_gates.run_coverage_gate,

codeframe/lib/quality_gates.py

Lines changed: 150 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,74 @@ async def run_linting_gate(self, task: Task, task_category: Optional[str] = None
508508

509509
return result
510510

511+
async def run_build_gate(self, task: Task, task_category: Optional[str] = None) -> QualityGateResult:
512+
"""Execute build validation gate - verify project configuration files are valid.
513+
514+
This gate validates that pyproject.toml and/or package.json can be parsed
515+
and dependencies resolved, catching configuration errors before slower gates run.
516+
517+
Detection Logic:
518+
- If pyproject.toml exists → run uv sync --no-install-project (fallback: pip)
519+
- If package.json exists → run npm install --dry-run --ignore-scripts
520+
521+
Args:
522+
task: Task to validate
523+
task_category: Optional category string for logging
524+
525+
Returns:
526+
QualityGateResult with pass/fail status
527+
"""
528+
start_time = datetime.now(timezone.utc)
529+
failures: List[QualityGateFailure] = []
530+
531+
has_python = (self.project_root / "pyproject.toml").exists()
532+
has_node = (self.project_root / "package.json").exists()
533+
534+
if has_python:
535+
build_result = self._run_python_build()
536+
if build_result["returncode"] != 0:
537+
failures.append(
538+
QualityGateFailure(
539+
gate=QualityGateType.BUILD,
540+
reason=f"Build validation failed for pyproject.toml: {build_result['summary']}",
541+
details=build_result["output"],
542+
severity=Severity.HIGH,
543+
)
544+
)
545+
546+
if has_node:
547+
build_result = self._run_node_build()
548+
if build_result["returncode"] != 0:
549+
failures.append(
550+
QualityGateFailure(
551+
gate=QualityGateType.BUILD,
552+
reason=f"Build validation failed for package.json: {build_result['summary']}",
553+
details=build_result["output"],
554+
severity=Severity.HIGH,
555+
)
556+
)
557+
558+
execution_time = (datetime.now(timezone.utc) - start_time).total_seconds()
559+
560+
status = "passed" if len(failures) == 0 else "failed"
561+
result = QualityGateResult(
562+
task_id=task.id, # type: ignore[arg-type]
563+
status=status,
564+
failures=failures,
565+
execution_time_seconds=execution_time,
566+
)
567+
568+
self.db.update_quality_gate_status(
569+
task_id=task.id, # type: ignore[arg-type]
570+
status=status,
571+
failures=failures,
572+
)
573+
574+
if not result.passed:
575+
self._create_quality_blocker(task, failures, task_category)
576+
577+
return result
578+
511579
async def run_skip_detection_gate(self, task: Task, task_category: Optional[str] = None) -> QualityGateResult:
512580
"""Execute skip pattern detection gate - detect test skips across all languages.
513581
@@ -720,7 +788,16 @@ async def run_all_gates(self, task: Task) -> QualityGateResult:
720788
logger.info(f"Skipping linting gate for task {task.id}: {reason}")
721789
skipped_gates.append(QualityGateType.LINTING.value)
722790

723-
# 2. Type check gate (fast)
791+
# 2. Build gate (fast, validates configuration)
792+
if QualityGateType.BUILD in applicable_gates:
793+
build_result = await self.run_build_gate(task, category.value)
794+
all_failures.extend(build_result.failures)
795+
else:
796+
reason = rules.get_skip_reason(category, QualityGateType.BUILD)
797+
logger.info(f"Skipping build gate for task {task.id}: {reason}")
798+
skipped_gates.append(QualityGateType.BUILD.value)
799+
800+
# 3. Type check gate (fast)
724801
if QualityGateType.TYPE_CHECK in applicable_gates:
725802
type_check_result = await self.run_type_check_gate(task, category.value)
726803
all_failures.extend(type_check_result.failures)
@@ -1092,10 +1169,81 @@ def _run_eslint(self) -> Dict[str, Any]:
10921169
"summary": "Skipped",
10931170
}
10941171

1172+
def _run_python_build(self) -> Dict[str, Any]:
1173+
"""Run Python build validation using uv (fallback to pip)."""
1174+
try:
1175+
result = subprocess.run(
1176+
["uv", "sync", "--no-install-project"],
1177+
cwd=str(self.project_root),
1178+
capture_output=True,
1179+
text=True,
1180+
timeout=60,
1181+
)
1182+
output = result.stdout + result.stderr
1183+
summary = self._extract_build_error_summary(output, "pyproject.toml")
1184+
return {"returncode": result.returncode, "output": output, "summary": summary}
1185+
except FileNotFoundError:
1186+
# uv not available, try pip
1187+
try:
1188+
result = subprocess.run(
1189+
["pip", "install", "-e", ".", "--dry-run"],
1190+
cwd=str(self.project_root),
1191+
capture_output=True,
1192+
text=True,
1193+
timeout=60,
1194+
)
1195+
output = result.stdout + result.stderr
1196+
summary = self._extract_build_error_summary(output, "pyproject.toml")
1197+
return {"returncode": result.returncode, "output": output, "summary": summary}
1198+
except FileNotFoundError:
1199+
return {"returncode": 0, "output": "No Python build tool found, skipping", "summary": "Skipped"}
1200+
except subprocess.TimeoutExpired:
1201+
return {"returncode": 1, "output": "pip install timed out after 60 seconds", "summary": "Timeout"}
1202+
except subprocess.TimeoutExpired:
1203+
return {"returncode": 1, "output": "Build validation timed out after 60 seconds", "summary": "Timeout"}
1204+
1205+
def _run_node_build(self) -> Dict[str, Any]:
1206+
"""Run Node.js build validation using npm."""
1207+
try:
1208+
result = subprocess.run(
1209+
["npm", "install", "--dry-run", "--ignore-scripts"],
1210+
cwd=str(self.project_root),
1211+
capture_output=True,
1212+
text=True,
1213+
timeout=60,
1214+
)
1215+
output = result.stdout + result.stderr
1216+
summary = self._extract_build_error_summary(output, "package.json")
1217+
return {"returncode": result.returncode, "output": output, "summary": summary}
1218+
except FileNotFoundError:
1219+
return {"returncode": 0, "output": "npm not found, skipping", "summary": "Skipped"}
1220+
except subprocess.TimeoutExpired:
1221+
return {"returncode": 1, "output": "npm install timed out after 60 seconds", "summary": "Timeout"}
1222+
10951223
# ========================================================================
10961224
# Helper Methods - Output Parsing
10971225
# ========================================================================
10981226

1227+
def _extract_build_error_summary(self, output: str, config_file: str) -> str:
1228+
"""Extract summary from build validation output."""
1229+
if not output.strip():
1230+
return "No errors"
1231+
# Detect common build error patterns
1232+
if "hatchling" in output.lower():
1233+
match = re.search(r"Invalid section \[([^\]]+)\]", output)
1234+
if match:
1235+
return f"Invalid section [{match.group(1)}] in {config_file}"
1236+
return f"hatchling build error in {config_file}"
1237+
if "invalid toml" in output.lower() or "failed to parse" in output.lower():
1238+
return f"Invalid TOML syntax in {config_file}"
1239+
if "invalid package.json" in output.lower() or "invalid json" in output.lower():
1240+
return f"Invalid JSON in {config_file}"
1241+
if "npm err" in output.lower():
1242+
return f"npm dependency resolution error in {config_file}"
1243+
# Generic fallback
1244+
first_line = output.strip().split("\n")[0][:120]
1245+
return first_line
1246+
10991247
def _extract_pytest_summary(self, output: str) -> str:
11001248
"""Extract summary from pytest output."""
11011249
# Look for "N passed, M failed" pattern
@@ -1246,7 +1394,7 @@ def _get_category_guidance(self, task_category: str) -> str:
12461394
),
12471395
"configuration": (
12481396
"This task was classified as a configuration task. "
1249-
"Only linting and type checking gates apply."
1397+
"Build validation, linting, and type checking gates apply."
12501398
),
12511399
"testing": (
12521400
"This task was classified as a testing task. "

0 commit comments

Comments
 (0)