Code Review

Code Review

Code Quality Checklist

Based on the project's review standard. Each item is tagged with a review role.

1. Functional Correctness

• [Staff Eng] All required features from the issue/task are implemented
• [Staff Eng] No uncaught exceptions or warnings during execution
• [Staff Eng] Exception handling catches and reports all error conditions
• [Staff Eng] No logic errors that pass CI but would fail in production (look for deep bugs that automated tests cannot catch)
• [Staff Eng] No completeness gaps — no missing expected behaviors or edge cases; actively flag any that are found

2. Code Design

• [Staff Eng] No duplicate functionality already present in the codebase (check quark/utils/tools.py and BaseApkinfo before adding helpers)
• [Staff Eng] Each function does exactly one thing
• [Lang] Function signatures have correct input/output types (use type annotations consistent with existing code)
• [Eng Manager] Architecture, data flow, and boundaries are explicitly defined — all hidden assumptions must be surfaced
• [Eng Manager] Edge cases and boundary conditions are clearly specified and enforced
• [Lang] Follows backend & API design patterns (REST, database, cache layer conventions — no ad-hoc assembly)
• [QA] Run SonarLint and resolve all detected issues

3. Code Style

• [Lang] Variables and classes use noun/noun-phrase names; functions start with a verb
• [Lang] Format with black --line-length 79 quark/ before committing — automate: bash .claude/skills/code-review/scripts/check_style.sh <files>
• [Lang] Run pylint on only the files changed in this PR and resolve all reported issues in those files (do not fix pre-existing issues in untouched files — it creates noise and defocuses the PR) — automate: bash .claude/skills/code-review/scripts/check_style.sh <files>
• [Lang] Run spell-checker on code — automate: bash .claude/skills/code-review/scripts/check_spelling.sh <files>
• [Lang] No lines exceed 79 characters
• [Lang] All functions and variables have complete, correct type hints — automate: bash .claude/skills/code-review/scripts/check_types.sh <files>

4. Testing & Regression Prevention

• [QA] New/modified functions are covered by tests
• [QA] Tests validate both expected and unexpected inputs
• [QA] Every bug fix is accompanied by a regression test that would have caught it — the same bug must never recur
• [QA] Aim for 100% coverage on changed code; replace any yolo/vibe-coded patches with proper tests — automate: bash .claude/skills/code-review/scripts/check_coverage.sh <module>

5. Security

• [Lang] Run language-level security scan (e.g., bandit for Python) and confirm no vulnerabilities — automate: bash .claude/skills/code-review/scripts/check_security.sh <files>
• [CSO] Only raise a security warning when confidence is ≥ 8/10 and independently verified — zero false positives
• [CSO] Every identified vulnerability (OWASP Top 10 or STRIDE) includes a concrete exploit scenario explaining how an attacker would actually abuse it in a real environment

6. Debugging Standards

• [Debugger] No fix is applied without a systematic root-cause investigation: trace data flow and test hypotheses before touching code
• [Debugger] If three consecutive fix attempts fail, stop and escalate rather than continuing to patch blindly

7. PR Preparation

• [Staff Eng] Commit message follows conventional commit format and passes sentence-quality checks — automate: bash .claude/skills/code-review/scripts/check_commit.sh
• [Staff Eng] PR title and description reference the relevant issue
• [QA] Smoke test counts updated if behavior counts changed (see @.claude/skills/workflows.md → Updating Smoke Test Counts)
• [Lang] If any quark.script public API changed: update docs/source/quark_script.rst and compile docs locally to confirm no errors

Review Automation Scripts

Bundled scripts in .claude/skills/code-review/scripts/ automate the checklist items marked with automate above.

Prerequisites: pip install black pylint mypy codespell bandit pytest-cov (or use uvx for one-off runs).

Review workflow

Phase 1 — Understand

1. Read the changed files and their surrounding context (callers, callees, related modules)
2. Understand the feature's intent from the linked issue, branch name, and commit messages. Ask questions if unclear.
3. Identify how the changes fit into the existing architecture (e.g., does a new method in TableObject affect all callers in PyEval?)
4. Check whether the change duplicates, conflicts with, or misuses existing patterns in the codebase

Phase 2 — Plan

5. Produce a test plan blueprint based on the Code Quality Checklist:
   • List every test item (automated scripts and manual checks), with a brief description of what each verifies
   • Present the blueprint to the user for review
   • Wait for user confirmation before executing any tests

Phase 3 — Automated checks

6. Run all automated checks: bash .claude/skills/code-review/scripts/run_all.sh <changed-files-or-dir>
7. Fix failures and re-run until all checks pass. If a failure is due to a missing dependency, install and retry up to 3 attempts; after that, skip the check and record it for the final summary.

Phase 4 — Manual review

9. Review checklist items without an automate marker — these require manual judgment, informed by the context gathered in Phase 1

Phase 5 — Finalize

10. Mark the review as complete only when both automated and manual checks pass
11. In the final summary, include a Skipped Tests section listing any checks skipped due to unresolvable tooling errors, with error details

Summary Format

Present the review result as a single consolidated table. Each row is one check (automated or manual) with its result and issue description.

Only show issues. Do not include a "What's Good" section. End by asking the user if they want any issues fixed.

See examples/summary.md for the expected output format.