# code-reviewer Code review automation for TypeScript, JavaScript, Python, Go, Swift, Kotlin, C#, .NET, Java, C, C++, Rust, Ruby, PHP, and Dart/Flutter. Analyzes PRs for complexity and risk, checks code quality for SOLID violations and code smells, generates review reports. Use when reviewing pull requests, analyzing code quality, identifying issues, generating review checklists. - **Kind:** skill - **Source:** https://github.com/alirezarezvani/claude-skills - **Page:** https://forefy.com/skills/f11abc1a-3a33-42af-9da6-46730dd87878 - **API (JSON + files):** https://forefy.com/api/asr/f11abc1a-3a33-42af-9da6-46730dd87878 --- ## README.md # code-reviewer Code review automation for TypeScript, JavaScript, Python, Go, Swift, Kotlin, C#, .NET, Java, C, C++, Rust, Ruby, PHP, and Dart/Flutter. Analyzes PRs for complexity and risk, checks code quality for SOLID violations and code smells, and generates review reports. The full skill spec is [`SKILL.md`](./SKILL.md). This README is a quick reference for the 3 bundled scripts. --- ## How to use ### Quick install check ```bash python scripts/pr_analyzer.py --help python scripts/code_quality_checker.py --help python scripts/review_report_generator.py --help ``` All three scripts are stdlib-only — no `pip install` required. ### Example 1 — review a pull request ```bash # From inside the repo you want to analyze: python /path/to/skills/code-reviewer/scripts/pr_analyzer.py . --base main --head HEAD ``` Outputs: complexity score (1-10), risk categorization (critical / high / medium / low), prioritized review order, commit-message validation. ### Example 2 — score a directory's code quality ```bash python scripts/code_quality_checker.py /path/to/code # Filter by language python scripts/code_quality_checker.py /path/to/code --language csharp # Machine-readable python scripts/code_quality_checker.py /path/to/code --json ``` Outputs: quality score (0-100), letter grade, detected code smells, SOLID violations. ### Example 3 — combine into a review report ```bash python scripts/review_report_generator.py /path/to/repo --format markdown --output review.md ``` Outputs: review verdict (approve / request changes / block), score, prioritized action items. --- ## Examples bundled with the skill | File | Purpose | |------|---------| | [`assets/sample_csharp_smells.cs`](./assets/sample_csharp_smells.cs) | C# file with every C#-specific pattern this skill detects, labelled inline | | [`assets/sample_csharp_clean.cs`](./assets/sample_csharp_clean.cs) | Same code refactored per `rules/universal.md` + `languages/csharp.md` | | [`assets/sample_java_smells.java`](./assets/sample_java_smells.java) | Java file with every Java-specific pattern this skill detects, labelled inline | | [`assets/sample_java_clean.java`](./assets/sample_java_clean.java) | Same code refactored per `rules/universal.md` + `languages/java.md` | | [`assets/sample_c_smells.c`](./assets/sample_c_smells.c) | C file with every C-specific pattern this skill detects, labelled inline | | [`assets/sample_c_clean.c`](./assets/sample_c_clean.c) | Same code refactored per `rules/universal.md` + `languages/c.md` | | [`expected_outputs/*.json`](./expected_outputs/) | Expected `code_quality_checker.py --json` output for each fixture | Use them as a regression-detection harness: ```bash python scripts/code_quality_checker.py assets/sample_java_smells.java --json > /tmp/check.json diff /tmp/check.json expected_outputs/sample_java_smells_quality.json # silence means the detector still behaves as documented ``` --- ## What it detects See [`SKILL.md`](./SKILL.md) for the full pattern list, severity tiers, and references. Quick summary: - **PR Analyzer** (`scripts/pr_analyzer.py`): hardcoded secrets / connection strings, SQL injection, debug statements (`console.*` / `System.out` / `printStackTrace`), analyzer suppressions (ESLint / Roslyn / `@SuppressWarnings`), `any` / `dynamic` overuse, TODO/FIXME, `unsafe` blocks, null-forgiving `!`, `async void`, blocking on `Task`. - **Code Quality Checker** (`scripts/code_quality_checker.py`): long methods, large files, god classes, deep nesting, too many parameters, high cyclomatic complexity, swallowed exceptions, missing `await`, undisposed `IDisposable`, `new HttpClient()` in method body, unused `using` directives. Language-specific smell packs for C# (`async void`, blocking on `Task`), Java (empty catch, `printStackTrace`, swallowed `InterruptedException`, unclosed resources, per-call `ObjectMapper` / `Gson`), and C (banned functions `gets`/`strcpy`/`strcat`/`sprintf`/`vsprintf`, format-string vulnerability `printf(var)`, unbounded `scanf("%s")`, malloc-without-NULL-check, free-without-zeroing, `system()` with non-literal argument). - **Review Report Generator** (`scripts/review_report_generator.py`): combines the above into a single markdown or JSON verdict. --- ## Review rules Rules are split so every review loads exactly two files — the cross-language baseline plus one language guide (see the dispatch table in [`SKILL.md`](./SKILL.md)): - [`rules/universal.md`](./rules/universal.md) — cross-language rules: security, async/concurrency, resource management, exception handling, performance - [`languages/`](./languages/) — one self-contained guide per language (`python`, `typescript`, `go`, `swift`, `kotlin`, `csharp`, `java`, `c`, `cpp`, `rust`, `ruby`, `php`, `dart`), each with Security / Async / Resource Management / Exception Handling / Performance / Idioms sections ## SKILL.md --- name: "code-reviewer" description: Code review automation for TypeScript, JavaScript, Python, Go, Swift, Kotlin, C#, .NET, Java, C, C++, Rust, Ruby, PHP, and Dart/Flutter. Analyzes PRs for complexity and risk, checks code quality for SOLID violations and code smells, generates review reports. Use when reviewing pull requests, analyzing code quality, identifying issues, generating review checklists. --- # Code Reviewer Automated code review tools for analyzing pull requests, detecting code quality issues, and generating review reports. --- ## How This Skill Is Organized ``` code-reviewer/ SKILL.md ← you are here (tools + dispatch table) rules/ universal.md ← security, async, resources, exceptions, performance — all languages languages/ python.md ← Python-specific rules + idioms typescript.md ← TypeScript / JavaScript-specific rules + idioms go.md ← Go-specific rules + idioms swift.md ← Swift-specific rules + idioms kotlin.md ← Kotlin-specific rules + idioms csharp.md ← C# / .NET-specific rules + idioms java.md ← Java-specific rules + idioms c.md ← C -specific rules + idioms cpp.md ← C++ -specific rules + idioms rust.md ← Rust -specific rules + idioms ruby.md ← Ruby -specific rules + idioms php.md ← PHP-specific rules + idioms dart.md ← Dart / Flutter-specific rules + idioms ``` ### Loading order for every review 1. This file (`SKILL.md`) — tools and thresholds 2. `rules/universal.md` — always, for every language 3. The matching `languages/*.md` — one file based on the extension table below That is always exactly **2 additional files**, regardless of scope. | Extension(s) | Load | |---|---| | `.py` | `languages/python.md` | | `.ts`, `.tsx`, `.js`, `.jsx`, `.mjs` | `languages/typescript.md` | | `.go` | `languages/go.md` | | `.swift` | `languages/swift.md` | | `.kt`, `.kts` | `languages/kotlin.md` | | `.cs`, `.csx`, `.razor`, `.cshtml` | `languages/csharp.md` | | `.java` | `languages/java.md` | | `.c`, `.h` | `languages/c.md` | | `.cpp`, `.cc`, `.cxx`, `.hpp`, `.hh`, `.hxx` | `languages/cpp.md` | | `.rs` | `languages/rust.md` | | `.rb`, `.rake`, `.gemspec`, `.ru` | `languages/ruby.md` | | `.php`, `.phtml` | `languages/php.md` | | `.dart` | `languages/dart.md` | --- ## Tools ### PR Analyzer Analyzes git diff between branches to assess review complexity and identify risks. ```bash # Analyze current branch against main python scripts/pr_analyzer.py /path/to/repo # Compare specific branches python scripts/pr_analyzer.py . --base main --head feature-branch # JSON output for integration python scripts/pr_analyzer.py /path/to/repo --json ``` **What it detects (universal — see also language file for language-specific signals):** - Hardcoded secrets (passwords, API keys, tokens, connection strings) - SQL / query injection patterns - Debug statements left in production code - Lint / analyzer suppression annotations - TODO/FIXME comments **Language-specific detections** are defined in each `languages/*.md` file. **Output includes:** - Complexity score (1-10) - Risk categorization (critical, high, medium, low) - File prioritization for review order - Commit message validation --- ### Code Quality Checker Analyzes source code for structural issues, code smells, and SOLID violations. ```bash # Analyze a directory python scripts/code_quality_checker.py /path/to/code # Analyze specific language # Valid values: python, typescript, javascript, go, swift, kotlin, csharp, java, c, cpp, rust, ruby, php, dart python scripts/code_quality_checker.py . --language java # JSON output python scripts/code_quality_checker.py /path/to/code --json ``` **Universal thresholds:** | Issue | Threshold | |-------|-----------| | Long function | >50 lines | | Large file | >500 lines | | God class | >20 methods | | Too many params | >5 | | Deep nesting | >4 levels | | High complexity | >10 branches | Language-specific checks are defined in each `languages/*.md` file. --- ### Review Report Generator Combines PR analysis and code quality findings into structured review reports. ```bash # Generate report for current repo python scripts/review_report_generator.py /path/to/repo # Markdown output python scripts/review_report_generator.py . --format markdown --output review.md # Use pre-computed analyses python scripts/review_report_generator.py . \ --pr-analysis pr_results.json \ --quality-analysis quality_results.json ``` **Verdicts:** | Score | Verdict | |-------|---------| | 90+ with no high issues | Approve | | 75+ with ≤2 high issues | Approve with suggestions | | 50-74 | Request changes | | <50 or critical issues | Block | --- ## Adding a New Language **Reviewer guidance (required):** 1. Create `languages/.md` using any existing language file as a template — it must have sections: PR Analyzer Signals, Code Quality Checks, Security, Async, Resource Management, Exception Handling, Performance, Idioms. 2. Add the extension row to the dispatch table above. That is all the agent-driven review needs. **Deterministic analyzer support (optional, recommended):** the bundled scripts only flag a language they explicitly know. To make `code_quality_checker.py` score the new language: 3. Add the extensions to `LANGUAGE_EXTENSIONS` in `scripts/code_quality_checker.py` (this also adds the `--language` choice). 4. Add `function` / `class` / `method` regex entries for the language in the same file; otherwise it falls back to the Python patterns. 5. Optionally add a `check__specific_smells(...)` detector (see the C#, Java, and C ones) and call it from `analyze_file`. 6. Add `assets/sample__smells.` + `_clean` fixtures and commit the expected `--json` output under `expected_outputs/` as a regression guard. --- ## Regression Fixtures Labelled fixtures live in `assets/` with their committed `--json` output in `expected_outputs/` (C#, Java, and C). Drift from the committed JSON signals a behaviour change in the analyzer: ```bash python scripts/code_quality_checker.py assets/sample_java_smells.java --json \ | diff - expected_outputs/sample_java_smells_quality.json ``` ## assets ``` ``` ## assets/sample_c_clean.c ``` ``` ## assets/sample_c_smells.c ``` ``` ## assets/sample_csharp_clean.cs ``` ``` ## assets/sample_csharp_smells.cs ``` ``` ## assets/sample_java_clean.java ``` ``` ## assets/sample_java_smells.java ``` ``` ## expected_outputs ``` ``` ## expected_outputs/sample_c_clean_quality.json ```json { "file": "/home/user/claude-skills/engineering-team/skills/code-reviewer/assets/sample_c_clean.c", "language": "c", "metrics": { "lines": { "total": 72, "code": 43, "blank": 17, "comment": 12 }, "functions": 4, "classes": 0, "avg_complexity": 1.8 }, "quality_score": 100, "grade": "A", "smells": [ { "type": "long_function", "severity": "medium", "message": "Function 'safe_input' has 61 lines (max: 50)", "location": "safe_input" }, { "type": "magic_number", "severity": "low", "message": "Magic number 100 should be a named constant", "location": "line 29" }, { "type": "magic_number", "severity": "low", "message": "Magic number 100 should be a named constant", "location": "line 68" } ], "solid_violations": [], "function_details": [ { "name": "safe_input", "parameters": 1, "lines": 61, "complexity": 3 }, { "name": "checked_alloc", "parameters": 1, "lines": 31, "complexity": 2 }, { "name": "run_safe_cmd", "parameters": 1, "lines": 15, "complexity": 1 }, { "name": "main", "parameters": 2, "lines": 9, "complexity": 1 } ], "class_details": [] } ``` ## expected_outputs/sample_c_smells_quality.json ```json { "file": "/home/user/claude-skills/engineering-team/skills/code-reviewer/assets/sample_c_smells.c", "language": "c", "metrics": { "lines": { "total": 67, "code": 37, "blank": 17, "comment": 13 }, "functions": 4, "classes": 0, "avg_complexity": 2.0 }, "quality_score": 4, "grade": "F", "smells": [ { "type": "long_function", "severity": "medium", "message": "Function 'unsafe_input' has 54 lines (max: 50)", "location": "unsafe_input" }, { "type": "magic_number", "severity": "low", "message": "Magic number 242 should be a named constant", "location": "line 17" }, { "type": "magic_number", "severity": "low", "message": "Magic number 100 should be a named constant", "location": "line 27" }, { "type": "magic_number", "severity": "low", "message": "Magic number 134 should be a named constant", "location": "line 31" }, { "type": "magic_number", "severity": "low", "message": "Magic number 120 should be a named constant", "location": "line 35" }, { "type": "magic_number", "severity": "low", "message": "Magic number 690 should be a named constant", "location": "line 41" }, { "type": "magic_number", "severity": "low", "message": "Magic number 416 should be a named constant", "location": "line 47" }, { "type": "magic_number", "severity": "low", "message": "Magic number 100 should be a named constant", "location": "line 62" }, { "type": "c_banned_gets", "severity": "high", "message": "'gets()' is unsafe: no bounds check, removed from C11 (CWE-242)", "location": "offset 117" }, { "type": "c_banned_strcpy", "severity": "high", "message": "'strcpy()' is unsafe: no bounds check \u2014 prefer strncpy or strlcpy", "location": "offset 157" }, { "type": "c_banned_strcpy", "severity": "high", "message": "'strcpy()' is unsafe: no bounds check \u2014 prefer strncpy or strlcpy", "location": "offset 447" }, { "type": "c_banned_strcat", "severity": "high", "message": "'strcat()' is unsafe: no bounds check \u2014 prefer strncat or strlcat", "location": "offset 186" }, { "type": "c_banned_sprintf", "severity": "high", "message": "'sprintf()' is unsafe: no bounds check \u2014 prefer snprintf", "location": "offset 238" }, { "type": "c_format_string", "severity": "high", "message": "'printf(buf)' uses a non-literal format string \u2014 CWE-134 format string vulnerability", "location": "offset 284" }, { "type": "c_unbounded_scanf", "severity": "high", "message": "scanf '%s' without a width specifier \u2014 unbounded read can overflow the destination buffer", "location": "offset 326" }, { "type": "c_malloc_unchecked", "severity": "medium", "message": "'buf' from malloc/calloc/realloc is not NULL-checked within 5 lines \u2014 dereferencing NULL is UB (CWE-690)", "location": "line 36" }, { "type": "c_free_without_null", "severity": "low", "message": "'free(buf)' not followed by 'buf = NULL;' \u2014 dangling pointer can be reused (CWE-416)", "location": "line 42" }, { "type": "c_system_non_literal", "severity": "high", "message": "'system(cmd_from_user)' with a non-literal argument \u2014 command injection (CWE-78); use execve with validated args", "location": "offset 571" } ], "solid_violations": [], "function_details": [ { "name": "unsafe_input", "parameters": 1, "lines": 54, "complexity": 2 }, { "name": "leaky_alloc", "parameters": 1, "lines": 28, "complexity": 2 }, { "name": "run_user_cmd", "parameters": 1, "lines": 15, "complexity": 2 }, { "name": "main", "parameters": 2, "lines": 9, "complexity": 2 } ], "class_details": [] } ``` ## expected_outputs/sample_csharp_clean_quality.json ```json { "file": "/home/user/claude-skills/engineering-team/skills/code-reviewer/assets/sample_csharp_clean.cs", "language": "csharp", "metrics": { "lines": { "total": 101, "code": 67, "blank": 14, "comment": 20 }, "functions": 13, "classes": 3, "avg_complexity": 1.3 }, "quality_score": 98, "grade": "A", "smells": [ { "type": "csharp_unused_using", "severity": "low", "message": "'using System;' appears unused", "location": "System" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System.Net.Http;' appears unused", "location": "System.Net.Http" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System.Threading.Tasks;' appears unused", "location": "System.Threading.Tasks" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System.Data.SqlClient;' appears unused", "location": "System.Data.SqlClient" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using Microsoft.Extensions.Logging;' appears unused", "location": "Microsoft.Extensions.Logging" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using Microsoft.Extensions.Options;' appears unused", "location": "Microsoft.Extensions.Options" } ], "solid_violations": [], "function_details": [ { "name": "HttpClient", "parameters": 0, "lines": 2, "complexity": 1 }, { "name": "UserService", "parameters": 3, "lines": 8, "complexity": 1 }, { "name": "Task", "parameters": 1, "lines": 2, "complexity": 2 }, { "name": "HandleClickAsync", "parameters": 0, "lines": 8, "complexity": 1 }, { "name": "FetchAsync", "parameters": 0, "lines": 12, "complexity": 2 }, { "name": "InvalidOperationException", "parameters": 1, "lines": 21, "complexity": 3 }, { "name": "FirstValue", "parameters": 1, "lines": 4, "complexity": 1 }, { "name": "GetName", "parameters": 1, "lines": 4, "complexity": 1 }, { "name": "SqlConnection", "parameters": 1, "lines": 3, "complexity": 1 }, { "name": "SqlCommand", "parameters": 2, "lines": 7, "complexity": 1 } ], "class_details": [ { "name": "DbOptions", "methods": 0, "lines": 7 }, { "name": "UserService", "methods": 8, "lines": 75 }, { "name": "User", "methods": 0, "lines": 3 } ] } ``` ## expected_outputs/sample_csharp_smells_quality.json ```json { "file": "/home/user/claude-skills/engineering-team/skills/code-reviewer/assets/sample_csharp_smells.cs", "language": "csharp", "metrics": { "lines": { "total": 79, "code": 43, "blank": 11, "comment": 25 }, "functions": 7, "classes": 1, "avg_complexity": 1.3 }, "quality_score": 45, "grade": "F", "smells": [ { "type": "csharp_async_void", "severity": "high", "message": "'async void HandleClick' \u2014 only safe for event handlers; prefer 'async Task'", "location": "HandleClick" }, { "type": "csharp_blocking_async", "severity": "high", "message": "Blocking call on async operation ('.Result' / '.Wait()' / '.GetAwaiter().GetResult()') \u2014 can deadlock in ASP.NET contexts", "location": "offset 430" }, { "type": "csharp_swallowed_exception", "severity": "high", "message": "Empty catch block swallows exceptions silently", "location": "offset 853" }, { "type": "csharp_undisposed_idisposable", "severity": "medium", "message": "'HttpClient' looks like IDisposable but is not wrapped in 'using' / 'using var'", "location": "offset 555" }, { "type": "csharp_undisposed_idisposable", "severity": "medium", "message": "'SqlCommand' looks like IDisposable but is not wrapped in 'using' / 'using var'", "location": "offset 1289" }, { "type": "csharp_new_httpclient", "severity": "medium", "message": "'new HttpClient()' \u2014 prefer IHttpClientFactory or a long-lived static instance to avoid socket exhaustion", "location": "offset 606" }, { "type": "csharp_missing_await", "severity": "medium", "message": "Async method called without 'await' \u2014 Task is discarded", "location": "line 42" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System;' appears unused", "location": "System" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System.Net.Http;' appears unused", "location": "System.Net.Http" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System.Threading.Tasks;' appears unused", "location": "System.Threading.Tasks" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System.Data.SqlClient;' appears unused", "location": "System.Data.SqlClient" }, { "type": "csharp_unused_using", "severity": "low", "message": "'using System.Diagnostics.CodeAnalysis;' appears unused", "location": "System.Diagnostics.CodeAnalysis" } ], "solid_violations": [], "function_details": [ { "name": "HandleClick", "parameters": 2, "lines": 9, "complexity": 1 }, { "name": "FetchAsync", "parameters": 0, "lines": 3, "complexity": 1 }, { "name": "HttpClient", "parameters": 0, "lines": 3, "complexity": 1 }, { "name": "HttpClient", "parameters": 0, "lines": 24, "complexity": 3 }, { "name": "Pointers", "parameters": 0, "lines": 11, "complexity": 1 }, { "name": "GetName", "parameters": 2, "lines": 5, "complexity": 1 }, { "name": "SqlCommand", "parameters": 2, "lines": 6, "complexity": 1 } ], "class_details": [ { "name": "UserService", "methods": 4, "lines": 61 } ] } ``` ## expected_outputs/sample_java_clean_quality.json ```json { "file": "/home/user/claude-skills/engineering-team/skills/code-reviewer/assets/sample_java_clean.java", "language": "java", "metrics": { "lines": { "total": 56, "code": 33, "blank": 9, "comment": 14 }, "functions": 3, "classes": 1, "avg_complexity": 2.0 }, "quality_score": 100, "grade": "A", "smells": [ { "type": "magic_number", "severity": "low", "message": "Magic number 1000 should be a named constant", "location": "line 49" } ], "solid_violations": [], "function_details": [ { "name": "UserService", "parameters": 1, "lines": 5, "complexity": 1 }, { "name": "getName", "parameters": 2, "lines": 17, "complexity": 3 }, { "name": "process", "parameters": 0, "lines": 10, "complexity": 2 } ], "class_details": [ { "name": "UserService", "methods": 3, "lines": 38 } ] } ``` ## expected_outputs/sample_java_smells_quality.json ```json { "file": "/home/user/claude-skills/engineering-team/skills/code-reviewer/assets/sample_java_smells.java", "language": "java", "metrics": { "lines": { "total": 57, "code": 29, "blank": 10, "comment": 18 }, "functions": 3, "classes": 1, "avg_complexity": 2.0 }, "quality_score": 68, "grade": "D", "smells": [ { "type": "magic_number", "severity": "low", "message": "Magic number 1000 should be a named constant", "location": "line 44" }, { "type": "java_empty_catch", "severity": "high", "message": "Empty catch block swallows exceptions silently", "location": "offset 684" }, { "type": "java_print_stack_trace", "severity": "medium", "message": "'printStackTrace()' is not real error handling \u2014 log via a proper logger or rethrow with context", "location": "offset 913" }, { "type": "java_swallowed_interrupt", "severity": "high", "message": "InterruptedException caught without 'Thread.currentThread().interrupt()' \u2014 breaks cooperative cancellation", "location": "offset 841" }, { "type": "java_unclosed_resource", "severity": "medium", "message": "'FileInputStream' looks like an AutoCloseable but is not in a try-with-resources statement", "location": "offset 366" }, { "type": "java_per_use_heavy_object", "severity": "medium", "message": "'new ObjectMapper()' is expensive \u2014 share a singleton instance instead of constructing per call", "location": "offset 451" } ], "solid_violations": [], "function_details": [ { "name": "getName", "parameters": 2, "lines": 18, "complexity": 3 }, { "name": "process", "parameters": 0, "lines": 11, "complexity": 2 }, { "name": "log", "parameters": 1, "lines": 6, "complexity": 1 } ], "class_details": [ { "name": "UserService", "methods": 3, "lines": 40 } ] } ``` ## languages ``` ``` ## languages/c.md --- language: c extensions: [".c", ".h"] --- # C — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only C-specific rules and idioms. --- ## PR Analyzer — C Risk Signals - `printf` / debug `fprintf(stderr, ...)` statements left in production code - `// TODO` / `// FIXME` comments near memory management code — high risk - Disabled compiler warnings (`#pragma GCC diagnostic ignore`, `-w` flags in Makefile) - Hardcoded credentials or keys in source - Use of banned functions: `gets`, `strcpy`, `strcat`, `sprintf`, `scanf` without width limits --- ## Code Quality — C Checks - Functions longer than 50 lines — C functions tend to grow organically and become hard to reason about - Missing `NULL` check after `malloc` / `calloc` / `realloc` - Return value of functions ignored without explicit `(void)` cast - Global mutable state used across translation units without clear ownership - Magic numbers without `#define` or `const` — especially sizes and offsets - Mixed `malloc`/`free` ownership — unclear which caller is responsible for freeing --- ## Security - Flag `gets()` — no bounds checking, always a buffer overflow; replace with `fgets()` - Flag `strcpy()` / `strcat()` — use `strncpy()` / `strncat()` with explicit size, or `strlcpy()` / `strlcat()` - Flag `sprintf()` — use `snprintf()` with explicit buffer size - Flag `scanf("%s", buf)` without a width specifier — unbounded read - Flag `strlen()` result used as a signed integer — potential truncation on 64-bit - Flag user-controlled data used as a format string (`printf(user_input)`) — format string attack - Flag integer arithmetic used as array index without bounds check - Flag signed integer overflow — undefined behavior in C --- ## Async / Concurrency - Flag shared global or `static` variables accessed from multiple threads without a mutex or `_Atomic` - Flag `pthread_mutex_t` / `sem_t` not initialized before use - Flag signal handlers that call non-async-signal-safe functions (`malloc`, `printf`, etc.) - Flag `volatile` used as a substitute for proper synchronization — it is not sufficient - Flag lock acquisition order inconsistency across call sites — deadlock risk --- ## Resource Management - Flag every `malloc` / `calloc` / `realloc` path — verify a matching `free` exists on all exit paths - Flag `fopen` without a matching `fclose` on all paths including error paths - Flag `dup` / `socket` / `open` file descriptors not closed on all paths - Flag stack-allocated VLAs (variable-length arrays) of unbounded size — stack overflow risk - Flag `realloc` return value assigned directly to the source pointer — leaks on failure --- ## Exception Handling - Flag ignored return values from `malloc`, `fopen`, `read`, `write`, `close` — all can fail - Flag `errno` checked after a function that doesn't set it, or not checked immediately after one that does - Flag `perror` / `strerror` as the sole error handling in library code — propagate errors to callers - Flag functions that return `-1` on error without documenting which `errno` values are possible - Flag `assert()` used for runtime error handling — disabled by `NDEBUG` in production builds --- ## Performance - Flag `strlen()` called repeatedly on the same string in a loop — cache the result - Flag unnecessary copies of large structs passed by value — pass by pointer - Flag `memcpy` / `memset` on overlapping regions — use `memmove` for overlapping - Flag repeated heap allocations in a tight loop — consider a pool or stack allocation - Flag `volatile` on variables not accessed by hardware or signal handlers — prevents optimization --- ## Idioms and Best Practices ### Memory Safety - Every pointer must have a clear owner responsible for freeing it — document ownership in comments - Set pointers to `NULL` immediately after `free` to catch use-after-free early - Prefer `calloc` over `malloc` + `memset` for zero-initialized allocations - Use `const` on pointer parameters that the function does not modify ### Defensive Coding - Always check `NULL` returns from allocation functions - Use `size_t` for sizes and counts — never `int` - Prefer `snprintf` and `fgets` over any unbounded string function - Compile with `-Wall -Wextra -Werror` and treat warnings as errors ### Portability - Do not assume pointer size equals `int` size — use `intptr_t` / `uintptr_t` - Do not rely on undefined behavior for performance — use compiler intrinsics instead - Use `stdint.h` types (`uint32_t`, `int64_t`) for fixed-width requirements ## languages/cpp.md --- language: cpp extensions: [".cpp", ".cc", ".cxx", ".hpp", ".hh", ".hxx"] --- # C++ — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only C++-specific rules and idioms. --- ## PR Analyzer — C++ Risk Signals - Raw `new` / `delete` outside of smart pointer wrappers - `reinterpret_cast` — almost always a red flag; require justification - Disabled compiler warnings (`#pragma warning(disable:...)`, `-w`) - `// TODO` / `// FIXME` near ownership or lifetime code - Hardcoded credentials or keys in source - Use of deprecated C-style functions: `strcpy`, `sprintf`, `gets` --- ## Code Quality — C++ Checks - Raw owning pointers (`T*`) used where `unique_ptr` / `shared_ptr` would express ownership - `shared_ptr` overused where `unique_ptr` suffices — implies shared ownership unnecessarily - `std::endl` used in hot paths — flushes the buffer every call; prefer `'\n'` - Implicit conversions between signed and unsigned integers - Virtual destructor missing on base classes with virtual methods - `catch (...)` swallowing all exceptions without logging or re-throwing --- ## Security - Flag `reinterpret_cast` on user-controlled data — potential type confusion - Flag raw array indexing without bounds check — use `.at()` or assert bounds - Flag `std::string` data passed to C APIs without null-termination guarantee — use `.c_str()` - Flag hardcoded buffer sizes — derive from `sizeof` or use `std::array` - Flag `sscanf` / `sprintf` — use `std::istringstream` or `std::format` (C++20) - Flag user-controlled data used as a format string --- ## Async / Concurrency - Flag `std::shared_ptr` accessed from multiple threads — the pointer itself is not thread-safe for write; use `std::atomic>` (C++20) or external locking - Flag `std::vector` / `std::map` mutated from multiple threads without a mutex - Flag `std::mutex` locked twice in the same thread without `std::recursive_mutex` — deadlock - Flag detached threads (`std::thread::detach`) with no lifetime coordination - Flag `volatile` used instead of `std::atomic` for inter-thread communication --- ## Resource Management - Flag raw `new` returning an owning pointer — wrap immediately in `std::make_unique` or `std::make_shared` - Flag `delete` called manually outside of a destructor or smart pointer — ownership confusion - Flag RAII violations — resources acquired in constructor but not released via destructor - Flag `std::ifstream` / `std::ofstream` not checked for open failure before use - Flag exceptions thrown from destructors — causes `std::terminate` if thrown during stack unwinding --- ## Exception Handling - Flag `catch (...)` that swallows exceptions without logging or re-throwing - Flag exceptions thrown from destructors — wrap in `try/catch` inside the destructor - Flag `noexcept` on functions that can actually throw — causes `std::terminate` - Flag exception specifications (`throw(...)`) — deprecated since C++11, removed in C++17 - Flag using exceptions for control flow in performance-critical paths --- ## Performance - Flag pass-by-value for non-trivial types where pass-by-const-reference suffices - Flag `std::vector::push_back` in a loop without `reserve` when size is known — repeated reallocations - Flag `std::map` used where `std::unordered_map` would give O(1) lookup - Flag `std::endl` in loops — prefer `'\n'` to avoid repeated buffer flushes - Flag unnecessary copies from missing `std::move` on local temporaries being returned or passed --- ## Idioms and Best Practices ### Ownership and Lifetime - Prefer `std::unique_ptr` for sole ownership, `std::shared_ptr` only for shared ownership - Prefer `std::make_unique` / `std::make_shared` over `new` — exception-safe - Use `std::weak_ptr` to break `shared_ptr` cycles - Never use raw owning pointers in new code — they are for non-owning observation only ### Modern C++ (17/20) - Prefer `std::optional` over sentinel values or nullable pointers for optional returns - Prefer `std::variant` over tagged unions - Prefer `std::string_view` over `const std::string&` for read-only string parameters - Prefer range-based `for` loops over index loops where the index isn't needed - Prefer `if constexpr` over `#ifdef` for compile-time branching ### Type Safety - Prefer `static_cast` over C-style casts — explicit and auditable - Avoid `reinterpret_cast` except in low-level I/O or FFI code with a comment - Use `enum class` over plain `enum` to avoid implicit integer conversions ## languages/csharp.md --- language: csharp extensions: [".cs", ".csx", ".razor", ".cshtml"] --- # C# / .NET — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only C#-specific rules and idioms. --- ## PR Analyzer — C# Risk Signals - `#pragma warning disable` and `[SuppressMessage]` — verify they are justified - `unsafe { }` blocks — require explicit sign-off - Null-forgiving operator (`!`) used broadly without justification - `dynamic` used outside of interop scenarios - Hardcoded connection strings in source files --- ## Code Quality — C# Checks - `async void` methods (except event handlers) - `Task` returned but not awaited - `IDisposable` objects not in `using` / `using var` - Bare `catch { }` or `catch (Exception e) { }` swallowing silently - Nullable reference types feature disabled at project level --- ## Security - Flag raw string interpolation in SQL queries — require parameterized queries (`SqlCommand`) or EF Core - Flag missing `[ValidateAntiForgeryToken]` on state-changing controller actions - Flag user-controlled data passed to `Process.Start()` or `File` APIs without validation - Flag hardcoded connection strings — require `appsettings.json` + secrets management - Flag `[AllowAnonymous]` on endpoints that should be protected --- ## Async / Await - Flag `async void` methods outside of event handlers — cannot be awaited and swallow exceptions - Flag `.Result`, `.Wait()`, or `.GetAwaiter().GetResult()` on `Task` — causes deadlocks in ASP.NET contexts - Flag missing `ConfigureAwait(false)` in library (non-application) code - Flag `Task.Run()` wrapping synchronous code inside ASP.NET request handlers unnecessarily - Flag `CancellationToken` not threaded through to downstream async calls --- ## Resource Management - Flag `IDisposable` objects (`SqlConnection`, `HttpClient`, `FileStream`, etc.) not wrapped in `using` / `using var` - Flag `HttpClient` instantiated with `new` inside a method — use `IHttpClientFactory` or a shared static instance to avoid socket exhaustion - Flag `DbContext` registered as a singleton in DI — it must be scoped - Flag `MemoryStream` / `MemoryCache` growing unboundedly without eviction policy --- ## Exception Handling - Flag `catch { }` or `catch (Exception) { }` with no logging or re-throw — silent swallow - Flag `catch (Exception e) { throw e; }` — resets the stack trace; use `throw;` instead - Flag catching `Exception` when a specific type (`IOException`, `HttpRequestException`) is appropriate - Flag exception filters (`when`) used for side effects that suppress the exception - Flag exceptions used for control flow in hot paths — use `Try*` pattern methods instead --- ## Performance - Flag `.ToList()` / `.ToArray()` on `IQueryable` before filtering — forces all rows into memory; filter server-side first - Flag `string` concatenation in loops — use `StringBuilder` - Flag `Enumerable.Count()` on `IQueryable` when only an existence check is needed — use `Any()` - Flag `await` in a loop where `Task.WhenAll()` would parallelize the work - Flag synchronous file or network I/O in an `async` method — use the async overload --- ## Idioms and Best Practices ### Null Safety - Ensure `enable` is set in the project file - Flag excessive use of `!` (null-forgiving) without a comment explaining why - Prefer `is null` / `is not null` over `== null` for null checks ### LINQ - Flag `First()` where `FirstOrDefault()` is safer - Flag complex LINQ chains that would be clearer as explicit loops ### Modern C# (10+) - Prefer `record` types for immutable data carriers - Prefer `switch` expressions over `switch` statements where a value is returned - Prefer primary constructors (C# 12) for simple dependency injection - Prefer file-scoped namespaces (`namespace Foo;`) over block-scoped - Prefer `is` pattern matching over explicit casts ## languages/dart.md --- language: dart extensions: [".dart"] --- # Dart / Flutter — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Dart and Flutter-specific rules and idioms. --- ## PR Analyzer — Dart / Flutter Risk Signals - `print()` statements left in production code — use a logging package - `// ignore:` lint suppression comments — verify they are justified - `!` null assertion operator used broadly without justification - Hardcoded API keys, tokens, or URLs in Dart source — use environment variables or a secrets package - `TODO` / `FIXME` near widget lifecycle or state management code --- ## Code Quality — Dart Checks - `dynamic` used where a concrete type is known — defeats static analysis - `!` (null assertion) used broadly — prefer null-safe patterns - `StatefulWidget` used where `StatelessWidget` suffices — prefer stateless - `setState` called with heavy computation inside — offload before calling - `BuildContext` used across async gaps without checking `mounted` - Missing `const` constructor on widgets that could be constant --- ## Security - Flag API keys or secrets hardcoded in Dart source or `pubspec.yaml` — use `--dart-define` or a secrets manager - Flag `http` package used without certificate validation disabled intentionally - Flag `SharedPreferences` used to store sensitive data — use `flutter_secure_storage` - Flag user-controlled input used in `dart:io` file path operations without sanitization - Flag `WebView` loading arbitrary user-supplied URLs without validation - Flag deep link / URL scheme handlers that don't validate the incoming URL before acting on it --- ## Async / Concurrency - Flag `BuildContext` used after an `await` without checking `if (!mounted) return` — context may be invalid - Flag `Future` returned but not `await`-ed and without `.catchError()` or `unawaited()` — floating future - Flag `Isolate.spawn` without a clear message-passing protocol - Flag heavy computation on the main isolate — offload with `compute()` or `Isolate.run()` - Flag `StreamController` not closed when the owning widget is disposed — memory leak - Flag `async*` / `yield*` generators with no error handling on the stream consumer side --- ## Resource Management - Flag `StreamController` not closed in `dispose()` - Flag `AnimationController` not disposed in `dispose()` - Flag `TextEditingController` / `FocusNode` / `ScrollController` not disposed in `dispose()` - Flag `Timer` not cancelled in `dispose()` - Flag listeners added to `ChangeNotifier` / `ValueNotifier` without a corresponding `removeListener` --- ## Exception Handling - Flag empty `catch` blocks — swallowed errors - Flag `catchError` with no handler body — silent failure - Flag `Future.error` not surfaced to the UI — show an error state - Flag `FlutterError.onError` overridden without calling the original handler - Prefer typed `on ExceptionType catch (e)` over generic `catch (e)` where the exception type is known --- ## Performance - Flag `setState` called for changes that only affect a small subtree — use `ValueNotifier` / `provider` / `Riverpod` to scope rebuilds - Flag expensive computation inside `build()` — move to `initState`, a controller, or a `FutureBuilder` - Flag `ListView` without `ListView.builder` for long or infinite lists — builds all children at once - Flag missing `const` on widgets that never change — prevents unnecessary rebuilds - Flag `Image.network` without a caching package in a list — re-downloads on every scroll - Flag `RepaintBoundary` missing around frequently-repainted widgets (animations, counters) --- ## Idioms and Best Practices ### Null Safety - Prefer `?.` safe navigation and `??` null coalescing over `!` assertions - Use `late` only when initialization is guaranteed before first access — document why - Prefer early returns over deeply nested null checks ### Flutter Widget Patterns - Prefer `StatelessWidget` + external state management over `StatefulWidget` for business logic - Keep `build()` methods pure — no side effects, no heavy computation - Extract repeated widget subtrees into named widget classes, not just methods, for better rebuild granularity - Use `const` constructors wherever possible — compile-time constant widgets skip rebuilds entirely ### State Management - Do not mix multiple state management approaches in the same feature - Flag business logic inside `build()` — it belongs in a ViewModel, Notifier, or BLoC - Prefer `Riverpod` / `provider` / `BLoC` over raw `setState` for anything beyond local UI state ### Modern Dart (3.x) - Prefer `sealed` classes for exhaustive pattern matching on domain types - Use records (`(int, String)`) for lightweight multi-value returns instead of ad hoc classes - Use `switch` expressions with pattern matching instead of long `if/else` chains - Prefer `final` for local variables — immutability by default ## languages/go.md --- language: go extensions: [".go"] --- # Go — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Go-specific rules and idioms. --- ## PR Analyzer — Go Risk Signals - `fmt.Println` / `log.Println` debug statements left in production code - `//nolint` comments — verify they are justified - `unsafe` package imports — require explicit sign-off - Hardcoded credentials or tokens in source --- ## Code Quality — Go Checks - Errors returned but not checked (`_ = someFunc()`) - `panic()` used outside of package initialization - Goroutines started without a clear lifetime or cancellation path - `interface{}` / `any` used where a concrete type or typed interface would work - Missing context propagation (`context.Context` not threaded through call chains) --- ## Security - Flag `database/sql` queries built with `fmt.Sprintf` — require `?` / `$N` placeholders - Flag `os/exec` calls with user-controlled arguments without sanitization - Flag `html/template` bypassed in favor of `text/template` for HTML output - Flag `http.ListenAndServeTLS` with `InsecureSkipVerify: true` --- ## Async / Concurrency - Flag goroutines started with no clear lifetime or cancellation path — always pass `context.Context` - Flag goroutines that write to a channel with no receiver and no `select` default — causes a leak - Flag `time.Sleep()` used inside a goroutine as a synchronization mechanism - Flag `sync.WaitGroup.Add()` called inside the goroutine it tracks — race condition - Flag `sync.Mutex` copied by value — must always be used as a pointer or embedded in a struct --- ## Resource Management - Flag `http.Response.Body` not closed after reading — even on error paths (`defer resp.Body.Close()`) - Flag `os.File` not closed — use `defer f.Close()` immediately after opening - Flag `rows.Close()` missing after `sql.Query()` — leaks the DB connection - Flag `context.WithCancel` / `context.WithTimeout` cancel function not called — context and resources leak --- ## Exception Handling - Flag errors assigned to `_` without a comment explaining why it is safe to ignore - Flag errors not wrapped with `fmt.Errorf("...: %w", err)` — loses stack context - Flag `errors.New` / `fmt.Errorf` strings starting with a capital letter or ending in punctuation — violates Go conventions - Flag `panic()` used for expected runtime errors — reserve for programming errors and unrecoverable states - Flag `recover()` used to silently swallow panics without logging --- ## Performance - Flag `fmt.Sprintf` used for simple string concatenation — use `strings.Builder` or `+` for small cases - Flag `append()` in a tight loop without pre-allocating slice capacity — use `make([]T, 0, n)` - Flag `json.Marshal` / `json.Unmarshal` on large structs in hot paths — consider `json.Encoder` / streaming - Flag goroutines spawned per-request without a worker pool for CPU-bound tasks --- ## Idioms and Best Practices ### Error Handling - All returned errors must be checked — never assign to `_` without a comment - Prefer wrapping with `fmt.Errorf("...: %w", err)` for stack context - Use `errors.Is` / `errors.As` for error inspection — never string comparison ### Concurrency - Every goroutine must have an owner responsible for its lifetime - Always pass `context.Context` as the first argument to functions that do I/O or block - Prefer `sync.WaitGroup` or `errgroup` over ad-hoc channel coordination ### Modern Go (1.18+) - Prefer generics over `interface{}` for container types and utility functions - Use `any` (alias for `interface{}`) in new code for readability ## languages/java.md --- language: java extensions: [".java"] --- # Java — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Java-specific rules and idioms. --- ## PR Analyzer — Java Risk Signals - `System.out.println` / `e.printStackTrace()` left in production code - `@SuppressWarnings` annotations — verify they are justified - Hardcoded JDBC URLs or credentials in source - Raw type usage (`List`, `Map` without generics) --- ## Code Quality — Java Checks - Empty `catch` blocks swallowing exceptions silently - Checked exceptions caught and not re-thrown with context - `Closeable` / `AutoCloseable` resources not in try-with-resources - Raw type usage — defeats generics type safety - Missing `@Override` on overriding methods - `InterruptedException` caught without calling `Thread.currentThread().interrupt()` --- ## Security - Flag JPQL / HQL or native SQL string concatenation — require named parameters or `CriteriaBuilder` - Flag `@RequestMapping` without explicit HTTP method restriction on state-changing endpoints - Flag user-controlled input passed to `Runtime.exec()` or `ProcessBuilder` without validation - Flag `ObjectInputStream.readObject()` on untrusted data — unsafe deserialization - Flag hardcoded JDBC URLs or credentials — require environment variables or a vault --- ## Async / Concurrency - Flag `ExecutorService.submit()` return value ignored — exceptions are swallowed - Flag `Thread.sleep()` used as a synchronization mechanism — use `CountDownLatch`, `CompletableFuture`, or `await()` - Flag `CompletableFuture` chains with no `.exceptionally()` or `.handle()` terminal handler - Flag `InterruptedException` caught without calling `Thread.currentThread().interrupt()` - Flag `synchronized` on a non-final field — the lock object can be replaced - Flag `HashMap` used in multi-threaded context — use `ConcurrentHashMap` --- ## Resource Management - Flag `InputStream`, `OutputStream`, `Connection`, `ResultSet`, `PreparedStatement` not wrapped in try-with-resources - Flag manual `finally { resource.close() }` — replace with try-with-resources - Flag `HttpURLConnection` not disconnected after use - Flag JDBC `Connection` obtained from a pool and not returned (missing `close()`) on all paths - Flag `static` `HttpClient` or `Connection` fields shared across threads without connection pool management --- ## Exception Handling - Flag empty `catch` blocks — `catch (Exception e) {}` - Flag `InterruptedException` caught without `Thread.currentThread().interrupt()` — breaks cooperative cancellation - Flag checked exceptions swallowed in a `catch` and not re-thrown or logged with context - Flag `throw new RuntimeException(e)` without a descriptive message — loses context - Flag `printStackTrace()` as the sole error handling — use a proper logger --- ## Performance - Flag `String` concatenation in loops — use `StringBuilder` - Flag `List.contains()` / `Map.get()` in a loop on large collections — review data structure choice - Flag N+1 JPA / Hibernate queries — use `JOIN FETCH` or `@BatchSize` - Flag `new ObjectMapper()` / `new Gson()` instantiated per-request — share a singleton - Flag `ResultSet` fully iterated when only the first result is needed — use `LIMIT 1` in the query --- ## Idioms and Best Practices ### Null Safety - Prefer returning `Optional` over `null` from methods - Flag unchecked dereferences without a prior null guard - Do not catch `NullPointerException` — fix the root cause instead ### Collections and Streams - Flag `==` used to compare `String` or boxed types — use `.equals()` - Flag `.collect(Collectors.toList())` where `.toList()` (Java 16+) suffices - Flag premature `.stream().collect()` round-trips that could be a single-pass operation ### Generics - Flag raw types in any new code — always parameterize (`List`, not `List`) - Flag unchecked cast warnings suppressed without explanation ### Modern Java (11+) - Prefer `var` for local variables where the type is obvious from the right-hand side - Prefer records for pure data carriers over manual POJOs with getters/setters - Prefer `instanceof` pattern matching (`if (obj instanceof String s)`) over explicit casts - Prefer `switch` expressions over `switch` statements where a value is returned ## languages/kotlin.md --- language: kotlin extensions: [".kt", ".kts"] --- # Kotlin — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Kotlin-specific rules and idioms. --- ## PR Analyzer — Kotlin Risk Signals - `println()` statements left in production code - `@Suppress` annotations — verify they are justified - `!!` (not-null assertion) used broadly without justification - Hardcoded credentials or API keys in source --- ## Code Quality — Kotlin Checks - `!!` used broadly — prefer `?.let`, `?:`, or `requireNotNull()` - `lateinit var` accessed before initialization - Coroutines launched with `GlobalScope` — prefer scoped coroutines - `runBlocking` used outside of tests or top-level entry points --- ## Security - Flag Room / SQLite queries built with string concatenation — require parameterized queries - Flag `WebView.loadUrl()` with user-controlled input without validation - Flag credentials stored in `SharedPreferences` — require `EncryptedSharedPreferences` or Keychain --- ## Async / Coroutines - Flag `GlobalScope.launch` / `GlobalScope.async` in production code — use a structured scope - Flag `runBlocking` outside of tests or top-level main functions - Flag `launch` / `async` without a `CoroutineExceptionHandler` or `supervisorScope` where individual failures should not cancel siblings - Flag `Dispatchers.Main` used for CPU-bound work — use `Dispatchers.Default` - Flag coroutine cancellation not respected — long loops should check `isActive` or call `yield()` --- ## Resource Management - Flag `Closeable` / `AutoCloseable` not wrapped in `.use { }` (Kotlin's try-with-resources equivalent) - Flag `OkHttpClient` / `Retrofit` instantiated per-request — share a singleton - Flag `BroadcastReceiver` registered without a corresponding `unregisterReceiver` — memory / battery leak - Flag coroutines that hold a resource across a `suspend` point without structured cleanup in `finally` --- ## Exception Handling - Flag `runCatching { }.getOrNull()` used broadly — silently swallows all exceptions - Flag `catch (e: Exception)` in coroutines without re-throwing `CancellationException` — breaks structured concurrency - Flag empty `catch` blocks - Flag `throw RuntimeException(e)` without a descriptive message - Prefer typed `sealed class` error hierarchies over raw exceptions for domain errors in coroutine flows --- ## Performance - Flag `buildString` / `StringBuilder` not used for multi-step string construction in loops - Flag `List` used for frequent `contains` checks — prefer `Set` - Flag `flow.collect {}` re-subscribing on every recomposition in Jetpack Compose — use `collectAsStateWithLifecycle` - Flag `Dispatchers.IO` used for CPU-bound work — use `Dispatchers.Default` - Flag `suspend` functions calling non-suspend blocking APIs directly — wrap with `withContext(Dispatchers.IO)` --- ## Idioms and Best Practices ### Null Safety - Prefer safe call (`?.`) and Elvis operator (`?:`) over `!!` - Use `requireNotNull()` / `checkNotNull()` with a descriptive message when null means a programming error - Prefer `val` over `var` — immutability by default ### Modern Kotlin - Prefer `data class` for value carriers - Prefer `sealed class` / `sealed interface` for exhaustive `when` expressions - Prefer extension functions over utility classes - Prefer `object` declarations for singletons ## languages/php.md --- language: php extensions: [".php", ".phtml", ".php3", ".php4", ".php5", ".phps"] --- # PHP — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only PHP-specific rules and idioms. --- ## PR Analyzer — PHP Risk Signals - `var_dump` / `print_r` / `echo` debug statements left in production code - `@` error suppression operator — masks real errors; verify it is justified - `// phpcs:ignore` / `// phpstan-ignore` comments — verify they are justified - Hardcoded credentials, database passwords, or API keys in source - `eval()` anywhere — almost always a security issue - `$_GET` / `$_POST` / `$_REQUEST` / `$_COOKIE` used without sanitization --- ## Code Quality — PHP Checks - Missing type declarations on function parameters and return types - `mixed` return type used broadly — tighten to specific types - Global variables (`global $var`) — pass dependencies explicitly - Long functions (>50 lines) — PHP functions tend to accumulate logic - `isset()` / `empty()` used to mask type errors instead of fixing the root cause - Missing `strict_types=1` declaration at the top of the file --- ## Security - Flag `$_GET` / `$_POST` / `$_REQUEST` used directly in SQL queries — require PDO prepared statements - Flag `mysqli_query($conn, "SELECT ... WHERE id = " . $_GET['id'])` — SQL injection - Flag `echo $_GET['name']` or any unescaped output — XSS; use `htmlspecialchars()` with `ENT_QUOTES` - Flag `include` / `require` with user-controlled paths — local/remote file inclusion - Flag `eval()` — remote code execution risk; no legitimate use in application code - Flag `shell_exec` / `exec` / `system` / `passthru` with user-controlled input — command injection - Flag `unserialize()` on untrusted data — arbitrary object instantiation and code execution - Flag `move_uploaded_file` without MIME type validation and extension whitelist — file upload attack - Flag `header("Location: " . $_GET['url'])` without validation — open redirect - Flag missing CSRF token validation on state-changing form endpoints --- ## Async / Concurrency - Flag long-running synchronous operations in a request cycle — offload to a queue (Laravel Queue, RabbitMQ) - Flag `sleep()` used inside a request handler — blocks the PHP-FPM worker - Flag shared mutable state in `static` properties accessed across requests in long-running processes (Swoole, RoadRunner) - Flag missing idempotency in queued jobs — jobs can be retried on failure --- ## Resource Management - Flag database connections not closed or returned to the pool (`$pdo = null` or `$conn->close()`) - Flag `fopen` / `fwrite` without a matching `fclose` on all paths - Flag `curl_init` without `curl_close` — leaks the curl handle - Flag unbounded file uploads with no size or type restriction - Flag sessions not explicitly closed (`session_write_close()`) before long operations — session locking blocks other requests --- ## Exception Handling - Flag empty `catch` blocks — swallowed exceptions - Flag `catch (Exception $e) {}` without logging — silent failure - Flag `die()` / `exit()` used for error handling in library code — use exceptions - Flag `@` operator used to suppress errors from functions that can fail — check return values instead - Flag `trigger_error` used in new code — prefer exceptions --- ## Performance - Flag N+1 Eloquent / Doctrine queries — use eager loading (`with()`, `load()`, `join`) - Flag `count($array)` called repeatedly in a loop condition — cache the result - Flag `array_push($arr, $val)` — use `$arr[] = $val` which is faster - Flag `in_array` on large arrays without the strict third argument — use `isset` on a flipped array for O(1) lookup - Flag `file_get_contents` on remote URLs in a request cycle — use an HTTP client with timeout and async where possible - Flag Eloquent `all()` without pagination — loads entire table into memory --- ## Idioms and Best Practices ### Type Safety - Always declare `declare(strict_types=1)` at the top of every file - Use union types (`int|string`) and nullable types (`?string`) rather than `mixed` - Use typed properties on classes — avoid untyped `public $foo` - Use constructor promotion for simple value objects ### Modern PHP (8.x) - Prefer `match` expressions over `switch` — strict comparison, no fall-through - Use named arguments for functions with many optional parameters - Use `enum` for fixed sets of values instead of class constants - Use `readonly` properties for immutable data - Use nullsafe operator (`?->`) instead of nested `isset` checks - Use `first-class callable syntax` (`strlen(...)`) instead of string references ### Laravel / Symfony Specific - Keep controllers thin — logic belongs in service classes or action classes - Use form requests for validation — never validate in the controller directly - Prefer Eloquent relationships over manual joins for readability - Flag raw queries where the ORM can express the same intent safely ## languages/python.md --- language: python extensions: [".py"] --- # Python — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Python-specific rules and idioms. --- ## PR Analyzer — Python Risk Signals - `print()` statements left in production code - `# noqa` and `# type: ignore` comments — verify they are justified - `eval()` / `exec()` with any user-controlled input - `pickle` used to deserialize untrusted data - Hardcoded credentials or tokens in source --- ## Code Quality — Python Checks - Bare `except:` or `except Exception:` swallowing silently - Mutable default arguments (`def foo(items=[])`) — shared across calls - `import *` — pollutes namespace and hides dependencies - Missing type hints on public functions and methods - `assert` used for runtime validation — stripped by `-O` flag --- ## Security - Flag `eval()` / `exec()` with any user-controlled input - Flag `pickle.loads()` on untrusted data — use `json` or `msgpack` - Flag `subprocess` calls with `shell=True` and user input - Flag `flask.render_template_string()` with user data (SSTI) - Flag `SECRET_KEY` / `DEBUG = True` committed to source --- ## Async - Flag `asyncio.get_event_loop().run_until_complete()` inside an already-running loop - Flag mixing `threading` and `asyncio` without a clear bridge (`run_in_executor`) - Flag CPU-bound work inside an `async def` without offloading to `ProcessPoolExecutor` - Flag `time.sleep()` inside async functions — use `await asyncio.sleep()` --- ## Resource Management - Flag `open()` not used as a context manager (`with open(...) as f`) - Flag `requests.Session` created per-request instead of shared/reused - Flag database connections not closed or returned to a pool on all paths - Flag large files read entirely into memory with `.read()` — prefer streaming / chunked reads --- ## Exception Handling - Flag bare `except:` — catches `BaseException` including `KeyboardInterrupt` and `SystemExit` - Flag `except Exception: pass` — silently swallows errors - Flag re-raising with `raise e` instead of `raise` — loses the original traceback - Flag `except` clause too broad when the `try` block covers multiple operations with different failure modes — split them --- ## Performance - Flag `+` string concatenation in loops — use `"".join()` - Flag repeated `re.compile()` inside a loop — compile once at module level - Flag `list.append()` in a loop where a list comprehension would be more efficient - Flag `in` membership tests on `list` where the collection is large — use `set` - Flag loading entire large files into memory — prefer streaming or chunked reads --- ## Idioms and Best Practices ### Type Safety - All public functions and methods should have type annotations - Prefer `X | None` (Python 3.10+) over `Optional[X]` - Use `TypedDict` or `dataclass` over plain `dict` for structured data ### Modern Python (3.10+) - Prefer `match` statements over long `if/elif` chains - Prefer `dataclass` or `NamedTuple` over plain classes for data carriers - Prefer `pathlib.Path` over `os.path` for file operations - Prefer f-strings over `.format()` or `%` formatting ### None Safety - Prefer explicit `if x is None` over falsy checks when `0` or `""` are valid values - Flag functions returning `None` implicitly — make it explicit or raise ## languages/ruby.md --- language: ruby extensions: [".rb", ".rake", ".gemspec", ".ru"] --- # Ruby — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Ruby-specific rules and idioms. --- ## PR Analyzer — Ruby Risk Signals - `puts` / `p` / `pp` debug statements left in production code - `# rubocop:disable` comments — verify they are justified - `eval` / `instance_eval` / `class_eval` with user-controlled input - Hardcoded credentials, tokens, or `SECRET_KEY_BASE` in source - `binding.pry` / `byebug` / `debugger` left in code --- ## Code Quality — Ruby Checks - Methods longer than 15 lines — Ruby idioms favor very small methods - Classes with more than 10 public methods — possible god object - `rescue Exception` — catches `SignalException` and `SystemExit`; use `rescue StandardError` or more specific types - `method_missing` implemented without `respond_to_missing?` - Deeply nested blocks (>3 levels) — extract to methods - String interpolation used where a symbol would suffice (hash keys, etc.) --- ## Security - Flag `eval` / `instance_eval` with user-controlled strings — remote code execution - Flag `system()` / `exec()` / backtick calls with user-controlled input — shell injection - Flag `YAML.load` on untrusted data — use `YAML.safe_load` - Flag `Marshal.load` on untrusted data — arbitrary code execution - Flag raw SQL string interpolation in ActiveRecord — use parameterized queries (`where("name = ?", name)`) - Flag `params` passed directly to `redirect_to` without validation — open redirect - Flag `render inline:` with user data — XSS via ERB - Flag missing `strong_parameters` in Rails controllers — mass assignment vulnerability --- ## Async / Concurrency - Flag shared mutable state accessed from multiple threads without a `Mutex` - Flag `Thread.new` without storing the thread reference — exceptions are silently swallowed - Flag `sleep` used as a synchronization mechanism in threaded code - Flag `@@class_variables` mutated in multi-threaded contexts — not thread-safe - Flag Sidekiq / ActiveJob workers that are not idempotent — jobs can be retried --- ## Resource Management - Flag `File.open` without a block form — the block form guarantees `close` - Flag database connections or HTTP clients not released in `ensure` blocks - Flag `ActiveRecord` queries inside loops — N+1 pattern; use `includes` / `preload` / `eager_load` - Flag `ObjectSpace` usage in production — memory and performance impact --- ## Exception Handling - Flag `rescue Exception` — use `rescue StandardError` or a specific exception class - Flag empty `rescue` blocks — swallowed errors - Flag `rescue` used for control flow (e.g. rescuing `ActiveRecord::RecordNotFound` instead of using `find_by`) - Flag re-raising with `raise e` instead of bare `raise` — loses the original backtrace - Flag `ensure` blocks that can raise — masks the original exception --- ## Performance - Flag N+1 ActiveRecord queries — use `includes`, `preload`, or `eager_load` - Flag `Array#each` with string concatenation — use `map` + `join` - Flag `select` + `map` that could be a single `filter_map` - Flag `.count` on an ActiveRecord relation inside a view or loop — triggers a query each time - Flag `require` inside a method body — constant overhead on every call - Flag `Hash#merge` in a loop — use `merge!` or `each_with_object` --- ## Idioms and Best Practices ### Ruby Style - Prefer `map` / `select` / `reject` / `reduce` over manual `each` + accumulator - Prefer `&method(:name)` over `{ |x| some_method(x) }` for method reference blocks - Prefer `freeze` on string constants to avoid repeated object allocation - Use `attr_reader` / `attr_writer` / `attr_accessor` instead of manual getter/setter methods - Prefer `Symbol#to_proc` (`&:method_name`) for simple single-method blocks ### Rails-Specific - Keep controllers thin — logic belongs in service objects, models, or concerns - Use `before_action` for authentication/authorization checks — never inline - Prefer `find_by` over `where(...).first` — more intent-revealing - Flag `after_commit` callbacks with side effects that should be in a service object - Prefer `respond_to` blocks over separate controller actions for format variants ### Modern Ruby (3.x) - Prefer pattern matching (`case/in`) for complex data destructuring - Use numbered block parameters (`_1`, `_2`) only for very short, obvious blocks - Prefer `Data.define` for simple immutable value objects (Ruby 3.2+) ## languages/rust.md --- language: rust extensions: [".rs"] --- # Rust — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Rust-specific rules and idioms. --- ## PR Analyzer — Rust Risk Signals - `unsafe { }` blocks — require explicit justification and sign-off - `#[allow(...)]` attributes suppressing lints — verify they are justified - `.unwrap()` / `.expect("")` on `Option` or `Result` outside of tests or prototypes - Hardcoded credentials or tokens in source - `TODO` / `FIXME` comments near `unsafe` or ownership code --- ## Code Quality — Rust Checks - `.unwrap()` used broadly in production code — prefer `?`, `if let`, or `match` - `clone()` called excessively — may indicate ownership design issues - `Arc>` used where a simpler ownership model would work - `Box` used where generics (`impl Trait`) would avoid heap allocation - `pub` fields on structs that should enforce invariants — use accessor methods --- ## Security - Flag `unsafe` blocks accessing raw pointers without clear safety invariant documented in a comment - Flag `std::mem::transmute` — almost always a logic error or undefined behavior; require strong justification - Flag `from_utf8_unchecked` on user-controlled data — use `from_utf8` with error handling - Flag `unwrap()` on user-supplied input parsing — panics are a denial-of-service vector in server code - Flag hardcoded secrets — use environment variables or a secrets crate --- ## Async / Concurrency - Flag `std::sync::Mutex` used in async code — use `tokio::sync::Mutex` to avoid blocking the async runtime - Flag `.await` inside a `std::sync::MutexGuard` scope — holds the lock across an await point, blocking other tasks - Flag `spawn` without storing the `JoinHandle` — panics in the spawned task are silently ignored - Flag `Arc>` cloned excessively — consider message passing via channels instead - Flag blocking I/O calls (`std::fs`, `std::net`) inside async functions — use async equivalents --- ## Resource Management - Flag manual `drop` called explicitly where the natural scope boundary suffices - Flag `Rc` used in multi-threaded code — use `Arc`; the compiler catches this but flag in review for architecture discussion - Flag `Vec` or `String` with large pre-allocated capacity never trimmed — call `.shrink_to_fit()` if long-lived - Flag `impl Drop` that can panic — causes `abort` during stack unwinding --- ## Exception Handling - Flag `.unwrap()` in production code outside of tests — use `?` to propagate or handle explicitly - Flag `.expect("todo")` or `.expect("")` — messages must explain the invariant that guarantees safety - Flag `panic!` used for recoverable errors — use `Result` - Flag `unwrap_or_default()` where the default silently masks a real error - Prefer typed error enums (`thiserror`) over `Box` for library crates - Prefer `anyhow` for application-level error context; `thiserror` for library error types --- ## Performance - Flag `.clone()` on large types in hot paths — review whether a reference or `Cow` would work - Flag `format!` used only to create a `String` from a literal — use `.to_string()` or `String::from` - Flag `collect::>()` followed immediately by `.iter()` — chain iterators instead - Flag `Box` for small types where stack allocation is fine - Flag `Mutex` contention on a hot path — consider `RwLock` for read-heavy workloads or sharding --- ## Idioms and Best Practices ### Ownership - Prefer borrowing (`&T`, `&mut T`) over cloning wherever the lifetime allows - Use `Cow<'_, str>` for functions that sometimes need to own and sometimes borrow - Prefer `impl Trait` in function signatures over `Box` for static dispatch ### Error Handling - Use `?` operator to propagate errors — avoid manual `match Err(e) => return Err(e)` - Define domain error types with `thiserror` in libraries; use `anyhow` in binaries - Never use `.unwrap()` in library code — it panics the caller's thread ### Modern Rust - Prefer `if let` / `while let` for single-variant matches over full `match` - Prefer `?` over `unwrap` everywhere errors are recoverable - Use `#[derive(Debug, Clone, PartialEq)]` consistently on data types - Prefer `iter()` chains over manual loops — they compose and optimize well - Use `clippy` and treat its lints as required — flag any `#[allow(clippy::...)]` in review ## languages/swift.md --- language: swift extensions: [".swift"] --- # Swift — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only Swift-specific rules and idioms. --- ## PR Analyzer — Swift Risk Signals - `print()` statements left in production code - Force unwrap (`!`) on optionals outside of tests or justified init - Force cast (`as!`) without a safe fallback - Hardcoded credentials or API keys in source --- ## Code Quality — Swift Checks - Force unwrap (`!`) used broadly — prefer `guard let` or `if let` - `try!` used outside of guaranteed-safe contexts - Retain cycles in closures — missing `[weak self]` or `[unowned self]` - `@objc` / `dynamic` used without an Objective-C interop reason --- ## Security - Flag credentials stored in `UserDefaults` — require Keychain - Flag `URLSession` requests over plain HTTP in production - Flag `WKWebView` loading arbitrary user-supplied URLs without validation --- ## Async / Concurrency - Flag `DispatchQueue.main.sync` called from the main thread — deadlock - Flag `@escaping` closures capturing `self` strongly in reference cycles — use `[weak self]` - Flag mixing `async/await` and `DispatchQueue` for the same operation without clear reasoning - Flag `Task { }` (unstructured) where a structured `async let` or `TaskGroup` would maintain structure - Flag data races — shared mutable state accessed from multiple tasks without an actor --- ## Resource Management - Flag `URLSessionDataTask` started with no cancellation handle stored — cannot be cancelled if the view disappears - Flag `NotificationCenter` observers added without a corresponding `removeObserver` — memory leak - Flag `CLLocationManager` / `AVCaptureSession` not stopped when the owning view controller is dismissed --- ## Exception Handling - Flag `try!` outside of guaranteed-safe contexts (test fixtures, constants) — crashes on failure - Flag `try?` discarding errors where the failure mode matters to the caller - Flag error types conforming to `Error` with no associated values or message — makes debugging hard - Flag throwing functions calling `fatalError()` as a fallback — choose one error strategy --- ## Performance - Flag `UIImage(named:)` called repeatedly for the same asset without caching - Flag synchronous network calls on the main thread - Flag `Array` used for frequent membership tests — prefer `Set` - Flag `String` interpolation inside tight loops where a pre-built string would avoid allocations --- ## Idioms and Best Practices ### Optionals - Prefer `guard let` for early exit; `if let` for local scope - Prefer optional chaining (`?.`) over force unwrap - Flag implicitly unwrapped optionals (`var x: String!`) outside of `@IBOutlet` ### Memory Management - Flag closures capturing `self` strongly in reference cycles — use `[weak self]` - Prefer `struct` over `class` for value semantics unless identity or inheritance is needed - Use `unowned` only when the lifetime is guaranteed — otherwise `weak` ### Concurrency (Swift 5.5+) - Prefer `async/await` over completion handlers in new code - Flag `DispatchQueue.main.async` where `@MainActor` or `await MainActor.run` is more appropriate ## languages/typescript.md --- language: typescript extensions: [".ts", ".tsx", ".js", ".jsx", ".mjs"] --- # TypeScript / JavaScript — Language-Specific Review Notes Load this file alongside `rules/universal.md`. Universal rules are not repeated here — only TypeScript/JavaScript-specific rules and idioms. --- ## PR Analyzer — TypeScript / JavaScript Risk Signals - `console.log` / `debugger` statements left in production code - `// eslint-disable` comments — verify they are justified - `any` type annotations — require explicit justification - `@ts-ignore` / `@ts-expect-error` — verify they are justified - `eval()` with any dynamic or user-controlled input - Hardcoded API keys or tokens in source --- ## Code Quality — TypeScript / JavaScript Checks - `any` used broadly instead of proper typing - Non-null assertion (`!`) used without justification - `var` declarations — prefer `const` / `let` - Missing `await` on async function calls - Floating promises (no `.catch()` and no `await`) - `==` used instead of `===` --- ## Security - Flag `innerHTML`, `outerHTML`, `document.write()` with user-controlled data — use `textContent` or a sanitizer - Flag `dangerouslySetInnerHTML` in React without a sanitizer - Flag `eval()` / `new Function()` with dynamic input - Flag JWT decoded without signature verification - Flag missing `httpOnly` / `secure` flags on cookies --- ## Async / Promises - Flag floating promises — async calls not `await`-ed and without `.catch()` - Flag `Promise.all()` where `Promise.allSettled()` is safer (one failure should not cancel siblings) - Flag `async` functions inside `forEach` — `forEach` does not await; use `for...of` or `Promise.all()` - Flag unhandled promise rejection (no global `unhandledRejection` handler in Node.js services) --- ## Resource Management - Flag `fs.createReadStream` / `fs.createWriteStream` with no `close` or `destroy` on error - Flag `EventEmitter` listeners added in a loop without removal — memory leak - Flag `setInterval` / `setTimeout` handles not cleared when the owning component unmounts or exits - Flag database clients / pools not released after use in Node.js --- ## Exception Handling - Flag `catch (e) {}` (empty catch) — swallowed error - Flag `catch (e)` where `e` is used as `any` without narrowing — type the error properly - Flag `Promise` rejection not handled — `.catch()` or `try/await/catch` required - Flag re-throwing a new `Error` without wrapping the original — loses stack context - Use `Error` subclasses for domain errors rather than plain strings or object literals --- ## Performance - Flag `Array.prototype.find` / `filter` / `map` chained multiple times over the same array — combine into one pass - Flag DOM queries (`document.querySelector`) inside loops — cache the result - Flag `JSON.parse` / `JSON.stringify` in a hot path on large objects — consider streaming or partial parsing - Flag `async` functions called sequentially in a loop where `Promise.all()` would parallelize them --- ## Idioms and Best Practices ### Type Safety (TypeScript) - Prefer `unknown` over `any` for truly unknown values — forces a type guard before use - Prefer type narrowing (`typeof`, `instanceof`, discriminated unions) over casting - Enable `strict` mode in `tsconfig.json` - Prefer `interface` for object shapes that may be extended; `type` for unions and aliases ### Modern JavaScript / TypeScript - Prefer `const` by default; `let` only when reassignment is needed - Prefer optional chaining (`?.`) and nullish coalescing (`??`) over manual null guards - Prefer `structuredClone()` over manual deep-copy patterns - Prefer named exports over default exports for better refactoring support ### Null / Undefined Safety - Distinguish between `null` (intentional absence) and `undefined` (not set) — be consistent - Flag `== null` checks that accidentally include `undefined` when only one is intended ## rules ``` ``` ## rules/universal.md # Universal Rules — All Languages These rules apply regardless of language. Load this file for every review, alongside the relevant `languages/*.md` file. --- ## Security - Flag any string interpolation or concatenation used to build SQL, shell, or LDAP queries — require parameterized queries or a safe API - Flag hardcoded credentials, API keys, tokens, or secrets anywhere in source — require environment variables or a secrets manager - Flag user-controlled input passed to file system, process execution, or URL redirect APIs without validation - Flag overly broad CORS or CSP policies --- ## Async / Concurrency - Flag shared mutable state accessed from multiple threads/coroutines/tasks without synchronization - Flag fire-and-forget async operations with no error handling path - Flag timeouts missing on any network or I/O call - Flag unbounded queues or thread pools with no backpressure mechanism --- ## Resource Management - Flag any resource (file, socket, DB connection, HTTP connection) acquired without a guaranteed release path - Flag connection pools not returned to the pool on all code paths (including exceptions) - Flag unbounded collections that grow without eviction — potential memory leak - Flag resources held open longer than the operation they serve --- ## Exception Handling - Flag empty catch/except blocks — swallowed exceptions hide bugs silently - Flag catching the broadest possible exception type (`Exception`, `Throwable`, `error`) where a specific type is appropriate - Flag exceptions used for normal control flow (signaling "not found", etc.) — use return values or `Optional` - Flag error context lost when re-throwing — always wrap with the original cause --- ## Performance - Flag N+1 query patterns — loading a collection then querying for each item individually - Flag unbounded queries or API calls with no pagination or limit - Flag synchronous I/O on a thread or event loop that serves concurrent requests - Flag large objects serialized/deserialized repeatedly when they could be cached - Flag string concatenation in tight loops — use a builder or join ## scripts ``` ``` ## scripts/code_quality_checker.py ```python #!/usr/bin/env python3 """ Code Quality Checker Analyzes source code for quality issues, code smells, complexity metrics, and SOLID principle violations. Usage: python code_quality_checker.py /path/to/file.py python code_quality_checker.py /path/to/directory --recursive python code_quality_checker.py . --language typescript --json """ import argparse import json import re import sys from pathlib import Path from typing import Dict, List, Optional # Language-specific file extensions. # `c` is declared before `cpp` so plain `.h` resolves to C, matching the # dispatch table in SKILL.md. C++ headers use `.hpp` / `.hh` / `.hxx`. LANGUAGE_EXTENSIONS = { "python": [".py"], "typescript": [".ts", ".tsx"], "javascript": [".js", ".jsx", ".mjs"], "go": [".go"], "swift": [".swift"], "kotlin": [".kt", ".kts"], "csharp": [".cs", ".csx", ".razor", ".cshtml"], "java": [".java"], "c": [".c", ".h"], "cpp": [".cpp", ".cc", ".cxx", ".hpp", ".hh", ".hxx"], "rust": [".rs"], "ruby": [".rb", ".rake", ".gemspec", ".ru"], "php": [".php", ".phtml"], "dart": [".dart"], } # Code smell thresholds THRESHOLDS = { "long_function_lines": 50, "too_many_parameters": 5, "high_complexity": 10, "god_class_methods": 20, "max_imports": 15 } def get_file_extension(filepath: Path) -> str: """Get file extension.""" return filepath.suffix.lower() def detect_language(filepath: Path) -> Optional[str]: """Detect programming language from file extension.""" ext = get_file_extension(filepath) for lang, extensions in LANGUAGE_EXTENSIONS.items(): if ext in extensions: return lang return None def read_file_content(filepath: Path) -> str: """Read file content safely.""" try: with open(filepath, "r", encoding="utf-8", errors="ignore") as f: return f.read() except Exception: return "" def calculate_cyclomatic_complexity(content: str) -> int: """ Estimate cyclomatic complexity based on control flow keywords. """ complexity = 1 # Base complexity # Control flow patterns that increase complexity patterns = [ r"\bif\b", r"\belif\b", r"\belse\b", r"\bfor\b", r"\bwhile\b", r"\bcase\b", r"\bcatch\b", r"\bexcept\b", r"\band\b", r"\bor\b", r"\|\|", r"&&" ] for pattern in patterns: matches = re.findall(pattern, content, re.IGNORECASE) complexity += len(matches) return complexity def count_lines(content: str) -> Dict[str, int]: """Count different types of lines in code.""" lines = content.split("\n") total = len(lines) blank = sum(1 for line in lines if not line.strip()) comment = 0 for line in lines: stripped = line.strip() if stripped.startswith("#") or stripped.startswith("//"): comment += 1 elif stripped.startswith("/*") or stripped.startswith("'''") or stripped.startswith('"""'): comment += 1 code = total - blank - comment return { "total": total, "code": code, "blank": blank, "comment": comment } def find_functions(content: str, language: str) -> List[Dict]: """Find function definitions and their metrics.""" functions = [] # Language-specific function patterns patterns = { "python": r"def\s+(\w+)\s*\(([^)]*)\)", "typescript": r"(?:function\s+(\w+)|(?:const|let|var)\s+(\w+)\s*=\s*(?:async\s+)?\([^)]*\)\s*=>)", "javascript": r"(?:function\s+(\w+)|(?:const|let|var)\s+(\w+)\s*=\s*(?:async\s+)?\([^)]*\)\s*=>)", "go": r"func\s+(?:\([^)]+\)\s+)?(\w+)\s*\(([^)]*)\)", "swift": r"func\s+(\w+)\s*\(([^)]*)\)", "kotlin": r"fun\s+(\w+)\s*\(([^)]*)\)", # C#: require at least one method modifier (public/private/etc. or static/async/...) # to distinguish declarations from invocations. "csharp": ( r"(?:(?:public|private|protected|internal|static|async|virtual|" r"override|sealed|abstract|partial|new|readonly|extern)\s+)+" r"(?:[\w<>?,\s\[\]\.]+?\s+)?(\w+)\s*\(([^)]*)\)" ), # Java: require at least one method modifier to distinguish # declarations from invocations (mirrors the C# approach). "java": ( r"(?:(?:public|private|protected|static|final|abstract|" r"synchronized|native|default|strictfp)\s+)+" r"(?:[\w<>?,\s\[\]\.]+?\s+)?(\w+)\s*\(([^)]*)\)" ), # C: require an opening brace after the parens so prototypes and # call sites don't get matched. Return type / qualifiers come first. # Skip C control-flow keywords that look like function calls. "c": ( r"^(?:static\s+|inline\s+|extern\s+|const\s+|unsigned\s+|" r"signed\s+|volatile\s+|register\s+)*" r"(?:[\w\*]+\s+\**)+" r"(?!(?:if|while|for|switch|return|sizeof)\b)" r"(\w+)\s*\(([^)]*)\)\s*\{" ), # C++: like C but also catches `ClassName::method(...)` definitions # and template return types like `std::vector`. "cpp": ( r"^(?:static\s+|inline\s+|extern\s+|const\s+|virtual\s+|" r"explicit\s+|constexpr\s+|noexcept\s+)*" r"(?:[\w:\*<>&,\s]+\s+\**)+" r"(?!(?:if|while|for|switch|return|sizeof)\b)" r"(\w+)(?:::\w+)?\s*\(([^)]*)\)\s*(?:const\s*)?" r"(?:noexcept\s*)?(?:override\s*)?(?:final\s*)?\{" ), # Rust: `fn` keyword is always present and unambiguous. "rust": ( r"(?:pub(?:\([^)]+\))?\s+)?(?:async\s+)?(?:unsafe\s+)?" r"(?:extern\s+\"[^\"]+\"\s+)?fn\s+(\w+)\s*" r"(?:<[^>]+>)?\s*\(([^)]*)\)" ), # Ruby: `def` keyword; params may be parenthesised or bare. "ruby": ( r"def\s+(?:self\.)?(\w+[?!=]?)(?:\s*\(([^)]*)\)|\s*$|\s+\w)" ), # PHP: `function` keyword is always present. "php": ( r"(?:(?:public|private|protected|static|abstract|final)\s+)*" r"function\s+(\w+)\s*\(([^)]*)\)" ), # Dart: typed return followed by name and parens. Constructors # (where name matches enclosing class) are not specially handled. "dart": ( r"^\s*(?:static\s+|external\s+)*" r"(?:Future<[^>]*>|Stream<[^>]*>|void|[\w<>?,\s]+?)\s+" r"(\w+)\s*\(([^)]*)\)\s*(?:async\*?\s*|sync\*?\s*)?\{" ), } pattern = patterns.get(language, patterns["python"]) matches = re.finditer(pattern, content, re.MULTILINE) for match in matches: name = next((g for g in match.groups() if g), "anonymous") params_str = match.group(2) if len(match.groups()) > 1 and match.group(2) else "" # Count parameters params = [p.strip() for p in params_str.split(",") if p.strip()] param_count = len(params) # Estimate function length start_pos = match.end() remaining = content[start_pos:] next_func = re.search(pattern, remaining) if next_func: func_body = remaining[:next_func.start()] else: func_body = remaining[:min(2000, len(remaining))] line_count = len(func_body.split("\n")) complexity = calculate_cyclomatic_complexity(func_body) functions.append({ "name": name, "parameters": param_count, "lines": line_count, "complexity": complexity }) return functions def find_classes(content: str, language: str) -> List[Dict]: """Find class definitions and their metrics.""" classes = [] patterns = { "python": r"class\s+(\w+)", "typescript": r"class\s+(\w+)", "javascript": r"class\s+(\w+)", "go": r"type\s+(\w+)\s+struct", "swift": r"class\s+(\w+)", "kotlin": r"class\s+(\w+)", "csharp": r"(?:class|struct|record|interface)\s+(\w+)", "java": r"(?:class|interface|enum|record)\s+(\w+)", # C has no classes; `struct` and `typedef struct` are the closest. "c": r"(?:typedef\s+)?struct\s+(\w+)", "cpp": r"(?:class|struct)\s+(\w+)", # Rust uses `struct`, `enum`, `trait`, `union` for type definitions. # `impl` blocks attach methods but are not type defs themselves. "rust": r"(?:pub(?:\([^)]+\))?\s+)?(?:struct|enum|trait|union)\s+(\w+)", "ruby": r"(?:class|module)\s+(\w+)", "php": ( r"(?:abstract\s+|final\s+)?" r"(?:class|interface|trait|enum)\s+(\w+)" ), # Dart 3 class modifiers: final / interface / base / sealed / mixin. "dart": ( r"(?:abstract\s+|sealed\s+|final\s+|base\s+|interface\s+)?" r"(?:class|mixin|enum|extension)\s+(\w+)" ), } pattern = patterns.get(language, patterns["python"]) matches = re.finditer(pattern, content) for match in matches: name = match.group(1) start_pos = match.end() remaining = content[start_pos:] next_class = re.search(pattern, remaining) if next_class: class_body = remaining[:next_class.start()] else: class_body = remaining # Count methods method_patterns = { "python": r"def\s+\w+\s*\(", "typescript": r"(?:public|private|protected)?\s*\w+\s*\([^)]*\)\s*[:{]", "javascript": r"\w+\s*\([^)]*\)\s*\{", "go": r"func\s+\(", "swift": r"func\s+\w+", "kotlin": r"fun\s+\w+", "csharp": ( r"(?:(?:public|private|protected|internal|static|async|virtual|" r"override|sealed|abstract|partial)\s+)+" r"(?:[\w<>?,\s\[\]\.]+?\s+)?\w+\s*\(" ), "java": ( r"(?:(?:public|private|protected|static|final|abstract|" r"synchronized|native|default|strictfp)\s+)+" r"(?:[\w<>?,\s\[\]\.]+?\s+)?\w+\s*\(" ), # C has no classes; struct members are typically function pointers # rather than methods. Use the function definition pattern. "c": ( r"^(?:static\s+|inline\s+)*(?:[\w\*]+\s+\**)+" r"(?!(?:if|while|for|switch|return|sizeof)\b)" r"\w+\s*\([^)]*\)\s*\{" ), "cpp": ( r"^(?:static\s+|inline\s+|virtual\s+|explicit\s+|" r"constexpr\s+)*(?:[\w:\*<>&,\s]+\s+\**)+" r"(?!(?:if|while|for|switch|return|sizeof)\b)" r"\w+(?:::\w+)?\s*\([^)]*\)" ), "rust": ( r"(?:pub(?:\([^)]+\))?\s+)?(?:async\s+)?(?:unsafe\s+)?" r"fn\s+\w+" ), "ruby": r"def\s+(?:self\.)?\w+[?!=]?", "php": ( r"(?:(?:public|private|protected|static|abstract|final)\s+)*" r"function\s+\w+\s*\(" ), "dart": ( r"^\s*(?:static\s+|external\s+)*" r"(?:Future<[^>]*>|Stream<[^>]*>|void|[\w<>?,\s]+?)\s+" r"\w+\s*\([^)]*\)\s*(?:async\*?\s*|sync\*?\s*)?\{" ), } method_pattern = method_patterns.get(language, method_patterns["python"]) methods = len(re.findall(method_pattern, class_body)) classes.append({ "name": name, "methods": methods, "lines": len(class_body.split("\n")) }) return classes def check_code_smells(content: str, functions: List[Dict], classes: List[Dict]) -> List[Dict]: """Check for code smells in the content.""" smells = [] # Long functions for func in functions: if func["lines"] > THRESHOLDS["long_function_lines"]: smells.append({ "type": "long_function", "severity": "medium", "message": f"Function '{func['name']}' has {func['lines']} lines (max: {THRESHOLDS['long_function_lines']})", "location": func["name"] }) # Too many parameters for func in functions: if func["parameters"] > THRESHOLDS["too_many_parameters"]: smells.append({ "type": "too_many_parameters", "severity": "low", "message": f"Function '{func['name']}' has {func['parameters']} parameters (max: {THRESHOLDS['too_many_parameters']})", "location": func["name"] }) # High complexity for func in functions: if func["complexity"] > THRESHOLDS["high_complexity"]: severity = "high" if func["complexity"] > 20 else "medium" smells.append({ "type": "high_complexity", "severity": severity, "message": f"Function '{func['name']}' has complexity {func['complexity']} (max: {THRESHOLDS['high_complexity']})", "location": func["name"] }) # God classes for cls in classes: if cls["methods"] > THRESHOLDS["god_class_methods"]: smells.append({ "type": "god_class", "severity": "high", "message": f"Class '{cls['name']}' has {cls['methods']} methods (max: {THRESHOLDS['god_class_methods']})", "location": cls["name"] }) # Magic numbers magic_pattern = r"\b(? str: """Remove // line comments and /* */ block comments so regex detectors don't match keywords inside prose.""" no_block = re.sub(r"/\*.*?\*/", "", content, flags=re.DOTALL) no_line = re.sub(r"//[^\n]*", "", no_block) return no_line def check_csharp_specific_smells(content: str) -> List[Dict]: """C# / .NET-specific code smells documented in SKILL.md.""" smells: List[Dict] = [] content = _strip_csharp_comments(content) # async void (event handler exception only — caller must justify) for match in re.finditer(r"\basync\s+void\s+(\w+)\s*\(", content): smells.append({ "type": "csharp_async_void", "severity": "high", "message": ( f"'async void {match.group(1)}' — only safe for event handlers; " "prefer 'async Task'" ), "location": match.group(1), }) # Blocking on async: .Result, .Wait(), .GetAwaiter().GetResult() for match in re.finditer( r"\.(?:Result\b|Wait\(\)|GetAwaiter\(\)\.GetResult\(\))", content ): smells.append({ "type": "csharp_blocking_async", "severity": "high", "message": ( "Blocking call on async operation ('.Result' / '.Wait()' / " "'.GetAwaiter().GetResult()') — can deadlock in ASP.NET contexts" ), "location": f"offset {match.start()}", }) # Bare catch / catch (Exception) that swallows swallow_pattern = re.compile( r"catch\s*(?:\(\s*(?:System\.)?Exception(?:\s+\w+)?\s*\))?\s*\{\s*\}" ) for match in swallow_pattern.finditer(content): smells.append({ "type": "csharp_swallowed_exception", "severity": "high", "message": "Empty catch block swallows exceptions silently", "location": f"offset {match.start()}", }) # IDisposable instantiated but not in `using` — heuristic: `new SomethingClient(` # / `new SomethingStream(` / `new SqlConnection(` outside a `using` line. disposable_hint = re.compile( r"^(?!\s*using\b)\s*(?:var|[\w<>]+)\s+\w+\s*=\s*new\s+" r"(\w*(?:Stream|Connection|Reader|Writer|Client|Context|Command))\s*\(", re.MULTILINE, ) for match in disposable_hint.finditer(content): smells.append({ "type": "csharp_undisposed_idisposable", "severity": "medium", "message": ( f"'{match.group(1)}' looks like IDisposable but is not wrapped in " "'using' / 'using var'" ), "location": f"offset {match.start()}", }) # HttpClient instantiated with `new` inside a method body (socket exhaustion) httpclient_inline = re.compile(r"new\s+HttpClient\s*\(\s*\)") for match in httpclient_inline.finditer(content): smells.append({ "type": "csharp_new_httpclient", "severity": "medium", "message": ( "'new HttpClient()' — prefer IHttpClientFactory or a long-lived " "static instance to avoid socket exhaustion" ), "location": f"offset {match.start()}", }) # Missing await: `Task.Run(` / async method call assigned but never awaited. # Heuristic: a statement ending in `Async()` or `Async(...)` followed by `;` # with no `await` keyword on the same line. for line_no, line in enumerate(content.split("\n"), 1): stripped = line.strip() if not stripped or stripped.startswith(("//", "/*", "*")): continue if re.search(r"\b\w+Async\s*\([^)]*\)\s*;\s*$", stripped) and "await " not in stripped: # Skip `return ...Async();` (forwarding the Task is legitimate) if stripped.startswith("return "): continue smells.append({ "type": "csharp_missing_await", "severity": "medium", "message": "Async method called without 'await' — Task is discarded", "location": f"line {line_no}", }) # Unnecessary `using` directives — heuristic: `using` directive whose # namespace tail isn't referenced anywhere else in the file. using_directives = re.findall( r"^using\s+(?:static\s+)?([A-Z]\w*(?:\.\w+)*)\s*;", content, re.MULTILINE ) body = re.sub(r"^using\s+[^;]+;\s*$", "", content, flags=re.MULTILINE) for ns in using_directives: tail = ns.split(".")[-1] if not re.search(rf"\b{re.escape(tail)}\b", body): smells.append({ "type": "csharp_unused_using", "severity": "low", "message": f"'using {ns};' appears unused", "location": ns, }) return smells def check_java_specific_smells(content: str) -> List[Dict]: """Java-specific code smells documented in languages/java.md.""" smells: List[Dict] = [] # Java comment syntax matches C#, so the same stripper applies. content = _strip_csharp_comments(content) # Empty catch block — swallows the exception silently. for match in re.finditer(r"catch\s*\([^)]*\)\s*\{\s*\}", content): smells.append({ "type": "java_empty_catch", "severity": "high", "message": "Empty catch block swallows exceptions silently", "location": f"offset {match.start()}", }) # printStackTrace() as error handling — use a logger instead. for match in re.finditer(r"\.printStackTrace\s*\(\s*\)", content): smells.append({ "type": "java_print_stack_trace", "severity": "medium", "message": ( "'printStackTrace()' is not real error handling — log via a " "proper logger or rethrow with context" ), "location": f"offset {match.start()}", }) # InterruptedException caught without restoring the interrupt flag. for match in re.finditer( r"catch\s*\(\s*InterruptedException\s+(\w+)\s*\)\s*\{(.*?)\}", content, re.DOTALL, ): if "interrupt()" not in match.group(2): smells.append({ "type": "java_swallowed_interrupt", "severity": "high", "message": ( "InterruptedException caught without " "'Thread.currentThread().interrupt()' — breaks cooperative " "cancellation" ), "location": f"offset {match.start()}", }) # Closeable resource instantiated outside try-with-resources (leak heuristic). resource_hint = re.compile( r"^(?!\s*try\b)\s*(?:final\s+)?[\w<>\[\]]+\s+\w+\s*=\s*new\s+" r"(\w*(?:InputStream|OutputStream|Reader|Writer|Stream|Connection))\s*\(", re.MULTILINE, ) for match in resource_hint.finditer(content): smells.append({ "type": "java_unclosed_resource", "severity": "medium", "message": ( f"'{match.group(1)}' looks like an AutoCloseable but is not in a " "try-with-resources statement" ), "location": f"offset {match.start()}", }) # Heavy object built per use instead of shared as a singleton. # A `static` field assignment is the recommended singleton form — skip it. heavy_object = re.compile( r"^(?!.*\bstatic\b).*\bnew\s+(ObjectMapper|Gson)\s*\(\s*\)", re.MULTILINE, ) for match in heavy_object.finditer(content): smells.append({ "type": "java_per_use_heavy_object", "severity": "medium", "message": ( f"'new {match.group(1)}()' is expensive — share a singleton " "instance instead of constructing per call" ), "location": f"offset {match.start()}", }) return smells def check_c_specific_smells(content: str) -> List[Dict]: """C-specific code smells documented in languages/c.md. Focuses on memory-safety and command/format-string patterns that the CERT C Coding Standard and the CWE catalogue rank as the highest-impact footguns. C uses the same line/block comment syntax as C# and Java, so the existing comment stripper applies. """ smells: List[Dict] = [] content = _strip_csharp_comments(content) # 1. Banned functions — no bounds check on any of them. banned = { "gets": "no bounds check, removed from C11 (CWE-242)", "strcpy": "no bounds check — prefer strncpy or strlcpy", "strcat": "no bounds check — prefer strncat or strlcat", "sprintf": "no bounds check — prefer snprintf", "vsprintf": "no bounds check — prefer vsnprintf", } for fn, reason in banned.items(): for m in re.finditer(rf"\b{fn}\s*\(", content): smells.append({ "type": f"c_banned_{fn}", "severity": "high", "message": f"'{fn}()' is unsafe: {reason}", "location": f"offset {m.start()}", }) # 2. Format-string vulnerability — printf/syslog called with a bare # identifier as the format argument (CWE-134). Skip when the first # arg is a string literal. for fn in ("printf", "syslog"): pattern = rf"\b{fn}\s*\(\s*(?!\")(\w+)\s*[,\)]" for m in re.finditer(pattern, content): smells.append({ "type": "c_format_string", "severity": "high", "message": ( f"'{fn}({m.group(1)})' uses a non-literal format string " "— CWE-134 format string vulnerability" ), "location": f"offset {m.start()}", }) # 3. Unbounded scanf — `%s` without a width specifier invites overflow. scanf_call = re.compile( r"\b(?:scanf|fscanf|sscanf)\s*\(\s*[^)]*?\"([^\"]*)\"" ) for m in scanf_call.finditer(content): fmt = m.group(1) if "%s" in fmt and not re.search(r"%\d+s", fmt): smells.append({ "type": "c_unbounded_scanf", "severity": "high", "message": ( "scanf '%s' without a width specifier — unbounded read " "can overflow the destination buffer" ), "location": f"offset {m.start()}", }) # 4. malloc / calloc / realloc result dereferenced without a NULL check # within 5 lines. CWE-690. lines = content.split("\n") malloc_assign = re.compile( r"^\s*(?:[\w\*]+\s+)?\*?(\w+)\s*=\s*\(?[\w\s\*]*\)?\s*" r"(?:m|c|re)alloc\s*\(" ) for i, line in enumerate(lines): m = malloc_assign.match(line) if not m: continue var = m.group(1) window = "\n".join(lines[i + 1 : i + 6]) null_check = re.compile( rf"\bif\s*\([^)]*(?:{re.escape(var)}\s*==\s*NULL" rf"|NULL\s*==\s*{re.escape(var)}" rf"|!\s*{re.escape(var)}\b" rf"|{re.escape(var)}\s*!=\s*NULL)" ) if not null_check.search(window): smells.append({ "type": "c_malloc_unchecked", "severity": "medium", "message": ( f"'{var}' from malloc/calloc/realloc is not NULL-checked " "within 5 lines — dereferencing NULL is UB (CWE-690)" ), "location": f"line {i + 1}", }) # 5. free(p) without setting p to NULL on the next real line. # CWE-416 use-after-free guardrail. free_call = re.compile(r"^\s*free\s*\(\s*(\w+)\s*\)\s*;") for i, line in enumerate(lines): m = free_call.match(line) if not m: continue var = m.group(1) for j in range(i + 1, min(i + 3, len(lines))): nxt = lines[j].strip() if not nxt: continue if re.match(rf"^{re.escape(var)}\s*=\s*NULL\s*;", nxt): break smells.append({ "type": "c_free_without_null", "severity": "low", "message": ( f"'free({var})' not followed by '{var} = NULL;' — " "dangling pointer can be reused (CWE-416)" ), "location": f"line {i + 1}", }) break # 6. system() with a non-string-literal argument — command injection. system_pattern = re.compile(r"\bsystem\s*\(\s*(?!\"|NULL\b)(\w+)\s*\)") for m in system_pattern.finditer(content): smells.append({ "type": "c_system_non_literal", "severity": "high", "message": ( f"'system({m.group(1)})' with a non-literal argument — " "command injection (CWE-78); use execve with validated args" ), "location": f"offset {m.start()}", }) return smells def check_solid_violations(content: str) -> List[Dict]: """Check for potential SOLID principle violations.""" violations = [] # OCP: Type checking instead of polymorphism type_checks = len(re.findall(r"isinstance\(|type\(.*\)\s*==|typeof\s+\w+\s*===", content)) if type_checks > 2: violations.append({ "principle": "OCP", "name": "Open/Closed Principle", "severity": "medium", "message": f"Found {type_checks} type checks - consider using polymorphism" }) # LSP/ISP: NotImplementedError not_impl = len(re.findall(r"raise\s+NotImplementedError|not\s+implemented", content, re.IGNORECASE)) if not_impl: violations.append({ "principle": "LSP/ISP", "name": "Liskov/Interface Segregation", "severity": "low", "message": f"Found {not_impl} unimplemented methods - may indicate oversized interface" }) # DIP: Too many direct imports imports = len(re.findall(r"^(?:import|from)\s+", content, re.MULTILINE)) if imports > THRESHOLDS["max_imports"]: violations.append({ "principle": "DIP", "name": "Dependency Inversion Principle", "severity": "low", "message": f"File has {imports} imports - consider dependency injection" }) return violations def calculate_quality_score( line_metrics: Dict, functions: List[Dict], classes: List[Dict], smells: List[Dict], violations: List[Dict] ) -> int: """Calculate overall quality score (0-100).""" score = 100 # Deduct for code smells for smell in smells: if smell["severity"] == "high": score -= 10 elif smell["severity"] == "medium": score -= 5 elif smell["severity"] == "low": score -= 2 # Deduct for SOLID violations for violation in violations: if violation["severity"] == "high": score -= 8 elif violation["severity"] == "medium": score -= 4 elif violation["severity"] == "low": score -= 2 # Bonus for good comment ratio (10-30%) if line_metrics["total"] > 0: comment_ratio = line_metrics["comment"] / line_metrics["total"] if 0.1 <= comment_ratio <= 0.3: score += 5 # Bonus for reasonable function sizes if functions: avg_lines = sum(f["lines"] for f in functions) / len(functions) if avg_lines < 30: score += 5 return max(0, min(100, score)) def get_grade(score: int) -> str: """Convert score to letter grade.""" if score >= 90: return "A" elif score >= 80: return "B" elif score >= 70: return "C" elif score >= 60: return "D" else: return "F" def analyze_file(filepath: Path) -> Dict: """Analyze a single file for code quality.""" language = detect_language(filepath) if not language: return {"error": f"Unsupported file type: {filepath.suffix}"} content = read_file_content(filepath) if not content: return {"error": f"Could not read file: {filepath}"} line_metrics = count_lines(content) functions = find_functions(content, language) classes = find_classes(content, language) smells = check_code_smells(content, functions, classes) if language == "csharp": smells.extend(check_csharp_specific_smells(content)) if language == "java": smells.extend(check_java_specific_smells(content)) if language == "c": smells.extend(check_c_specific_smells(content)) violations = check_solid_violations(content) score = calculate_quality_score(line_metrics, functions, classes, smells, violations) return { "file": str(filepath), "language": language, "metrics": { "lines": line_metrics, "functions": len(functions), "classes": len(classes), "avg_complexity": round(sum(f["complexity"] for f in functions) / max(1, len(functions)), 1) }, "quality_score": score, "grade": get_grade(score), "smells": smells, "solid_violations": violations, "function_details": functions[:10], "class_details": classes[:10] } def analyze_directory( dir_path: Path, recursive: bool = True, language: Optional[str] = None ) -> Dict: """Analyze all files in a directory.""" results = [] extensions = [] if language: extensions = LANGUAGE_EXTENSIONS.get(language, []) else: for exts in LANGUAGE_EXTENSIONS.values(): extensions.extend(exts) pattern = "**/*" if recursive else "*" for ext in extensions: for filepath in dir_path.glob(f"{pattern}{ext}"): if "node_modules" in str(filepath) or ".git" in str(filepath): continue result = analyze_file(filepath) if "error" not in result: results.append(result) if not results: return {"error": "No supported files found"} total_score = sum(r["quality_score"] for r in results) avg_score = total_score / len(results) total_smells = sum(len(r["smells"]) for r in results) total_violations = sum(len(r["solid_violations"]) for r in results) return { "directory": str(dir_path), "files_analyzed": len(results), "average_score": round(avg_score, 1), "overall_grade": get_grade(int(avg_score)), "total_code_smells": total_smells, "total_solid_violations": total_violations, "files": sorted(results, key=lambda x: x["quality_score"]) } def print_report(analysis: Dict) -> None: """Print human-readable analysis report.""" if "error" in analysis: print(f"Error: {analysis['error']}") return print("=" * 60) print("CODE QUALITY REPORT") print("=" * 60) if "file" in analysis: print(f"\nFile: {analysis['file']}") print(f"Language: {analysis['language']}") print(f"Quality Score: {analysis['quality_score']}/100 ({analysis['grade']})") metrics = analysis["metrics"] print(f"\nLines: {metrics['lines']['total']} ({metrics['lines']['code']} code, {metrics['lines']['comment']} comments)") print(f"Functions: {metrics['functions']}") print(f"Classes: {metrics['classes']}") print(f"Avg Complexity: {metrics['avg_complexity']}") if analysis["smells"]: print("\n--- CODE SMELLS ---") for smell in analysis["smells"][:10]: print(f" [{smell['severity'].upper()}] {smell['message']} ({smell['location']})") if analysis["solid_violations"]: print("\n--- SOLID VIOLATIONS ---") for v in analysis["solid_violations"]: print(f" [{v['principle']}] {v['message']}") else: print(f"\nDirectory: {analysis['directory']}") print(f"Files Analyzed: {analysis['files_analyzed']}") print(f"Average Score: {analysis['average_score']}/100 ({analysis['overall_grade']})") print(f"Total Code Smells: {analysis['total_code_smells']}") print(f"Total SOLID Violations: {analysis['total_solid_violations']}") print("\n--- FILES BY QUALITY ---") for f in analysis["files"][:10]: print(f" {f['quality_score']:3d}/100 [{f['grade']}] {f['file']}") print("\n" + "=" * 60) def main(): parser = argparse.ArgumentParser( description="Analyze code quality, smells, and SOLID violations" ) parser.add_argument( "path", help="File or directory to analyze" ) parser.add_argument( "--recursive", "-r", action="store_true", default=True, help="Recursively analyze directories (default: true)" ) parser.add_argument( "--language", "-l", choices=list(LANGUAGE_EXTENSIONS.keys()), help="Filter by programming language" ) parser.add_argument( "--json", action="store_true", help="Output in JSON format" ) parser.add_argument( "--output", "-o", help="Write output to file" ) args = parser.parse_args() target = Path(args.path).resolve() if not target.exists(): print(f"Error: Path does not exist: {target}", file=sys.stderr) sys.exit(1) if target.is_file(): analysis = analyze_file(target) else: analysis = analyze_directory(target, args.recursive, args.language) if args.json: output = json.dumps(analysis, indent=2, default=str) if args.output: with open(args.output, "w") as f: f.write(output) print(f"Results written to {args.output}") else: print(output) else: print_report(analysis) if __name__ == "__main__": main() ``` ## scripts/pr_analyzer.py ```python #!/usr/bin/env python3 """ PR Analyzer Analyzes pull request changes for review complexity, risk assessment, and generates review priorities. Usage: python pr_analyzer.py /path/to/repo python pr_analyzer.py . --base main --head feature-branch python pr_analyzer.py /path/to/repo --json """ import argparse import json import os import re import subprocess import sys from pathlib import Path from typing import Dict, List, Optional, Tuple # File categories for review prioritization FILE_CATEGORIES = { "critical": { "patterns": [ r"auth", r"security", r"password", r"token", r"secret", r"payment", r"billing", r"crypto", r"encrypt" ], "weight": 5, "description": "Security-sensitive files requiring careful review" }, "high": { "patterns": [ r"api", r"database", r"migration", r"schema", r"model", r"config", r"env", r"middleware" ], "weight": 4, "description": "Core infrastructure files" }, "medium": { "patterns": [ r"service", r"controller", r"handler", r"util", r"helper" ], "weight": 3, "description": "Business logic files" }, "low": { "patterns": [ r"test", r"spec", r"mock", r"fixture", r"story", r"readme", r"docs", r"\.md$" ], "weight": 1, "description": "Tests and documentation" } } # Risky patterns to flag RISK_PATTERNS = [ { "name": "hardcoded_secrets", "pattern": r"(password|secret|api_key|token|connection_?string)\s*[=:]\s*['\"][^'\"]+['\"]", "severity": "critical", "message": "Potential hardcoded secret or connection string detected" }, { "name": "todo_fixme", "pattern": r"(TODO|FIXME|HACK|XXX):", "severity": "low", "message": "TODO/FIXME comment found" }, { "name": "console_log", "pattern": ( r"console\.(log|debug|info|warn|error)\(|\bDebug\.WriteLine\(|" r"\bSystem\.out\.print(?:ln)?\(|\.printStackTrace\(" ), "severity": "medium", "message": ( "Debug output statement found " "(console.* / Debug.WriteLine / System.out / printStackTrace)" ) }, { "name": "debugger", "pattern": r"\bdebugger\b", "severity": "high", "message": "Debugger statement found" }, { "name": "analyzer_disable", "pattern": ( r"eslint-disable|#pragma\s+warning\s+disable|\[SuppressMessage|" r"@SuppressWarnings" ), "severity": "medium", "message": ( "Static-analyzer rule disabled " "(ESLint / Roslyn / SuppressMessage / @SuppressWarnings)" ) }, { "name": "loose_type", "pattern": r":\s*any\b|\bdynamic\s+\w+\s*[=;]", "severity": "medium", "message": "Loose type used (TypeScript 'any' or C# 'dynamic')" }, { "name": "sql_concatenation", "pattern": r"(SELECT|INSERT|UPDATE|DELETE).*\+.*['\"]|(?:FromSql|ExecuteSql)\w*\([^)]*\$\"", "severity": "critical", "message": "Potential SQL injection (string concatenation or interpolation in query)" }, { "name": "csharp_unsafe_block", "pattern": ( r"\bunsafe\s+(?:\{|public|private|protected|internal|static|sealed|" r"partial|class|struct|void|int|string|long|short|byte|double|float|" r"bool|char|ref|out|fixed)\b" ), "severity": "high", "message": "C# 'unsafe' code — requires memory-safety review" }, { "name": "csharp_null_forgiving", "pattern": r"(?:\)\s*!\.|\w+!\.\w+)", "severity": "medium", "message": "Null-forgiving operator (!) used — verify the value is truly non-null" }, { "name": "csharp_async_void", "pattern": r"\basync\s+void\s+\w+\s*\(", "severity": "high", "message": "'async void' method — use only for event handlers" }, { "name": "csharp_blocking_async", "pattern": r"\.(?:Result\b|Wait\(\)|GetAwaiter\(\)\.GetResult\(\))", "severity": "high", "message": "Blocking call on async operation — can deadlock in ASP.NET contexts" } ] def run_git_command(cmd: List[str], cwd: Path) -> Tuple[bool, str]: """Run a git command and return success status and output.""" try: result = subprocess.run( cmd, cwd=cwd, capture_output=True, text=True, timeout=30 ) return result.returncode == 0, result.stdout.strip() except subprocess.TimeoutExpired: return False, "Command timed out" except Exception as e: return False, str(e) def get_changed_files(repo_path: Path, base: str, head: str) -> List[Dict]: """Get list of changed files between two refs.""" success, output = run_git_command( ["git", "diff", "--name-status", f"{base}...{head}"], repo_path ) if not success: # Try without the triple dot (for uncommitted changes) success, output = run_git_command( ["git", "diff", "--name-status", base, head], repo_path ) if not success or not output: # Fall back to staged changes success, output = run_git_command( ["git", "diff", "--name-status", "--cached"], repo_path ) files = [] for line in output.split("\n"): if not line.strip(): continue parts = line.split("\t") if len(parts) >= 2: status = parts[0][0] # First character of status filepath = parts[-1] # Handle renames (R100\told\tnew) status_map = { "A": "added", "M": "modified", "D": "deleted", "R": "renamed", "C": "copied" } files.append({ "path": filepath, "status": status_map.get(status, "modified") }) return files def get_file_diff(repo_path: Path, filepath: str, base: str, head: str) -> str: """Get diff content for a specific file.""" success, output = run_git_command( ["git", "diff", f"{base}...{head}", "--", filepath], repo_path ) if not success: success, output = run_git_command( ["git", "diff", "--cached", "--", filepath], repo_path ) return output if success else "" def categorize_file(filepath: str) -> Tuple[str, int]: """Categorize a file based on its path and name.""" filepath_lower = filepath.lower() for category, info in FILE_CATEGORIES.items(): for pattern in info["patterns"]: if re.search(pattern, filepath_lower): return category, info["weight"] return "medium", 2 # Default category def analyze_diff_for_risks(diff_content: str, filepath: str) -> List[Dict]: """Analyze diff content for risky patterns.""" risks = [] # Only analyze added lines (starting with +) added_lines = [ line[1:] for line in diff_content.split("\n") if line.startswith("+") and not line.startswith("+++") ] content = "\n".join(added_lines) for risk in RISK_PATTERNS: matches = re.findall(risk["pattern"], content, re.IGNORECASE) if matches: risks.append({ "name": risk["name"], "severity": risk["severity"], "message": risk["message"], "file": filepath, "count": len(matches) }) return risks def count_changes(diff_content: str) -> Dict[str, int]: """Count additions and deletions in diff.""" additions = 0 deletions = 0 for line in diff_content.split("\n"): if line.startswith("+") and not line.startswith("+++"): additions += 1 elif line.startswith("-") and not line.startswith("---"): deletions += 1 return {"additions": additions, "deletions": deletions} def calculate_complexity_score(files: List[Dict], all_risks: List[Dict]) -> int: """Calculate overall PR complexity score (1-10).""" score = 0 # File count contribution (max 3 points) file_count = len(files) if file_count > 20: score += 3 elif file_count > 10: score += 2 elif file_count > 5: score += 1 # Total changes contribution (max 3 points) total_changes = sum(f.get("additions", 0) + f.get("deletions", 0) for f in files) if total_changes > 500: score += 3 elif total_changes > 200: score += 2 elif total_changes > 50: score += 1 # Risk severity contribution (max 4 points) critical_risks = sum(1 for r in all_risks if r["severity"] == "critical") high_risks = sum(1 for r in all_risks if r["severity"] == "high") score += min(2, critical_risks) score += min(2, high_risks) return min(10, max(1, score)) def analyze_commit_messages(repo_path: Path, base: str, head: str) -> Dict: """Analyze commit messages in the PR.""" success, output = run_git_command( ["git", "log", "--oneline", f"{base}...{head}"], repo_path ) if not success or not output: return {"commits": 0, "issues": []} commits = output.strip().split("\n") issues = [] for commit in commits: if len(commit) < 10: continue # Check for conventional commit format message = commit[8:] if len(commit) > 8 else commit # Skip hash if not re.match(r"^(feat|fix|docs|style|refactor|test|chore|perf|ci|build|revert)(\(.+\))?:", message): issues.append({ "commit": commit[:7], "issue": "Does not follow conventional commit format" }) if len(message) > 72: issues.append({ "commit": commit[:7], "issue": "Commit message exceeds 72 characters" }) return { "commits": len(commits), "issues": issues } def analyze_pr( repo_path: Path, base: str = "main", head: str = "HEAD" ) -> Dict: """Perform complete PR analysis.""" # Get changed files changed_files = get_changed_files(repo_path, base, head) if not changed_files: return { "status": "no_changes", "message": "No changes detected between branches" } # Analyze each file all_risks = [] file_analyses = [] for file_info in changed_files: filepath = file_info["path"] category, weight = categorize_file(filepath) # Get diff for the file diff = get_file_diff(repo_path, filepath, base, head) changes = count_changes(diff) risks = analyze_diff_for_risks(diff, filepath) all_risks.extend(risks) file_analyses.append({ "path": filepath, "status": file_info["status"], "category": category, "priority_weight": weight, "additions": changes["additions"], "deletions": changes["deletions"], "risks": risks }) # Sort by priority (highest first) file_analyses.sort(key=lambda x: (-x["priority_weight"], x["path"])) # Analyze commits commit_analysis = analyze_commit_messages(repo_path, base, head) # Calculate metrics complexity = calculate_complexity_score(file_analyses, all_risks) total_additions = sum(f["additions"] for f in file_analyses) total_deletions = sum(f["deletions"] for f in file_analyses) return { "status": "analyzed", "summary": { "files_changed": len(file_analyses), "total_additions": total_additions, "total_deletions": total_deletions, "complexity_score": complexity, "complexity_label": get_complexity_label(complexity), "commits": commit_analysis["commits"] }, "risks": { "critical": [r for r in all_risks if r["severity"] == "critical"], "high": [r for r in all_risks if r["severity"] == "high"], "medium": [r for r in all_risks if r["severity"] == "medium"], "low": [r for r in all_risks if r["severity"] == "low"] }, "files": file_analyses, "commit_issues": commit_analysis["issues"], "review_order": [f["path"] for f in file_analyses[:10]] # Top 10 priority files } def get_complexity_label(score: int) -> str: """Get human-readable complexity label.""" if score <= 2: return "Simple" elif score <= 4: return "Moderate" elif score <= 6: return "Complex" elif score <= 8: return "Very Complex" else: return "Critical" def print_report(analysis: Dict) -> None: """Print human-readable analysis report.""" if analysis["status"] == "no_changes": print("No changes detected.") return summary = analysis["summary"] risks = analysis["risks"] print("=" * 60) print("PR ANALYSIS REPORT") print("=" * 60) print(f"\nComplexity: {summary['complexity_score']}/10 ({summary['complexity_label']})") print(f"Files Changed: {summary['files_changed']}") print(f"Lines: +{summary['total_additions']} / -{summary['total_deletions']}") print(f"Commits: {summary['commits']}") # Risk summary print("\n--- RISK SUMMARY ---") print(f"Critical: {len(risks['critical'])}") print(f"High: {len(risks['high'])}") print(f"Medium: {len(risks['medium'])}") print(f"Low: {len(risks['low'])}") # Critical and high risks details if risks["critical"]: print("\n--- CRITICAL RISKS ---") for risk in risks["critical"]: print(f" [{risk['file']}] {risk['message']} (x{risk['count']})") if risks["high"]: print("\n--- HIGH RISKS ---") for risk in risks["high"]: print(f" [{risk['file']}] {risk['message']} (x{risk['count']})") # Commit message issues if analysis["commit_issues"]: print("\n--- COMMIT MESSAGE ISSUES ---") for issue in analysis["commit_issues"][:5]: print(f" {issue['commit']}: {issue['issue']}") # Review order print("\n--- SUGGESTED REVIEW ORDER ---") for i, filepath in enumerate(analysis["review_order"], 1): file_info = next(f for f in analysis["files"] if f["path"] == filepath) print(f" {i}. [{file_info['category'].upper()}] {filepath}") print("\n" + "=" * 60) def main(): parser = argparse.ArgumentParser( description="Analyze pull request for review complexity and risks" ) parser.add_argument( "repo_path", nargs="?", default=".", help="Path to git repository (default: current directory)" ) parser.add_argument( "--base", "-b", default="main", help="Base branch for comparison (default: main)" ) parser.add_argument( "--head", default="HEAD", help="Head branch/commit for comparison (default: HEAD)" ) parser.add_argument( "--json", action="store_true", help="Output in JSON format" ) parser.add_argument( "--output", "-o", help="Write output to file" ) args = parser.parse_args() repo_path = Path(args.repo_path).resolve() if not (repo_path / ".git").exists(): print(f"Error: {repo_path} is not a git repository", file=sys.stderr) sys.exit(1) analysis = analyze_pr(repo_path, args.base, args.head) if args.json: output = json.dumps(analysis, indent=2) if args.output: with open(args.output, "w") as f: f.write(output) print(f"Results written to {args.output}") else: print(output) else: print_report(analysis) if __name__ == "__main__": main() ``` ## scripts/review_report_generator.py ```python #!/usr/bin/env python3 """ Review Report Generator Generates comprehensive code review reports by combining PR analysis and code quality findings into structured, actionable reports. Usage: python review_report_generator.py /path/to/repo python review_report_generator.py . --pr-analysis pr_results.json --quality-analysis quality_results.json python review_report_generator.py /path/to/repo --format markdown --output review.md """ import argparse import json import os import subprocess import sys from datetime import datetime from pathlib import Path from typing import Dict, List, Optional, Tuple # Severity weights for prioritization SEVERITY_WEIGHTS = { "critical": 100, "high": 75, "medium": 50, "low": 25, "info": 10 } # Review verdict thresholds VERDICT_THRESHOLDS = { "approve": {"max_critical": 0, "max_high": 0, "max_score": 100}, "approve_with_suggestions": {"max_critical": 0, "max_high": 2, "max_score": 85}, "request_changes": {"max_critical": 0, "max_high": 5, "max_score": 70}, "block": {"max_critical": float("inf"), "max_high": float("inf"), "max_score": 0} } def load_json_file(filepath: str) -> Optional[Dict]: """Load JSON file if it exists.""" try: with open(filepath, "r") as f: return json.load(f) except (FileNotFoundError, json.JSONDecodeError): return None def run_pr_analyzer(repo_path: Path) -> Dict: """Run pr_analyzer.py and return results.""" script_path = Path(__file__).parent / "pr_analyzer.py" if not script_path.exists(): return {"status": "error", "message": "pr_analyzer.py not found"} try: result = subprocess.run( [sys.executable, str(script_path), str(repo_path), "--json"], capture_output=True, text=True, timeout=120 ) if result.returncode == 0: return json.loads(result.stdout) return {"status": "error", "message": result.stderr} except Exception as e: return {"status": "error", "message": str(e)} def run_quality_checker(repo_path: Path) -> Dict: """Run code_quality_checker.py and return results.""" script_path = Path(__file__).parent / "code_quality_checker.py" if not script_path.exists(): return {"status": "error", "message": "code_quality_checker.py not found"} try: result = subprocess.run( [sys.executable, str(script_path), str(repo_path), "--json"], capture_output=True, text=True, timeout=300 ) if result.returncode == 0: return json.loads(result.stdout) return {"status": "error", "message": result.stderr} except Exception as e: return {"status": "error", "message": str(e)} def calculate_review_score(pr_analysis: Dict, quality_analysis: Dict) -> int: """Calculate overall review score (0-100).""" score = 100 # Deduct for PR risks if "risks" in pr_analysis: risks = pr_analysis["risks"] score -= len(risks.get("critical", [])) * 15 score -= len(risks.get("high", [])) * 10 score -= len(risks.get("medium", [])) * 5 score -= len(risks.get("low", [])) * 2 # Deduct for code quality issues if "issues" in quality_analysis: issues = quality_analysis["issues"] score -= len([i for i in issues if i.get("severity") == "critical"]) * 12 score -= len([i for i in issues if i.get("severity") == "high"]) * 8 score -= len([i for i in issues if i.get("severity") == "medium"]) * 4 score -= len([i for i in issues if i.get("severity") == "low"]) * 1 # Deduct for complexity if "summary" in pr_analysis: complexity = pr_analysis["summary"].get("complexity_score", 0) if complexity > 7: score -= 10 elif complexity > 5: score -= 5 return max(0, min(100, score)) def determine_verdict(score: int, critical_count: int, high_count: int) -> Tuple[str, str]: """Determine review verdict based on score and issue counts.""" if critical_count > 0: return "block", "Critical issues must be resolved before merge" if score >= 90 and high_count == 0: return "approve", "Code meets quality standards" if score >= 75 and high_count <= 2: return "approve_with_suggestions", "Minor improvements recommended" if score >= 50: return "request_changes", "Several issues need to be addressed" return "block", "Significant issues prevent approval" def generate_findings_list(pr_analysis: Dict, quality_analysis: Dict) -> List[Dict]: """Combine and prioritize all findings.""" findings = [] # Add PR risk findings if "risks" in pr_analysis: for severity, items in pr_analysis["risks"].items(): for item in items: findings.append({ "source": "pr_analysis", "severity": severity, "category": item.get("name", "unknown"), "message": item.get("message", ""), "file": item.get("file", ""), "count": item.get("count", 1) }) # Add code quality findings if "issues" in quality_analysis: for issue in quality_analysis["issues"]: findings.append({ "source": "quality_analysis", "severity": issue.get("severity", "medium"), "category": issue.get("type", "unknown"), "message": issue.get("message", ""), "file": issue.get("file", ""), "line": issue.get("line", 0) }) # Sort by severity weight findings.sort( key=lambda x: -SEVERITY_WEIGHTS.get(x["severity"], 0) ) return findings def generate_action_items(findings: List[Dict]) -> List[Dict]: """Generate prioritized action items from findings.""" action_items = [] seen_categories = set() for finding in findings: category = finding["category"] severity = finding["severity"] # Group similar issues if category in seen_categories and severity not in ["critical", "high"]: continue action = { "priority": "P0" if severity == "critical" else "P1" if severity == "high" else "P2", "action": get_action_for_category(category, finding), "severity": severity, "files_affected": [finding["file"]] if finding.get("file") else [] } action_items.append(action) seen_categories.add(category) return action_items[:15] # Top 15 actions def get_action_for_category(category: str, finding: Dict) -> str: """Get actionable recommendation for issue category.""" actions = { "hardcoded_secrets": "Remove hardcoded credentials and use environment variables or a secrets manager", "sql_concatenation": "Use parameterized queries to prevent SQL injection", "debugger": "Remove debugger statements before merging", "console_log": "Remove or replace console statements with proper logging", "todo_fixme": "Address TODO/FIXME comments or create tracking issues", "disable_eslint": "Address the underlying issue instead of disabling lint rules", "any_type": "Replace 'any' types with proper type definitions", "long_function": "Break down function into smaller, focused units", "god_class": "Split class into smaller, single-responsibility classes", "too_many_params": "Use parameter objects or builder pattern", "deep_nesting": "Refactor using early returns, guard clauses, or extraction", "high_complexity": "Reduce cyclomatic complexity through refactoring", "missing_error_handling": "Add proper error handling and recovery logic", "duplicate_code": "Extract duplicate code into shared functions", "magic_numbers": "Replace magic numbers with named constants", "large_file": "Consider splitting into multiple smaller modules" } return actions.get(category, f"Review and address: {finding.get('message', category)}") def format_markdown_report(report: Dict) -> str: """Generate markdown-formatted report.""" lines = [] # Header lines.append("# Code Review Report") lines.append("") lines.append(f"**Generated:** {report['metadata']['generated_at']}") lines.append(f"**Repository:** {report['metadata']['repository']}") lines.append("") # Executive Summary lines.append("## Executive Summary") lines.append("") summary = report["summary"] verdict = summary["verdict"] verdict_emoji = { "approve": "✅", "approve_with_suggestions": "✅", "request_changes": "⚠️", "block": "❌" }.get(verdict, "❓") lines.append(f"**Verdict:** {verdict_emoji} {verdict.upper().replace('_', ' ')}") lines.append(f"**Score:** {summary['score']}/100") lines.append(f"**Rationale:** {summary['rationale']}") lines.append("") # Issue Counts lines.append("### Issue Summary") lines.append("") lines.append("| Severity | Count |") lines.append("|----------|-------|") for severity in ["critical", "high", "medium", "low"]: count = summary["issue_counts"].get(severity, 0) lines.append(f"| {severity.capitalize()} | {count} |") lines.append("") # PR Statistics (if available) if "pr_summary" in report: pr = report["pr_summary"] lines.append("### Change Statistics") lines.append("") lines.append(f"- **Files Changed:** {pr.get('files_changed', 'N/A')}") lines.append(f"- **Lines Added:** +{pr.get('total_additions', 0)}") lines.append(f"- **Lines Removed:** -{pr.get('total_deletions', 0)}") lines.append(f"- **Complexity:** {pr.get('complexity_label', 'N/A')}") lines.append("") # Action Items if report.get("action_items"): lines.append("## Action Items") lines.append("") for i, item in enumerate(report["action_items"], 1): priority = item["priority"] emoji = "🔴" if priority == "P0" else "🟠" if priority == "P1" else "🟡" lines.append(f"{i}. {emoji} **[{priority}]** {item['action']}") if item.get("files_affected"): lines.append(f" - Files: {', '.join(item['files_affected'][:3])}") lines.append("") # Critical Findings critical_findings = [f for f in report.get("findings", []) if f["severity"] == "critical"] if critical_findings: lines.append("## Critical Issues (Must Fix)") lines.append("") for finding in critical_findings: lines.append(f"- **{finding['category']}** in `{finding.get('file', 'unknown')}`") lines.append(f" - {finding['message']}") lines.append("") # High Priority Findings high_findings = [f for f in report.get("findings", []) if f["severity"] == "high"] if high_findings: lines.append("## High Priority Issues") lines.append("") for finding in high_findings[:10]: lines.append(f"- **{finding['category']}** in `{finding.get('file', 'unknown')}`") lines.append(f" - {finding['message']}") lines.append("") # Review Order (if available) if "review_order" in report: lines.append("## Suggested Review Order") lines.append("") for i, filepath in enumerate(report["review_order"][:10], 1): lines.append(f"{i}. `{filepath}`") lines.append("") # Footer lines.append("---") lines.append("*Generated by Code Reviewer*") return "\n".join(lines) def format_text_report(report: Dict) -> str: """Generate plain text report.""" lines = [] lines.append("=" * 60) lines.append("CODE REVIEW REPORT") lines.append("=" * 60) lines.append("") lines.append(f"Generated: {report['metadata']['generated_at']}") lines.append(f"Repository: {report['metadata']['repository']}") lines.append("") summary = report["summary"] verdict = summary["verdict"].upper().replace("_", " ") lines.append(f"VERDICT: {verdict}") lines.append(f"SCORE: {summary['score']}/100") lines.append(f"RATIONALE: {summary['rationale']}") lines.append("") lines.append("--- ISSUE SUMMARY ---") for severity in ["critical", "high", "medium", "low"]: count = summary["issue_counts"].get(severity, 0) lines.append(f" {severity.capitalize()}: {count}") lines.append("") if report.get("action_items"): lines.append("--- ACTION ITEMS ---") for i, item in enumerate(report["action_items"][:10], 1): lines.append(f" {i}. [{item['priority']}] {item['action']}") lines.append("") critical = [f for f in report.get("findings", []) if f["severity"] == "critical"] if critical: lines.append("--- CRITICAL ISSUES ---") for f in critical: lines.append(f" [{f.get('file', 'unknown')}] {f['message']}") lines.append("") lines.append("=" * 60) return "\n".join(lines) def generate_report( repo_path: Path, pr_analysis: Optional[Dict] = None, quality_analysis: Optional[Dict] = None ) -> Dict: """Generate comprehensive review report.""" # Run analyses if not provided if pr_analysis is None: pr_analysis = run_pr_analyzer(repo_path) if quality_analysis is None: quality_analysis = run_quality_checker(repo_path) # Generate findings findings = generate_findings_list(pr_analysis, quality_analysis) # Count issues by severity issue_counts = { "critical": len([f for f in findings if f["severity"] == "critical"]), "high": len([f for f in findings if f["severity"] == "high"]), "medium": len([f for f in findings if f["severity"] == "medium"]), "low": len([f for f in findings if f["severity"] == "low"]) } # Calculate score and verdict score = calculate_review_score(pr_analysis, quality_analysis) verdict, rationale = determine_verdict( score, issue_counts["critical"], issue_counts["high"] ) # Generate action items action_items = generate_action_items(findings) # Build report report = { "metadata": { "generated_at": datetime.now().isoformat(), "repository": str(repo_path), "version": "1.0.0" }, "summary": { "score": score, "verdict": verdict, "rationale": rationale, "issue_counts": issue_counts }, "findings": findings, "action_items": action_items } # Add PR summary if available if pr_analysis.get("status") == "analyzed": report["pr_summary"] = pr_analysis.get("summary", {}) report["review_order"] = pr_analysis.get("review_order", []) # Add quality summary if available if quality_analysis.get("status") == "analyzed": report["quality_summary"] = quality_analysis.get("summary", {}) return report def main(): parser = argparse.ArgumentParser( description="Generate comprehensive code review reports" ) parser.add_argument( "repo_path", nargs="?", default=".", help="Path to repository (default: current directory)" ) parser.add_argument( "--pr-analysis", help="Path to pre-computed PR analysis JSON" ) parser.add_argument( "--quality-analysis", help="Path to pre-computed quality analysis JSON" ) parser.add_argument( "--format", "-f", choices=["text", "markdown", "json"], default="text", help="Output format (default: text)" ) parser.add_argument( "--output", "-o", help="Write output to file" ) parser.add_argument( "--json", action="store_true", help="Output as JSON (shortcut for --format json)" ) args = parser.parse_args() repo_path = Path(args.repo_path).resolve() if not repo_path.exists(): print(f"Error: Path does not exist: {repo_path}", file=sys.stderr) sys.exit(1) # Load pre-computed analyses if provided pr_analysis = None quality_analysis = None if args.pr_analysis: pr_analysis = load_json_file(args.pr_analysis) if not pr_analysis: print(f"Warning: Could not load PR analysis from {args.pr_analysis}") if args.quality_analysis: quality_analysis = load_json_file(args.quality_analysis) if not quality_analysis: print(f"Warning: Could not load quality analysis from {args.quality_analysis}") # Generate report report = generate_report(repo_path, pr_analysis, quality_analysis) # Format output output_format = "json" if args.json else args.format if output_format == "json": output = json.dumps(report, indent=2) elif output_format == "markdown": output = format_markdown_report(report) else: output = format_text_report(report) # Write or print output if args.output: with open(args.output, "w") as f: f.write(output) print(f"Report written to {args.output}") else: print(output) if __name__ == "__main__": main() ```