11 - Automatic Code Review with Copilot

# In the repository Settings page:
# 1. Go to Settings > Code review > Copilot
# 2. Select "Automatic" under "Copilot code review"
# 3. Choose the trigger:
#    - "All pull requests" - automatic review on every PR
#    - "When requested" - only when manually requested

# To enable via GitHub API:
# PATCH /repos/{{ '{' }}owner}/{{ '{' }}repo}
# {{ '{' }}
#   "copilot_code_review": {{ '{' }}
#     "automatic": true,
#     "trigger": "all_pull_requests"
#   }
# }

# At the organization level (for all repositories):
# Organization Settings > Copilot > Policies > Code Review
# Enable: "Automatic code review for all repositories"

# .github/workflows/copilot-review.yml
name: Request Copilot Code Review

on:
  pull_request:
    types: [opened, synchronize]
    branches:
      - main
      - release/*
    paths:
      - 'src/**'
      - 'lib/**'
      - '!src/**/*.test.ts'
      - '!src/**/*.spec.ts'
      - '!docs/**'

jobs:
  request-review:
    runs-on: ubuntu-latest
    steps:
      - name: Request Copilot Review
        uses: actions/github-script@v7
        with:
          script: |
            // Request review only if the PR has more than 10 modified lines
            const {{ '{' }} data: files } = await github.rest.pulls.listFiles({{ '{' }}
              owner: context.repo.owner,
              repo: context.repo.repo,
              pull_number: context.payload.pull_request.number
            });

            const totalChanges = files.reduce((sum, f) =>
              sum + f.additions + f.deletions, 0);

            if (totalChanges > 10) {{ '{' }}
              await github.rest.pulls.requestReviewers({{ '{' }}
                owner: context.repo.owner,
                repo: context.repo.repo,
                pull_number: context.payload.pull_request.number,
                reviewers: ['copilot']  // Request review from Copilot
              });
              console.log(`Requested Copilot review for PR #${{ '{' }}context.payload.pull_request.number}`);
            } else {{ '{' }}
              console.log(`Skipped: only ${{ '{' }}totalChanges} lines changed`);
            }

# Copilot Code Review Instructions

## Team Code Standards

### TypeScript
- Use `strict: true` in tsconfig.json
- Never use `any`: prefer `unknown` with type guard
- Prefer `interface` over `type` for objects
- Use `readonly` for properties that must not change
- Always enumerate cases in switch on union type
- Use optional chaining (`?.`) instead of manual null checks
- Prefer `const` over `let`, never `var`

### Naming Conventions
- Classes and interfaces: PascalCase (e.g. `UserService`, `IUserRepository`)
- Variables and functions: camelCase (e.g. `getUserById`, `isActive`)
- Constants: UPPER_SNAKE_CASE (e.g. `MAX_RETRY_COUNT`)
- Files: kebab-case (e.g. `user-service.ts`)
- Tests: [name].test.ts or [name].spec.ts
- Booleans: prefix is/has/should (e.g. `isActive`, `hasPermission`)

### Error Handling
- Use custom error classes (AppError, NotFoundError, etc.)
- Do not catch errors without re-throwing or logging them
- Include context in the error: `throw new NotFoundError('User', userId)`
- Do not use try/catch for flow control
- All API endpoints must have an error handler

### Security (HIGH PRIORITY)
- Never concatenate user input in SQL queries
- Validate ALL inputs with class-validator
- Sanitize HTML output to prevent XSS
- Verify authorization for every protected endpoint
- Do not expose stack traces or internal details in API responses
- Rate limiting on public endpoints
- Secrets only via environment variables

### Performance
- Avoid N+1 queries: use include/eager loading
- Mandatory pagination for lists: max 100 items
- Caching for frequently accessed data
- Async/await for I/O operations
- Avoid blocking synchronous operations

### Tests
- Every new public function must have at least one test
- Tests must be independent (no execution order)
- Mock only external dependencies (database, API, file system)
- Descriptive names: "should [action] when [condition]"
- At least one test for the error case

### Angular (Frontend)
- Use standalone components
- Signals for state management
- OnPush change detection for all components
- Lazy loading for non-critical routes
- No `subscribe()` in templates: use async pipe or toSignal

# Language-specific rules

## Python (ML Backend)
- Type hints mandatory for all parameters and return
- Google style docstring for every public function
- Use `dataclasses` or `pydantic` for data objects
- Black formatting (line length 88)
- Imports sorted with isort

## SQL (Migrations)
- Never DROP TABLE without a documented backup plan
- ALTER TABLE must be reversible
- Index for every foreign key
- Column names in snake_case
- SQL comment for each migration explaining the "why"

## YAML/Config
- Comments for every non-obvious section
- Secrets referenced via variables, never hardcoded values
- Schema validation for configuration files

# Architectural Patterns

## Layered Architecture
The project follows a layered architecture:
- Controller → Service → Repository
- Controllers MUST NOT access repositories directly
- Repositories MUST NOT contain business logic
- Each layer communicates only with the layer immediately below

