From b0bcb2132c36b022caf5f80bd47471eb9cc66fb4 Mon Sep 17 00:00:00 2001 From: John Preston Date: Fri, 6 Feb 2026 14:39:48 +0400 Subject: [PATCH] [ai] Extract REVIEW.md for review task step. --- .claude/commands/task.md | 7 ++- .codex/skills/task-think/PROMPTS.md | 7 ++- AGENTS.md | 53 +--------------- REVIEW.md | 96 +++++++++++++++++++++++++++++ 4 files changed, 106 insertions(+), 57 deletions(-) create mode 100644 REVIEW.md diff --git a/.claude/commands/task.md b/.claude/commands/task.md index 968a1b3638..d4a8e4f90c 100644 --- a/.claude/commands/task.md +++ b/.claude/commands/task.md @@ -334,12 +334,13 @@ You are a code review agent for Telegram Desktop (C++ / Qt). Read these files: - .ai//context.md - Codebase context - .ai//plan.md - Implementation plan +- REVIEW.md - Style and formatting rules to enforce 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~ -- . ":(exclude).ai" -(Ask git log to figure out how many commits back the implementation started, or diff against the base branch. The goal is to see ONLY the implementation diff, excluding .ai/ files.) + 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 read the modified source files in full to understand changes in context. @@ -359,7 +360,7 @@ REVIEW CRITERIA (in order of importance): 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. -7. **Style compliance**: Verify adherence to CLAUDE.md conventions — no unnecessary comments, `auto` usage, empty line before closing brace, operators at start of continuation lines, no hardcoded sizes (must use .style definitions), etc. +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. 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 825e334ba5..b997cbce66 100644 --- a/.codex/skills/task-think/PROMPTS.md +++ b/.codex/skills/task-think/PROMPTS.md @@ -86,14 +86,17 @@ If blocked by locked files or access errors, stop and report exact blocker. ```text You are the review phase for task "" in repository . -Read CLAUDE.md for the basic coding rules and guidelines. +Read AGENTS.md for the basic coding rules and guidelines. +Read REVIEW.md for the style and formatting rules you must enforce. Read: - .ai//context.md - .ai//plan.md - .ai//implementation.md -Perform a code review focused on regressions, thread-safety, performance, and missing tests. +Perform a code review focused on: +- Style and formatting rules from REVIEW.md +- Regressions, thread-safety, performance, and missing tests Write: - .ai//review.md diff --git a/AGENTS.md b/AGENTS.md index df29ec3c61..20f53c6669 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,58 +125,7 @@ if (user->isPremium()) { // with the first message in each group. ``` -**Empty line before closing brace:** - -Always add an empty line before the closing brace of a class (after all private fields): - -```cpp -// GOOD: -class MyClass { -public: - void foo(); - -private: - int _value = 0; - -}; - -// BAD: -class MyClass { -public: - void foo(); - -private: - int _value = 0; -}; -``` - -**Multi-line expressions — operators at the start of continuation lines:** - -When splitting an expression across multiple lines, place operators (like `&&`, `||`, `;`, `+`, etc.) at the **beginning** of continuation lines, not at the end of the previous line. This makes it immediately obvious from the left edge whether a line is a continuation or new code. - -```cpp -// BAD - continuation looks like scope code: -if (const auto &lottie = animation->lottie; - lottie && lottie->valid() && lottie->framesCount() > 1) { - lottie->animate([=] { - -// GOOD - semicolon at start signals continuation: -if (const auto &lottie = animation->lottie - ; lottie && lottie->valid() && lottie->framesCount() > 1) { - lottie->animate([=] { - -// BAD - trailing && makes next line look like independent code: -if (veryLongExpression() && - anotherLongExpression() && - anotherOne()) { - doSomething(); - -// GOOD - leading && clearly marks continuation: -if (veryLongExpression() - && anotherLongExpression() - && anotherOne()) { - doSomething(); -``` +**Style and formatting rules** are in `REVIEW.md` — see that file for empty-line-before-closing-brace, operator placement in multi-line expressions, if-with-initializer, and other mechanical style rules. **Use `auto` for type deduction:** diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000000..f968dfd0ec --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,96 @@ +# Code Review Style Guide + +This file contains style and formatting rules that the review subagent must check and fix. These are mechanical issues that should always be caught during code review. + +## Empty line before closing brace + +Always add an empty line before the closing brace of a class (after all private fields): + +```cpp +// BAD: +class MyClass { +public: + void foo(); + +private: + int _value = 0; +}; + +// GOOD: +class MyClass { +public: + void foo(); + +private: + int _value = 0; + +}; +``` + +## Multi-line expressions — operators at the start of continuation lines + +When splitting an expression across multiple lines, place operators (like `&&`, `||`, `;`, `+`, etc.) at the **beginning** of continuation lines, not at the end of the previous line. This makes it immediately obvious from the left edge whether a line is a continuation or new code. + +```cpp +// BAD - continuation looks like scope code: +if (const auto &lottie = animation->lottie; + lottie && lottie->valid() && lottie->framesCount() > 1) { + lottie->animate([=] { + +// GOOD - semicolon at start signals continuation: +if (const auto &lottie = animation->lottie + ; lottie && lottie->valid() && lottie->framesCount() > 1) { + lottie->animate([=] { + +// BAD - trailing && makes next line look like independent code: +if (veryLongExpression() && + anotherLongExpression() && + anotherOne()) { + doSomething(); + +// GOOD - leading && clearly marks continuation: +if (veryLongExpression() + && anotherLongExpression() + && anotherOne()) { + doSomething(); +``` + +## Minimize type checks — prefer direct cast over is + as + +Don't check a type and then cast — just cast and check for null. `asUser()` already returns `nullptr` when the peer is not a user, so calling `isUser()` first is redundant. The same applies to `asChannel()`, `asChat()`, etc. + +```cpp +// BAD - redundant isUser() check, then asUser(): +if (peer && peer->isUser()) { + peer->asUser()->setNoForwardFlags( + +// GOOD - just cast and null-check: +if (const auto user = peer->asUser()) { + user->setNoForwardFlags( +``` + +When you need a specific subtype, look up the specific subtype directly instead of loading a generic type and then casting: + +```cpp +// BAD - loads generic peer, then casts: +if (const auto peer = session().data().peerLoaded(peerId) + ; peer && peer->isUser()) { + peer->asUser()->setNoForwardFlags( + +// GOOD - look up the specific subtype directly: +const auto userId = peerToUser(peerId); +if (const auto user = session().data().userLoaded(userId)) { + user->setNoForwardFlags( +``` + +Avoid C++17 `if` with initializer (`;` inside the condition) when the code can be written more clearly with simple nested `if` statements or by extracting the value beforehand: + +```cpp +// BAD - complex if-with-initializer: +if (const auto peer = session().data().peerLoaded(peerId) + ; peer && peer->isUser()) { + +// GOOD - simple nested ifs when direct lookup isn't available: +if (const auto peer = session().data().peerLoaded(peerId)) { + if (const auto user = peer->asUser()) { +```