# Aslanxie Rtp LLM Xpu Code Review > --- - Skill: `tomevault-io/aslanxie-rtp-llm-xpu-code-review` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/aslanxie-rtp-llm-xpu-code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/aslanxie-rtp-llm-xpu-code-review/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: AI & ML - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/aslanxie-rtp-llm-xpu-code-review --- --- name: code-review description: 'Perform AI code review on local changes or a GitHub PR, duplicating the LLLLKKKK/rtp-llm review bot format (P0-P3 ratings, checklists, strengths). Use when: pre-reviewing before PR submission, reviewing a PR diff, or self-checking staged changes.' --- # AI Code Review Bot Perform structured code review that matches the rtp-llm upstream review system (LLLLKKKK's AI-assisted review bot). ## Inputs - **Target**: one of: - `local` — review unstaged/staged git diff in the workspace (default) - `branch ` — review diff of branch vs main - `pr ` — fetch and review a GitHub PR diff - `files ...` — review specific files - **Scope** (optional): `full` (default) or `quick` (skip checklists, issues only) - **Language** (optional): `zh` (Chinese, default — matches upstream) or `en` (English) ## When to Use - Before submitting a PR to alibaba/rtp-llm — catch issues the review bot will flag - After pushing fixes to check if review round is clean - Self-review of any code change ## Procedure ### 1. Gather the Diff Based on the target: - **local**: Run `git diff HEAD` (or `git diff --cached` for staged only) - **branch**: Run `git diff main... -- . ':!internal_source'` - **pr**: Fetch diff via `curl -sH "Accept: application/vnd.github.v3.diff" https://api.github.com/repos/{owner}/{repo}/pulls/{number}` - **files**: Read the specified files and compare against main Filter out: - Files under `internal_source/` (internal overlay, not reviewed) - Binary files, generated files, lock files (unless lock file is the point of the PR) - Files with only whitespace/comment changes (note them as P3 at most) ### 2. Analyze Each Changed File For each file in the diff, analyze against the three checklists below. Track: - **Issues**: problems that should be raised as P0/P1/P2/P3 - **Checklist violations**: checklist items that fail (linked to issues or checklist-only) - **Strengths**: positive patterns worth calling out ### 3. Classify Issues by Priority | Level | Name | Criteria | Effect | |-------|------|----------|--------| | **P0** | Critical | Security vulnerability, credential/URL leak, data loss, crash in production path | BLOCKING — must fix before merge | | **P1** | Blocking | Correctness bug, breaking API change, missing referenced target, test infrastructure that silently skips, state/resource leak | BLOCKING — must fix before merge | | **P2** | Non-blocking Suggestion | DRY violation, missing tests for new logic, hardcoded paths, loose test tolerance, type discipline issues, missing error handling | Non-blocking — should fix but won't block merge | | **P3** | Minor Nit | Trailing whitespace, import order, naming conventions, docstring style, dead code comments | Non-blocking — nice to fix | **Status decision:** - If **any P0 or P1** exists → Status: **BLOCKING** - Otherwise → Status: **LGTM** + include "lgtm ready to ci" ### 4. Output the Review Use this exact format (matching upstream bot output): ``` ## AI Code Review - PR # Status: Summary: P0/ · P1/ · P2/ · P3/ [if LGTM] lgtm ready to ci ### Blocking Issues #### P0 • @ `` ◦ 建议： #### P1 • @ `` ◦ 建议： ### Non-blocking Suggestions #### P2 • @ `` ◦ 建议： #### P3 • @ `` ◦ 建议： ### Checklist Violations ( fail / total) General Principles Checklist • [6.1] — → issue `` OR • [6.1] — → checklist-only RTP-LLM Checklist • [] — → issue `` Python Static-First Checklist • [P.] — → issue `` ### Strengths • • ``` **Formatting rules:** - If a P-level section has 0 items, omit that section entirely - Each issue must reference a specific file:line - Each suggestion (建议) must be concrete and actionable — no vague "consider improving" - Checklist items link to issues with `→ issue ''` or note `→ checklist-only` with rationale - Strengths should acknowledge genuinely good patterns (not filler) - Default language is Chinese for issue titles and suggestions (matching upstream) --- ## Checklists ### General Principles Checklist [6.1] Run ALL items against every PR. Mark pass/fail for each. Only report failures in output. #### Software Engineering - [ ] **SRP**: Each class/function has a single responsibility - [ ] **OCP**: Extension via new code, not modifying existing stable interfaces - [ ] **LSP**: Subclass/override preserves base class contract (return types, exceptions, invariants) - [ ] **ISP**: Interfaces are cohesive; callers don't depend on methods they don't use - [ ] **DIP**: High-level policy doesn't depend on unnecessary concrete details - [ ] **DRY**: Repeated non-trivial logic is extracted or explicitly reused (>5 lines duplicated = violation) - [ ] **YAGNI**: No speculative features or premature abstractions added #### Architecture - [ ] **Error semantics**: fail-fast / retry / fallback / silent behavior is explicit, not accidental - [ ] **Compatibility**: Public API / persisted data / config / env migration is safe - New enum values append-only (no renumber) - Default value changes evaluated for existing user impact - Return type changes have compatibility handling - New config fields propagated to all model variants - [ ] **Observability**: Logs / metrics / timeouts are actionable, not noise - New enum/status values have corresponding string mappings - Error logs include enough context to diagnose - No swallowed exceptions in production paths - [ ] **Rollback path**: Risky behavior has an operational rollback mechanism (feature flag, config switch) - [ ] **Dependency direction**: No circular deps or cross-layer surprises - [ ] **Concurrency**: Shared mutable state protected; async cleanup via try/finally - ContextVar.set() has matching .reset() in finally block - No race conditions in shared data structures - [ ] **State invariants**: Create/update/fail/retry/rollback paths are valid #### Tests - [ ] **Unit tests**: New logic has focused unit tests + related integration/smoke tests - [ ] **Boundary cases**: Empty, single element, max value, off-by-one covered - [ ] **Cross-platform**: Distributed / cross-platform changes have corresponding coverage - [ ] **Test infrastructure**: Tests actually run (not commented out in BUILD, not silently skipped) - unittest.main() at end of file, not mid-file - BUILD targets enabled (not commented out) — use tags=["manual"] if hardware-specific - Test tolerance meaningful (atol << output magnitude) - Self-checks that trivial inputs don't produce trivially-passing results #### Quality - [ ] **No unrelated formatting**: Logic changes don't include unrelated whitespace/import changes - [ ] **PR description**: Motivation and design explained - [ ] **Commit structure**: Logical commits that aid review and bisection - [ ] **Mega-PR split**: If >500 lines changed, has the PR been split into independent changes? --- ### RTP-LLM Domain Checklist Run for PRs touching rtp-llm-specific code. Skip for pure infra/docs changes. #### [A] Compatibility & Configuration - [ ] Default value changes evaluated for existing user impact - [ ] New model config fields propagated to all model variants - [ ] Bazel `environ` list includes all env vars read by repo rules - [ ] requirements and lock files are in sync (version, hash, Python version match) - [ ] No internal URLs (alibaba-inc.com, artlab, /mnt/nas) in open-source files #### [B] Correctness & Logic - [ ] No logic errors, off-by-one, null/zero check gaps - [ ] No dead code / unreachable branches (commented-out code, impossible conditions) - [ ] Shared object in-place modification uses shallow copy - [ ] Interface return type changes have compatible handling for callers - [ ] Boundary cases: empty input, single element, max value handled - [ ] `x or default` vs `x if x is not None else default` — correct falsy semantics - [ ] select() branches all resolve to existing targets #### [C] API & Protocol - [ ] OpenAI API response format matches spec (streaming chunks, finish_reason placement) - [ ] SSE format: `data: {...}\n\n` with proper termination `data: [DONE]\n\n` - [ ] Content-Type headers correct for response type #### [D] Performance - [ ] No per-forward memory allocation on hot path (can move to __init__) - [ ] No unnecessary tensor copies (use view/reshape when possible) - [ ] Lazy imports for heavy optional dependencies #### [E] Device Integration (CUDA / ROCm / XPU) - [ ] Device-specific code behind proper config guards (--config=xpu, using_xpu select) - [ ] New device paths follow existing registration patterns (same as CUDA/ROCm siblings) - [ ] Dummy/stub targets for anything not yet implemented - [ ] Device capability checks at strategy/executor/router registration time - [ ] No cross-device imports at module level (lazy import within guarded blocks) #### [H] Testing & CI - [ ] Test coverage sufficient: major refactors have equivalence coverage, new features have end-to-end - [ ] CPU-runnable tests exist for platform-agnostic logic (config parsing, weight loading, factory selection) - [ ] Hardware-specific tests use tags=["manual", "<device>"] not commented-out BUILD entries - [ ] Test BUILD targets are registered and enabled (not commented out) #### [I] Code Quality - [ ] Same functionality uses unified helper functions (no copy-paste between sibling classes) - [ ] Pre-commit checks pass (trailing whitespace, isort, black) - [ ] Import order follows project convention --- ### Python Static-First Checklist Run for PRs with Python changes. Skip for pure C++/Bazel/config changes. #### [P.A] Static Structure & Type Discipline - [ ] No getattr/setattr with literal string keys — declare explicit class attributes - [ ] No hasattr for control flow branching (use isinstance or explicit attribute) - [ ] Public APIs don't use **kwargs pass-through without validation - [ ] String dispatch uses Enum or Literal, not raw string comparison (note: existing code may use strings — only flag if introducing NEW string dispatch) #### [P.B] Function Design - [ ] Functions have explicit return types (not implicit None) - [ ] No mutable default arguments (list, dict, set) - [ ] Generator/iterator functions are clearly marked #### [P.F] Language Pitfalls - [ ] No module-level import side effects (unless matching existing pattern in same file) - [ ] No bare `except:` — always catch specific exceptions - [ ] f-string expressions don't have side effects - [ ] `isinstance()` preferred over `type() ==` for type checks #### [P.G] Test Conventions - [ ] Data-driven tests use pytest.mark.parametrize (or note if unittest style matches directory convention) - [ ] Test method names describe the scenario, not just "test_1", "test_2" - [ ] No `print()` debugging left in test code #### [P.H] Type Annotations - [ ] `Any` usage has a comment explaining why - [ ] New public functions/methods have type annotations - [ ] Complex return types are aliased for readability --- ## Special Rules ### Checklist-only vs Issue When a checklist item fails but doesn't warrant its own P-level issue: - Mark as `→ checklist-only` with brief rationale (e.g., "matches existing pattern in same file", "impact too small to justify separate fix") - This happens when: the violation is inherited from existing code, the fix would be a separate refactor PR, or the risk is purely theoretical ### Comparing with Peer Implementations Before flagging a pattern as a violation, check: 1. Do CUDA/ROCm/CPU implementations of the same component use the same pattern? 2. Is this an established convention in the rtp-llm codebase? 3. If yes → note as `checklist-only` ("沿用既有模式，不单独立 issue") ### Scaling Checklist Depth - **Small PRs** (1-3 files, <100 lines): Run all checklists but with ~23-30 total items - **Medium PRs** (4-10 files, 100-500 lines): Full checklists, ~56 total items - **Large PRs** (10+ files, 500+ lines): Full checklists with sub-items expanded, ~93-104 total items - Report total as `(<fail> fail / <total checked> total)` ### Internal URL Detection (always P0) Always scan for these patterns — they are automatic P0 blockers: ``` alibaba-inc.com artlab.alibaba /mnt/nas1/ aliyuncs.com.*key --access-key-id --access-key-secret ``` ### Version Number Checks If PR updates dependency versions: - Verify version is monotonically increasing (not regressing) - Verify lock file hash matches the new wheel/package - Verify Python version in lock file matches target runtime ## Output The complete review in the format specified in Step 4 above. Must include: 1. Status line (LGTM or BLOCKING) 2. Summary counts 3. All P-level issue sections (only non-empty ones) 4. Checklist violations section 5. Strengths section (minimum 2 items) --- > Source: [aslanxie/rtp-llm-xpu](https://github.com/aslanxie/rtp-llm-xpu) — distributed by [TomeVault](https://tomevault.io). <!-- tomevault:4.0:skill_md:2026-06-16 -->