## Dependency Injection
- All dependencies injected via constructor
- Interfaces for dependencies (IUserService, IUserRepository)
- No manual singleton pattern: use the DI container

## Event-Driven Communication
- Cross-service communication via events
- Events must be immutable
- Each event has a unique type and a typed payload
- Event handlers must be idempotent

## VERIFY:
- If a controller calls a repository directly: ERROR
- If a service instantiates a dependency directly: WARNING
- If a SQL query appears outside a repository: ERROR
- If business logic appears in a controller: WARNING

// File: src/repositories/user.repository.ts
// Line added in the PR:

async findByName(name: string): Promise<User[]> {{ '{' }}
  const query = `SELECT * FROM users WHERE name LIKE '%${{ '{' }}name}%'`;
  return this.db.raw(query);
}

async findByName(name: string): Promise<User[]> {{ '{' }}
  return this.db.query(
    'SELECT * FROM users WHERE name LIKE $1',
    [`%${{ '{' }}name}%`]
  );
}

// File: src/services/order.service.ts

async getOrdersWithDetails(userId: string): Promise<OrderDetail[]> {{ '{' }}
  const orders = await this.orderRepo.findByUserId(userId);

  const result = [];
  for (const order of orders) {{ '{' }}
    const items = await this.itemRepo.findByOrderId(order.id);
    const customer = await this.customerRepo.findById(order.customerId);
    result.push({{ '{' }} ...order, items, customer });
  }

  return result;
}

async getOrdersWithDetails(userId: string): Promise<OrderDetail[]> {{ '{' }}
  const orders = await this.orderRepo.findByUserId(userId);

  if (orders.length === 0) return [];

  const orderIds = orders.map(o => o.id);
  const customerIds = [...new Set(orders.map(o => o.customerId))];

  // Batch query: 3 total queries instead of 2N+1
  const [allItems, allCustomers] = await Promise.all([
    this.itemRepo.findByOrderIds(orderIds),
    this.customerRepo.findByIds(customerIds)
  ]);

  const itemsByOrder = new Map();
  for (const item of allItems) {{ '{' }}
    if (!itemsByOrder.has(item.orderId)) {{ '{' }}
      itemsByOrder.set(item.orderId, []);
    }
    itemsByOrder.get(item.orderId).push(item);
  }

  const customersById = new Map(
    allCustomers.map(c => [c.id, c])
  );

  return orders.map(order => ({{ '{' }}
    ...order,
    items: itemsByOrder.get(order.id) || [],
    customer: customersById.get(order.customerId)
  }));
}

// File: src/controllers/payment.controller.ts

async processPayment(req: Request, res: Response) {{ '{' }}
  try {{ '{' }}
    const result = await this.paymentService.charge(
      req.body.amount,
      req.body.cardToken
    );
    res.json(result);
  } catch (e) {{ '{' }}
    console.log(e);
    res.status(500).json({{ '{' }} error: 'Something went wrong' });
  }
}

async processPayment(req: Request, res: Response) {{ '{' }}
  // Input validation
  const {{ '{' }} amount, cardToken } = req.body;

  if (!amount || typeof amount !== 'number' || amount <= 0) {{ '{' }}
    return res.status(400).json({{ '{' }}
      error: 'Invalid amount',
      detail: 'Amount must be a positive number'
    });
  }

  if (!cardToken || typeof cardToken !== 'string') {{ '{' }}
    return res.status(400).json({{ '{' }}
      error: 'Invalid card token',
      detail: 'Card token is required'
    });
  }

  try {{ '{' }}
    const result = await this.paymentService.charge(amount, cardToken);
    res.json(result);
  } catch (error) {{ '{' }}
    logger.error('Payment processing failed', {{ '{' }}
      amount,
      error: error.message,
      stack: error.stack,
      requestId: req.id
    });

    if (error instanceof PaymentDeclinedError) {{ '{' }}
      return res.status(402).json({{ '{' }}
        error: 'Payment declined',
        detail: error.declineReason
      });
    }

    if (error instanceof ValidationError) {{ '{' }}
      return res.status(400).json({{ '{' }}
        error: 'Validation error',
        detail: error.message
      });
    }

    res.status(500).json({{ '{' }}
      error: 'Internal server error',
      requestId: req.id  // For debugging without exposing details
    });
  }
}

# Recommended Branch Protection Rules to integrate Copilot

# Settings > Branches > Branch protection rules > main

# Mandatory rules:
# 1. Require pull request reviews: 1 reviewer minimum
# 2. Require review from Code Owners: enabled
# 3. Dismiss stale reviews: enabled (when new pushes invalidate reviews)
# 4. Require status checks: enabled
#    - Required checks:
#      - ci/build (CI pipeline)
#      - ci/test (test suite)
#      - security/codeql (static analysis)
# 5. Require Copilot code review: enabled (optional)
#
# Note: Copilot review can be required as a mandatory check,
# but it is recommended NOT to make it blocking
# to avoid false positives that slow down merging.
#
# Recommended strategy:
# - Copilot review: mandatory but not blocking
# - Human reviewer: mandatory and blocking
# - CI/CD checks: mandatory and blocking