diff --git a/PLAN.md b/PLAN.md index 280ff63..ad8b24c 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1,74 +1,701 @@ -# Review Modes — Implementation Plan +# codehawk — Sprint 3: Review Quality + Coverage Enforcement -> Add two mutually exclusive CLI flags (--verify-fixes and --check-new-findings) to control whether a review run verifies prior findings, looks for new issues, or does both (current default). Introduces a ReviewMode enum threaded through the pipeline, with mode-specific agent prompt injection and defense-in-depth guards in _write_findings(). +> Fix review quality by aligning tool-call limits, adding 100% coverage enforcement with risk-based depth, expanding finding categories, reducing noise via comment merging, adding language intelligence for 15 languages, and implementing deterministic fix verification with parallel batch execution. --- ## Tasks -### Phase 1: Foundation — Enum, Config, CLI +### Phase 1: Prompt Alignment + Determinism -#### Task 1: Add ReviewMode enum and thread through config -- **Change:** (1) Add ReviewMode(str, Enum) to src/models/review_models.py with values FULL = "full", VERIFY_FIXES = "verify_fixes", CHECK_NEW = "check_new". (2) Add review_mode: ReviewMode = ReviewMode.FULL field to ReviewJobConfig in src/review_job.py. Import ReviewMode. (3) Add review_mode: ReviewMode = ReviewMode.FULL parameter to BatchReviewJob.__init__ in src/batch_review_job.py, store as self.review_mode. Import ReviewMode. -- **Files:** src/models/review_models.py, src/review_job.py, src/batch_review_job.py -- **Tier:** cheap -- **Done when:** from models.review_models import ReviewMode works; ReviewMode.FULL, .VERIFY_FIXES, .CHECK_NEW are valid; ReviewJobConfig(review_mode=ReviewMode.VERIFY_FIXES) accepted; BatchReviewJob accepts review_mode kwarg; all 190 existing tests pass. -- **Blockers:** None +#### Task 1: Align tool-call limit +- **Change:** In `commands/review-pr-core.md`, Step 0 constraints block — replace "max 10 tool calls" with "max 40 tool calls". This aligns with the deployment budget in `docker-compose.yml` (`MAX_TURNS=40`) and the Claude wrapper (`review-pr.claude.md`: "max 40 tool calls"). +- **Files:** `commands/review-pr-core.md` +- **Tier:** standard +- **Done when:** Grep for "max 10" in `review-pr-core.md` returns zero matches; "max 40" appears in Step 0. +- **Blockers:** none + +#### Task 2: Rewrite TURN EFFICIENCY block +- **Change:** In `src/agents/openai_runner.py`, `SYSTEM_PROMPT` constant, the block starting with "TURN EFFICIENCY" — keep "Do NOT call `get_file_diff`, `get_change_analysis`, `get_blast_radius`, or `get_pr` — all data is pre-injected." Remove "Your goal: review all pre-injected diffs and produce findings with ZERO or minimal tool calls." Replace with: + + > You have 40 tool calls available. Use `read_local_file` or `get_file_content` when you need full-file context to verify a finding. Use `get_callers` to check blast radius on high-risk changes. The pre-injected diffs save you from fetching diffs — but reading full files for verification is expected and encouraged. Spend your turns where they matter most: verify before flagging, prioritize high-risk files, and ensure every file in your batch is covered. + +- **Files:** `src/agents/openai_runner.py` +- **Tier:** standard +- **Done when:** Grep for "ZERO or minimal" in `SYSTEM_PROMPT` returns zero matches; the new text appears in its place. +- **Blockers:** none + +#### Task 3: Add verify-before-CRITICAL rule +- **Change:** In `commands/review-pr-core.md`, after Step 5d (the existing review steps) — insert new Step 5e: + + > **5e — Verify before flagging CRITICAL.** Before emitting any finding with severity `critical`, you MUST call `read_local_file` or `get_file_content` to verify the issue exists in the full file. Do not flag CRITICAL findings based on diff context alone. If verification shows the issue doesn't exist, downgrade to `suggestion` or drop the finding. + +- **Files:** `commands/review-pr-core.md` +- **Tier:** standard +- **Done when:** Step 5e exists in `review-pr-core.md` with the verify-before-CRITICAL text. +- **Blockers:** none + +#### Task 4: Set temperature and seed on API calls +- **Change 1:** In `src/agents/openai_runner.py`, `_run_chat_completions` method, at the `client.chat.completions.create()` call — add `temperature=0.3` and `seed=42`. +- **Change 2:** In `src/agents/openai_runner.py`, `_run_responses_api` method, at the `client.responses.create()` call — add `temperature=0.3` to the kwargs dict. +- **Why 0.3:** Matches the old pipeline (pipeline 268) which produced more consistent results at this temperature. `temperature=0` risks degenerate repetition patterns with long prompts. +- **Note on `seed`:** OpenAI's seed parameter improves but does not guarantee determinism. The primary consistency gain comes from temperature. Expect reduced variance, not identical outputs across runs. +- **Files:** `src/agents/openai_runner.py` +- **Tier:** standard +- **Done when:** `temperature=0.3` appears in both API call sites; `seed=42` appears in `_run_chat_completions`. +- **Blockers:** none + +#### Task 5: Surface failed diff fetches +- **Change:** In `src/review_job.py`, `_pre_fetch_diffs` method — when the `except` block catches a diff fetch failure, instead of only logging a warning, also append the file path to a `failed_diffs` list. After the loop, if `failed_diffs` is non-empty, inject into the prompt context: + + > The following files could not be pre-fetched. You MUST fetch them via `get_file_diff` or `read_local_file` during review: [list]. These files still count toward 100% coverage. -#### Task 2: Add CLI flags and thread review_mode through BatchReviewJob -- **Change:** (1) In src/run_agent.py, add a mutually exclusive argparse group with --verify-fixes (store_true) and --check-new-findings (store_true). Resolve to ReviewMode enum and pass review_mode= to BatchReviewJob(...). (2) In BatchReviewJob.run(), conditionally skip _fetch_previous_findings() when review_mode == CHECK_NEW. Pass review_mode=self.review_mode to every ReviewJobConfig(...) constructor (both single-session path ~line 92 and batched path ~line 223). -- **Files:** src/run_agent.py, src/batch_review_job.py +- **Files:** `src/review_job.py` - **Tier:** standard -- **Done when:** python src/run_agent.py --help shows both flags; passing both raises argparse error; --verify-fixes sets review_mode=VERIFY_FIXES; --check-new-findings sets review_mode=CHECK_NEW; default is FULL; BatchReviewJob.run() skips _fetch_previous_findings() for CHECK_NEW; all 190 existing tests pass. -- **Blockers:** Task 1 +- **Done when:** Failed diff file paths are collected and injected into prompt context when non-empty. +- **Blockers:** none -#### VERIFY: Phase 1 — Foundation -- Run pytest tests/ — all 190 existing tests pass -- Verify ReviewMode enum imports correctly -- Verify CLI flags appear in --help and are mutually exclusive -- Verify review_mode threads through BatchReviewJob to ReviewJobConfig -- Report: tests passing, any regressions +#### VERIFY: Prompt Alignment + Determinism +- `python -m pytest tests/ -v` — MOCKED UNIT TESTS ONLY. Use unittest.mock / pytest-mock for ALL external dependencies (API calls, file I/O, etc). Do NOT run integration tests. +- Grep `SYSTEM_PROMPT` in `openai_runner.py` for "ZERO or minimal" — must not appear +- Grep `review-pr-core.md` for "max 10" — must not appear +- Confirm `temperature=0.3` appears in both `_run_chat_completions` and `_run_responses_api` call sites +- Grep `review-pr-core.md` for "5e" or "Verify before flagging CRITICAL" — must appear (Task 3 verification) +- Grep `review_job.py` `_pre_fetch_diffs` for `failed_diffs` — list must be collected and injected into prompt context when non-empty (Task 5 verification) --- -### Phase 2: Conditional Behavior — Prompt, Write Guards, Post-Findings +### Phase 2: 100% Coverage System + +#### Task 7: Risk classifier +- **Change:** Create `src/risk_classifier.py` — pure Python, zero LLM cost. Runs before the agent session using data already available from `FetchPRDetailsActivity` and `_pre_compute_analysis`. + + **Input:** List of changed files with their stats (additions, deletions, change_type) + optional graph analysis (caller count, test gaps). + + **Output:** Dict mapping each file path to `{ risk: "HIGH" | "MEDIUM" | "LOW", score: float, reasons: [str] }`. + + **Formula:** + ``` + file_risk = ( + 0.25 * normalize(lines_changed, max=500) + + 0.25 * path_sensitivity + + 0.15 * (1.0 if change_type == "add" else 0.0) + + 0.15 * (1.0 if no_test_coverage else 0.0) + + 0.10 * normalize(caller_count, max=10) + + 0.10 * file_type_risk + ) + ``` + + **Thresholds (configurable in `config.py`):** + - `risk_high_threshold: float = 0.6` → HIGH + - `risk_medium_threshold: float = 0.3` → MEDIUM + - Below 0.3 → LOW + + **Path sensitivity patterns (configurable, loaded from `languages.yml` or a `risk-patterns` section):** + - HIGH (1.0): `auth/`, `crypto/`, `permissions/`, `security/`, `middleware/`, `Controllers/`, `api/` + - MEDIUM (0.5): `services/`, `models/`, `database/`, `migrations/` + - LOW (0.0): `tests/`, `docs/`, `utils/`, `helpers/`, `constants/` + + **Graceful degradation:** When graph analysis is unavailable, `caller_count` and `no_test_coverage` default to 0.0. The classifier still works — it relies more on lines changed, path sensitivity, and file type. Every PR gets risk classification regardless of graph availability. -#### Task 3: ReviewJob mode-specific prompt injection and write guards -- **Change:** (1) In ReviewJob.create_findings() (~line 87), wrap the standalone prior-findings fetch in if self.config.review_mode != ReviewMode.CHECK_NEW. (2) In ReviewJob._build_prompt() (~line 422), after the existing previous_findings section, add mode-specific instruction blocks: for VERIFY_FIXES append verify-only instructions ("ONLY verify prior findings, findings[] MUST be empty, populate fix_verifications[] for every prior cr_id, skip Steps 3-5"); for CHECK_NEW append fresh-review instructions ("no prior findings, fix_verifications[] MUST be empty, skip Step 6"). Add two new private methods _build_verify_only_instructions() and _build_check_new_instructions(). (3) In ReviewJob._write_findings() (or wherever findings.json is written), add defense-in-depth guards: if VERIFY_FIXES, strip findings[] to empty and set review_modes to include "verify_fixes"; if CHECK_NEW, strip fix_verifications[] to empty and set review_modes to include "check_new". -- **Files:** src/review_job.py + **Calibration note:** These weights are heuristic — initial values based on the pattern that verified false positives all involved files with high `path_sensitivity` or high `caller_count` that the agent didn't verify. Expect to recalibrate after 20-30 production runs. All weights and thresholds should be configurable via `config.py` fields so tuning doesn't require code changes. + +- **Files:** `src/risk_classifier.py` (new), `src/config.py` (risk thresholds + weights) +- **Tier:** standard +- **Done when:** `risk_classifier.classify(files)` returns HIGH/MEDIUM/LOW for a mock file list; thresholds configurable in `config.py`. +- **Blockers:** none + +#### Task 8: Inject risk table into prompt +- **Change:** In `src/review_job.py`, `_build_review_context` method — after the existing analysis and diffs sections, call `risk_classifier.classify(...)` and inject the result as a markdown table: + + ```markdown + ### File Risk Classification (100% coverage required) + + | File | Risk | Depth | Reason | + |------|------|-------|--------| + | src/auth/LoginController.cs | HIGH | Full review + verify | auth path, 120 lines changed, 8 callers | + | src/models/User.cs | MEDIUM | Diff review + read if needed | 45 lines changed, model layer | + | src/utils/DateHelper.cs | LOW | Diff scan | 3 lines changed, utility | + + You MUST review every file above. HIGH files get deep review with full-file reads. LOW files get a diff-level scan. Every file must appear in `findings[]` or `files_clean[]`. + ``` + +- **Files:** `src/review_job.py` +- **Tier:** standard +- **Done when:** Risk classification table is injected into the review context prompt. +- **Blockers:** Task 7 + +#### Task 9: Add risk-based depth instructions to prompt +- **Change:** In `commands/review-pr-core.md`, Step 4 (after the existing T1-T5 tier table) — insert: + + > **100% coverage is required regardless of tier.** Every code file in your batch must be reviewed. You must either produce a finding for a file or list it in `files_clean[]` in findings.json. Files that appear in neither are considered skipped — skipped files fail the CI gate. + > + > **Within your tier strategy, depth per file is determined by risk tier** (shown in the pre-computed risk table): + > - **HIGH risk** — Full review. Read the full file via `read_local_file` before flagging. Check callers via `get_callers`. Verify CRITICAL findings against full context. Spend multiple turns if needed. + > - **MEDIUM risk** — Review from the pre-injected diff. Flag obvious issues. Use `read_local_file` only if something looks wrong but you need more context. + > - **LOW risk** — Scan the diff for security issues and critical bugs only. If nothing critical, add to `files_clean[]` and move on. + > + > Budget your 40 turns wisely: spend more on HIGH, less on LOW. But every file must appear in the output. + > + > T1-T5 sets the overall strategy (e.g., T5 = use repomix, prioritize by churn). Risk tiers set depth per file within that strategy. Both apply together. + +- **Files:** `commands/review-pr-core.md` - **Tier:** standard -- **Done when:** VERIFY_FIXES mode: prompt contains "VERIFY-ONLY MODE", _write_findings strips any agent-produced findings; CHECK_NEW mode: prompt contains "FRESH REVIEW MODE", prior-findings fetch skipped, _write_findings strips fix_verifications; FULL mode: no changes to current behavior; all 190 existing tests pass. -- **Blockers:** Tasks 1-2 +- **Done when:** Risk-based depth instructions and 100% coverage requirement appear in Step 4 of `review-pr-core.md`. +- **Blockers:** none + +#### Task 10: Add `files_clean[]` to findings schema +- **Change 1:** In `commands/review-pr-core.md`, Step 7 (findings.json schema definition) — add `files_clean` array to the schema: + + ```json + { + "findings": [ ... ], + "files_clean": ["src/utils/DateHelper.cs", "src/constants/AppColors.cs"] + } + ``` + + Instruction: "Include a `files_clean` array listing every file path you reviewed and found no issues in. Every code file in your batch MUST appear in either `findings[].file` or `files_clean[]`." + +- **Change 2:** In `src/models/review_models.py` — add `files_clean: List[str] = []` to the findings output model. -#### Task 4: post_findings.py mode-aware summary and gating -- **Change:** (1) In post_findings.run() (~line 803), detect verify-only mode: is_verify_only = "verify_fixes" in findings_file.review_modes and not findings_file.findings. (2) When is_verify_only: skip confidence filtering/capping (no findings), skip posting inline comments, still handle fix verifications and generate score comparison. Gate passes (no new findings to fail on). (3) In _build_summary_markdown() (~line 533), when "verify_fixes" in review_modes, use title "AI Code Review — Fix Verification Only" instead of standard title. Suppress the "Findings" section when there are no findings. -- **Files:** src/post_findings.py +- **Change 3:** Update the JSON parsing in `src/review_job.py` (the `_extract_findings` method) to read the `files_clean` field. + +- **Files:** `commands/review-pr-core.md`, `src/models/review_models.py`, `src/review_job.py` - **Tier:** standard -- **Done when:** verify-only mode with empty findings: no inline comments posted, fix verifications processed, summary title says "Fix Verification Only", gate passes; check-new mode: normal scoring/gating with no fix_verifications section; full mode unchanged; all 190 existing tests pass. -- **Blockers:** Task 3 +- **Done when:** `files_clean` parses from findings JSON; the field exists in the model; the prompt schema includes it. +- **Blockers:** none + +#### Task 11: Coverage gate and penalty +- **Change 1 — Coverage calculation.** In `src/post_findings.py`, summary construction section (near `_build_summary_markdown`) — replace the current `files_reviewed` derivation with: + + ```python + files_with_findings = set(f.file for f in findings_file.findings) + files_clean = set(findings_file.files_clean or []) + files_reviewed = files_with_findings | files_clean + coverage = len(files_reviewed) / total_code_files if total_code_files > 0 else 1.0 + ``` + +- **Change 2 — Gate rule.** In `src/post_findings.py`, `_evaluate_gate` — add a coverage check controlled by `coverage_gate_mode`: + - `"hard"` (default): Fail gate if coverage < 100% + - `"log"`: Log coverage % but don't fail gate (debugging/emergency use only) + + Gate failure message: "Gate failed: {n} file(s) not reviewed ({list}). 100% code review coverage is required." -#### VERIFY: Phase 2 — Conditional Behavior -- Run pytest tests/ — all 190 existing tests pass -- Verify prompt injection for each mode by inspecting _build_prompt() output -- Verify write guards strip correct fields per mode -- Verify post_findings summary title changes for verify-only -- Report: tests passing, any regressions +- **Change 3 — Summary display.** Change "Files Reviewed" in summary markdown to show coverage explicitly: `"Files Reviewed: 12 / 12 (100%)"` or `"Files Reviewed: 8 / 12 (67%) — 4 files not reviewed"`. + +- **Change 4 — Coverage penalty.** In `src/pr_scorer.py` — add coverage penalty method. For coverage below 100%, add penalty: `(1 - coverage_ratio) * 50`. This ensures incomplete reviews score poorly even if the gate mode is `"log"`. + +- **Change 5 — Config fields.** In `src/config.py` — add three new fields (`batch_max_turns` already exists at default 15 and is updated separately in Task 12): + + ```python + coverage_gate_mode: str = Field(default="hard", description="Coverage gate: hard (default) | log (debugging only)") + risk_high_threshold: float = Field(default=0.6) + risk_medium_threshold: float = Field(default=0.3) + ``` + +- **Files:** `src/post_findings.py`, `src/pr_scorer.py`, `src/config.py` +- **Tier:** standard +- **Done when:** Coverage calculation uses `files_clean`; gate mode `"hard"` fails on incomplete coverage; `"log"` does not fail; summary shows `X / Y (Z%)`; coverage penalty applied in scorer; three new config fields added with correct defaults. +- **Blockers:** Task 10 + +#### Task 12: Align batch turn budget to 40 +- **Change 1:** In `src/config.py`, `batch_max_turns` field — change the existing `default=15` to `default=40`. The field already exists at line 125-127 of `src/config.py` (`batch_max_turns: int = Field(default=15, ge=5, ...)`). This change removes the artificial 15-turn cap per batch so every batch gets the full 40-turn deployment budget. + +- **Change 2:** In `commands/review-pr-core.md` — ensure no prompt text contradicts the 40-turn budget. Every context where the agent runs — single review, batched review, Claude wrapper — must say 40. + +- **Files:** `src/config.py`, `commands/review-pr-core.md` +- **Tier:** standard +- **Done when:** `config.py` `batch_max_turns` default is 40 (not 15); grep entire codebase for turn/tool-call limits — every value must be 40. +- **Blockers:** Task 11 (other new config fields added there; apply in same session) + +#### VERIFY: 100% Coverage System +- `python -m pytest tests/ -v` — MOCKED UNIT TESTS ONLY. Use unittest.mock / pytest-mock for ALL external dependencies (API calls, file I/O, etc). Do NOT run integration tests. +- `risk_classifier.py` produces HIGH/MEDIUM/LOW for a mock file list +- `files_clean` parses from findings JSON +- Coverage calculation: 5 findings files + 3 clean files / 8 total = 100% +- Coverage calculation: 5 findings files + 0 clean files / 8 total = 62.5% +- Gate mode `"hard"` (default) DOES fail gate on incomplete coverage +- Gate mode `"log"` does NOT fail gate (for debugging only) +- `config.py` `batch_max_turns` default is 40, not 15 +- Grep entire codebase for turn/tool-call limits — every value must be 40 --- -### Phase 3: Tests +### Phase 3: Scoring + Categories -#### Task 5: Unit tests for all new review mode behavior -- **Change:** Create tests/unit/test_review_modes.py with these test groups: Enum tests (ReviewMode values, string comparison, all three modes exist); CLI tests (--verify-fixes parsed, --check-new-findings parsed, both-together rejected, default is FULL); BatchReviewJob tests (CHECK_NEW skips _fetch_previous_findings(), VERIFY_FIXES and FULL call it, review_mode passed to ReviewJobConfig); Prompt tests (VERIFY_FIXES prompt contains "VERIFY-ONLY MODE", CHECK_NEW contains "FRESH REVIEW MODE", FULL contains neither); Write guard tests (VERIFY_FIXES strips findings[], CHECK_NEW strips fix_verifications[], FULL preserves both); post_findings tests (verify-only mode: no inline comments, fix verifications processed, summary title correct; gate passes with zero findings in verify-only). -- **Files:** tests/unit/test_review_modes.py (new) +#### Task 14: New `testing` category with zero-weight scoring +- **Change 1:** In `src/post_findings.py`, `VALID_CATEGORIES` — add `"testing"`. In `CATEGORY_REMAP` — remove `"testing": "best_practices"`. + +- **Change 2:** In `commands/scoring.md`, penalty matrix — add a `testing` row with zero-weight penalties: + + | Category | critical | warning | suggestion | + |----------|----------|---------|------------| + | testing | 0.0 | 0.0 | 0.0 | + + Test findings are always posted as inline comments but never affect the star rating or CI gate. + +- **Change 3:** In `commands/review-pr-core.md`, the test-gap instruction (currently: "Flag missing test coverage from the test gaps list as findings.") — append: "Use category `testing` (not `best_practices`) for all test-gap findings. These are informational — they appear as inline comments but do not affect the CI gate or star rating." + +- **Files:** `src/post_findings.py`, `commands/scoring.md`, `commands/review-pr-core.md` - **Tier:** standard -- **Done when:** pytest tests/unit/test_review_modes.py -v passes; at least 15 test cases covering all groups above; all 190+ existing tests still pass. -- **Blockers:** Tasks 1-4 +- **Done when:** `"testing"` is in `VALID_CATEGORIES`, not in `CATEGORY_REMAP`; scoring.md has zero-weight testing row; review-pr-core.md instructs to use `testing` category for test-gap findings. +- **Blockers:** none + +#### Task 15: Expand valid categories +- **Change 1:** In `src/post_findings.py`, `VALID_CATEGORIES` — add `"architecture"`, `"correctness"`, and `"error_handling"`. Remove those keys from `CATEGORY_REMAP`. + + Updated sets: + ```python + VALID_CATEGORIES = { + "security", "performance", "best_practices", "code_style", + "documentation", "testing", "architecture", "correctness", "error_handling" + } + CATEGORY_REMAP = { + "reliability": "best_practices", + "maintainability": "best_practices", + "naming": "code_style", + "formatting": "code_style", + } + ``` + +- **Change 2:** In `src/models/review_models.py` — update any category validation/enum to include the new categories. + +- **Change 3:** In `commands/scoring.md`, penalty matrix — add rows for new categories: + + | Category | critical | warning | suggestion | + |----------|----------|---------|------------| + | architecture | 2.0 | 1.0 | 0.5 | + | correctness | 2.0 | 1.0 | 0.5 | + | error_handling | 1.5 | 0.75 | 0.25 | + +- **Files:** `src/post_findings.py`, `src/models/review_models.py`, `commands/scoring.md` +- **Tier:** standard +- **Done when:** All 9 categories in `VALID_CATEGORIES`; `CATEGORY_REMAP` only has 4 entries; new penalty rows in scoring.md; model updated. +- **Blockers:** Task 14 + +#### Task 16: Architecture review checklist +- **Change:** Create `commands/review-mode-architecture.md` with this content: + + ```markdown + # Architecture Review Checklist + + ## API Design + - [ ] Public API changes are backward-compatible or versioned + - [ ] Breaking changes documented in PR description + - [ ] Return types use interfaces, not concrete classes (e.g., IReadOnlyList over List) + + ## Coupling + Cohesion + - [ ] No circular dependencies between modules + - [ ] New dependencies follow the existing dependency direction + - [ ] Cross-layer calls go through defined interfaces + + ## Separation of Concerns + - [ ] Business logic not in controllers/handlers + - [ ] Data access not in UI layer + - [ ] Configuration not hardcoded + + ## Contracts + - [ ] DTOs/models don't expose internal implementation + - [ ] Serialization attributes present on public models + - [ ] Nullable annotations consistent + ``` + + In `commands/review-pr-core.md`, the mode references (currently "architecture — (inline in scoring.md)") — replace with reference to `commands/review-mode-architecture.md`. + +- **Files:** `commands/review-mode-architecture.md` (new), `commands/review-pr-core.md` +- **Tier:** standard +- **Done when:** Architecture checklist file exists; `review-pr-core.md` references it. +- **Blockers:** none + +#### Task 17: Performance review checklist +- **Change:** Create `commands/review-mode-performance.md` with this content: + + ```markdown + # Performance Review Checklist + + ## Database + - [ ] No N+1 queries — use .Include() or projection + - [ ] Bulk operations use ExecuteUpdate/ExecuteDelete (not loop + SaveChanges) + - [ ] Read-only queries use .AsNoTracking() + - [ ] Pagination present on list endpoints + + ## I/O + - [ ] No synchronous I/O on hot paths (use async/await) + - [ ] HTTP calls use HttpClientFactory, not new HttpClient() + - [ ] File operations buffered or streamed for large payloads + + ## Collections + Algorithms + - [ ] No O(n^2) loops on unbounded collections + - [ ] LINQ queries materialized once (.ToList()) not re-enumerated + - [ ] Large read-only collections use FrozenSet/FrozenDictionary (.NET 8+) + + ## Caching + - [ ] Frequently-read, rarely-changed data cached appropriately + - [ ] Cache keys include all discriminating parameters + - [ ] Cache expiry set (no indefinite caching) + ``` + + In `commands/review-pr-core.md`, the mode references (currently "performance — (inline in scoring.md)") — replace with reference to `commands/review-mode-performance.md`. + +- **Files:** `commands/review-mode-performance.md` (new), `commands/review-pr-core.md` +- **Tier:** standard +- **Done when:** Performance checklist file exists; `review-pr-core.md` references it. +- **Blockers:** none + +#### VERIFY: Scoring + Categories +- `python -m pytest tests/ -v` — MOCKED UNIT TESTS ONLY. Use unittest.mock / pytest-mock for ALL external dependencies (API calls, file I/O, etc). Do NOT run integration tests. +- Create a finding with `category: "testing"` — verify 0 penalty points +- Create a finding with `category: "architecture"` — verify it is NOT remapped to `best_practices` +- Both new checklist files exist and are referenced from `review-pr-core.md` + +--- + +### Phase 4: Noise Reduction + Audit + +#### Task 19: Comment merging +- **Change:** In `src/post_findings.py`, between the existing `filter_by_confidence` step and `cap_findings` step — add a `merge_similar_findings(findings)` function. + + **Merge criteria (ALL must match):** + - Same `file` path + - Line numbers within +/- 5 of each other + - Title + message similarity > 0.7 using `difflib.SequenceMatcher.ratio()` + + **Why SequenceMatcher:** It's stdlib (no new dependency), operates on character sequences (good for code-related text), and 0.7 threshold is conservative enough to avoid false merges. If semantic similarity becomes needed later, switch to embedding-based comparison — but start simple. + + **Merge behavior:** Keep the finding with higher severity. Append the merged finding's message as a footnote: "(Also flagged at line {n}: {merged_title})". Reduce total finding count. + +- **Files:** `src/post_findings.py` +- **Tier:** standard +- **Done when:** Given 3 findings on same file with lines 10, 12, 50 — the first two merge, the third stays separate. Given 2 findings on different files with same title — they do NOT merge. +- **Blockers:** none + +#### Task 20: Audit trail export +- **Change:** In `src/post_findings.py`, after the summary posting step, before the output construction — write a timestamped markdown file: + + ```python + export_path = Path(workspace) / ".cr" / f"review_{pr_id}_{datetime.now():%Y%m%d_%H%M%S}.md" + ``` + + **Contents:** + - PR metadata (ID, repo, branch, author) + - Score breakdown (total penalty, per-category penalties) + - Files reviewed vs skipped (with coverage %) + - All findings (posted and filtered/capped) + - Token usage (from agent runner stats) + - Risk classification table (from Phase 2) + - Gate decision and reason + +- **Files:** `src/post_findings.py` +- **Tier:** standard +- **Done when:** Audit file written to `.cr/` with correct timestamp format; contains all expected sections (PR metadata, score breakdown, files reviewed, findings, token usage, risk table, gate decision). +- **Blockers:** none (Phase 4 runs after Phase 2; risk classifier from Task 7 must be complete for the risk table section of the audit — satisfied by phase ordering) + +#### VERIFY: Noise Reduction + Audit +- `python -m pytest tests/ -v` — MOCKED UNIT TESTS ONLY. Use unittest.mock / pytest-mock for ALL external dependencies (API calls, file I/O, etc). Do NOT run integration tests. +- Given 3 findings on same file with lines 10, 12, 50 — the first two merge, the third stays separate +- Given 2 findings on different files with same title — they do NOT merge +- Audit file written to `.cr/` with correct timestamp format +- Audit file contains all expected sections + +--- + +### Phase 5: Language Intelligence + +#### Task 22: Language registry +- **Change:** Create `commands/languages.yml` — full registry with 15 language entries: + + ```yaml + languages: + csharp: + extensions: [.cs] + config_files: ["*.csproj", "NuGet.config", "packages.config"] + rules_file: commands/lang-rules/csharp.md + javascript: + extensions: [.js, .jsx, .mjs] + config_files: ["package.json"] + rules_file: commands/lang-rules/javascript.md + typescript: + extensions: [.ts, .tsx] + config_files: ["package.json", "tsconfig.json"] + rules_file: commands/lang-rules/typescript.md + react: + extensions: [] + config_files: ["package.json"] + rules_file: commands/lang-rules/react.md + python: + extensions: [.py] + config_files: ["requirements.txt", "pyproject.toml", "Pipfile", "setup.py"] + rules_file: commands/lang-rules/python.md + java: + extensions: [.java] + config_files: ["pom.xml", "build.gradle", "build.gradle.kts"] + rules_file: commands/lang-rules/java.md + kotlin: + extensions: [.kt, .kts] + config_files: ["build.gradle.kts", "build.gradle"] + rules_file: commands/lang-rules/kotlin.md + go: + extensions: [.go] + config_files: ["go.mod"] + rules_file: commands/lang-rules/go.md + rust: + extensions: [.rs] + config_files: ["Cargo.toml"] + rules_file: commands/lang-rules/rust.md + cpp: + extensions: [.cpp, .cc, .cxx, .c, .h, .hpp, .hxx] + config_files: ["CMakeLists.txt", "*.vcxproj", "Makefile"] + rules_file: commands/lang-rules/cpp.md + swift: + extensions: [.swift] + config_files: ["Package.swift", "Podfile"] + rules_file: commands/lang-rules/swift.md + android: + extensions: [] + config_files: ["AndroidManifest.xml"] + rules_file: commands/lang-rules/android.md + ruby: + extensions: [.rb] + config_files: ["Gemfile"] + rules_file: commands/lang-rules/ruby.md + php: + extensions: [.php] + config_files: ["composer.json"] + rules_file: commands/lang-rules/php.md + sql: + extensions: [.sql] + config_files: [] + rules_file: commands/lang-rules/sql.md + ``` + + Overlay languages (React, Android) have no file extensions — detected from config files (`package.json` with `react` in dependencies, `AndroidManifest.xml` presence) and their rules are injected alongside the base language. + +- **Files:** `commands/languages.yml` (new) +- **Tier:** standard +- **Done when:** `languages.yml` exists with all 15 entries; each entry has extensions, config_files, and rules_file. +- **Blockers:** none + +#### Task 23: Stack detector +- **Change:** Create `src/stack_detector.py` — reads project config files from the workspace to extract exact framework versions. + + **Config file parsing:** + + | Config File | Extract | + |------------|---------| + | `*.csproj` | `` → .NET version, `` → EF Core / ASP.NET version | + | `package.json` | `dependencies` → React/Angular/Vue version, `devDependencies` → TypeScript version, `engines.node` | + | `tsconfig.json` | TypeScript `target`, `strict` mode | + | `requirements.txt` / `pyproject.toml` / `Pipfile` | Python version, Django/Flask/FastAPI version | + | `pom.xml` | Java version (``), Spring Boot version | + | `build.gradle` / `build.gradle.kts` | Java/Kotlin version, Android SDK level, Spring Boot version, Compose usage | + | `go.mod` | Go version, module dependencies | + | `Cargo.toml` | Rust edition (2018/2021/2024), dependency crates | + | `CMakeLists.txt` / `*.vcxproj` | C/C++ standard (C++11/14/17/20/23), compiler flags | + | `Package.swift` / `Podfile` / `*.xcodeproj` | Swift version, iOS deployment target, SwiftUI vs UIKit | + | `AndroidManifest.xml` | minSdkVersion, targetSdkVersion (combined with build.gradle) | + | `Gemfile` | Ruby version, Rails version | + | `composer.json` | PHP version, Laravel/Symfony version | + + **Output:** `StackProfile` dataclass: + ```python + @dataclass + class StackProfile: + languages: list[str] + frameworks: dict[str, dict[str, str]] + detected_from: list[str] + ``` + + **Monorepo handling:** For initial implementation, detect from the repo root config files only, ignore nested configs. Note this limitation in a code comment and address in a future sprint. + +- **Files:** `src/stack_detector.py` (new) +- **Tier:** standard +- **Done when:** `stack_detector.detect(workspace)` on a workspace with a `.csproj` targeting `net8.0` returns `{"languages": ["csharp"], "frameworks": {"csharp": {"dotnet": "8.0"}}}`. +- **Blockers:** Task 22 + +#### Task 24: Version-aware rules files (batch 1 — csharp, javascript, typescript, react, python, java, kotlin, go) +- **Change:** Create `commands/lang-rules/` directory and the first 8 rules files. Each file uses version-sectioned headings. 30-50 checklist items max per file. Focus on patterns that produce real bugs or security issues, not formatting preferences. + + | Rules File | Version Sections | Source | + |-----------|-----------------|--------| + | `csharp.md` | All Versions, .NET 6+, .NET 8+, EF Core 6+, ASP.NET Core | Microsoft .NET coding conventions, Framework Design Guidelines | + | `javascript.md` | All Versions, ES2020+, ES2022+ | ESLint recommended, Airbnb style guide (patterns only) | + | `typescript.md` | All Versions, TS 4.x, TS 5.x | typescript-eslint recommended rules | + | `react.md` | React 16, React 17+, React 18+ (hooks, Suspense, Server Components) | React official docs, eslint-plugin-react-hooks | + | `python.md` | All Versions, Python 3.8+, Python 3.10+, Python 3.12+ | PEP 8, PEP 484, ruff rules | + | `java.md` | All Versions, Java 11+, Java 17+, Java 21+ | Google Java Style Guide, Effective Java | + | `kotlin.md` | All Versions, Kotlin 1.5+, Kotlin 2.0+ | Kotlin official coding conventions | + | `go.md` | All Versions, Go 1.18+, Go 1.21+ | Effective Go, Go Code Review Comments | + +- **Files:** `commands/lang-rules/csharp.md`, `commands/lang-rules/javascript.md`, `commands/lang-rules/typescript.md`, `commands/lang-rules/react.md`, `commands/lang-rules/python.md`, `commands/lang-rules/java.md`, `commands/lang-rules/kotlin.md`, `commands/lang-rules/go.md` (all new) +- **Tier:** standard +- **Done when:** All 8 files exist with version-sectioned headings and 30-50 checklist items each. +- **Blockers:** none + +#### Task 25: Version-aware rules files (batch 2 — rust, cpp, swift, android, ruby, php, sql) +- **Change:** Create the remaining 7 rules files: + + | Rules File | Version Sections | Source | + |-----------|-----------------|--------| + | `rust.md` | All Versions, Edition 2021, Edition 2024 | Rust API Guidelines, Clippy lints | + | `cpp.md` | All Versions, C++14, C++17, C++20, C++23 | C++ Core Guidelines, CERT C++ | + | `swift.md` | All Versions, Swift 5.5+ (async/await, actors), SwiftUI vs UIKit | Swift API Design Guidelines | + | `android.md` | All, API 26+, Jetpack Compose, Kotlin-first | Android developer guides | + | `ruby.md` | All Versions, Ruby 3.0+, Rails 7+ | Ruby Style Guide, Rails Best Practices | + | `php.md` | All Versions, PHP 8.0+, Laravel 10+, Symfony 6+ | PSR-12, PHP-FIG standards | + | `sql.md` | All (version-agnostic) | OWASP SQL Injection Prevention, migration safety | + +- **Files:** `commands/lang-rules/rust.md`, `commands/lang-rules/cpp.md`, `commands/lang-rules/swift.md`, `commands/lang-rules/android.md`, `commands/lang-rules/ruby.md`, `commands/lang-rules/php.md`, `commands/lang-rules/sql.md` (all new) +- **Tier:** standard +- **Done when:** All 7 files exist with version-sectioned headings and 30-50 checklist items each. +- **Blockers:** none + +#### Task 26: Integration into review_job.py +- **Change 1:** In `src/review_job.py`, `_build_config_section` method — update the flow: + 1. If `.codereview.md` exists → load it (project-specific overrides always win) + 2. If no `.codereview.md` → call `stack_detector.detect(workspace)` to get the `StackProfile` + 3. For each detected language + version → load the matching `commands/lang-rules/.md`, filter to only the applicable version sections + 4. Inject into the prompt: "Auto-detected stack: .NET 8.0 + React 18.2 + TypeScript 5.3. The following language-specific review rules apply:" followed by the filtered rule sections + 5. If `.codereview.md` exists AND auto-detection runs, the `.codereview.md` rules take precedence where they conflict + +- **Change 2:** In `src/file_filter.py` — replace the extension blacklist with a registry lookup. Any file whose extension matches a registered language in `languages.yml` gets reviewed; everything else is skipped. Log which files were skipped and why (unregistered extension). + +- **Change 3:** In `commands/review-pr-core.md`, Step 1 (project config loading) — update to: "If `.codereview.md` exists, load it. Otherwise, the system reads your project config files (package.json, *.csproj, etc.) to detect exact framework versions and injects version-appropriate review rules. The detected stack and active rules are shown in the 'Pre-loaded Project Config' section below." + +- **Files:** `src/review_job.py`, `src/file_filter.py`, `commands/review-pr-core.md` +- **Tier:** standard +- **Done when:** Stack detection auto-injects version-filtered rules into prompt; `.codereview.md` overrides auto-detection; `file_filter.py` uses registry lookup; `review-pr-core.md` Step 1 updated. +- **Blockers:** Tasks 22, 23, 24, 25 + +#### VERIFY: Language Intelligence +- `python -m pytest tests/ -v` — MOCKED UNIT TESTS ONLY. Use unittest.mock / pytest-mock for ALL external dependencies (API calls, file I/O, etc). Do NOT run integration tests. +- `stack_detector.detect()` on a workspace with a `.csproj` targeting `net8.0` returns `{"languages": ["csharp"], "frameworks": {"csharp": {"dotnet": "8.0"}}}` +- The prompt for a .NET 8 project includes `.NET 8+` rules but NOT `.NET 6+ only` rules that conflict +- The prompt for a .NET 6 project does NOT include `.NET 8+` rules +- A workspace with `.codereview.md` ignores auto-detection (existing behavior preserved) +- `file_filter.py` skips `.json` files and reviews `.cs` files when `csharp` is registered +- All 15 lang-rules files exist in `commands/lang-rules/` + +--- + +### Phase 6: Fix Verification + Parallelism + +#### Task 28: Fix verifier +- **Change:** Create `src/fix_verifier.py` — move fix verification OUT of the agent prompt (`review-pr-core.md` Step 6) and into deterministic Python code. + + **Algorithm:** + + 1. **Load and group.** + - Load old findings (from previous review's findings.json or ADO thread data) + - Group old findings by file path → `Dict[file_path, List[Finding]]` + - Run `git diff --name-status` between old review commit and current HEAD to get file renames/deletes + + 2. **Deterministic resolution (no LLM needed).** + - If a file was **deleted** → all findings in that file are `not_relevant` + - If a file was **renamed** → remap findings to the new path, continue to Step 3 + - If a file has **zero diff** since the last review (unchanged) → all findings are `still_present` (nothing changed, nothing fixed) + + 3. **File-level LLM verification (one call per file with unresolved findings).** + For each file that was modified and still has unresolved findings: + - Read the **full current file** via workspace file read + - Build a verification prompt: + + ``` + You are verifying whether previously flagged code review findings have been fixed. + + ## File: {file_path} + + ## Current file content: + {full_file_content} + + ## Previous findings to verify: + | # | Title | Original description | Original line | + |---|-------|---------------------|---------------| + | 1 | Missing null check on request param | The endpoint accepts ... | 42 | + | 2 | SQL injection in search query | Raw string interpolation ... | 118 | + + For each finding, determine: + - `fixed` — the specific issue described is no longer present in the current code + - `still_present` — the issue still exists (may be at a different line now) + + Respond as JSON array: [{"finding": 1, "status": "fixed", "reason": "..."}, ...] + ``` + + 4. **Blast radius check (when graph is available).** + After file-level verification, for any finding originally flagged as `critical` or `architecture` category: + - Use `get_blast_radius` (from Sprint 2 graph tools) to check if the fix introduced new issues in dependent files + - If the finding was in a function whose callers changed since the last review, flag for attention: "Finding #{n} was fixed, but callers of `{function_name}` also changed — verify the fix didn't break dependents." + - This is informational only (logged in the audit trail, posted as a note on the thread) — doesn't change the fixed/still_present verdict + + 5. **Post results to ADO threads.** + - `fixed` → resolve the thread with "Verified fixed: {reason}" + - `still_present` → reply on the thread with "Still present: {reason}" + - `not_relevant` → resolve the thread with "File deleted/removed" + - Blast radius notes → post as a new reply on the thread + + **LLM call budget:** One call per modified file with old findings (not per finding). Cap: If more than 15 files need verification, batch the remaining files into groups of 5 per LLM call. Max ~18 calls (15 individual + ~1-2 batched). + + **Default for any LLM call that fails or returns ambiguous results: `still_present`** (never "assume fixed"). + + **Prompt change:** In `commands/review-pr-core.md`, Step 6 — remove the fix verification instructions. Replace with: "Fix verification is handled automatically by the system after your review completes. Do not attempt to verify old findings — focus on reviewing the current code." + +- **Files:** `src/fix_verifier.py` (new), `commands/review-pr-core.md` +- **Tier:** standard +- **Done when:** Fix verifier handles deleted (not_relevant), unchanged (still_present), and modified (LLM verification) files correctly; defaults to `still_present` on failure; respects 15-file cap with batching; `review-pr-core.md` Step 6 no longer contains fix verification instructions. +- **Blockers:** none + +#### Task 29: Parallel batch execution +- **Change:** In `src/batch_review_job.py`, the batch loop (currently sequential `for i, batch_files in enumerate(batches)`) — replace with `concurrent.futures.ThreadPoolExecutor`: + + ```python + max_workers = min(3, len(batches)) + with ThreadPoolExecutor(max_workers=max_workers) as pool: + futures = {pool.submit(self._run_batch, i, batch): i for i, batch in enumerate(batches, 1)} + for future in as_completed(futures): + batch_num = futures[future] + result = future.result() + # ... collect results + ``` + + **Max parallelism: 3 concurrent batches.** Balances speed, cost, and API rate limits. If rate limit errors occur, the `_run_batch` method should retry with exponential backoff (1s, 2s, 4s, max 3 retries) before failing. Each batch is independent — a failure in one batch doesn't affect others. + + **Why 3:** OpenAI's tier 3+ rate limits allow ~5000 RPM. Each batch makes up to 40 API calls over 2-5 minutes. 3 concurrent batches = ~120 calls/minute, well within limits. + +- **Files:** `src/batch_review_job.py` +- **Tier:** standard +- **Done when:** Batches execute in parallel with max 3 workers; retry with exponential backoff on rate limit errors; batch failure isolated from other batches; parallel batches complete faster than sequential (testable with mock delays). +- **Blockers:** none + +#### VERIFY: Fix Verification + Parallelism +- `python -m pytest tests/ -v` — MOCKED UNIT TESTS ONLY. Use unittest.mock / pytest-mock for ALL external dependencies (API calls, file I/O, etc). Do NOT run integration tests. +- Fix verifier: file deleted since last review → all findings in that file return `not_relevant` +- Fix verifier: file unchanged since last review → all findings return `still_present` (no LLM call) +- Fix verifier: file modified, 3 old findings → one LLM call with full file + all 3 findings, returns per-finding verdicts +- Fix verifier: 5 files with findings, each modified → 5 LLM calls (one per file, not per finding) +- Fix verifier: 20 files with findings → 15 individual calls + 1 batched call (files 16-20 grouped) +- Fix verifier: LLM call fails or returns garbage → default is `still_present`, NOT `fixed` +- Fix verifier: critical finding marked fixed + graph available → blast radius check logged in audit +- Parallel batches: 3 batches complete in less wall-clock time than sequential (use mock delays in test) +- `review-pr-core.md` Step 6 no longer contains fix verification instructions + +--- + +## Constraints + +| Constraint | Value | +|-----------|-------| +| Branch | `feat/review-quality` | +| Base | `origin/main` | +| Commits | 1 per phase (not per task) | +| Tests | Mocked unit tests ONLY — NO integration tests | +| Protected branches | NEVER push to main, master, or development | -#### VERIFY: Phase 3 — Tests -- Run full pytest tests/ -v — all tests pass (old + new) -- Confirm test_review_modes.py covers enum, CLI, batch, prompt, write guards, post_findings -- Report: total test count, pass rate, any failures +### Commit messages (1 per phase): +1. `fix(prompts): align tool-call limits and add temperature/seed` +2. `feat(coverage): add risk classifier, files_clean[], and coverage gate` +3. `fix(scoring): add testing category, expand valid categories, add review checklists` +4. `feat(quality): add comment merging and review audit export` +5. `feat(lang): add stack detector and version-aware review rules` +6. `feat(verify): deterministic fix verifier and parallel batches` --- @@ -76,14 +703,11 @@ | Risk | Impact | Mitigation | |------|--------|------------| -| Agent ignores verify-only prompt instructions, still produces findings | Med | Defense-in-depth: _write_findings() strips findings regardless of agent output | -| --verify-fixes with no prior findings (first push) produces empty output | Low | Log warning "no prior findings to verify", produce valid empty findings.json with note in summary | -| 5-star score in verify-only mode is misleading | Low | Summary title makes mode explicit; comparison markdown shows fix delta | -| review_modes list gets verify_fixes/check_new values that PRScorer.apply_mode_multipliers() doesn't recognize | Low | Scorer ignores unrecognized modes — no side effects | -| Default behavior regression — existing pipeline breaks | High | All 190 existing tests must pass; FULL mode code paths are untouched | - -## Notes -- Each task should result in a git commit -- Verify tasks are checkpoints — stop and report after each one -- Base branch: main -- Branch: feat/review-modes +| OpenAI seed parameter doesn't guarantee determinism | Findings may still vary across runs | Primary consistency gain comes from temperature=0.3; seed is supplementary. Measure with Jaccard similarity on 10 PR sample. | +| Risk classifier weights are heuristic | May over/under-classify file risk initially | All weights configurable in config.py. Recalibrate after 20-30 production runs by correlating risk scores with actual false positive rates. | +| SequenceMatcher may not catch semantic duplicates | Some duplicate comments may slip through | Start with 0.7 threshold (conservative). Upgrade to embedding-based similarity if false merge/miss rate is too high. | +| Rate limits with parallel batches (3 concurrent) | API rate limit errors under heavy load | Exponential backoff retry (1s, 2s, 4s, max 3 retries). 3 workers stays well within tier 3+ limits (~120 calls/min vs 5000 RPM). | +| Monorepo per-directory profiles deferred | Multi-framework monorepos get root-config rules only | Root-config detection covers most repos. Per-directory profiles noted for future sprint. | +| Fix verifier LLM calls add cost | Each re-review costs 1 LLM call per modified file | Capped at 18 calls max (15 individual + batched). Deterministic resolution handles deleted/unchanged files with zero LLM cost. | +| Prompt token budget blow-up for multi-language repos | 15 lang-rules files × 30-50 items each could inject thousands of tokens in a full-stack repo (C#, TypeScript, React, SQL), crowding out diff context | Cap total injected rules tokens. Only inject rules for languages that have changed files in the PR — not for all detected languages in the repo. | +| Coverage hard-gate blocking PRs on day one due to agent bugs | If the agent has an edge case (unrecognized file type, unrecovered diff fetch failure), the gate blocks the PR and ops must manually toggle to `"log"` mode | Run in `"log"` mode for the first 5-10 production PRs to validate agent file tracking, then switch to `"hard"` mode. | diff --git a/commands/findings-schema.json b/commands/findings-schema.json index ab35fcf..799b88c 100644 --- a/commands/findings-schema.json +++ b/commands/findings-schema.json @@ -36,6 +36,11 @@ "$ref": "#/$defs/Finding" } }, + "files_clean": { + "type": "array", + "items": { "type": "string" }, + "description": "Files reviewed with no findings — every code file must appear in either findings[].file or files_clean[]" + }, "fix_verifications": { "type": "array", "description": "Fix verification results from a re-push (optional)", @@ -85,7 +90,7 @@ }, "category": { "type": "string", - "enum": ["security", "performance", "best_practices", "code_style", "documentation"], + "enum": ["security", "performance", "best_practices", "architecture", "correctness", "error_handling", "code_style", "documentation", "testing"], "description": "Finding category" }, "title": { diff --git a/commands/lang-rules/android.md b/commands/lang-rules/android.md new file mode 100644 index 0000000..3c1293c --- /dev/null +++ b/commands/lang-rules/android.md @@ -0,0 +1,43 @@ +# Android Review Rules + +## All Versions + +- [ ] No `Context` stored in static fields or singletons (memory leak) +- [ ] `AsyncTask` not used — deprecated; use Kotlin coroutines or `WorkManager` +- [ ] `SharedPreferences` not used for sensitive data — use `EncryptedSharedPreferences` or Keystore +- [ ] `WebView` does not enable `setJavaScriptEnabled(true)` unless explicitly required and documented +- [ ] `android:debuggable="true"` not set in production manifests +- [ ] Permissions declared in the manifest are the minimum required — no over-broad permissions +- [ ] `BroadcastReceiver` unregistered in `onStop` / `onDestroy` if registered in `onStart` / `onCreate` +- [ ] `Cursor` objects closed after use to avoid resource leaks +- [ ] No network calls on the main thread (causes `NetworkOnMainThreadException`) +- [ ] `Activity` / `Fragment` references not held in long-lived objects (memory leak) +- [ ] `Intent` extras validated before use — `getStringExtra()` returns null if the key is absent +- [ ] `PendingIntent` flags include `FLAG_IMMUTABLE` (required on API 31+) + +## API 26+ + +- [ ] `NotificationChannel` created before posting notifications (required API 26+) +- [ ] Background service uses `JobIntentService` or `WorkManager`, not a plain `Service` +- [ ] `JobScheduler` / `WorkManager` used for deferred background work instead of AlarmManager +- [ ] Auto-fill hints (`android:autofillHints`) set on form fields for password managers + +## Jetpack Compose + +- [ ] Composable functions are stateless where possible — state hoisted to parent +- [ ] `remember` / `rememberSaveable` used to survive recompositions and process death respectively +- [ ] `LaunchedEffect` dependencies are correct — all values read inside must be in the key list +- [ ] `SideEffect` / `DisposableEffect` used for non-Compose side effects with cleanup +- [ ] `derivedStateOf` used when derived state computation is expensive and inputs change frequently +- [ ] `Modifier` parameter present and used as the first modifier in composable public APIs +- [ ] `CompositionLocal` not used for data that could be passed explicitly through parameters + +## Kotlin-first + +- [ ] Kotlin coroutines use structured concurrency — no `GlobalScope.launch` +- [ ] `viewModelScope` used inside `ViewModel`, `lifecycleScope` inside `Activity`/`Fragment` +- [ ] Flow collected with `repeatOnLifecycle(Lifecycle.State.STARTED)` to avoid collecting in background +- [ ] `StateFlow` / `SharedFlow` used instead of `LiveData` in new Kotlin-first code +- [ ] `hiltViewModel()` used for Hilt-injected ViewModels in Compose — not manual `ViewModelProvider` +- [ ] `NavController` navigation not called from inside `LaunchedEffect` with a key that re-fires on recomposition +- [ ] Room database accessed only on background threads — not on `Dispatchers.Main` diff --git a/commands/lang-rules/cpp.md b/commands/lang-rules/cpp.md new file mode 100644 index 0000000..a33723d --- /dev/null +++ b/commands/lang-rules/cpp.md @@ -0,0 +1,46 @@ +# C++ Review Rules + +## All Versions + +- [ ] No raw `new` / `delete` — use `std::unique_ptr`, `std::shared_ptr`, or stack allocation +- [ ] No manual memory management in code that can throw — RAII wrappers used throughout +- [ ] `std::string` not constructed from `nullptr` (undefined behavior) +- [ ] Integer overflow on signed integers considered — use `unsigned` or checked arithmetic for external input +- [ ] Array bounds not accessed out-of-range — prefer iterators or range-based for loops +- [ ] `reinterpret_cast` usage documented with proof that the aliasing is defined behavior +- [ ] No `const_cast` removing constness on values actually declared `const` +- [ ] Virtual destructors on all base classes with virtual methods +- [ ] Rule of Five: if one of destructor, copy ctor, copy assignment, move ctor, move assignment is defined, all five are defined or `= delete`d +- [ ] No `using namespace std;` in header files (pollutes consumer namespaces) +- [ ] Thread-shared mutable data protected with `std::mutex` or `std::atomic` +- [ ] `[[nodiscard]]` on functions whose return value must be checked + +## C++14 + +- [ ] `auto` used for iterator types and complex template types, not for obscuring simple types +- [ ] Generic lambdas used instead of boilerplate `struct` with templated `operator()` +- [ ] `std::make_unique` used instead of `new` with `unique_ptr` constructor + +## C++17 + +- [ ] Structured bindings (`auto [a, b] = pair`) used for multi-value returns +- [ ] `if constexpr` used for compile-time branching in templates instead of specialization +- [ ] `std::optional` used for nullable return values instead of sentinel values or output parameters +- [ ] `std::variant` / `std::visit` used for type-safe unions instead of tagged unions with `void*` +- [ ] Parallel algorithms (`std::sort(std::execution::par, ...)`) used only where thread safety of data is guaranteed +- [ ] `std::string_view` used for read-only string parameters to avoid copies + +## C++20 + +- [ ] Concepts used to constrain template type parameters instead of SFINAE +- [ ] Ranges (`std::ranges::sort`, `std::views::filter`) used for composable sequence operations +- [ ] Coroutines (`co_await`, `co_yield`) require an appropriate promise type — not used ad-hoc +- [ ] `std::span` used instead of pointer + length pairs for buffer parameters + +## C++23 + +- [ ] `std::expected` used for error-returning functions instead of exceptions or output parameters +- [ ] `std::mdspan` used for multidimensional array views instead of manual pointer arithmetic +- [ ] `std::print` / `std::println` used in new code instead of `printf` +- [ ] `std::stacktrace` used for capturing call stack in error reporting where diagnostics are needed +- [ ] `if consteval` used to branch compile-time vs runtime evaluation in constexpr functions diff --git a/commands/lang-rules/csharp.md b/commands/lang-rules/csharp.md new file mode 100644 index 0000000..c66f407 --- /dev/null +++ b/commands/lang-rules/csharp.md @@ -0,0 +1,109 @@ +# C# Review Rules + +## All Versions + +- [ ] No hardcoded credentials, connection strings, or secrets in source code +- [ ] Exceptions are not swallowed silently (`catch (Exception) {}`) +- [ ] `IDisposable` types used in `using` statements or disposed explicitly +- [ ] `async void` methods only used for event handlers — never for fire-and-forget without error handling +- [ ] Nullable reference type annotations (`?`) present where nulls are valid inputs +- [ ] No use of `Thread.Sleep` on async code paths — use `Task.Delay` +- [ ] `ConfigureAwait(false)` used in library code to avoid deadlocks +- [ ] `string` concatenation inside loops uses `StringBuilder`, not `+` +- [ ] `==` not used to compare strings where culture-sensitive comparison is needed +- [ ] Static mutable state (static fields/properties) is thread-safe +- [ ] LINQ queries do not evaluate multiple times (materialize with `.ToList()` before re-use) +- [ ] No `dynamic` type used in performance-critical paths +- [ ] `public` API surfaces have XML documentation on all members +- [ ] Interfaces used for dependencies rather than concrete types (DI-friendly) +- [ ] No reflection over private members of external types +- [ ] Enum flags decorated with `[Flags]` attribute when bit-masking is intended + +## .NET Framework 4.x + +- [ ] C# version limited to 7.3 — no `record` types, `init` properties, `required` keyword, file-scoped namespaces, global usings, switch expressions, pattern matching `is not null`, top-level statements, raw string literals +- [ ] `using` declarations without braces (C# 8) have limited support — prefer explicit `using` blocks +- [ ] `string.Contains(char)` overload not available — use `string.Contains(string)` or `IndexOf` +- [ ] `string.Contains(value, StringComparison)` not available — use `IndexOf(value, comparison) >= 0` +- [ ] `ArgumentNullException.ThrowIfNull()` not available — use manual `if (x == null) throw new ArgumentNullException()` +- [ ] `Span`, `Memory`, `ValueTask` not available or limited — use `byte[]` and `Task` +- [ ] `System.Text.Json` not available — use `Newtonsoft.Json` / `JsonConvert` +- [ ] `IAsyncEnumerable` not available — use `Task>` or `Task>` +- [ ] `ILogger` / `Microsoft.Extensions.Logging` not standard — check if project uses it before suggesting +- [ ] `HttpClientFactory` not available — use `new HttpClient()` with proper disposal or a static instance +- [ ] Null-conditional assignment (`x ??= y`) not available (C# 8+) — use explicit null check +- [ ] `Index` and `Range` types (`^1`, `..`) not available (C# 8+) — use `Length - 1` and `Substring` +- [ ] `IHttpActionResult` is the return type for Web API controllers — not `IActionResult` or `ActionResult` + +## EF 6.x + +- [ ] No `ExecuteUpdateAsync()` / `ExecuteDeleteAsync()` — use loop + `SaveChanges()` or raw SQL +- [ ] No compiled queries — use parameterized raw SQL for performance-critical paths +- [ ] No `IAsyncEnumerable` query streaming — use `ToListAsync()` +- [ ] No bulk insert/update operations built-in — use `AddOrUpdate()` for seeds, raw SQL for bulk ops +- [ ] Migrations use `DbMigration` base class, not `Migration` +- [ ] `DbContext` uses `DbSet` with `DbModelBuilder` configuration, not `OnModelCreating` fluent API style +- [ ] Connection strings in `Web.config`, not `appsettings.json` +- [ ] `SqlQuery()` for raw SQL queries, not `FromSqlRaw()` +- [ ] No `.AsNoTracking()` on `DbSet` — use `AsNoTracking()` via extension or manual detach +- [ ] `Include()` for eager loading uses string paths (`Include("Orders.Items")`) or lambda — verify overload exists + +## ASP.NET Web API 5.x + +- [ ] No `[ApiController]` attribute — validation is manual, not automatic +- [ ] No `ActionResult` — use `IHttpActionResult` (`Ok()`, `NotFound()`, `BadRequest()`) +- [ ] No minimal APIs — all endpoints are controller actions +- [ ] No `[FromBody]` auto-binding on complex types — may need explicit `[FromBody]` attribute +- [ ] No built-in model validation auto-response — check `ModelState.IsValid` manually +- [ ] Route templates use `[Route("api/controller/{id}/{action}")]` convention +- [ ] No `ProblemDetails` — return custom error objects or use `BadRequest(ModelState)` +- [ ] `GlobalConfiguration.Configuration.DependencyResolver` for DI — not `IServiceProvider` +- [ ] CORS configured via `EnableCorsAttribute` or `Web.config`, not middleware pipeline +- [ ] No `IOptions` pattern — configuration via `ConfigurationManager` or custom settings classes + +## LINQ (All .NET Versions) + +- [ ] `IQueryable` queries not materialized prematurely — `.ToList()` only when results are needed +- [ ] `IEnumerable` results not iterated multiple times — materialize with `.ToList()` before re-use +- [ ] `.FirstOrDefault()` / `.SingleOrDefault()` results null-checked before property access +- [ ] `.Where().First()` replaced with `.First(predicate)` for clarity +- [ ] `.Any()` used instead of `.Count() > 0` for existence checks (short-circuits) +- [ ] `.Select()` projections do not include side effects — LINQ should be pure +- [ ] `.OrderBy()` not called multiple times — use `.ThenBy()` for secondary sort +- [ ] `.GroupBy()` results not re-enumerated inside the loop — materialize or use lookup + +## .NET 6+ + +- [ ] `DateOnly` and `TimeOnly` used instead of `DateTime` for date-only or time-only values +- [ ] `ArgumentNullException.ThrowIfNull()` used instead of manual null guard code +- [ ] Minimal API endpoints validate inputs before processing +- [ ] `ILogger` injected via DI, not instantiated directly +- [ ] Hot reload–incompatible patterns (source generators, static constructors with side effects) documented +- [ ] `CancellationToken` threaded through async call chains from entry points +- [ ] Record types used for immutable data transfer objects where appropriate + +## .NET 8+ + +- [ ] `FrozenDictionary` / `FrozenSet` used for large, read-only lookup tables +- [ ] Primary constructors used consistently for simple classes/structs without extra initialization +- [ ] `TimeProvider` abstracted for testable time-dependent logic +- [ ] Keyed services used where multiple implementations of the same interface are registered +- [ ] `SearchValues` used for high-frequency char/byte search patterns in hot paths + +## EF Core 6+ + +- [ ] No N+1 queries — related data loaded with `.Include()` or projection +- [ ] Read-only queries use `.AsNoTracking()` +- [ ] Bulk deletes/updates use `ExecuteDeleteAsync()` / `ExecuteUpdateAsync()` not loop + SaveChanges +- [ ] Migrations do not drop columns that are still referenced in code +- [ ] `DbContext` not shared across async paths (not thread-safe) +- [ ] Raw SQL (`FromSqlRaw`) uses parameterized queries — no string interpolation + +## ASP.NET Core + +- [ ] `[Authorize]` or `[AllowAnonymous]` explicitly on all controller actions — no implicit open endpoints +- [ ] `ModelState.IsValid` checked before processing in controller actions +- [ ] CORS policy explicitly defined — no wildcard `*` origin in production config +- [ ] Response caching headers set appropriately on read endpoints +- [ ] File uploads validate extension, MIME type, and maximum size +- [ ] Anti-forgery tokens validated on state-changing form actions diff --git a/commands/lang-rules/css.md b/commands/lang-rules/css.md new file mode 100644 index 0000000..c1f7620 --- /dev/null +++ b/commands/lang-rules/css.md @@ -0,0 +1,76 @@ +# CSS / SCSS Review Rules + +## All Versions + +### Layout & Box Model +- [ ] No `!important` unless overriding third-party styles — document reason in a comment +- [ ] `box-sizing: border-box` applied globally or per component — not mixed models +- [ ] `z-index` values use a defined scale (10, 20, 30...) — no arbitrary large numbers (99999) +- [ ] Flexbox or Grid used for layout — no `float`-based layouts in new code +- [ ] No negative margins for layout — use gap, padding, or flexbox alignment +- [ ] Fixed dimensions (`width: 300px`) not used on containers that hold dynamic content — use `min-width`/`max-width` +- [ ] `overflow: hidden` not used to hide layout bugs — fix the underlying overflow cause + +### Naming & Organization +- [ ] Class names use project convention (camelCase for CSS Modules, BEM for global CSS) — consistent within file +- [ ] No element selectors (`div`, `span`, `p`) for styling — use class selectors for specificity control +- [ ] No ID selectors (`#header`) for styling — IDs are for anchors and JS hooks only +- [ ] Selectors max 3 levels deep — avoid `.parent .child .grandchild .great-grandchild` specificity chains +- [ ] No inline styles in markup for static styling — use classes; inline only for truly dynamic values (JS-computed) +- [ ] Color values use project design tokens / CSS custom properties — not hardcoded hex/rgb in component styles +- [ ] Repeated values (colors, spacing, fonts, shadows) extracted to CSS custom properties or SCSS variables + +### Typography & Spacing +- [ ] Font sizes use relative units (`rem`, `em`) or project scale — not arbitrary `px` values +- [ ] Line height uses unitless values (`1.5`) — not pixel values that break on font-size changes +- [ ] Spacing uses consistent scale (multiples of 4px or 8px) — not arbitrary values +- [ ] `text-overflow: ellipsis` paired with `overflow: hidden` and `white-space: nowrap` (all three required) + +### Responsive & Cross-Browser +- [ ] Media queries use the project's defined breakpoints — no arbitrary pixel values +- [ ] Mobile-first (`min-width`) or desktop-first (`max-width`) approach consistent within project — not mixed +- [ ] No vendor prefixes in source CSS when autoprefixer is configured — let the tool handle it +- [ ] `touch-action` set on interactive elements for mobile — prevents 300ms tap delay +- [ ] Minimum touch target size 44x44px on interactive elements + +### Performance +- [ ] No `*` universal selector in component styles — acceptable only in reset/normalize +- [ ] `will-change` used sparingly and only on elements that actually animate — not as a blanket optimization +- [ ] Animations use `transform` and `opacity` — not `top`/`left`/`width`/`height` (triggers layout) +- [ ] Transitions under 300ms for UI feedback; `prefers-reduced-motion` media query respected +- [ ] Large background images use `image-set()` or responsive `srcset` — not desktop-size images on mobile +- [ ] `contain: layout` or `contain: content` used on independently-rendered components to limit paint scope + +### Accessibility +- [ ] Color contrast meets WCAG AA (4.5:1 for text, 3:1 for large text / UI components) +- [ ] Focus styles visible and not removed (`outline: none` only when replaced with equivalent visible indicator) +- [ ] `:focus-visible` used instead of `:focus` to avoid focus rings on mouse click +- [ ] Content not hidden with `display: none` when it should be screen-reader accessible — use `.sr-only` pattern +- [ ] No `user-select: none` on text content users might need to copy +- [ ] Decorative elements use `aria-hidden="true"` — no alt text on decorative images + +### Common Bugs +- [ ] `position: sticky` has a scrollable ancestor — fails silently if any ancestor has `overflow: hidden` +- [ ] `gap` property supported by target browsers when used with flexbox (not supported in older Safari) +- [ ] Percentage heights have an explicit height on the parent — percentage height on `auto`-height parent is 0 +- [ ] `calc()` expressions have spaces around operators — `calc(100%-20px)` silently fails +- [ ] `transition: all` not used — specify exact properties to avoid animating unintended changes + +## CSS Modules + +- [ ] File naming follows convention: `ComponentName.module.css` (or `.module.scss`) +- [ ] Import as `styles` object: `import styles from './Component.module.css'` +- [ ] Class access uses dot notation (`styles.container`) — not bracket notation (`styles['container']`) +- [ ] `:global(.class)` used sparingly — only for third-party overrides, documented with comment +- [ ] `composes` used for style composition — not `@extend` or class concatenation in markup + +## SCSS + +- [ ] Nesting max 3 levels deep — deeper nesting produces overly specific selectors +- [ ] `@use` and `@forward` used instead of `@import` (deprecated in Dart Sass) +- [ ] Mixins used for repeated patterns with parameters — not for single-use style blocks +- [ ] `@each` / `@for` loops used for generating utility classes — not copy-pasted variants +- [ ] Placeholder selectors (`%placeholder`) used with `@extend` — not class-based `@extend` (avoids unintended selector bloat) +- [ ] Variables use meaningful names (`$color-primary`, `$spacing-md`) — not generic (`$a`, `$temp`) +- [ ] Maps used for structured data (breakpoints, colors, z-index scale) — accessed with `map.get()` +- [ ] Partials prefixed with underscore (`_variables.scss`) and loaded via `@use` diff --git a/commands/lang-rules/go.md b/commands/lang-rules/go.md new file mode 100644 index 0000000..2996fd0 --- /dev/null +++ b/commands/lang-rules/go.md @@ -0,0 +1,47 @@ +# Go Review Rules + +## All Versions + +- [ ] All error return values checked — no `_` discarding errors from functions that can fail +- [ ] Errors wrapped with `fmt.Errorf("context: %w", err)` to preserve the error chain +- [ ] `panic` not used for recoverable errors — only for unrecoverable programming mistakes +- [ ] Goroutines have a defined lifetime and are cleaned up (not leaked on function return) +- [ ] `context.Context` threaded through all functions that do I/O or can be cancelled +- [ ] `defer` used for cleanup (file close, mutex unlock) even in error paths +- [ ] Mutex locks not held across `select` or channel sends that could block indefinitely +- [ ] No `init()` functions with side effects that affect global state and complicate testing +- [ ] Interface types defined in the consumer package, not the implementor package +- [ ] Struct fields exported only when they need to be serialized or accessed externally +- [ ] `make([]T, 0, capacity)` used when the slice size is known in advance +- [ ] `sync.WaitGroup` `Add()` called before `go` — never inside the goroutine +- [ ] HTTP response bodies always closed via `defer resp.Body.Close()` +- [ ] `strings.Builder` used for string concatenation in loops — not `+` +- [ ] SQL queries use `database/sql` with `?` / `$N` placeholders — no string formatting with user input +- [ ] Channel directions (`chan<-` / `<-chan`) specified in function signatures to restrict usage +- [ ] `select` with a `default` case used only when non-blocking channel access is explicitly intended +- [ ] `sync.Once` used for one-time initialization instead of flag variables with a mutex +- [ ] Goroutine stacks not grown unboundedly — recursive functions with deep call chains validated +- [ ] `os.Exit` not called inside library code — only acceptable in `main()` after cleanup +- [ ] Table-driven tests (`t.Run`) used for functions with multiple input/output cases + +- [ ] Pointer receiver used when method mutates state or receiver is large; value receiver for small immutable types +- [ ] `context.WithCancel` / `context.WithTimeout` results cancelled via `defer cancel()` to avoid context leak +- [ ] No goroutine spawned without a way to signal shutdown — use context cancellation or done channel +- [ ] `http.Client` has explicit `Timeout` set — default is no timeout +- [ ] `json.Decoder` used for streaming/large JSON — `json.Unmarshal` for small known-size payloads +- [ ] Race conditions tested with `go test -race` — enabled in CI + +## Go 1.18+ + +- [ ] Generics used to eliminate code duplication across types, not as over-engineering for single-type cases +- [ ] Generic constraints use `comparable` for map keys and set elements +- [ ] Fuzz tests (`testing.F`) added for functions parsing untrusted external input +- [ ] `any` alias for `interface{}` used consistently in new code + +## Go 1.21+ + +- [ ] `slices.Sort()` / `slices.Contains()` from `slices` package used instead of manual implementations +- [ ] `maps.Keys()` / `maps.Values()` used instead of manual map iteration for key/value extraction +- [ ] `log/slog` used for structured logging in new code replacing `log.Printf` +- [ ] `min()` / `max()` builtins used instead of if-else branches for simple comparisons +- [ ] `errors.Is()` / `errors.As()` used for error type checking — not direct type assertions on errors diff --git a/commands/lang-rules/java.md b/commands/lang-rules/java.md new file mode 100644 index 0000000..cf70969 --- /dev/null +++ b/commands/lang-rules/java.md @@ -0,0 +1,55 @@ +# Java Review Rules + +## All Versions + +- [ ] No hardcoded credentials, API keys, or connection strings in source code +- [ ] `equals()` and `hashCode()` overridden together — never one without the other +- [ ] `null` not returned from public methods where an empty collection or `Optional` is cleaner +- [ ] Resources (`Connection`, `Stream`, `Reader`) closed in `finally` or via try-with-resources +- [ ] `synchronized` not used on `this` for public classes — use private lock objects +- [ ] `String` concatenation in loops uses `StringBuilder` +- [ ] SQL queries use `PreparedStatement` — no string concatenation with user input +- [ ] Serializable classes define `serialVersionUID` explicitly +- [ ] Collections typed with generics — no raw types (`List` instead of `List`) +- [ ] `instanceof` checks followed immediately by a cast — not separated by unrelated code +- [ ] Checked exceptions not swallowed silently — at minimum logged with stack trace +- [ ] Static utility methods are `static` and not called on instances +- [ ] `final` fields used for values that don't change after construction +- [ ] Thread-shared mutable state access synchronized or uses `java.util.concurrent` types + +## Java 8+ + +- [ ] `Optional` used for return types that may be absent — not null returns from public methods +- [ ] Stream API does not perform side effects in `map()` or `filter()` — use `forEach()` or `peek()` for debugging +- [ ] `Collectors.toUnmodifiableList()` or `List.copyOf()` used when returned collection should not be modified +- [ ] Lambda expressions in `Comparator.comparing()` instead of anonymous `Comparator` classes +- [ ] `CompletableFuture` chains handle exceptions with `.exceptionally()` or `.handle()` — not left unhandled +- [ ] `ConcurrentHashMap` used instead of `Collections.synchronizedMap()` for concurrent access +- [ ] `DateTimeFormatter` and `LocalDate`/`LocalDateTime` used instead of `SimpleDateFormat` and `Date` +- [ ] Stream pipelines terminated — no intermediate operations without a terminal operation +- [ ] `@FunctionalInterface` annotation on interfaces intended as lambda targets + +## Java 11+ + +- [ ] `var` used only where the type is obvious from the right-hand side (improves readability) +- [ ] `String.isBlank()` used instead of `trim().isEmpty()` +- [ ] `Optional.ifPresentOrElse()` used for side-effecting Optional consumption instead of `.isPresent()` + branch +- [ ] HTTP requests use `java.net.http.HttpClient` (not `HttpURLConnection`) +- [ ] `Files.readString()` / `Files.writeString()` used for simple file reads/writes + +## Java 17+ + +- [ ] `sealed` classes / `permits` used to model closed type hierarchies +- [ ] `record` types used for immutable data carriers instead of POJO with boilerplate +- [ ] `switch` expressions used (not statements) where all cases return a value +- [ ] Pattern matching `instanceof` used — `if (obj instanceof String s)` instead of explicit cast +- [ ] Text blocks used for multi-line string literals (SQL, JSON templates) + +## Java 21+ + +- [ ] Virtual threads (`Thread.ofVirtual()`) used for I/O-bound concurrent work, not platform threads +- [ ] `SequencedCollection` methods (`getFirst()`, `getLast()`, `reversed()`) used where applicable +- [ ] Unnamed classes/instance main methods only in scripting contexts — not production code +- [ ] Pattern matching in `switch` guards all cases or has an explicit `default` +- [ ] `StructuredTaskScope` used for structured concurrency — not raw thread + `CompletableFuture` chains +- [ ] `String.formatted()` used instead of `String.format()` for readability in new code diff --git a/commands/lang-rules/javascript.md b/commands/lang-rules/javascript.md new file mode 100644 index 0000000..a75bd93 --- /dev/null +++ b/commands/lang-rules/javascript.md @@ -0,0 +1,88 @@ +# JavaScript Review Rules + +## All Versions + +- [ ] No `eval()` or `new Function(string)` with user-controlled input +- [ ] `==` not used for equality — always `===` +- [ ] `var` not used — use `const` or `let` +- [ ] Promises have `.catch()` handlers or are returned to a caller that handles rejection +- [ ] No swallowed errors in `try/catch` blocks without at least logging +- [ ] Global variables not introduced via implicit `window.myVar = ...` +- [ ] No mutation of function arguments (avoid side-effects on inputs) +- [ ] `typeof` used before accessing properties on potentially-undefined values +- [ ] JSON parsed with try/catch around `JSON.parse()` +- [ ] `innerHTML` not set with unsanitized user content (XSS risk) +- [ ] Event listeners removed when component/element is destroyed to prevent memory leaks +- [ ] `console.log` / `console.error` not left in production code paths +- [ ] No circular `require()` / `import` dependencies +- [ ] `null` and `undefined` handled distinctly where both are possible values +- [ ] Large arrays not iterated with `Array.prototype.reduce` when a simple loop is clearer +- [ ] `fetch()` responses checked with `response.ok` before reading body — non-2xx does not throw +- [ ] `fetch()` / `XMLHttpRequest` calls have error handling for network failures +- [ ] `setTimeout` / `setInterval` cleaned up on component unmount or scope exit +- [ ] `FormData.append()` not called with `undefined` values — converts to string `"undefined"` +- [ ] `parseInt()` called with explicit radix: `parseInt(value, 10)` — not `parseInt(value)` +- [ ] No prototype pollution — user-controlled keys not used in `obj[key] = value` without validation +- [ ] `postMessage` origin validated in `message` event handlers — not accepting from `*` +- [ ] `localStorage` / `sessionStorage` values parsed with try/catch — may contain non-JSON +- [ ] `Array.isArray()` used for array checks — not `instanceof Array` (fails across iframes) +- [ ] `Object.keys()` / `Object.entries()` used for iteration — not `for...in` without `hasOwnProperty` guard + +## ES2017-ES2019 + +### Available +- [ ] `async/await` available (ES2017) — prefer over `.then()` chains +- [ ] `Object.entries()` / `Object.values()` available (ES2017) +- [ ] `String.padStart()` / `String.padEnd()` available (ES2017) +- [ ] `Object.getOwnPropertyDescriptors()` available (ES2017) +- [ ] Rest/spread properties on objects available (ES2018): `const { a, ...rest } = obj` +- [ ] `Promise.finally()` available (ES2018) +- [ ] `for await...of` available (ES2018) for async iteration +- [ ] `Array.flat()` / `Array.flatMap()` available (ES2019) +- [ ] `Object.fromEntries()` available (ES2019) +- [ ] `String.trimStart()` / `String.trimEnd()` available (ES2019) +- [ ] `try { } catch { }` without binding available (ES2019) + +### DO NOT suggest (not available pre-ES2020) +- [ ] No optional chaining (`?.`) — use manual null checks: `obj && obj.prop && obj.prop.method()` +- [ ] No nullish coalescing (`??`) — use ternary or `||` (but beware of `0`/`""` being falsy with `||`) +- [ ] No `Promise.allSettled()` — use `Promise.all()` with individual `.catch()` wrappers +- [ ] No `globalThis` — use `window` (browser) or `global` (Node) +- [ ] No `BigInt` literals (`123n`) +- [ ] No `String.matchAll()` — use regex with `exec()` in a loop +- [ ] No `import.meta` — use `__dirname` / `__filename` in Node +- [ ] No `Array.at()` (ES2022) — use `arr[arr.length - 1]` for last element +- [ ] No `Object.hasOwn()` (ES2022) — use `Object.prototype.hasOwnProperty.call(obj, key)` +- [ ] No `structuredClone()` — use `JSON.parse(JSON.stringify(obj))` or lodash `cloneDeep` +- [ ] No `String.replaceAll()` (ES2021) — use regex with global flag: `str.replace(/pattern/g, replacement)` +- [ ] No private class fields (`#field`) (ES2022) — use underscore convention `_field` +- [ ] No logical assignment operators (`&&=`, `||=`, `??=`) (ES2021) +- [ ] No `Error` cause option (`{ cause: err }`) (ES2022) + +### Node.js 12.x constraints (if applicable) +- [ ] No `fs/promises` — use `util.promisify(fs.readFile)` or callbacks +- [ ] No `AbortController` — use manual cancellation flags +- [ ] No optional chaining in Node-executed code (only in webpack-transpiled browser code) +- [ ] No top-level `await` +- [ ] No `worker_threads` stable API for CPU-bound work (experimental in 12) + +## ES2020+ + +- [ ] Optional chaining (`?.`) used instead of manual null checks for deep property access +- [ ] Nullish coalescing (`??`) used instead of `||` where `0` or `""` are valid values +- [ ] `Promise.allSettled()` used when all results are needed regardless of failure +- [ ] `BigInt` used for integer values exceeding `Number.MAX_SAFE_INTEGER` +- [ ] Dynamic `import()` used for code-splitting large modules in browser environments +- [ ] `globalThis` used instead of `window` / `global` for cross-environment code + +## ES2022+ + +- [ ] Private class fields (`#field`) used instead of convention-based `_field` for encapsulation +- [ ] `Array.at(-1)` used instead of `arr[arr.length - 1]` for last-element access +- [ ] Top-level `await` only used in modules where the async dependency graph is understood +- [ ] `Object.hasOwn()` used instead of `Object.prototype.hasOwnProperty.call()` +- [ ] Error cause (`new Error("msg", { cause: err })`) used when re-throwing wrapped errors +- [ ] Static class blocks (`static { ... }`) used for one-time static initialization instead of IIFE patterns +- [ ] `Array.prototype.findLast()` / `findLastIndex()` used for reverse searches instead of manual `[...arr].reverse().find()` +- [ ] `WeakRef` and `FinalizationRegistry` used only when the semantics are understood — not for general caching +- [ ] Class `#privateField` not accessed via `Object.getOwnPropertyNames` workarounds — treat as a true encapsulation boundary diff --git a/commands/lang-rules/kotlin.md b/commands/lang-rules/kotlin.md new file mode 100644 index 0000000..6a4dd02 --- /dev/null +++ b/commands/lang-rules/kotlin.md @@ -0,0 +1,40 @@ +# Kotlin Review Rules + +## All Versions + +- [ ] `!!` (not-null assertion) avoided — use safe call `?.`, `let`, or `requireNotNull` with a message +- [ ] Coroutine scopes not leaked — structured concurrency used, scopes tied to lifecycle owners +- [ ] `runBlocking` not called from a coroutine context or main thread in production code +- [ ] `GlobalScope` not used — use `viewModelScope`, `lifecycleScope`, or a DI-provided scope +- [ ] `lateinit var` only on properties initialized before first use — avoid in containers that may be null +- [ ] Data classes not used for entities with business logic — use regular classes +- [ ] `companion object` members that don't access instance state marked `@JvmStatic` in JVM-interop code +- [ ] Sealed classes cover all subclass cases in `when` expressions (compiler-verified exhaustiveness) +- [ ] Extension functions do not access internal implementation details of external types +- [ ] `object` singletons with mutable state document their thread-safety guarantees +- [ ] `with` / `apply` / `run` / `let` / `also` scoping functions used consistently and not nested more than 2 levels +- [ ] Flows collected inside `lifecycleScope.launch { collect {} }` not `GlobalScope` +- [ ] `StateFlow` preferred over `LiveData` in new Kotlin-only code for observable state +- [ ] `Result` used for operations that can fail instead of nullable return + out-of-band error +- [ ] `suspend` functions do not block the calling thread (no `Thread.sleep`, no blocking I/O without `Dispatchers.IO`) +- [ ] `Dispatchers.IO` used for blocking I/O operations, `Dispatchers.Default` for CPU-bound work +- [ ] Coroutine exception handlers (`CoroutineExceptionHandler`) attached to root scopes that launch fire-and-forget coroutines +- [ ] `flow { emit(...) }` builders do not call `emit` from a different coroutine context +- [ ] `SharedFlow` replay and extra buffer capacity set intentionally — default `0` for events, `1` for state +- [ ] `by lazy` properties that access Android context or lifecycle use `LazyThreadSafetyMode.NONE` only when single-threaded access is guaranteed +- [ ] String templates preferred over string concatenation with `+` +- [ ] `when` expressions without an `else` branch on non-sealed types — add exhaustive `else` or convert to sealed hierarchy + +## Kotlin 1.5+ + +- [ ] `value class` used for type-safe wrappers around primitives (replaces inline classes) +- [ ] Sealed interfaces used for event/state hierarchies instead of sealed abstract classes +- [ ] `@JvmRecord` applied to data classes used in Java interop requiring Java records + +## Kotlin 2.0+ + +- [ ] K2 compiler mode implications understood — annotation processor compatibility verified +- [ ] Smart cast improvements (K2) not relied upon in ways that break on older compilers +- [ ] `@Suppress("UNCHECKED_CAST")` replaceable — verify if K2 type inference eliminates the need +- [ ] Multiplatform targets tested — expect/actual declarations verified for all expected platforms +- [ ] `context receivers` (if enabled) used sparingly and documented, as the API is still in flux diff --git a/commands/lang-rules/php.md b/commands/lang-rules/php.md new file mode 100644 index 0000000..7a109bd --- /dev/null +++ b/commands/lang-rules/php.md @@ -0,0 +1,43 @@ +# PHP Review Rules + +## All Versions + +- [ ] No hardcoded passwords, API keys, or database credentials in source files +- [ ] User input never interpolated directly into SQL — use PDO prepared statements with `?` placeholders +- [ ] `htmlspecialchars()` or template auto-escaping applied before outputting user data to HTML +- [ ] `eval()` not called with user-controlled strings +- [ ] File inclusion (`include`, `require`) paths not derived from user input (local file inclusion) +- [ ] `$_GET`, `$_POST`, `$_COOKIE`, `$_REQUEST` values never used unvalidated in SQL/shell/file paths +- [ ] Passwords stored with `password_hash()` (bcrypt/argon2) — not MD5, SHA1, or plain text +- [ ] Error display (`display_errors`) disabled in production — errors logged, not rendered +- [ ] `session_regenerate_id(true)` called after authentication to prevent session fixation +- [ ] File uploads validate extension and MIME type server-side — not just client-side +- [ ] Type declarations present on all function parameters and return types (strict_types=1) +- [ ] `declare(strict_types=1)` at the top of all PHP files +- [ ] Exceptions not swallowed silently — caught exceptions logged or re-thrown + +## PHP 8.0+ + +- [ ] Named arguments used for clarity on functions with many optional parameters +- [ ] `match` expressions used instead of `switch` where a value is returned (no fall-through) +- [ ] `nullsafe` operator (`?->`) used for chained method calls on nullable objects +- [ ] Union types (`int|string`) used in signatures instead of doc-block only annotations +- [ ] Attributes (`#[Attribute]`) used instead of docblock annotations where a library supports both + +## Laravel 10+ + +- [ ] Form requests validate input in `rules()` before it reaches the controller +- [ ] `Policy` or `Gate` used for authorization — not manual `if ($user->role == "admin")` checks +- [ ] Eloquent mass assignment protected by `$fillable` or `$guarded` on all models +- [ ] Queue jobs implement `ShouldBeUnique` or are idempotent for safe retry +- [ ] Environment-specific config loaded from `.env` — not hardcoded in `config/` files +- [ ] Migrations are reversible (`down()` method matches `up()` rollback) + +## Symfony 6+ + +- [ ] Services are autowired — no manual `new` inside service constructors +- [ ] Voters used for authorization logic instead of inline `isGranted` string-based checks +- [ ] Twig templates use `{{ variable }}` (auto-escaped) not `{{ variable|raw }}` for user data +- [ ] Event subscribers registered via `#[AsEventListener]` or `getSubscribedEvents()` consistently +- [ ] Messenger handlers are idempotent — safe to retry on failure +- [ ] Cache warmer implemented for expensive service initialization done at deployment time diff --git a/commands/lang-rules/python.md b/commands/lang-rules/python.md new file mode 100644 index 0000000..30d1bfb --- /dev/null +++ b/commands/lang-rules/python.md @@ -0,0 +1,132 @@ +# Python Review Rules + +## All Versions + +### Security +- [ ] No hardcoded credentials, API keys, tokens, or connection strings — use environment variables or secrets manager +- [ ] Never log secrets, tokens, full connection strings, or PII — use redacted/hashed representations +- [ ] `os.system()` / `subprocess.run(shell=True)` not used with user-controlled input (command injection) +- [ ] SQL queries use parameterized form — no f-string or `%` formatting of query strings with user data +- [ ] File paths from user input sanitized — disallow `..`, `/`, `\` path separators; restrict to safe characters +- [ ] CSV export sanitizes fields starting with `=`, `+`, `-`, `@` to prevent formula injection +- [ ] Error messages do not expose raw exception details, connection strings, or internal paths to end users +- [ ] Redact URL query parameters containing credentials (`sig`, `code`, `token`, `sas`) before logging +- [ ] All HTTP requests use HTTPS — do not disable certificate verification + +### Code Quality +- [ ] `except Exception` not used as a bare catch-all without re-raise or detailed logging +- [ ] Catch specific exceptions (`requests.RequestException`, `FileNotFoundError`, `json.JSONDecodeError`) not bare `except:` +- [ ] `logger.exception()` used in `except` blocks instead of `logger.error(str(e))` to capture stack traces +- [ ] Try/except blocks narrow in scope — only wrap code that can throw the caught exception +- [ ] Re-raise or raise a more specific exception with context if you catch for logging but cannot handle +- [ ] Mutable default arguments not used (`def f(items=[])`) — use `None` and initialize inside body +- [ ] Context managers (`with`) used for file handles, DB connections, locks — always clean up resources +- [ ] `isinstance()` used for type checks, not `type() ==` +- [ ] Generators preferred over list comprehensions for large sequences consumed once +- [ ] Circular imports avoided — modules organized so lower-level modules do not import higher-level ones +- [ ] `__all__` defined on public modules to control exported names +- [ ] Regular expressions compiled with `re.compile()` when used in a loop +- [ ] `dict.get(key, default)` used instead of direct indexing for optional or external data +- [ ] Thread-shared mutable state protected with `threading.Lock` +- [ ] Cyclomatic complexity kept low (under 10) — functions small and focused on a single task +- [ ] No unused variables, imports, or redundant code — remove dead code +- [ ] Distinct variable names for different responses — avoid copy-paste errors with wrong references +- [ ] No duplicate method definitions — remove shadowed methods + +### Logging +- [ ] `logging` used instead of `print()` in library/production code +- [ ] Dedicated loggers per module (`logger = logging.getLogger(__name__)`) — not root logger with `logging.basicConfig()` +- [ ] Explicit `encoding='utf-8'` on file handlers +- [ ] Sanitize and truncate user inputs before logging at INFO level +- [ ] PII logged only at DEBUG level with truncation — never at INFO +- [ ] Hash identifiers before logging when correlation is needed but full value shouldn't be exposed + +### Documentation & Type Hints +- [ ] All public modules, classes, and functions have docstrings (Google Style or NumPy Style) +- [ ] One-line docstrings end with a period (PEP 257) +- [ ] All function arguments and return values have type hints +- [ ] `Optional[T]` used for parameters that can be `None` — not bare `T` with default `None` +- [ ] `Dict[str, Any]`, `List[str]` used instead of bare `dict` or `list` +- [ ] Type hints match actual return values — don't annotate `str` if it can return `None` + +### File I/O +- [ ] Always specify `encoding='utf-8'` when opening text files +- [ ] Wrap file operations in try/except for `FileNotFoundError`, `PermissionError`, `IOError` +- [ ] `Path.mkdir(parents=True, exist_ok=True)` before writing files +- [ ] `pathlib.Path` used instead of `os.path` string manipulation + +### JSON & Data Parsing +- [ ] `json.load()` / `json.loads()` wrapped in try/except for `json.JSONDecodeError` +- [ ] Validate JSON structure after parsing — check required keys and expected types +- [ ] `.get()` with defaults used for optional or external data fields +- [ ] `if value is not None:` used instead of `if value:` when `0`, `""`, or `[]` are valid + +### Network & HTTP +- [ ] ALL HTTP requests (`requests`, `httpx`, OpenAI client) have explicit `timeout=` parameter +- [ ] Retry logic with exponential backoff for transient errors (rate limits, connection errors) +- [ ] Success based on HTTP status codes (`response.status_code == 200`), not response body presence +- [ ] `response.json()` wrapped in try/except for `ValueError` / `json.JSONDecodeError` +- [ ] `POST` with `json=payload` for JSON bodies — not `GET` with JSON body +- [ ] `GET` uses `params=` for query parameters — not `data=` or `json=` + +### Input Validation +- [ ] Validate all external inputs at function entry — check for `None`, empty strings, whitespace-only +- [ ] Strip and normalize string inputs with `.strip()` before validation +- [ ] `None` checked explicitly (`if value is None`) when empty strings are valid +- [ ] `isinstance()` checks for public API input types +- [ ] Validate lists by checking all items, not just the first one + +### Async +- [ ] No blocking synchronous calls inside async functions — use `asyncio.run_in_executor()` or async clients +- [ ] `asyncio.to_thread()` for synchronous operations in async contexts +- [ ] `await` used for async tool invocations in agent frameworks +- [ ] `async def` and `await` used consistently when calling async APIs + +### Performance +- [ ] `ThreadPoolExecutor` workers capped — use configurable limits, not `max_workers=len(items)` +- [ ] Avoid unnecessary `.copy()` calls on large objects +- [ ] Hoist loop-invariant computations outside loops +- [ ] Never mutate cached objects — return shallow copy when modifying cached data +- [ ] Cache keys use hashed representations of sensitive data, not raw values + +### Configuration +- [ ] Deep merge (recursive) for nested config dicts — shallow `{**base, **override}` overwrites nested dicts +- [ ] `yaml.safe_load()` wrapped in try/except for `yaml.YAMLError` +- [ ] `os.getenv()` with sensible defaults — validate required env vars at startup +- [ ] Configurable thresholds instead of hardcoded magic numbers +- [ ] Non-interactive backends for Matplotlib (`matplotlib.use("Agg")`) before importing pyplot in CI + +### Dependencies +- [ ] Optional dependency imports wrapped in try/except with helpful error messages +- [ ] `TYPE_CHECKING` for imports only needed for type hints — avoid runtime import errors +- [ ] `DataFrame.to_markdown()` wrapped in try/except (requires `tabulate`) +- [ ] Lazy imports for optional dependencies — import when needed, not at module level + +### OpenAI & LLM Clients +- [ ] Set `timeout=` and `max_retries=` when creating OpenAI clients +- [ ] Catch `RateLimitError`, `APIConnectionError` and other client-specific exceptions +- [ ] Validate response structure before accessing fields — handle missing or malformed responses +- [ ] Use configuration-provided model names — don't hardcode model names + +## Python 3.8+ + +- [ ] Walrus operator (`:=`) used for clarity, not in complex nested expressions that obscure control flow +- [ ] `typing` annotations present on all public function signatures +- [ ] `TypedDict` used for dict-typed parameters/returns rather than bare `Dict[str, Any]` +- [ ] `Protocol` used for structural subtyping instead of ABC when no state is shared + +## Python 3.10+ + +- [ ] `match` / `case` (structural pattern matching) used for exhaustive variant dispatch +- [ ] Union types written as `X | Y` instead of `Union[X, Y]` in annotations +- [ ] `ParamSpec` used when decorators must preserve callable signatures + +## Python 3.12+ + +- [ ] `type` statement used for type aliases instead of `TypeAlias` assignment +- [ ] `@override` decorator applied to methods that override base class implementations +- [ ] Generic classes use PEP 695 `class Foo[T]:` syntax rather than `Generic[T]` where supported +- [ ] `sys.monitoring` used for performance-sensitive profiling hooks instead of `sys.settrace` +- [ ] f-string nesting and complex expressions inside `{}` kept readable — extract to a variable if needed +- [ ] `ExceptionGroup` and `except*` used when multiple unrelated exceptions can be raised concurrently (e.g., in `asyncio.TaskGroup`) +- [ ] `dataclasses.field(default_factory=...)` used for mutable defaults in `@dataclass` fields diff --git a/commands/lang-rules/react.md b/commands/lang-rules/react.md new file mode 100644 index 0000000..4a1079c --- /dev/null +++ b/commands/lang-rules/react.md @@ -0,0 +1,94 @@ +# React Review Rules + +## React 16 + +### Available patterns +- [ ] `componentDidMount` / `componentDidUpdate` / `componentWillUnmount` lifecycle methods balance subscriptions with cleanup +- [ ] `setState` in `componentDidUpdate` guarded with a condition to avoid infinite update loops +- [ ] Error boundaries (`componentDidCatch`) present around UI sections that render user data +- [ ] `key` prop on list items uses a stable unique ID, not array index when list is reordered/filtered +- [ ] `propTypes` or TypeScript types defined for all component props +- [ ] Hooks (`useState`, `useEffect`, `useCallback`, `useMemo`, `useRef`, `useReducer`, `useContext`) available from React 16.8+ — prefer functional components with hooks over class components +- [ ] `useSelector` / `useDispatch` available from react-redux 7.1+ — verify project has react-redux 7+ +- [ ] `React.memo()` for preventing unnecessary re-renders on pure functional components +- [ ] `React.lazy()` with `Suspense` for code-splitting (React 16.6+) +- [ ] `React.createContext` / `useContext` for prop drilling avoidance + +### DO NOT suggest (not available in React 16) +- [ ] No `useId()` — generate IDs manually or use a counter +- [ ] No `useTransition()` / `startTransition()` — no concurrent rendering +- [ ] No `useDeferredValue()` — use manual debounce patterns +- [ ] No `useSyncExternalStore()` — use `useEffect` + `useState` for external store subscriptions +- [ ] No `use()` hook for data fetching — use `useEffect` + `useState` +- [ ] No `useOptimistic()` — implement optimistic updates manually with `useState` +- [ ] No automatic batching of state updates outside React event handlers — only batched inside React events +- [ ] No `createRoot()` / `hydrateRoot()` — use `ReactDOM.render()` and `ReactDOM.hydrate()` +- [ ] No Server Components or `"use client"` / `"use server"` directives +- [ ] No `Suspense` for data fetching — `Suspense` only works with `React.lazy()` in React 16 +- [ ] No `flushSync` — state updates are always synchronous in React event handlers + +### React Router v5 (common with React 16 projects) +- [ ] Uses `` not `` — `` is react-router v6+ +- [ ] Uses `useHistory()` not `useNavigate()` — `useNavigate()` is v6+ +- [ ] Uses `` or `` not `}>` +- [ ] No `` or nested route elements — use render props or component prop +- [ ] `useParams()`, `useLocation()`, `useRouteMatch()` available in v5 + +### Redux 3.x-4.x (common with React 16 projects) +- [ ] No Redux Toolkit (`createSlice`, `createAsyncThunk`, `configureStore`, RTK Query) +- [ ] Uses `createStore()` with manual reducers and action types +- [ ] Uses `combineReducers()` for reducer composition +- [ ] Middleware via `applyMiddleware()` — not `configureStore` middleware option +- [ ] Side effects via redux-saga or redux-thunk — not RTK Query + +## React 17+ + +- [ ] `useEffect` cleanup function cancels subscriptions, timers, and async operations +- [ ] `useEffect` dependency array includes all variables from the enclosing scope that are used inside +- [ ] `useCallback` / `useMemo` used only when the memoized value is passed to `React.memo` children or in hot render paths — not applied indiscriminately +- [ ] `useState` setter called with a function form (`setState(prev => ...)`) when new state depends on previous +- [ ] Custom hooks follow `use*` naming convention and only call other hooks at the top level +- [ ] `useRef` used for values that must persist across renders without triggering re-renders +- [ ] No direct DOM manipulation (`document.getElementById`) inside render — use refs +- [ ] Controlled inputs pair `value` with `onChange` — never `value` without `onChange` +- [ ] Context values memoized (`useMemo`) to avoid re-rendering all consumers on every parent render +- [ ] `useReducer` used instead of multiple related `useState` calls when state transitions are complex + +## React 18+ + +- [ ] `startTransition` wraps non-urgent state updates to keep the UI responsive during heavy renders +- [ ] `useDeferredValue` used to debounce expensive derived computations from user input +- [ ] Concurrent-mode–safe effects do not rely on `useLayoutEffect` for data fetching +- [ ] `Suspense` boundaries placed at meaningful loading-state granularity — not wrapping entire pages +- [ ] `use()` hook (data fetching) only called inside `Suspense`-wrapped trees +- [ ] Server Components do not import client-only code (event handlers, browser APIs, stateful hooks) +- [ ] Client Components marked with `"use client"` directive at the top of the file +- [ ] `useId()` used for generating stable IDs for accessibility attributes (`aria-describedby`, `htmlFor`) +- [ ] `act()` wraps all state updates in tests to prevent async act warnings +- [ ] `React.StrictMode` enabled in development to surface double-invocation bugs +- [ ] `flushSync` used only when synchronous DOM reads are required after a state update — not as a general pattern +- [ ] Context `Provider` `value` object memoized to avoid unnecessary re-renders on every parent render cycle +- [ ] `useOptimistic` used for optimistic UI updates that may need to roll back on server error +- [ ] Error boundaries reset their state when `key` changes — not left in error state after navigation +- [ ] `React.lazy()` combined with `Suspense` for code-split components — not dynamic imports without a boundary + +## Accessibility (All React Versions) + +- [ ] Interactive elements (`button`, `a`, `input`) used for clickable content — not `div` or `span` with `onClick` +- [ ] `aria-label` or `aria-labelledby` on icon-only buttons and links +- [ ] `alt` text on all `` elements — empty `alt=""` for decorative images +- [ ] Form inputs have associated `