2553 Code Reviewinstructions 13235676

Code Review Instructions

Structured code review guidelines for this repository. These complement the language-specific instructions already in place:

• Bicep: bicep-code-best-practices.instructions.md (AVM-first, TLS 1.2, managed identity, naming limits)
• PowerShell: powershell.instructions.md (comment-based help, parameter validation, $ErrorActionPreference = 'Stop')
• Shell: shell.instructions.md (set -euo pipefail, trap, jq/yq for structured data)
• JavaScript: javascript.instructions.md (ES modules, node: protocol imports, validation script patterns)
• Python: python.instructions.md (Ruff linting, uv packages, diagrams library, async MCP patterns)
• Markdown: markdown.instructions.md (120-char lines, ATX headings)

When reviewing code, apply these general guidelines in addition to the language-specific rules. Language-specific instructions take precedence on any conflicting point.

Review Language

When performing a code review, respond in English.

Review Priorities

When performing a code review, prioritize issues in the following order:

🔴 CRITICAL (Block merge)

• Security: Vulnerabilities, exposed secrets, authentication/authorization issues
• Correctness: Logic errors, data corruption risks, race conditions
• Breaking Changes: API contract changes without versioning
• Data Loss: Risk of data loss or corruption

🟡 IMPORTANT (Requires discussion)

• Code Quality: Severe violations of SOLID principles, excessive duplication
• Test Coverage: Missing tests for critical paths or new functionality
• Performance: Obvious performance bottlenecks (N+1 queries, memory leaks)
• Architecture: Significant deviations from established patterns

🟢 SUGGESTION (Non-blocking improvements)

• Readability: Poor naming, complex logic that could be simplified
• Optimization: Performance improvements without functional impact
• Best Practices: Minor deviations from conventions
• Documentation: Missing or incomplete comments/documentation

General Review Principles

When performing a code review, follow these principles:

1. Be specific: Reference exact lines, files, and provide concrete examples
2. Provide context: Explain WHY something is an issue and the potential impact
3. Suggest solutions: Show corrected code when applicable, not just what's wrong
4. Be constructive: Focus on improving the code, not criticizing the author
5. Recognize good practices: Acknowledge well-written code and smart solutions
6. Be pragmatic: Not every suggestion needs immediate implementation
7. Group related comments: Avoid multiple comments about the same topic

Code Quality Standards

When performing a code review, check for:

Clean Code

