qt-cpp-review

What this skill does

# Qt Code Review

A structured, read-only code review skill for Qt6 C++ code that
combines deterministic linting with parallel agent-driven deep
analysis across six focused domains.

## When to use this skill

- When the user mentions review-related tasks: "review", "check",
  "audit", "look over", "code review", "sanity check"
- Suggest running this skill **before committing** code
- When the user asks to validate Qt6 C++ code quality

## Arguments

- `/qt-cpp-review` — review using universal Qt6 C++ rules only
- `/qt-cpp-review framework` — also apply Qt framework/module
  development rules (BC, exports, d-pointers, qdoc, QML
  versioning)

## Framework mode detection

If `$ARGUMENTS` contains "framework", enable framework mode.

If the argument is not passed, auto-detect by scanning the first
few files in scope for framework signals. If **two or more** of
the following are found, suggest to the user:
"This looks like Qt framework/module code. Run
`/qt-cpp-review framework` to also apply framework-specific
rules (BC, exports, qdoc, QML versioning)?"

**Framework signals** (any two = likely framework code):
- `QT_BEGIN_NAMESPACE` / `QT_END_NAMESPACE`
- `Q_CORE_EXPORT`, `Q_GUI_EXPORT`, `Q_WIDGETS_EXPORT`, or any
  `Q_*_EXPORT` macro
- `#include <QtModule/private/*_p.h>` (private headers)
- `Q_DECLARE_PRIVATE`, `Q_D()`, `Q_Q()`
- `qt_internal_add_module` or `qt_add_module` in CMakeLists.txt
- `sync.profile` or `.qmake.conf` in the repository root

Do **not** auto-enable framework mode — only suggest it. Let the
user confirm.

When framework mode is enabled:
1. Pass `--framework` to the linter (if supported)
2. Load `references/qt-framework-checklist.md` alongside the
   universal checklist
3. Include framework rules in each agent's mission context

## Scope detection

Detect the user's intended scope from their language:

### Diff/commit scope (narrow)
Triggered by language like: "this commit", "these changes",
"the diff", "what I changed", "my changes", "staged changes",
"outstanding changes", "before I commit"

**Action**: Run `git diff` (unstaged) and `git diff --cached`
(staged) to obtain the changeset. If the user says "this commit",
use `git diff HEAD~1..HEAD`. Review only the changed lines plus
sufficient surrounding context (±50 lines) for understanding.
Only report issues found in the changed lines — do not report
issues in unchanged surrounding context.

### Codebase scope (wide)
Triggered by language like: "review the codebase", "audit the
project", "check the repository", "review src/", or when a specific
file/directory path is given without commit language.

**Action**: Glob for `*.cpp`, `*.h`, `*.hpp` files in the
specified scope. Review all matched files.

## Execution order

The review proceeds in three phases. **Never skip a phase.**

### Phase 1: Deterministic linting (scripts)

Run the unified Python linter against the target files. Requires
Python 3.6+ (no external dependencies). If Python is not
available, warn the user and skip to Phase 2.

```bash
python3 references/lint-scripts/qt_review_lint.py <files...>
# If python3 is not found, fall back to:
python references/lint-scripts/qt_review_lint.py <files...>
```

This single-pass scanner encodes all mechanically-checkable rules
from the Qt review guidelines. It reads each file once and
evaluates all rules per line. Output is deterministic and
repeatable. The linter is authoritative — do not second-guess
its output.

Collect all output before proceeding to Phase 2.

**Rule categories** (60+ checks):
- **INC** (Includes) — ordering, qglobal.h, qNN duplication
- **DEP** (Deprecated) — obsolete Qt/std class usage
- **PAT** (Patterns) — anti-patterns (min/max, std::optional,
  NRVO, COW detach, etc.)
- **MDL** (Model) — QAbstractItemModel contract (begin/end
  balance, dataChanged roles, flags, default: in data())
