Qt Cpp Review

# 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 cleaning up owned children recursively
- Abstract interfaces with no implementations beyond one class
  (YAGNI violation — codebase scope only)

**References**: `references/qt-review-checklist.md` § Ownership
& Lifecycle, § Polymorphic Classes, § RAII Classes

---

### Agent 3: Thread Safety

**Scope**: Cross-thread QObject access, mutex consistency,
signal emission from worker threads.

**Check for**:
- QObject member variables written from `QtConcurrent::run()`
  or `QThread` worker without synchronization (mutex, atomic,
  queued connection, or other thread-safe primitive)
- Signals emitted from worker threads connected with
  `Qt::DirectConnection` (or explicit non-queued connections)
  to main-thread receivers
- Model mutations (`addNote`, `removeRows`, etc.) from
  background threads
- Shared containers (`QList`, `QHash`) modified from multiple
  threads without consistent synchronization
- Non-atomic increment/decrement of shared counters
  (`m_operationCount++` from multiple threads)
- QTimer or other QObject operations from non-owner thread

**References**: `references/qt-review-checklist.md` § Thread
Safety

---

### Agent 4: API, Naming & C++ Correctness

**Scope**: Qt naming conventions, const-correctness, move
semantics, enum hygiene, noexcept correctness.

**Check for**:
- `get`-prefix on mere getters (Qt reserves `get` for user
  interaction or out-parameter decomposition)
- Non-const getter methods (especially Q_PROPERTY READ
  accessors — UB via meta-object system)
- Missing `std::forward<T>()` on forwarding/universal references
- `return std::move(localVar)` preventing NRVO
- `const` local variable preventing implicit move on return
  (e.g. `const QJsonDocument doc(...); return doc;` forces copy)
- `const` method returning mutable pointer through raw pointer
  indirection (`findById() const` returning `T*` lets callers
  mutate via a const accessor — const doesn't propagate through
  raw pointers)
- `noexcept` on functions containing `Q_ASSERT` (incompatible —
  Q_ASSERT may throw for testing, noexcept terminates)
- Unscoped enums without explicit underlying type
- Missing trailing comma on last enumerator
- `switch` over enum with `default:` label (suppresses -Wswitch)
- `QList<QString>` instead of `QStringList`
- Missing `const` on methods that don't modify state
- Case-sensitive string comparison for user-facing sort
- Duplicated validation logic across classes
- `const QMetaObject::Connection` preventing handle cleanup

**References**: `references/qt-review-checklist.md` § API &
Naming, § Enums, § Methods, § Move Semantics, § Operators

---

### Agent 5: Error Handling & Validation

**Scope**: Missing error checks, input validation, security.

**Check for**:
- `QFile::open()` return value ignored
- `QJsonDocument::fromJson()` result not checked for
  `isNull()`/`isObject()` before use
- `QNetworkReply::error()` not checked before `readAll()`
- XML writer `hasError()` not checked after writing
- Hardcoded `http://` instead of `https://` in URLs
- No SSL error handling (`QNetworkAccessManager::sslErrors`)
- No timeout on network requests (`setTransferTimeout`)
- Negative values accepted where only positive are valid
  (e.g. timer intervals, font sizes)
- No schema/version validation on imported data
- No input length validation on imported/downloaded data
  (unbounded strings from untrusted sources)
- `QString::arg()` with wrong placeholder count
- `saveToFile()` returning true regardless of I/O errors
- Inconsistent error reporting patterns across methods

**References**: `references/qt-review-checklist.md` § Error
Handling & Validation

---

### Agent 6: Performance & Code Quality

**Scope**: Performance anti-patterns, dead code, unnecessary
copies, code smells.

**Check for**:
- `QRegularExpression` constructed inside a loop (expensive
  compilation on every iteration)
- `roleNames()` rebuilding QHash on every call (should cache)
- Non-const range-for over COW-shared QList/QHash triggering
  unnecessary detach/deep-copy
- Non-const `operator[]` on shared QHash (triggers detach) —
  use `.value()` for reads
- Expensive operation before cheap early-exit check (wasted
  allocation)
