From 3e7a9943a7f790dbe42db50fc8a7fedbd38f890b Mon Sep 17 00:00:00 2001 From: John Preston Date: Fri, 6 Feb 2026 20:33:29 +0400 Subject: [PATCH] [ai] Improve review subagent. --- .claude/commands/task.md | 16 ++++++++-------- .codex/skills/task-think/PROMPTS.md | 16 +++++++++++++--- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/.claude/commands/task.md b/.claude/commands/task.md index d4a8e4f90c..e682f3de18 100644 --- a/.claude/commands/task.md +++ b/.claude/commands/task.md @@ -338,9 +338,7 @@ Read these files: 1, also read:> - .ai//review.md - Previous review (to see what was already addressed) -Then run this command to see all changes made by the implementation: - git diff HEAD~ -(Ask git log to figure out how many commits back the implementation started, or diff against the base branch. The goal is to see the implementation diff.) +Then run `git diff` to see all uncommitted changes made by the implementation. Implementation agents do not commit, so `git diff` shows exactly the current feature's changes. Then read the modified source files in full to understand changes in context. @@ -352,15 +350,17 @@ REVIEW CRITERIA (in order of importance): 2. **Dead code**: Any code added or left behind that is never called or used, within the scope of the changes. Unused variables, unreachable branches, leftover scaffolding. -3. **Code duplication**: Unnecessary repetition of logic that should be shared. Look for near-identical blocks that differ only in minor details and could be unified. +3. **Redundant changes**: Changes in the diff that have no functional effect — moving declarations or code blocks to a different location without reason, reformatting untouched code, reordering includes or fields with no purpose. Every line in the diff should serve the feature. If a file appears in `git diff` but contains only no-op rearrangements, flag it for revert. -4. **Wrong placement**: Code added to a module where it doesn't logically belong. If another existing module is a clearly better fit for the new code, flag it. Consider the existing module boundaries and responsibilities visible in context.md. +4. **Code duplication**: Unnecessary repetition of logic that should be shared. Look for near-identical blocks that differ only in minor details and could be unified. -5. **Function decomposition**: For longer functions (roughly 50+ lines), consider whether a logical sub-task could be cleanly extracted into a separate function. This is NOT a hard rule — a 100-line function that flows naturally and isn't easily divisible is perfectly fine. But sometimes even a 20-line function contains a clear isolated subtask that reads better as two 10-line functions. The key is to think about it each time: does extracting improve readability and reduce cognitive load, or does it just scatter logic across call sites for no real benefit? Only suggest extraction when there's a genuinely self-contained piece of logic with a clear name and purpose. +5. **Wrong placement**: Code added to a module where it doesn't logically belong. If another existing module is a clearly better fit for the new code, flag it. Consider the existing module boundaries and responsibilities visible in context.md. -6. **Module structure**: Only in exceptional cases — if a large amount of newly added code (hundreds of lines) is logically distinct from the rest of its host module, suggest extracting it into a new module. But do NOT suggest new modules lightly: every module adds significant build overhead due to PCH and heavy template usage. Only suggest this when the new code is both large enough AND logically separated enough to justify it. At the same time, don't let modules grow into multi-thousand-line monoliths either. +6. **Function decomposition**: For longer functions (roughly 50+ lines), consider whether a logical sub-task could be cleanly extracted into a separate function. This is NOT a hard rule — a 100-line function that flows naturally and isn't easily divisible is perfectly fine. But sometimes even a 20-line function contains a clear isolated subtask that reads better as two 10-line functions. The key is to think about it each time: does extracting improve readability and reduce cognitive load, or does it just scatter logic across call sites for no real benefit? Only suggest extraction when there's a genuinely self-contained piece of logic with a clear name and purpose. -7. **Style compliance**: Verify adherence to REVIEW.md rules (empty line before closing brace, operators at start of continuation lines, minimize type checks with direct cast instead of is+as, no if-with-initializer when simpler alternatives exist) and CLAUDE.md conventions (no unnecessary comments, `auto` usage, no hardcoded sizes — must use .style definitions), etc. +7. **Module structure**: Only in exceptional cases — if a large amount of newly added code (hundreds of lines) is logically distinct from the rest of its host module, suggest extracting it into a new module. But do NOT suggest new modules lightly: every module adds significant build overhead due to PCH and heavy template usage. Only suggest this when the new code is both large enough AND logically separated enough to justify it. At the same time, don't let modules grow into multi-thousand-line monoliths either. + +8. **Style compliance**: Verify adherence to REVIEW.md rules (empty line before closing brace, operators at start of continuation lines, minimize type checks with direct cast instead of is+as, no if-with-initializer when simpler alternatives exist) and CLAUDE.md conventions (no unnecessary comments, `auto` usage, no hardcoded sizes — must use .style definitions), etc. IMPORTANT GUIDELINES: - Review ONLY the changes made, not pre-existing code in the repository. diff --git a/.codex/skills/task-think/PROMPTS.md b/.codex/skills/task-think/PROMPTS.md index b997cbce66..f27a9d037b 100644 --- a/.codex/skills/task-think/PROMPTS.md +++ b/.codex/skills/task-think/PROMPTS.md @@ -94,9 +94,19 @@ Read: - .ai//plan.md - .ai//implementation.md -Perform a code review focused on: -- Style and formatting rules from REVIEW.md -- Regressions, thread-safety, performance, and missing tests +Run `git diff` to see all uncommitted changes made by the implementation. Implementation phases do not commit, so `git diff` shows exactly the current feature's changes. Then read the modified source files in full. + +Perform a code review using these criteria (in order of importance): + +1. Correctness and safety: logic errors, null-check gaps at API boundaries, crashes, use-after-free, dangling references, race conditions. +2. Dead code: code added or left behind that is never called or used. Unused variables, unreachable branches, leftover scaffolding. +3. Redundant changes: changes in the diff with no functional effect — moving declarations or code blocks without reason, reformatting untouched code, reordering includes or fields with no purpose. Every line in the diff should serve the feature. If a file appears in `git diff` but contains only no-op rearrangements, flag it for revert. +4. Code duplication: unnecessary repetition of logic that should be shared. +5. Wrong placement: code added to a module where it doesn't logically belong. +6. Function decomposition: for longer functions (~50+ lines), consider whether a sub-task could be cleanly extracted. Only suggest when there is a genuinely self-contained piece of logic. +7. Module structure: only flag if a large amount of new code (hundreds of lines) is logically distinct from its host module. +8. Style compliance: verify adherence to REVIEW.md rules and AGENTS.md conventions. + Write: - .ai//review.md