- **ERR** (Error Handling) — QFile::open, QJsonDocument::isNull,
  QNetworkReply::error, SSL, timeouts, arg() mismatch
- **LCY** (Lifecycle) — deleteLater, Q_ASSERT side effects,
  null guards, unbounded containers, qDeleteAll depth
- **API** (Naming) — get-prefix, enum hygiene, QList<QString>
- **HDR/TMO/CND/VAL/TRN** — headers, timeouts, conditionals,
  value classes, ternary operator

### Phase 2: Agent-driven deep analysis (6 parallel agents)

Launch six focused review agents in parallel. Name each agent
descriptively when launching (e.g. "Agent 1: Model Contracts")
to provide progress visibility. Each agent has a tight scope
and a specific checklist. Agents are READ-ONLY — they must
never edit or write files.

**Tool-agnostic agent contract**: Each agent described below is
a self-contained review mission. In Claude Code, launch them as
general-purpose subagents. In other tools, implement each as
whatever subprocess, prompt chain, or analysis pass the tool
supports. The key requirement is that each agent:
- Has read access to all source files in scope
- Can search/grep the codebase to trace symbols
- Reports findings in the structured format below
- Applies confidence thresholds: >80 = confirmed finding,
  60–79 = investigation target (max 10 total across all
  agents), <60 = suppress
- Does NOT duplicate findings from Phase 1 lint output
  (pass lint output as context to each agent)

See **Agent missions** below for the six agents.

### Phase 3: Consolidation and reporting

Merge lint script output and all agent findings. Deduplicate
(same file+line+issue = one finding). Apply confidence scoring.
Format the final report using the output format below.

## Agent missions

Launch all six agents in parallel. Pass each agent:
1. The list of files in scope
2. The Phase 1 lint output (so they skip already-flagged issues)
3. Their specific mission below

Each agent should read all files in scope, then focus on its
assigned categories.

---

### Agent 1: Model Contracts

**Scope**: QAbstractItemModel signal protocol, role system,
index validity, proxy model correctness.

**Check for**:
- `beginInsertRows`/`endInsertRows` balance — every structural
  model change (add/remove/move) must use the correct begin/end
  pairs. `layoutChanged` is NOT a substitute for insert/remove.
- `roleNames()` returning roles that `data()` does not handle
  (missing switch cases, fall-through to default)
- `dataChanged` emitted with empty roles vector (forces full
  refresh instead of targeted update)
- `beginRemoveRows` called with `first > last` (edge case when
  container is empty — QAIM contract violation)
- `flags()` returning inappropriate flags (e.g. `ItemIsEditable`
  for non-editable items)
- `setData()` returning true without emitting `dataChanged`
- Proxy models accessing source model internals instead of going
  through `data()`/`index()` API
- Filter/proxy models using source-model indices to index into
  filtered containers (wrong index space)

**References**: `references/qt-review-checklist.md` § Model
Contracts

---

### Agent 2: Ownership & Lifecycle

**Scope**: Memory ownership, parent-child, resource cleanup,
Rule of Five, RAII correctness.

**Check for**:
- Structs/classes with raw pointers where `new` is visible and
  no corresponding `delete`/`deleteLater`/smart-pointer wrapping
  exists (Rule of Five violation)
- Missing `deleteLater()` on QNetworkReply in finished handlers
- `Q_ASSERT` wrapping side-effectful expressions (compiled out
  in release builds — the side effect disappears)
- `Q_ASSERT` as the sole null guard (crashes in release)
- Polymorphic QObject subclasses missing `Q_DISABLE_COPY_MOVE`
- Polymorphic classes missing virtual destructor
- QTimer/QObject created with `new` but no parent and no other
  lifecycle management (scope, smart pointer, explicit delete)
- `QObject::connect()` called with potentially null
  sender/receiver outside a null guard (runtime warning)
- `m_recentlyAccessed`-style tracking lists that maintain
  pointers to objects that may be deleted elsewhere (dangling)
- Unbounded container growth (append without cap or trim)
- Destructor not cl