Code Reviewer

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.

---

Tools

PR Analyzer

Analyzes git diff between branches to assess review complexity and identify risks.

# 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.

# 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:

Language-specific checks are defined in each languages/*.md file.

---

Review Report Generator

Combines PR analysis and code quality findings into structured review reports.

# 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:

---

Adding a New Language

Reviewer guidance (required):

1. Create languages/<name>.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_<name>_specific_smells(...) detector (see the C#, Java, and C ones) and call it from analyze_file. 6. Add assets/sample_<name>_smells.<ext> + _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:

python scripts/code_quality_checker.py assets/sample_java_smells.java --json \
  | diff - expected_outputs/sample_java_smells_quality.json

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

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<T, N>
• 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<std::shared_ptr<T>> (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<T> 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

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 <Nullable>enable</Nullable> 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

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

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

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<T> 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<String>, 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

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

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

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

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+)

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<Mutex<T>> used where a simpler ownership model would work
• Box<dyn Trait> 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<Mutex<T>> 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<T> used in multi-threaded code — use Arc<T>; 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<T, E>
• Flag unwrap_or_default() where the default silently masks a real error
• Prefer typed error enums (thiserror) over Box<dyn Error> 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<T> would work
• Flag format! used only to create a String from a literal — use .to_string() or String::from
• Flag collect::<Vec<_>>() followed immediately by .iter() — chain iterators instead
• Flag Box<T> 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<dyn Trait> 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

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

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

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`. This README is a quick reference for the 3 bundled scripts.

---

How to use

Quick install check

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

# 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

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

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

Use them as a regression-detection harness:

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` 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`):

• `rules/universal.md` — cross-language rules: security, async/concurrency, resource management, exception handling, performance
• `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

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