- Dead/unreachable code (functions never called, branches
  that are always true/false given preconditions)
- Magic numbers without named constants
- God classes violating Single Responsibility
- Copy-pasted validation/logic across classes
- Stale member caches not invalidated on model changes
  (e.g. search cache surviving data edits)
- `QMap`/`QHash` iteration order nondeterminism when selecting
  a "best" or "first" entry (`.first()` changes if keys are
  added; use deterministic tie-breaking)
- `QMap` for small fixed-size constant data (use array/switch)
- Returning QList/container by value from frequently-called
  methods (implicit deep copy on every call — return const ref
  or cache)
- Member variables maintained (appended, capped) but never
  read by any method (dead state — wasted CPU and memory)
- Missing re-entrancy guard on methods that emit signals
  which could trigger re-entry
- Setter silently resetting unrelated state without signal
- Early return skipping status/signal updates

**References**: `references/qt-review-checklist.md` §
Performance & Code Quality

---

## Confidence scoring guidelines

| Confidence | Meaning | Action |
|------------|---------|--------|
| 90–100 | Certain: direct rule violation with full symbol trace | Report as finding |
| 80–89 | High: rule violation confirmed but edge case possible | Report as finding |
| 60–79 | Medium: likely issue but cannot fully verify | Report as investigation target |
| <60 | Low: suspicion only | Suppress entirely |

**Investigation targets** are findings the agent believes are real
but cannot fully verify — e.g. noexcept correctness requiring
whole-program analysis, dead code that may have callers outside
scope, or design-intent judgments like virtual access levels.
These are presented in a separate section for human verification.
Maximum 10 investigation targets per report, prioritized by
confidence within the 60–79 band.

## Output format

Present the final report as follows. Use exactly this structure.

```
## Qt Code Review Report

**Scope**: [diff: `git diff HEAD~1..HEAD` | files: <paths>]
**Files reviewed**: N
**Issues found**: N (M from lint, K from deep analysis)

---

### Lint findings

For each lint finding:

#### [L-NNN] <Short title>
- **File**: `path/to/file.cpp:42`
- **Rule**: <rule ID from checklist>
- **Finding**: <what the script detected>
- **Mitigation**: <what to do, in prose — no code patches>

---

### Deep analysis findings

For each agent finding:

#### [D-NNN] <Short title>
- **File**: `path/to/file.cpp:42`
- **Category**: <agent name: Model Contracts | Ownership &
  Lifecycle | Thread Safety | API & C++ Correctness | Error
  Handling | Performance & Quality>
- **Confidence**: NN/100
- **Finding**: <description of the issue>
- **Trace**: <how the issue was confirmed — which symbols were
  followed, what was checked>
- **Mitigation**: <what to do, in prose — no code patches>

---

### Investigation targets (human verification needed)

Findings the agent identified but could not fully verify.
Maximum 10, sorted by confidence. These require human judgment.

For each investigation target:

#### [I-NNN] <Short title>
- **File**: `path/to/file.cpp:42`
- **Category**: <agent name>
- **Confidence**: NN/100
- **Finding**: <what the agent suspects>
- **Unverified because**: <what the agent could not confirm —
  e.g. "cannot trace all callees for throw potential",
  "only one implementation visible in scope">
- **How to verify**: <specific action for the reviewer>

---

### Summary

| Category | Lint | Deep | Investigate | Total |
|----------|------|------|-------------|-------|
| ...      | N    | N    | N           | N     |
| **Total**| **M**| **K**| **I**       | **N** |

Findings below confidence 60 are suppressed entirely.
```

## References

The following reference files contain detailed checklists
extracted from the Qt wiki "Things To Look Out For In Reviews":

- `references/qt-review-checklist.md` — Universal Qt6 C++ review
  rules (always loaded)
- `references/qt-framework-checklist.md` — Qt framework/module
  development rules (loaded only in framework mode)
- `references/qt-deprecated-classes.md` — Classes and patterns
  that should no longer be used in Qt implementation
- `references/lint-scripts/qt_review_lint.py` — Single-pass
  Python linter (runs all 60+ checks in <1s)