Code Review Component Skill
When to use this skill
When you need to perform a comprehensive audit of one or more components. This skill is for deep dives to identify technical debt, modernization opportunities, documentation gaps, and convention violations across an entire component.
Use this skill:
- Before major refactoring work.
- When onboarding to understand component quality.
- Periodically to assess technical debt.
- When preparing a component for production hardening.
How to use this skill
- Identify the component(s) to review.
- Follow the detailed instructions to audit each category.
- Generate findings categorised by severity.
- Add findings to the sprint backlog for remediation.
Detailed instructions
Step 1: Identify scope
Determine which component(s) to review. Components are located under
projects/ores.COMPONENT.
For each component, identify all parts and facets:
| Part | Contents |
|---|---|
include/ores.COMPONENT/ |
Header files organised by facet |
src/ |
Implementation files organised by facet |
tests/ |
Catch2 unit tests |
modeling/ |
PlantUML diagrams |
Reference: component-creator skill for expected structure.
Step 2: Component structure audit
Verify the component follows the expected structure.
Folder Structure
| Check | Description |
|---|---|
| Parts present | Component has include, src, tests, modeling folders. |
| Include nesting | include has single folder named ores.COMPONENT. |
| Facet consistency | Same facets exist in both include and src. |
CMake Configuration
| Check | Description |
|---|---|
| Component CMakeLists.txt | Present at projects/ores.COMPONENT/CMakeLists.txt. |
| Src CMakeLists.txt | Present at projects/ores.COMPONENT/src/CMakeLists.txt. |
| Tests CMakeLists.txt | Present at projects/ores.COMPONENT/tests/CMakeLists.txt. |
| Modeling CMakeLists.txt | Present at projects/ores.COMPONENT/modeling/CMakeLists.txt. |
| Dependencies correct | All required dependencies are linked. |
Documentation
| Check | Description |
|---|---|
| Namespace docs | Each facet folder has namespace documentation for Doxygen. |
| README | Component has README or equivalent documentation. |
Step 3: Headers & licensing audit
Review all .hpp and .cpp files in the component.
| Check | Description |
|---|---|
| Copyright header | Every file has the standard GPL copyright header. |
| Mode line | Files have Emacs mode line: /* -*- mode: c++; ... */ |
| Include guards | Headers use #ifndef ORES_COMPONENT_FACET_FILE_HPP pattern. |
Step 4: Code style audit
Naming Conventions
| Check | Description |
|---|---|
| snake_case (non-Qt) | All classes, methods, variables use snake_case. |
| CamelCase (Qt) | ores.qt uses CamelCase per Qt conventions. |
| File naming | Files match class names exactly. |
| Namespace naming | Namespaces follow ores::component::facet pattern. |
Include Order
| Check | Description |
|---|---|
| Standard library first | <algorithm>, <string>, etc. |
| Third-party second | <boost/...>, <rfl/...>, etc. |
| Project headers last | "ores.component/..." |
Comment Quality
| Check | Description |
|---|---|
| No stale comments | Remove historical, deprecated, or outdated TODO comments. |
| Useful comments only | Comments explain "why", not "what". |
| No commented-out code | Remove dead code, use version control instead. |
| Doxygen completeness | All public classes and methods have @brief documentation. |
Step 5: Platform isolation audit
| Check | Description |
|---|---|
| Platform macros | _WIN32, __linux__, __APPLE__ only in ores.platform. |
| Platform headers | <windows.h>, <unistd.h> only in ores.platform. |
| Abstraction usage | Other components use ores.platform abstractions. |
Step 6: Code locality audit
| Check | Description |
|---|---|
| Utility placement | Generic helpers belong in ores.utility or ores.platform. |
| Duplicate detection | Search for similar code patterns across components. |
| Function extraction | Identify repetitive code that should be factored out. |
| Single responsibility | Classes and functions do one thing well. |
Step 7: C++23 modernization audit
Identify opportunities to modernize code to C++23 idioms.
| Pattern | Modern Alternative |
|---|---|
| Raw pointers for ownership | std::unique_ptr, std::shared_ptr |
| Exceptions for expected errors | std::expected |
| Manual loops | Ranges and views |
typename template constraints |
Concepts |
| Stream concatenation | std::format |
NULL |
nullptr |
typedef |
using |
| C-style casts | static_cast, dynamic_cast |
| Manual RAII | Standard RAII wrappers |
Step 8: Domain types audit
For each domain type in the component, verify completeness per domain-type-creator skill.
Core Definition
| Check | Description |
|---|---|
| Struct with public fields | Domain type is a struct final with public fields. |
| Doxygen documentation | Struct and all fields have @brief comments. |
| Appropriate types | UUIDs, timestamps, strings use correct C++ types. |
Services
| Check | Description |
|---|---|
| JSON I/O | _json_io.hpp/.cpp exists and uses reflect-cpp. |
| Table I/O | _table_io.hpp/.cpp exists and uses libfort. |
| Generator | _generator.hpp/.cpp exists in generators facet. |
Temporality Patterns
| Check | Description |
|---|---|
| Audit fields | Domain type has recorded_by and recorded_at fields. |
| Entity mapping | Database entity has corresponding temporal columns. |
| Mapper correctness | Mapper converts temporal fields correctly. |
| Repository queries | Repository has read_at_timepoint() for historical queries. |
| Latest queries | Repository has read_latest() for current state. |
Repository
| Check | Description |
|---|---|
| Entity defined | _entity.hpp with sqlgen annotations. |
| Mapper defined | _mapper.hpp/.cpp with to_domain() and from_domain(). |
| Repository defined | _repository.hpp/.cpp with CRUD operations. |
| SQL method | Repository has sql() method for table creation. |
Step 9: Protocol & messaging audit
For components with messaging, verify per binary-protocol-developer skill.
Message Types
| Check | Description |
|---|---|
| Enum registration | All messages registered in message_types.hpp. |
| Correct range | Messages use component's allocated hex range. |
| Naming conventions | Requests end with _request, responses with _response. |
Protocol Structs
| Check | Description |
|---|---|
| Struct pattern | Request/response structs are struct final. |
| Serialization | serialize() documents wire format in Doxygen. |
| Deserialization | deserialize() returns std::expected. |
| Stream operator | operator<< implemented for debugging. |
Message Traits
| Check | Description |
|---|---|
| Traits defined | message_traits specialization for each request type. |
| Correct types | request_type, response_type, request_message_type defined. |
Handler
| Check | Description |
|---|---|
| Handler method | Each message type has a handle_* method. |
| Switch case | Handler is registered in message dispatch switch. |
| Authorization | Server-side authorization checks in place. |
Step 10: PlantUML diagram audit
Verify diagrams are up-to-date per plantuml-class-modeler skill.
| Check | Description |
|---|---|
| Diagram exists | modeling/ores.COMPONENT.puml exists. |
| All classes present | Diagram includes all domain types, services, repositories. |
| Relationships correct | Inheritance, composition, aggregation arrows are accurate. |
| Styling correct | Classes use correct colours and stereotypes. |
| Compiles | Diagram compiles without PlantUML errors. |
| Test suites | Test classes are included in the diagram. |
Step 11: Unit test audit
Verify test coverage per unit-test-writer skill.
Coverage
| Check | Description |
|---|---|
| Domain tests | Each domain type has domain_*_tests.cpp. |
| Repository tests | Each repository has repository_*_tests.cpp. |
| Service tests | Each service has service_*_tests.cpp. |
| Messaging tests | Protocol structs have serialization tests. |
Quality
| Check | Description |
|---|---|
| Naming convention | Tests follow layer_class_tests.cpp pattern. |
| No SECTIONs | Tests use separate TEST_CASE instead of SECTION. |
| Logging | Tests use BOOST_LOG_SEV for output. |
| Generators used | Tests use generators for synthetic data. |
| Database helper | Repository tests use database_helper. |
Step 12: Error handling audit
| Check | Description |
|---|---|
std::expected usage |
Operations that can fail return std::expected. |
| Error propagation | Errors are propagated, not silently ignored. |
| Meaningful messages | Error messages are descriptive and actionable. |
| Logging | Errors are logged at appropriate severity. |
| No exceptions for control flow | Exceptions reserved for truly exceptional cases. |
Step 13: Security audit
| Check | Description |
|---|---|
| No hardcoded secrets | No passwords, keys, or tokens in code. |
| Input validation | External input validated before use. |
| Authorization | Server-side checks for all privileged operations. |
| No client-provided identity | Identity from session, not request fields. |
| Secure hashing | Passwords use proper hashing (bcrypt, argon2). |
Step 14: Generate findings
For each issue found, categorise by severity:
Critical (Must Fix)
- Security vulnerabilities.
- Data corruption risks.
- Crashes or undefined behaviour.
- Missing critical functionality.
Important (Should Fix)
- Architecture problems.
- Missing services (JSON I/O, tests, etc.).
- Convention violations.
- Technical debt that impedes development.
- Outdated patterns that should be modernised.
Minor (Nice to Have)
- Code style preferences.
- Documentation improvements.
- Optimization opportunities.
- Refactoring suggestions.
Step 15: Document findings
For each issue provide:
- File line - Exact location (or file/folder for structural issues).
- What's wrong - Clear description of the problem.
- Why it matters - Impact of not fixing.
- How to fix - Suggested resolution.
- Effort estimate - Small/Medium/Large.
Step 16: Add to sprint backlog
Create stories in the sprint backlog per agile-product-owner skill.
Group issues by theme:
- Structure & CMake - Component structure issues.
- Headers & Style - Copyright, naming, formatting.
- Domain Completeness - Missing services for domain types.
- Test Coverage - Missing or incomplete tests.
- Modernization - C++23 upgrade opportunities.
- Documentation - Missing Doxygen, diagrams.
- Security - Security-related findings.
Example story format:
**** STARTED [component] Complete domain type services :code: Several domain types are missing standard services. ***** Tasks - [ ] Add JSON I/O for foo domain type - [ ] Add table I/O for foo domain type - [ ] Add generator for foo domain type - [ ] Add temporality fields to bar domain type ***** Notes Component review performed on YYYY-MM-DD.
Step 17: Summary report
Provide an overall assessment:
Component Health Score
Rate the component on each dimension (1-5):
| Dimension | Score | Notes |
|---|---|---|
| Structure | ||
| Code Style | ||
| Domain Completeness | ||
| Test Coverage | ||
| Documentation | ||
| Security | ||
| Modernization |
Priority Recommendations
List top 3-5 items to address first, considering:
- Critical issues (must fix immediately).
- High-impact improvements (good ROI).
- Quick wins (easy fixes with visible benefit).
Technical Debt Summary
Summarize the overall technical debt level:
- Low - Component is well-maintained, minor issues only.
- Medium - Some gaps, but fundamentally sound.
- High - Significant work needed before production-ready.