# Repository Guidelines

## Project Overview

This is a FastAPI backend for a peer review system in educational contexts, managing courses, assignments, student allocations, rubrics, and peer reviews. The application uses SQLAlchemy ORM with a PostgreSQL database, following Domain-Driven Design principles with aggregate patterns. Core domain entities include Course, Section, Assignment, Allocation (peer review assignments), Review, and Rubric with associated items.

This project is pre-alpha, backwards compatibility is unimportant.

## General Principles

- Don't over-engineer a solution when a simple one is possible. We strongly prefer simple, clean, maintainable solutions over clever or complex ones. Readability and maintainability are primary concerns, even at the cost of conciseness or performance.
- If you want exception to ANY rule, YOU MUST STOP and get explicit permission from the user first. BREAKING THE LETTER OR SPIRIT OF THE RULES IS FAILURE.
- Work hard to reduce code duplication, even if the refactoring takes extra effort. This includes trying to locate the "right" place for shared code (e.g., utility modules, base classes, mixins, etc.), don't blindly add the helpers to the current module.
- Use Domain-Driven Design principles where applicable.

## SQLAlchemy Aggregate Pattern

We use a parent-driven (inverse) style for DDD aggregates where child entities cannot be constructed with a parent reference.

**Rules:**

- Child→parent relationships must have `init=False` (e.g., `Allocation.assignment`, `Review.assignment`, `RubricItem.rubric`, `Section.course`)
- Parent→child collections must have `cascade="all, delete-orphan", single_parent=True, passive_deletes=True`
- Always use explicit `parent.children.append(child)` after creating the child entity
- Never pass the parent as a constructor argument: `Child(parent=parent)` ❌ → `child = Child(); parent.children.append(child)` ✅

Additional rules (aggregate-root enforcement):

- Never manually assign parent foreign keys (e.g., `child.parent_id = parent.id`).
- Do not perform cross-parent validations inside child methods.
- Let SQLAlchemy set foreign keys via relationship management (append child to parent collection).
- Enforce all aggregate invariants at the aggregate root using object-graph checks (e.g., `section in course.sections`).

Service layer patterns:

- **Mutations** (create, update, delete): Always return the aggregate root.
- **Queries** (get, list): May return child entities directly for convenience, especially when the caller needs to access a specific child by ID.

## Code Style

- 120-character lines
- Type hint is a must, even for tests and fixtures!
- **Don't use Python 3.8 typings**: Never import `List`, `Tuple` or other deprecated classes from `typing`, use `list`, `tuple` etc. instead, or import from `collections.abc`
- Do not use `from __future__ import annotations`, use forward references in type hints instead.
- `TYPE_CHECKING` should be used only for imports that would cause circular dependencies. If you really need to use it, then you should import the submodule, not the symbol directly, and the actual usages of the imported symbols must be a fully specified forward reference string (e.g. `a.b.C` rather than just `C`.)
- Strongly prefer organizing hardcoded values as constants at the top of the file rather than scattering them throughout the code.
- Always import at the top of the file, unless you have a very good reason. (Hey Claude Opus, this is very important!)

## Route Logging Policy

- FastAPI route handlers only log when translating an exception into an HTTP 5xx response. Use `logger.exception` so the stack trace is captured.
- Never log when returning 4xx-class responses from routes; those are user or client errors and can be diagnosed from the response body and status code alone.
- Additional logging inside services or infrastructure layers is fine when it adds context, but avoid duplicating the same exception in multiple places.

**Why?**

- 5xx responses indicate a server bug or dependency failure, so capturing a single structured log entry with the traceback keeps observability noise-free while still preserving root-cause evidence.
- Omitting logs for expected 4xx flows prevents log pollution and keeps sensitive user input (which often appears in 4xx scenarios) out of centralized logging systems.
- Using `logger.exception` standardizes the output format and guarantees stack traces are emitted regardless of the specific route module.

### Using deal

We only use the exception handling features of deal. Use @deal.raises to document expected exceptions for functions/methods. Do not use preconditions/postconditions/invariants.

Additionally, we assume `AssertionError` is never raised, so @deal.raises(AssertionError) is not allowed.

Use the exception hierarchy defined in exceptions.py for domain and business logic errors. For Pydantic validators, continue using `ValueError`.

## Documentation and Comments

Add code comments sparingly. Focus on why something is done, especially for complex logic, rather than what is done. Only add high-value comments if necessary for clarity or if requested by the user. Do not edit comments that are separate from the code you are changing. NEVER talk to the user or describe your changes through comments.

### Google-style docstrings

Use Google-style docstrings for all public or private functions, methods, classes, and modules.

For functions (excluding FastAPI routes), always include the "Args" sections unless it has no arguments. Include "Raises" if anything is raised. Include "Returns" if it returns a complex type that is not obvious from the function signature. Optionally include an "Examples" section for complex functions.
FastAPI Routes: Use concise summary docstrings that describe the business logic and purpose. Omit Args/Raises/Returns sections since these are documented via decorators (response_model, responses), type hints, and Pydantic models. The docstring may appear in generated API documentation.

