This guide helps reviewers verify that pull requests meet our quality standards. Use this as a systematic checklist when reviewing code to ensure consistency and catch issues early.
Note: This is a living document that will evolve as we learn from reviews. All automated checks (linters, tests, builds) must pass before review begins.
Verify contributors followed these project conventions:
- Branch name follows
{issue-number}-{description}format (see branching.md) - Commit messages use conventional format:
- With issue branch:
{type}: [#{issue}] {description} - Without issue branch:
{type}: {description} - See commit-process.md for details
- With issue branch:
- Pre-commit checks passed (contributor should have run
./scripts/pre-commit.sh)
- DDD layer placement is correct - business logic in domain, orchestration in application, external integrations in infrastructure (see ddd-layer-placement.md)
- Error handling uses explicit enum errors with context, not generic strings (see error-handling.md)
- Errors are actionable - include clear guidance on how to fix the issue
- Module organization follows conventions: public before private, top-down structure (see module-organization.md)
- Code is clean and maintainable - clear naming, minimal complexity, well-structured
- No obvious security vulnerabilities introduced
- New logic has appropriate test coverage (see testing/)
- Tests follow naming conventions (
it_should_...pattern) - Tests are isolated - use temporary resources, don't depend on external state
- Tests are readable - clear intent and easy to understand what's being tested
- Both production and test code meet quality standards (clean, maintainable, sustainable)
- Significant architectural decisions documented as ADRs in
docs/decisions/(see decisions/README.md) - Public APIs have rustdoc comments with clear descriptions
- Complex logic is explained with comments where necessary
- User-facing documentation updated if behavior changes affect users
- Tera templates use correct syntax:
{{ variable }}not{ { variable } }(see templates/tera.md) - Static Ansible playbooks (without
.teraextension) are registered insrc/infrastructure/external_tools/ansible/template/renderer/project_generator.rs
Watch for these common issues that indicate quality problems:
- ❌ File I/O in domain or application layers - Should be in infrastructure
- ❌ Business logic in infrastructure or presentation layers - Should be in domain
- ❌ Presentation layer directly calling infrastructure - Should go through application layer
- ❌ Domain layer depending on infrastructure - Violates dependency flow rules
- ❌ Mixing concerns across layers - Each layer should have clear responsibilities
- ❌ Generic string errors instead of typed enums - Use explicit error types
- ❌ Error messages without context - Include what, where, when, why
- ❌ Error messages without actionable guidance - Tell user how to fix it
- ❌ Lost error chains - Missing source preservation, can't trace root cause
- ❌ Using
anyhowwhere explicit enums would be better - Prefer pattern matching
- ❌ Private items before public items in modules - Public should come first
- ❌ Low-level details before high-level abstractions - Use top-down organization
- ❌ Error types mixed with main implementation logic - Separate concerns
- ❌ Inconsistent naming - Follow Rust conventions and project patterns
- ❌ Tests using
unwrap()without explanation - Use proper error handling - ❌ Tests creating real directories instead of temp directories - Use
TempDir - ❌ Missing tests for new error paths - Error handling should be tested
- ❌ Tests that depend on external state - Tests should be isolated
- ❌ Test code that doesn't meet production quality standards - Both should be clean
Be aware of expected behaviors documented in known-issues.md:
- ✅ SSH host key warnings in E2E test output are normal and expected
- ✅ Red error messages during setup don't always indicate failure
- ❌ New unexpected failures should be investigated and resolved
When providing feedback:
- Point to documentation - Reference specific contributing guides
- Be specific - Link to exact lines and explain the concern clearly
- Suggest alternatives - Provide examples or point to similar code in the codebase
- Explain why - Help contributors understand the reasoning behind standards
- Distinguish severity - Make clear whether something is blocking or a suggestion
Good feedback references our documentation and is constructive:
This error handling doesn't follow our guidelines from error-handling.md. Errors should use explicit enums with context rather than generic strings.
Please see the example in
src/domain/config/error.rswhich shows how to create a typed error enum with proper context fields. This will make the errors more actionable for users and easier to handle in calling code.
Better feedback is specific and actionable:
In
src/application/commands/provision.rsline 42, using.unwrap()will panic if the file doesn't exist. Instead:
- Define an error variant in the command's error enum for file not found
- Use
.map_err()to convert the I/O error to your domain error- Include the file path in the error context
See error-handling.md section 2 for the pattern.
Request Changes - Blocking issues that violate documented standards:
- Architectural violations (wrong DDD layer placement)
- Security vulnerabilities introduced
- Breaking existing functionality
- Missing required tests for new code
- Error handling that doesn't meet standards
- Pre-commit checks not passing
Comment - Suggestions or questions (non-blocking improvements):
- Minor style inconsistencies not caught by linters
- Optional refactoring opportunities
- Questions about approach or implementation
- Nice-to-have documentation improvements
- Suggestions for future enhancements
Approve - All standards met:
- All quality checklist items verified
- No blocking issues found
- Changes are minimal and focused
- Tests pass and provide good coverage
- Documentation is adequate
- Code follows project conventions
This is a living document. As we identify new patterns during reviews, we can:
- Update this guide with new checklist items or red flags
- Update specific contributing guides for detailed guidance on particular topics
- Add to "Quick Red Flags" section for common review issues we encounter
- Document new architectural decisions as ADRs in
docs/decisions/ - Update issue templates if acceptance criteria patterns emerge
If you notice patterns that should be added to this guide, please:
- Create an issue to discuss the addition
- Reference examples from recent PRs that show the pattern
- Propose specific checklist items or red flag entries
- Update relevant contributing guides with detailed guidance
- DDD Layer Placement Guide - Architectural guidance
- Error Handling Guide - Error handling patterns
- Module Organization - Code organization within files
- Branching Conventions - Branch naming rules
- Commit Process - Commit message format
- Testing Guide - Testing conventions and patterns
- Templates Guide - Working with Tera templates
- Known Issues - Expected behaviors and workarounds
- Development Principles - Core principles guiding all development
- Codebase Architecture - Overall architecture overview
- Architectural Decisions - Decision records and rationale
- Linting Guide - Running and configuring linters
- Pre-commit Script - Automated quality checks
Remember: Reviews are about maintaining quality and helping contributors improve. Focus on being helpful, constructive, and educational. Point to documentation, provide examples, and explain the "why" behind standards. Together we build better software! 🎉