• Descriptive and meaningful names for variables, functions, and classes
• Single Responsibility Principle: each function/class does one thing well
• DRY (Don't Repeat Yourself): no code duplication
• Functions should be small and focused (ideally < 20-30 lines)
• Avoid deeply nested code (max 3-4 levels)
• Avoid magic numbers and strings (use constants)
• Code should be self-documenting; comments only when necessary

Examples

// ❌ BAD: Poor naming and magic numbers
function calc(x, y) {
    if (x > 100) return y * 0.15;
    return y * 0.10;
}

// ✅ GOOD: Clear naming and constants
const PREMIUM_THRESHOLD = 100;
const PREMIUM_DISCOUNT_RATE = 0.15;
const STANDARD_DISCOUNT_RATE = 0.10;

function calculateDiscount(orderTotal, itemPrice) {
    const isPremiumOrder = orderTotal > PREMIUM_THRESHOLD;
    const discountRate = isPremiumOrder ? PREMIUM_DISCOUNT_RATE : STANDARD_DISCOUNT_RATE;
    return itemPrice * discountRate;
}

Error Handling

• Proper error handling at appropriate levels
• Meaningful error messages
• No silent failures or ignored exceptions
• Fail fast: validate inputs early
• Use appropriate error types/exceptions

Examples

# ❌ BAD: Silent failure and generic error
def process_user(user_id):
    try:
        user = db.get(user_id)
        user.process()
    except:
        pass

# ✅ GOOD: Explicit error handling
def process_user(user_id):
    if not user_id or user_id <= 0:
        raise ValueError(f"Invalid user_id: {user_id}")

    try:
        user = db.get(user_id)
    except UserNotFoundError:
        raise UserNotFoundError(f"User {user_id} not found in database")
    except DatabaseError as e:
        raise ProcessingError(f"Failed to retrieve user {user_id}: {e}")

    return user.process()

Security Review

When performing a code review, check for security issues:

• Sensitive Data: No passwords, API keys, tokens, or PII in code or logs
• Input Validation: All user inputs are validated and sanitized
• SQL Injection: Use parameterized queries, never string concatenation
• Authentication: Proper authentication checks before accessing resources
• Authorization: Verify user has permission to perform action
• Cryptography: Use established libraries, never roll your own crypto
• Dependency Security: Check for known vulnerabilities in dependencies

Examples

// ❌ BAD: SQL injection vulnerability
String query = "SELECT * FROM users WHERE email = '" + email + "'";

// ✅ GOOD: Parameterized query
PreparedStatement stmt = conn.prepareStatement(
    "SELECT * FROM users WHERE email = ?"
);
stmt.setString(1, email);

// ❌ BAD: Exposed secret in code
const API_KEY = "sk_live_abc123xyz789";

// ✅ GOOD: Use environment variables
const API_KEY = process.env.API_KEY;

Testing Standards

When performing a code review, verify test quality:

• Coverage: Critical paths and new functionality must have tests
• Test Names: Descriptive names that explain what is being tested
• Test Structure: Clear Arrange-Act-Assert or Given-When-Then pattern
• Independence: Tests should not depend on each other or external state
• Assertions: Use specific assertions, avoid generic assertTrue/assertFalse
• Edge Cases: Test boundary conditions, null values, empty collections
• Mock Appropriately: Mock external dependencies, not domain logic

Examples

// ❌ BAD: Vague name and assertion
test('test1', () => {
    const result = calc(5, 10);
    expect(result).toBeTruthy();
});

// ✅ GOOD: Descriptive name and specific assertion
test('should calculate 10% discount for orders under $100', () => {
    const orderTotal = 50;
    const itemPrice = 20;

    const discount = calculateDiscount(orderTotal, itemPrice);

    expect(discount).toBe(2.00);
});

Performance Considerations

When performing a code review, check for performance issues:

• Database Queries: Avoid N+1 queries, use proper indexing
• Algorithms: Appropriate time/space complexity for the use case
• Caching: Utilize caching for expensive or repeated operations
• Resource Management: Proper cleanup of connections, files, streams
• Pagination: Large result sets should be paginated
• Lazy Loading: Load data only when needed

Examples

# ❌ BAD: N+1 query problem
users = User.query.all()
for user in users:
    orders = Order.query.filter_by(user_id=user.id).all()  # N+1!

# ✅ GOOD: Use JOIN or eager loading
users = User.query.options(joinedload(User.orders)).all()
for user in users:
    orders = user.orders

Architecture and Design

When performing a code review, verify architectural principles:

• Separation of Concerns: Clear boundaries between layers/modules
• Dependency Direction: High-level modules don't depend on low-level details
• Interface Segregation: Prefer small, focused interfaces
• Loose Coupling: Components should be independently testable
• High Cohesion: Related functionality grouped together
• Consistent Patterns: Follow established patterns in the codebase

Documentation Standards

When performing a code review, check documentation:

• API Documentation: Public APIs must be documented (purpose, parameters, returns)
• Complex Logic: Non-obvious logic should have explanatory comments
• README Updates: Update README when adding features or changing setup
• Breaking Changes: Document any breaking changes clearly
• Examples: Provide usage examples for complex features

Comment Format Template

When performing a code review, use this format for comments:

**[PRIORITY] Category: Brief title**

Detailed description of the issue or suggestion.

**Why this matters:**
Explanation of the impact or reason for the suggestion.

**Suggested fix:**
[code example if applicable]

**Reference:** [link to relevant documentation or standard]

Example Comments

Critical Issue

**🔴 CRITICAL - Security: SQL Injection Vulnerability**

The query on line 45 concatenates user input directly into the SQL string,
creating a SQL injection vulnerability.

**Why this matters:**
An attacker could manipulate the email parameter to execute arbitrary SQL commands,
potentially exposing or deleting all database data.

**Suggested fix:**
```sql
-- Instead of:
query = "SELECT * FROM users WHERE email = '" + email + "'"

-- Use:
PreparedStatement stmt = conn.prepareStatement(
    "SELECT * FROM users WHERE email = ?"
);
stmt.setString(1, email);
```

**Reference:** OWASP SQL Injection Prevention Cheat Sheet

Important Issue

**🟡 IMPORTANT - Testing: Missing test coverage for critical path**

The `processPayment()` function handles financial transactions but has no tests
for the refund scenario.

**Why this matters:**
Refunds involve money movement and should be thoroughly tested to prevent
financial errors or data inconsistencies.

**Suggested fix:**
Add test case:
```javascript
test('should process full refund when order is cancelled', () => {
    const order = createOrder({ total: 100, status: 'cancelled' });

    const result = processPayment(order, { type: 'refund' });

    expect(result.refundAmount).toBe(100);
    expect(result.status).toBe('refunded');
});
```

Suggestion

**🟢 SUGGESTION - Readability: Simplify nested conditionals**

The nested if statements on lines 30-40 make the logic hard to follow.

**Why this matters:**
Simpler code is easier to maintain, debug, and test.

**Suggested fix:**
```javascript
// Instead of nested ifs:
if (user) {
    if (user.isActive) {
        if (user.hasPermission('write')) {
            // do something
        }
    }
}

// Consider guard clauses:
if (!user || !user.isActive || !user.hasPermission('write')) {
    return;
}
// do something
```

Review Checklist

When performing a code review, systematically verify:

Code Quality

• Code follows consistent style and conventions
• Names are descriptive and follow naming conventions
• Functions/methods are small and focused
• No code duplication
• Complex logic is broken into simpler parts
• Error handling is appropriate
• No commented-out code or TODO without tickets

Security

• No sensitive data in code or logs
• Input validation on all user inputs
• No SQL injection vulnerabilities
• Authentication and authorization properly implemented
• Dependencies are up-to-date and secure

Testing

• New code has appropriate test coverage
• Tests are well-named and focused
• Tests cover edge cases and error scenarios
• Tests are independent and deterministic
• No tests that always pass or are commented out

Performance

• No obvious performance issues (N+1, memory leaks)
• Appropriate use of caching
• Efficient algorithms and data structures
• Proper resource cleanup

Architecture

• Follows established patterns and conventions
• Proper separation of concerns
• No architectural violations
• Dependencies flow in correct direction

Documentation

• Public APIs are documented
• Complex logic has explanatory comments
• README is updated if needed
• Breaking changes are documented

Additional Resources

For more information on effective code reviews and GitHub Copilot customization:

• GitHub Copilot Prompt Engineering
• GitHub Copilot Custom Instructions
• Awesome GitHub Copilot Repository
• GitHub Code Review Guidelines
• Google Engineering Practices - Code Review
• OWASP Security Guidelines

Prompt Engineering Tips

When performing a code review, apply these prompt engineering principles from the GitHub Copilot documentation:

1. Start General, Then Get Specific: Begin with high-level architecture review, then drill into implementation details
2. Give Examples: Reference similar patterns in the codebase when suggesting changes
3. Break Complex Tasks: Review large PRs in logical chunks (security → tests → logic → style)
4. Avoid Ambiguity: Be specific about which file, line, and issue you're addressing
5. Indicate Relevant Code: Reference related code that might be affected by changes
6. Experiment and Iterate: If initial review misses something, review again with focused questions

Project Context

This repository's primary tech stack:

• Infrastructure as Code: Azure Bicep (AVM-first), future Terraform
• Scripting: PowerShell 7+, Node.js (.mjs), bash
• Diagrams: Python 3.10+ (diagrams library)
• Architecture: Multi-agent orchestration (8 agents + 3 subagents)
• Build/Lint: npm run lint:md, npm run validate:all
• Code Style: Conventional Commits, 120-char markdown lines
• Security Baseline: TLS 1.2+, HTTPS-only, managed identity, Azure AD-only SQL auth