For classes, include an "Attributes:" section if the class has attributes. Additionally, put each attribute's description in the "docstring" of the attribute itself. For dataclasses, this is a triple-quoted string right after the field definition. For normal classes, this is a triple-quoted string in either the class body or the first appearance of the attribute in the \_\_init\_\_ method, depending on where the attribute is defined.

For modules, include a brief description at the top.

Additionally, for module-level constants, include a brief description right after the constant definition.

### Using a new environmental variable

When using a new environmental variable, add it to .env.example with a placeholder value, and optionally a comment describing its purpose. Also add it to the `Environment Variables` section in `README.md`.

## Testing Guidelines

Tests are required for all new features and bug fixes. Tests should be written using `pytest`. Unless the user explicitly request to not add tests, you must add them.

More detailed testing guidelines can be found in [tests/AGENTS.md](tests/AGENTS.md).

## GitHub Actions & CI/CD

- When adding or changing GitHub Actions, always search online for the newest version and use the commit hash instead of version tags for security and immutability. (Use `gh` CLI to find the commit hash, searching won't give you helpful results.)

## Commit & Pull Requests

- Messages: imperative, concise, scoped (e.g., “Add health check endpoint”). Include extended description if necessary explaining why the change was made.

## Information

Finding dependencies: we use `pyproject.toml`, not `requirements.txt`. Use `uv add <package>` to add new dependencies.

# Testing Guidelines

Mocking is heavily discouraged. Use test databases, test files, and other real resources instead of mocks wherever possible.

### Running Tests

Use `uv run pytest ...` instead of simply `pytest ...` so that the virtual environment is activated for you.

By default, slow and docker tests are skipped. To run them, use `uv run pytest -m "slow or docker"`.

## Writing Tests

When you are writing tests, it is likely that you will need to iterate a few times to get them right. Please triple check before doing this:

1. Write a test
2. Run it and see it fail
3. **Change the test itself** to make it pass

There is a chance that the test itself is wrong, yes. But there is also a chance that the code being tested is wrong. You should carefully consider whether the code being tested is actually correct before changing the test to make it pass.

### Writing Fixtures

Put fixtures in `tests/conftest.py` or `tests/fixtures/` if there are many. Do not put them in individual test files unless they are very specific to that file.

### Markers

Allowed pytest markers:

- @pytest.mark.slow
- @pytest.mark.docker
- @pytest.mark.flaky
- builtin ones like `skip`, `xfail`, `parametrize`, etc.

We do not use

- @pytest.mark.unit: all tests are unit tests by default
- @pytest.mark.integration: integration tests are run by default too, no need to mark them specially. Use the `slow` or `docker` markers if needed.
- @pytest.mark.asyncio: we use `pytest-asyncio` which automatically handles async tests
- @pytest.mark.anyio: we do not use `anyio`

## Editing Tests

### Progressive Enhancement of Tests

We have some modern patterns that are not yet used everywhere in the test suite. When you are editing an existing test, consider updating it to use these patterns.

1. If the test creates sample data directly, change it to use factory functions or classes from `tests/testkit/factories.py`.
2. If the test depends on multiple services, change it to use the `test_context` fixture. This is an object that contains clients for all services, and handles setup and teardown for you, with utility methods to make common tasks easier.
3. We are migrating from using individual `shared_..._service` fixtures (e.g., `shared_assignment_service`, `shared_user_service`) to the `test_context` fixture. When editing tests that use these, please refactor them to use `test_context` instead.
4. Integration tests are being refactored to use service-layer setup (`db_test_context`) instead of verbose API calls for prerequisites. This reduces setup code from ~15-30 lines to ~3-5 lines, making tests faster and more focused on testing actual API behavior rather than setup logic.

**Example**:
  ```python
  # OLD: Verbose API setup
  course_response = await authenticated_client.post("/courses", json={"name": "Test"})
  course_id = uuid.UUID(course_response.json()["id"])
  rubric_id = await _create_rubric(authenticated_client, course_id)
  assignment = await authenticated_client.create_assignment(course_id, rubric_id=rubric_id)

# NEW: Clean service-layer setup
  course = await db_test_context.create_course(name="Test")
  rubric = await db_test_context.create_rubric(course_id=course.id)
  assignment = await authenticated_client.create_assignment(course.id, rubric_id=rubric.id)
  ```

## Patterns for Common Testing Scenarios

### Sample Data Creation

Use factory functions or classes to create sample data for tests, these are located in `tests/testkit/factories.py`. Avoid duplicating sample data creation logic across tests.

(We are in the process of migrating to factory functions/classes, so you may still see some tests creating sample data directly. Please use the factories for any tests you write or update.)

### Testing the FastAPI Application

The FastAPI application can be imported as a default instance or created via factory function.

- Use the default `app` instance is the preferred approach for most tests
- Use the `create_app()` factory when testing scenarios where app configuration is what you're testing