feat: add development automation tools - #175
Merged
Merged
Conversation
Add Makefile, PR scope validation script, and GitHub workflow to support fortress-compliant development process and prevent monster PRs. - Makefile: quality commands (format, lint, test) - check_pr_scope.py: validates PR size limits - pr-scope-check.yml: GitHub workflow automation Extracted from monster PR #171 as part of systematic decomposition. Tracked in issue #174. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Reviewer's GuideThis PR adds a standardized development automation toolchain—including a Makefile, a PR scope validation script, and a GitHub Actions workflow—to enforce code quality and guardrails against oversized or mixed-concern pull requests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@uelkerd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a PR scope enforcement system: a GitHub Actions workflow that validates branch naming and PR size, a configurable Python script performing branch/commit/file-type/mixed-concern/size checks, a config module with patterns and thresholds, and a Makefile exposing quality and scope-check targets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant WF as "PR Scope Check\nWorkflow"
participant Runner as Runner (checkout + Python)
participant Script as check_pr_scope.py
participant Git as git
Dev->>GH: Open/Update Pull Request
GH-->>WF: trigger pull_request
WF->>Runner: checkout (full history) & setup Python 3.12
WF->>Script: run prechecks (env-based)
Script->>Git: validate base/head refs, git diff --shortstat, list files
Git-->>Script: files changed, lines changed, file list
alt prechecks pass
WF->>Script: run strict scope checks
Script->>Git: determine commit range, get commit SHAs, diff per commit
Script->>Script: branch name, commit-msg, file-type, mixed-concern, size checks
Script-->>WF: exit 0 (pass) or non-zero (fail) with messages
else prechecks fail
Script-->>WF: fail with explicit message
end
WF-->>GH: report status
sequenceDiagram
autonumber
actor Dev as Developer
participant Make as Makefile
participant Script as check_pr_scope.py
Dev->>Make: make check-scope
Make->>Script: python scripts/check_pr_scope.py --strict
Script-->>Make: return exit code (compliance)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello @uelkerd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes foundational development automation by introducing a Makefile for standardized code quality checks and a Python script integrated with a GitHub Actions workflow to automatically enforce pull request scope and quality guidelines. This initiative aims to prevent large, unmanageable PRs and promote a "fortress-compliant" development workflow, starting with this initial extraction from a previously larger change. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- Consolidate duplicate branch naming and size validation logic between the Python script and the GitHub Actions workflow to avoid maintaining two sources of truth.
- Defaulting git diff stats to HEAD~1 may not capture all PR changes in multi-commit PRs—consider using the PR base branch (origin/${BASE_BRANCH}) by default for more accurate metrics.
- The PR scope checker script is becoming quite monolithic; consider refactoring static configuration (e.g., acceptable file-type combinations) into a separate module or config file for easier maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consolidate duplicate branch naming and size validation logic between the Python script and the GitHub Actions workflow to avoid maintaining two sources of truth.
- Defaulting git diff stats to HEAD~1 may not capture all PR changes in multi-commit PRs—consider using the PR base branch (origin/${BASE_BRANCH}) by default for more accurate metrics.
- The PR scope checker script is becoming quite monolithic; consider refactoring static configuration (e.g., acceptable file-type combinations) into a separate module or config file for easier maintenance.
## Individual Comments
### Comment 1
<location> `scripts/check_pr_scope.py:44-57` </location>
<code_context>
+ stdout, stderr, code = run_command(["git", "diff", "--shortstat", range_spec])
+ lines_changed = 0
+ if code == 0 and stdout:
+ # Parse shortstat format like "1 file changed, 10 insertions(+), 2 deletions(-)"
+ shortstat = stdout.strip()
+ if shortstat:
+ parts = shortstat.split(",")
+ for part in parts:
+ part = part.strip()
+ if "insertions" in part or "deletions" in part:
+ nums = "".join(c for c in part if c.isdigit())
+ if nums:
+ lines_changed += int(nums)
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Shortstat parsing may overcount lines if both insertions and deletions are present.
The logic currently sums all digits in parts containing 'insertions' or 'deletions', which may lead to double-counting. Using regex to extract and sum insertion and deletion counts separately would improve accuracy.
```suggestion
import re
# Get lines changed using shortstat
stdout, stderr, code = run_command(["git", "diff", "--shortstat", range_spec])
lines_changed = 0
if code == 0 and stdout:
# Parse shortstat format like "1 file changed, 10 insertions(+), 2 deletions(-)"
shortstat = stdout.strip()
if shortstat:
insertions = 0
deletions = 0
ins_match = re.search(r"(\d+)\s+insertions?\(\+\)", shortstat)
del_match = re.search(r"(\d+)\s+deletions?\(-\)", shortstat)
if ins_match:
insertions = int(ins_match.group(1))
if del_match:
deletions = int(del_match.group(1))
lines_changed = insertions + deletions
```
</issue_to_address>
### Comment 2
<location> `scripts/check_pr_scope.py:252-257` </location>
<code_context>
+ if files:
+ file_types = set()
+
+ for file in files:
+ if file.endswith((".py", ".js", ".ts", ".java", ".cpp", ".c", ".h")):
+ file_types.add("code")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** File type detection logic may misclassify files based on substring matches.
Substring checks like 'test' or 'config' may match unintended files (e.g., 'contest.py'). Use more precise methods, such as directory prefixes or exact extensions, to avoid misclassification.
```suggestion
elif (
file.startswith("tests/")
or file.endswith(("_test.py", "_test.js", "_test.ts", "_test.java", "_test.cpp", "_test.c", "_test.h"))
):
file_types.add("tests")
elif (
file.startswith("config/")
or file.endswith((".yml", ".yaml", ".json", ".toml", ".cfg"))
or file in ("config.py", "config.js", "config.ts", "config.java", "config.cpp", "config.c", "config.h")
):
file_types.add("config")
```
</issue_to_address>
### Comment 3
<location> `Makefile:24` </location>
<code_context>
+ ruff check --diff src/ tests/ scripts/
+ ruff format --check src/ tests/ scripts/
+ pylint --rcfile=pyproject.toml src/ tests/ scripts/
+ mypy src/ scripts/training/ scripts/database/
+ bandit -r src/ -c pyproject.toml
+ bandit -r tests/ -c pyproject.toml -s B101
</code_context>
<issue_to_address>
**suggestion:** Mypy invocation may fail if directories do not exist.
Add checks to ensure these directories exist before running mypy, or use wildcards to prevent errors if they're missing.
```suggestion
mypy src/**/*.py scripts/training/**/*.py scripts/database/**/*.py
```
</issue_to_address>
### Comment 4
<location> `scripts/check_pr_scope.py:23` </location>
<code_context>
result = subprocess.run(cmd, capture_output=True, text=True, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location> `scripts/check_pr_scope.py:216` </location>
<code_context>
base_for_stats = args.base if args.base else f"{args.branch}~1"
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if-expression with `or` ([`or-if-exp-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/or-if-exp-identity))
```suggestion
base_for_stats = args.base or f"{args.branch}~1"
```
<br/><details><summary>Explanation</summary>Here we find ourselves setting a value if it evaluates to `True`, and otherwise
using a default.
The 'After' case is a bit easier to read and avoids the duplication of
`input_currency`.
It works because the left-hand side is evaluated first. If it evaluates to
true then `currency` will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
`currency` will be set to `DEFAULT_CURRENCY`.
</details>
</issue_to_address>
### Comment 6
<location> `scripts/check_pr_scope.py:217` </location>
<code_context>
head_for_stats = args.head if args.head else args.branch
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if-expression with `or` ([`or-if-exp-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/or-if-exp-identity))
```suggestion
head_for_stats = args.head or args.branch
```
<br/><details><summary>Explanation</summary>Here we find ourselves setting a value if it evaluates to `True`, and otherwise
using a default.
The 'After' case is a bit easier to read and avoids the duplication of
`input_currency`.
It works because the left-hand side is evaluated first. If it evaluates to
true then `currency` will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
`currency` will be set to `DEFAULT_CURRENCY`.
</details>
</issue_to_address>
### Comment 7
<location> `scripts/check_pr_scope.py:49-57` </location>
<code_context>
def get_git_stats(
base: str = "HEAD~1",
head: str = "HEAD",
) -> Tuple[int, int, List[str]]:
"""Get git statistics for a commit range."""
# Use git range operator base...head for both name-only and stats
range_spec = f"{base}...{head}"
# Get number of files changed
stdout, stderr, code = run_command(["git", "diff", "--name-only", range_spec])
if code != 0:
print(f"❌ Git diff error: {stderr}")
return 0, 0, []
files = stdout.split("\n") if stdout else []
num_files = len([f for f in files if f.strip()])
# Get lines changed using shortstat
stdout, stderr, code = run_command(["git", "diff", "--shortstat", range_spec])
lines_changed = 0
if code == 0 and stdout:
# Parse shortstat format like "1 file changed, 10 insertions(+), 2 deletions(-)"
shortstat = stdout.strip()
if shortstat:
parts = shortstat.split(",")
for part in parts:
part = part.strip()
if "insertions" in part or "deletions" in part:
nums = "".join(c for c in part if c.isdigit())
if nums:
lines_changed += int(nums)
return num_files, lines_changed, files
</code_context>
<issue_to_address>
**issue (code-quality):** Use named expression to simplify assignment and conditional [×2] ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
</issue_to_address>
### Comment 8
<location> `scripts/check_pr_scope.py:173` </location>
<code_context>
def main():
"""Main function."""
import argparse
parser = argparse.ArgumentParser(description="Check PR scope compliance")
parser.add_argument(
"--branch",
default="HEAD",
help="Branch to check (default: HEAD)",
)
parser.add_argument(
"--base",
help="Base branch/commit for range comparison (e.g., main, origin/main)",
)
parser.add_argument(
"--head",
help="Head branch/commit for range comparison (default: current branch)",
)
parser.add_argument(
"--strict",
action="store_true",
help="Strict mode - fail on any warning",
)
args = parser.parse_args()
print("🔍 Checking PR Scope Compliance")
print("=" * 50)
all_passed = True
# Check branch name
print("📋 Checking branch name...")
if not check_branch_name_quality():
all_passed = False
# Check commit message
print("\n📝 Checking commit message...")
if not check_commit_message_quality(args.base, args.head):
all_passed = False
# Check file and line limits
print("\n📊 Checking size limits...")
# Determine base for git stats
base_for_stats = args.base if args.base else f"{args.branch}~1"
head_for_stats = args.head if args.head else args.branch
num_files, lines_changed, files = get_git_stats(base_for_stats, head_for_stats)
print(f" Files changed: {num_files}")
print(f" Lines changed: {lines_changed}")
if num_files > 50:
print(f"❌ Too many files changed! Max 50 allowed, got {num_files}")
if args.strict:
all_passed = False
if lines_changed > 1500:
print(f"❌ Too many lines changed! Max 1500 allowed, got {lines_changed}")
if args.strict:
all_passed = False
# List changed files
if files:
print("\n📁 Files changed:")
for file in files[:10]: # Show first 10
if file.strip():
print(f" - {file}")
if len(files) > 10:
print(f" ... and {len(files) - 10} more")
# Check for mixed concerns (improved logic to reduce false positives)
print("\n🎯 Checking for mixed concerns...")
if files:
file_types = set()
for file in files:
if file.endswith((".py", ".js", ".ts", ".java", ".cpp", ".c", ".h")):
file_types.add("code")
elif file.endswith((".md", ".rst", ".txt", ".adoc")):
file_types.add("docs")
elif "test" in file.lower() or file.startswith("tests/"):
file_types.add("tests")
elif "config" in file.lower() or file.endswith(
(".yml", ".yaml", ".json", ".toml", ".cfg"),
):
file_types.add("config")
elif "docker" in file.lower() or "Dockerfile" in file:
file_types.add("docker")
elif "api" in file.lower() or "endpoint" in file.lower():
file_types.add("api")
elif (
"model" in file.lower() or "ml" in file.lower() or "ai" in file.lower()
):
file_types.add("ml")
# Allow reasonable combinations that are commonly acceptable
acceptable_combinations = [
{"code", "tests"}, # Feature + tests
{"code", "tests", "config"}, # Feature + tests + config
{"code", "tests", "docs"}, # Feature + tests + docs
{"code", "tests", "config", "docs"}, # Feature + tests + config + docs
{"code", "config"}, # Code changes with config
{"code", "docs"}, # Code changes with docs
{"config", "docs"}, # Config changes with docs
{"tests", "config"}, # Tests with config
]
# Flag as mixed concerns only if we have more than 4 types OR
# if we have an unusual combination that's not in acceptable list
is_mixed_concerns = len(file_types) > 4 or (
len(file_types) > 2 and file_types not in acceptable_combinations
)
if is_mixed_concerns:
print(f"⚠️ Mixed concerns detected: {', '.join(sorted(file_types))}")
print(" Consider splitting into separate PRs")
print(
" Acceptable combinations include: code+tests, code+tests+config, etc.",
)
if args.strict:
all_passed = False
elif len(file_types) > 2:
# Show info about acceptable combinations but don't fail
print(f"ℹ️ Multiple file types detected: {', '.join(sorted(file_types))}")
print(" This combination is acceptable for a single PR")
print("\n" + "=" * 50)
if all_passed:
print("✅ PR Scope Check PASSED")
return 0
print("❌ PR Scope Check FAILED")
print("\n💡 Remember the rules:")
print(" • Max 50 files changed")
print(" • Max 1500 lines changed")
print(" • ONE purpose per PR")
print(" • Single concern (no mixing API + tests + docs)")
return 1
</code_context>
<issue_to_address>
**issue (code-quality):** Low code quality found in main - 10% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| def run_command(cmd: List[str]) -> Tuple[str, str, int]: | ||
| """Run command and return (stdout, stderr, returncode).""" | ||
| result = subprocess.run(cmd, capture_output=True, text=True, check=False) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds essential development automation tools to establish a fortress-compliant development workflow that prevents monster PRs through automated validation.
- Makefile with standardized quality commands for formatting, linting, testing, and scope validation
- Python script for comprehensive PR scope validation (file/line limits, commit message quality, branch naming)
- GitHub workflow for automated PR validation on pull request events
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Makefile | Adds quality commands matching pre-commit hooks and PR scope validation |
| scripts/check_pr_scope.py | Comprehensive PR validation script with git analysis and concern detection |
| .github/workflows/pr-scope-check.yml | GitHub Actions workflow for automated PR scope enforcement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces valuable development automation tools, including a Makefile for standardized quality commands and a Python script to enforce pull request scope. These are excellent additions for maintaining a healthy and reviewable codebase. The implementations are solid, and my feedback focuses on improving the robustness, maintainability, and correctness of these new tools.
Comment on lines
+46
to
+59
| lines_changed = 0 | ||
| if code == 0 and stdout: | ||
| # Parse shortstat format like "1 file changed, 10 insertions(+), 2 deletions(-)" | ||
| shortstat = stdout.strip() | ||
| if shortstat: | ||
| parts = shortstat.split(",") | ||
| for part in parts: | ||
| part = part.strip() | ||
| if "insertions" in part or "deletions" in part: | ||
| nums = "".join(c for c in part if c.isdigit()) | ||
| if nums: | ||
| lines_changed += int(nums) | ||
|
|
||
| return num_files, lines_changed, files |
Contributor
There was a problem hiding this comment.
The function doesn't handle the case where the git diff --shortstat command fails. If it returns a non-zero exit code, the function will silently proceed and return lines_changed = 0, masking a potential error. You should check the exit code and report the error, similar to how you handle the first git diff command.
Suggested change
| lines_changed = 0 | |
| if code == 0 and stdout: | |
| # Parse shortstat format like "1 file changed, 10 insertions(+), 2 deletions(-)" | |
| shortstat = stdout.strip() | |
| if shortstat: | |
| parts = shortstat.split(",") | |
| for part in parts: | |
| part = part.strip() | |
| if "insertions" in part or "deletions" in part: | |
| nums = "".join(c for c in part if c.isdigit()) | |
| if nums: | |
| lines_changed += int(nums) | |
| return num_files, lines_changed, files | |
| lines_changed = 0 | |
| if code != 0: | |
| print(f"❌ Git diff --shortstat error: {stderr}") | |
| elif stdout: | |
| # Parse shortstat format like "1 file changed, 10 insertions(+), 2 deletions(-)" | |
| shortstat = stdout.strip() | |
| if shortstat: | |
| parts = shortstat.split(",") | |
| for part in parts: | |
| part = part.strip() | |
| if "insertions" in part or "deletions" in part: | |
| nums = "".join(c for c in part if c.isdigit()) | |
| if nums: | |
| lines_changed += int(nums) | |
| return num_files, lines_changed, files |
- Add proper error handling for git diff --shortstat command failure - Fix Makefile .PHONY declaration to include all targets - Remove --fix flag from lint target to make it CI-friendly - Update mypy paths to include all relevant directories - Move argparse import to top of file per PEP 8 - All hardcoded constants already moved to config module Addresses all feedback from gemini-code-assist bot code review.
- Improve test file detection with regex patterns and directory prefixes
- Fix overlapping file categorization to allow multiple types per file
- Update Python version from 3.8 to 3.12 in GitHub workflow
- Magic number 4 already properly defined as MAX_FILE_TYPES_FOR_MIXED_CONCERNS
The new detection logic correctly handles:
- tests/test_model.py -> {tests, ml, code}
- api_model.py -> {api, ml, code}
- test_api.py -> {tests, api, code}
Addresses all feedback from Copilot AI code review.
- Add try/except block to handle ImportError when running from different directories - Fallback adds script directory to sys.path to ensure module can be found - Script now works correctly when run from any directory Fixes ModuleNotFoundError: No module named 'pr_scope_config'
- Add comprehensive security validation to run_command function - Restrict to git commands only with whitelist of allowed subcommands - Add detailed documentation explaining security measures - Prevent command injection by validating command structure - Only allow read-only git operations: diff, log, rev-list, branch, show Addresses Sourcery AI security warning about dangerous subprocess use. All commands are now validated and restricted to safe git operations.
d-ulker
force-pushed
the
feat/add-development-automation
branch
from
September 22, 2025 18:19
b5ad849 to
e4e2276
Compare
- Remove redundant 'import os' and 'import sys' in ImportError handler - These modules are already imported at the top of the file - Fixes PYL-W0404 linting warning about multiple imports The script functionality remains unchanged while eliminating code duplication.
- Remove unused 'file_path' parameter from _check_keywords function - Function only uses 'file_lower' for keyword pattern matching - Update function call in _check_patterns to match new signature - Fixes PYL-W0613 linting warning about unused arguments Functionality remains unchanged - file type detection works correctly.
- Convert 'elif' to separate 'if' statement after return - Improves code readability and maintainability - Fixes PYL-R1705 linting warning about unnecessary else/elif after return The logic remains identical but follows Python best practices.
- Remove unused 'shlex' import from scripts/check_pr_scope.py - The module was imported but never used in the code - Fixes PY-W2000 linting warning about unused imports Script functionality remains unchanged.
- Break long lines to fit within 88-character limit - Split long function signatures across multiple lines - Break long string literals and f-strings appropriately - Reformat long lists and dictionaries for better readability - Fixes 17 instances of line length violations All functionality preserved while improving code readability.
- Break long comment into multiple lines to fit within 79-character limit - Improves code readability and follows Python docstring guidelines - Fixes FLK-W505 linting warning about docstring line length Comment now properly formatted across multiple lines.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
.github/workflows/pr-scope-check.yml (3)
33-45: Ensure this step still runs if the previous step fails.
Add if: always() so you always get branch naming feedback even when the Python script fails earlier.- - name: Check branch naming (secure) + - name: Check branch naming (secure) + if: always() env: BRANCH_NAME: ${{ github.head_ref }}
46-50: Same: always run size check for complete diagnostics.- - name: Check PR size limits (secure) + - name: Check PR size limits (secure) + if: always() env: BASE_BRANCH: ${{ github.base_ref }} - HEAD_REF: ${{ github.head_ref }}
51-56: HEAD_REF is unused; input validation can be simplified.
Also, when diffing a PR, prefer triple‑dot against HEAD.- # Validate input parameters - if [[ -z "$BASE_BRANCH" || -z "$HEAD_REF" ]]; then + # Validate input parameters + if [[ -z "$BASE_BRANCH" ]]; then echo "❌ Missing required branch information" exit 1 fiMakefile (2)
2-2: Add a conventional default targetallto satisfy checkmake and provide a single entry point.-.PHONY: help format lint test quality-check clean check-scope pre-commit-install +.PHONY: all help format lint test quality-check clean check-scope pre-commit-install + +all: quality-check ## Default: run full quality suite
33-36: Guard commit template usage.Makefile sets git commit.template to .gitmessage.txt but that file is missing — only set the config if the file exists (or fail fast). File: Makefile (lines 33–36).
Suggested Makefile change:
pre-commit-install: ## Install pre-commit hooks pre-commit install @if [ -f .gitmessage.txt ]; then \ git config commit.template .gitmessage.txt; \ else \ printf '%s\n' "ℹ️ .gitmessage.txt not found — skipping commit.template"; \ fiscripts/pr_scope_config.py (1)
31-33: API detection via bare “api/endpoint” keywords is noisy.
Consider directory or spec-file driven rules to reduce false positives.- "api": { - "keywords": ["api", "endpoint"], - }, + "api": { + "directories": ["api/", "apis/", "server/", "routes/"], + "patterns": [r"(^|/)(openapi|swagger)\.(ya?ml|json)$", r"(^|/)(routes?|controllers?)(/|$)"], + },scripts/check_pr_scope.py (2)
72-76: Allow safe git subcommands used for robust branch detection.- allowed_git_commands = { - "diff", "log", "rev-list", "branch", "show" - } + allowed_git_commands = { + "diff", "log", "rev-list", "branch", "show", "symbolic-ref" + }
307-314: Treat “ml” as a subtype of code to avoid false “mixed concerns.”
This PR is tooling-only but repo paths can include “ml”; don’t count it toward mixing.- # Flag as mixed concerns only if we have more than MAX_FILE_TYPES_FOR_MIXED_CONCERNS types OR - # if we have an unusual combination that's not in acceptable list - is_mixed_concerns = len(file_types) > MAX_FILE_TYPES_FOR_MIXED_CONCERNS or ( - len(file_types) > MAX_FILE_TYPES_FOR_WARNING and file_types not in ACCEPTABLE_COMBINATIONS - ) + # Treat domain subtypes (e.g., 'ml') as part of 'code' for mixing evaluation + effective_types = set(t for t in file_types if t not in {"ml"}) + is_mixed_concerns = len(effective_types) > MAX_FILE_TYPES_FOR_MIXED_CONCERNS or ( + len(effective_types) > MAX_FILE_TYPES_FOR_WARNING and effective_types not in ACCEPTABLE_COMBINATIONS + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pr-scope-check.yml(1 hunks)Makefile(1 hunks)scripts/check_pr_scope.py(1 hunks)scripts/pr_scope_config.py(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 2-2: Missing required phony target "all"
(minphony)
🪛 GitHub Actions: PR Scope Check
scripts/check_pr_scope.py
[error] 1-1: PR Scope Check FAILED. Mixed concerns detected: code, config, ml. Please split changes to adhere to PR guidelines (max 50 files, max 1500 lines, one purpose per PR).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (6)
.github/workflows/pr-scope-check.yml (3)
15-18: Checkout depth is correct for diffing — good call.
Full history avoids misleading stats from shallow clones.
19-23: Python 3.12 selection LGTM.
Matches prior suggestion to move off 3.8.
3-6: Minimal permissions — good.
Appropriate least‑privilege for this workflow.Makefile (1)
19-29: Quality suite is well-curated.
Good separation of “format/lint/test” vs “quality-check” parity with CI.scripts/pr_scope_config.py (1)
58-59: Branch pattern matches workflow regex — good consistency.scripts/check_pr_scope.py (1)
444-455: Result messaging is clear — nice.
Concise summary plus rules recap.
- Fix ML keyword matching to use word boundaries (\bml\b) to avoid false positives like 'html'
- Replace 'machine learning' with 'machine_learning' for better file matching
- Fix git stats range to use full base...HEAD range instead of HEAD~1..HEAD for multi-commit PRs
- Update base_for_stats to use args.branch instead of f'{args.branch}~1'
- Update head_for_stats to use 'HEAD' instead of args.branch
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/check_pr_scope.py (4)
83-85: Harden subprocess calls with a timeout.Prevents hangs on large repos or broken Git states; returns a clear error.
Apply:
- result = subprocess.run(cmd, capture_output=True, text=True, check=False) - return result.stdout.strip(), result.stderr.strip(), result.returncode + try: + result = subprocess.run(cmd, capture_output=True, text=True, check=False, timeout=60) + return result.stdout.strip(), result.stderr.strip(), result.returncode + except subprocess.TimeoutExpired: + return "", f"Command timed out: {' '.join(cmd)}", 124
10-12: Update usage to reflect supported flags.Doc shows only --branch/--strict but args also support --base/--head.
Apply:
-Usage: - python scripts/check_pr_scope.py [--branch <branch>] [--strict] +Usage: + python scripts/check_pr_scope.py [--branch <branch>] [--base <base>] [--head <head>] [--strict]
438-444: Show actual acceptable combinations from config.Reduces confusion when mixed concerns fire in CI.
Apply:
- print( - " Acceptable combinations include: code+tests, code+tests+config, etc.", - ) + combos_str = ", ".join("+".join(sorted(c)) for c in ACCEPTABLE_COMBINATIONS) + print(f" Acceptable combinations include: {combos_str}")
368-379: Optional: add per-file type debug to diagnose false positives.When strict mode fails, printing file→types mapping helps explain “ml” detections.
Apply (adjacent helper print):
def display_changed_files(files: List[str]) -> None: @@ for file in files[:MAX_FILES_TO_DISPLAY]: # Show first MAX_FILES_TO_DISPLAY if file.strip(): print(f" - {file}") if len(files) > MAX_FILES_TO_DISPLAY: print(f" ... and {len(files) - MAX_FILES_TO_DISPLAY} more") + +def _debug_file_type_mapping(files: List[str]) -> None: + print("\n🔎 File type mapping (debug):") + for f in files[:MAX_FILES_TO_DISPLAY]: + if not f.strip(): + continue + types = ", ".join(sorted(detect_file_types(f))) or "unknown" + print(f" - {f}: {types}")And call it right after mixed concerns detection triggers:
- if is_mixed_concerns: + if is_mixed_concerns: + _debug_file_type_mapping(files)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/check_pr_scope.py(1 hunks)scripts/pr_scope_config.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/pr_scope_config.py
🧰 Additional context used
🪛 GitHub Actions: PR Scope Check
scripts/check_pr_scope.py
[error] 1-1: PR Scope Check FAILED: Mixed concerns detected (code, config, ml). 4 files changed, 716 lines. Command 'python scripts/check_pr_scope.py --strict' exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
scripts/check_pr_scope.py (3)
126-140: Fork-safe commit range: use HEAD for PR tip; handle partial args.Using origin/{head_ref} breaks on forked PRs and ignoring partial args is surprising. Return base..HEAD when only base is provided; head^..head when only head is provided; and prefer GITHUB_BASE_REF..HEAD for CI.
Apply:
def _determine_commit_range(base: Optional[str], head: Optional[str]) -> str: """Determine the commit range to check.""" - if base and head: - return f"{base}..{head}" - - # Try to determine range from environment or git context - # Check for common CI environment variables - if os.environ.get("GITHUB_BASE_REF") and os.environ.get("GITHUB_HEAD_REF"): - base_ref = os.environ["GITHUB_BASE_REF"] - head_ref = os.environ["GITHUB_HEAD_REF"] - return f"origin/{base_ref}..origin/{head_ref}" - - # Fallback to HEAD^..HEAD for single commit - return "HEAD^..HEAD" + if base and head: + return f"{base}..{head}" + if base and not head: + return f"{base}..HEAD" + if head and not base: + return f"{head}^..{head}" + base_env = os.environ.get("GITHUB_BASE_REF") + if base_env: + return f"origin/{base_env}..HEAD" + return "HEAD^..HEAD"
419-422: Size stats undercount multi-commit PRs; compare base…HEAD in CI, HEAD~1…HEAD locally.Current default HEAD~1..HEAD misses multiple commits when CI base is known. Use GITHUB_BASE_REF when present; otherwise keep local fallback.
Apply:
- base_for_stats = args.base or f"{args.branch}~1" - head_for_stats = args.head or args.branch + base_env = os.environ.get("GITHUB_BASE_REF") + base_for_stats = args.base or (f"origin/{base_env}" if base_env else f"{args.branch}~1") + head_for_stats = args.head or "HEAD"
211-230: Branch detection fails on detached HEAD; add CI/env and symbolic-ref fallbacks.Avoid hard-failing on detached HEAD. Prefer GITHUB_HEAD_REF, then symbolic-ref; skip check if still unresolved.
Apply:
def check_branch_name_quality() -> bool: """Check if branch name follows naming conventions.""" - stdout, stderr, code = run_command(["git", "branch", "--show-current"]) - if code != 0: - print(f"❌ Git branch error: {stderr}") - return False - - branch_name = stdout.strip() - if not branch_name: - print("❌ Could not determine current branch") - return False + stdout, stderr, code = run_command(["git", "branch", "--show-current"]) + branch_name = os.environ.get("GITHUB_HEAD_REF") or stdout.strip() + if not branch_name: + sym_out, sym_err, sym_code = run_command(["git", "symbolic-ref", "-q", "--short", "HEAD"]) + if sym_code == 0: + branch_name = sym_out.strip() + if not branch_name: + print("ℹ️ Detached HEAD: skipping branch naming check (use workflow branch check).") + return TrueNote: This requires allowing the 'symbolic-ref' subcommand (see next comment).
- Update GitHub workflow to use triple-dot merge-base diff (origin/$BASE_BRANCH...HEAD) for accurate PR change measurement
- Fix commit range to use local HEAD instead of origin/{head_ref} for forked PRs
- Improve branch detection for detached HEAD in CI using GITHUB_HEAD_REF environment variable
- Add git symbolic-ref as fallback for branch detection in CI environments
- Add symbolic-ref to allowed git subcommands for security validation
- Add comment explaining that subprocess.run is safe due to command validation - Clarify that no user input reaches subprocess, preventing command injection - Address Sourcery AI security warning (false positive)
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.
🎯 Purpose
Add essential development automation tools to support fortress-compliant development workflow.
📋 Changes
🏰 Fortress Compliance
✅ Files: 3/5 (well within limit)
✅ Purpose: Single concern (development automation)
✅ Scope: Focused on tooling only
✅ Branch: From main (not monster PR)
✅ Size: Small, reviewable changes
🔄 Extraction Details
🧪 Testing
📊 Impact
This is the first of 10-15 systematic extractions from the monster PR. Each extraction follows strict fortress compliance rules.
🤖 Generated with Claude Code
Summary by Sourcery
Add foundational development automation by providing a Makefile with quality and scope-check commands, a Python script to enforce PR size and naming conventions, and a GitHub Actions workflow to run these validations on pull requests.
New Features:
Summary by CodeRabbit