quicproquo/docs/src/contributing/coding-standards.md

# Coding Standards

This page defines the engineering standards for quicprochat. These are
non-negotiable -- all code merged into the repository must conform to these
rules. The standards exist to ensure that every milestone produces
production-ready, auditable, and secure code.

---

## Production-Ready Only

Every deliverable must be complete and functional. The following are prohibited
in any merged code:

- `todo!()` or `unimplemented!()` macros
- Stub implementations or placeholder logic
- Mock objects in production code paths (mocks are acceptable only in test code)
- Commented-out code blocks
- `#[allow(unused)]` on production code (acceptable on prost-generated code)

If a feature is out of scope for the current milestone, it is **explicitly
omitted** with a documented reason (in an ADR or code comment explaining why it
is deferred), not silently stubbed.

---

## YAGNI / KISS / DRY

- **YAGNI (You Aren't Gonna Need It):** Do not add features, abstractions, or
  generic type parameters that are not required by the current milestone.
- **KISS (Keep It Simple):** Favour clarity over cleverness. A straightforward
  `match` statement is preferred over a complex trait hierarchy.
- **DRY (Don't Repeat Yourself):** Extract shared logic into functions or
  modules, but do not create abstractions prematurely. Two occurrences is a
  coincidence; three is a pattern worth extracting.

---

## Spec-First Development

Document the design before implementing it:

1. **ADR (Architecture Decision Record)** for significant design decisions. ADRs
   live in `docs/src/design-rationale/` and are referenced from the
   [Design Rationale](../design-rationale/overview.md) section.
2. **Doc comments** on every public API (`///` for items, `//!` for modules).
   Doc comments must explain:
   - What the function/type does.
   - Invariants and preconditions.
   - Error conditions and what each error variant means.
   - Examples where the API is non-obvious.

```rust
/// Creates a new MLS group with the given group ID and returns
/// the initial `GroupMember` state.
///
/// # Errors
///
/// Returns `GroupError::CryptoBackend` if the MLS crypto provider
/// fails to generate the initial key schedule.
///
/// Returns `GroupError::InvalidGroupId` if `group_id` is empty.
pub fn create_group(
    group_id: &[u8],
    identity: &IdentityKeypair,
) -> Result<GroupMember, GroupError> {
    // ...
}
```

---

## Security-by-Design

### Secrets and Key Material

- All private key material must be wrapped in `Zeroizing<T>` (from the `zeroize`
  crate) or implement `Zeroize + ZeroizeOnDrop`.
- No secret material in log output at any level (`TRACE`, `DEBUG`, `INFO`,
  `WARN`, `ERROR`).
- When logging key-related operations, log only fingerprints (SHA-256 of the
  public key), never the key bytes themselves.

### Error Handling

- No `unwrap()` or `expect()` on cryptographic operations or I/O in non-test
  paths. All crypto errors must be typed and propagated.
- Use `thiserror` for library error types (`quicprochat-core`,
  `quicprochat-proto`, `quicprochat-rpc`, `quicprochat-sdk`) and `anyhow` for
  application-level error handling (`quicprochat-server`, `quicprochat-client`).
- `unwrap()` is acceptable only in:
  - Test code.
  - Cases where the invariant is provably guaranteed by the type system
    (e.g., indexing a fixed-size array with a constant).

### Constant-Time Comparisons

- Use constant-time comparison (`subtle::ConstantTimeEq` or equivalent) when
  comparing authentication tokens, key fingerprints, or any value that could
  be used in a timing side-channel attack.
- Never use `==` to compare secrets or tokens in authentication paths.

### Input Validation

- Validate all incoming data at the boundary (RPC handler entry point) before
  passing it to internal logic.
- Length checks: group ID (32 bytes), identity key (32 bytes), channel ID
  (16 bytes), payload (max 5 MB).
- Reject unexpected enum variants or unknown wire versions.

---

## Containerisation

- The server runs in Docker. The `Dockerfile` and `docker-compose.yml` must
  always be kept up to date with the current build.
- Multi-stage build: `rust:bookworm` builder stage, `debian:bookworm-slim`
  runtime stage.
- The Docker image must build and run correctly after every merge to the main
  branch.
- The builder stage does not install extra system packages for code generation
  — `protobuf-src` vendors `protoc` automatically.

---

## Dependency Hygiene

### Pinned Major Versions

All dependencies are pinned to a major version in `Cargo.toml`. Minor and patch
updates are allowed; major version bumps require justification and review.

### Preferred Ecosystem

| Domain | Preferred Crate(s) | Notes |
|--------|-------------------|-------|
| Classical crypto (signing) | `ed25519-dalek` | |
| Classical crypto (key exchange) | `x25519-dalek` | |
| OPAQUE authentication | `opaque-ke` (v4) | Ristretto255 + Argon2 |
| MLS | `openmls 0.5`, `openmls_rust_crypto` | RFC 9420 |
| Post-quantum KEM | `ml-kem` (ML-KEM-768, FIPS 203) | |
| Serialisation / RPC (v2) | `prost`, `prost-build`, `protobuf-src` | Primary wire format |
| Serialisation (v1 legacy) | `capnp`, `capnp-rpc` | Legacy only, not for new code |
| Async runtime | `tokio` | |
| QUIC transport | `quinn` | |
| Middleware | `tower` | |
| Storage | `rusqlite` with `bundled-sqlcipher` | |
| Zeroisation | `zeroize` | |
| Internal serialisation | `bincode` | For MLS entities and file-backed store |

Do not introduce new dependencies without justification. In particular:

- No alternative async runtimes (async-std, smol).
- No alternative serialisation formats for wire protocol use (new code must
  use Protobuf via `prost`; Cap'n Proto is legacy-only).
- No alternative crypto libraries unless the preferred crate lacks required
  functionality.

### Dependency Auditing

- `cargo audit` must pass in CI with no known vulnerabilities.
- `cargo deny check` for license compatibility and duplicate detection.

---

## Git Standards

### Signed Commits

All commits must be GPG-signed. Unsigned commits will be rejected by CI.

Configure signing:

```bash
git config --global commit.gpgsign true
git config --global user.signingkey <YOUR_GPG_KEY_ID>
```

### Conventional Commits

Commit messages follow the [Conventional Commits](https://www.conventionalcommits.org/)
specification:

| Prefix | Use |
|--------|-----|
| `feat:` | A new feature or capability |
| `fix:` | A bug fix |
| `chore:` | Maintenance (deps, CI, config) |
| `docs:` | Documentation changes |
| `test:` | Adding or updating tests |
| `refactor:` | Code restructuring without behaviour change |

### Commit Message Format

```
<type>: <short description>

<body: explain WHY, not just WHAT>
```

The body describes the motivation and context for the change, not just a
restatement of the diff. Why was this change necessary? What problem does it
solve? What alternatives were considered?

Example:

```
feat: add KeyPackage TTL eviction on fetch

KeyPackages older than 24 hours are now filtered out at fetch time,
preventing stale key material from being used in MLS group additions.

Background sweep is deferred to M6 (requires persistent storage).
```

### Branch Strategy

- Feature branches per milestone: `feat/m1-noise-transport`,
  `feat/m2-keypackage-as`, etc.
- Branch names use lowercase with hyphens.
- All work happens on feature branches; direct commits to the main branch are
  prohibited.

---

## Code Style

### Formatting

- `cargo fmt` with default settings. No custom `rustfmt.toml` overrides.
- CI rejects unformatted code.

### Linting

- `cargo clippy --workspace -- -D warnings`. No `#[allow(clippy::...)]` without a
  comment explaining why the lint is suppressed.
- CI treats clippy warnings as errors (`-D warnings`).

### Naming

- Types: `PascalCase` (Rust convention).
- Functions and variables: `snake_case`.
- Constants: `SCREAMING_SNAKE_CASE`.
- Module names: `snake_case`, matching the file name.

### Module Organisation

- One concept per file. A file should not exceed ~500 lines (guideline, not
  hard rule).
- Public API at the top of the file; private helpers at the bottom.
- `mod.rs` files are avoided; use `foo.rs` + `foo/` directory when a module
  has submodules.

---

## Review Checklist

Before presenting any code for review, verify:

- [ ] No missing error handling (all `Result` values handled).
- [ ] No security gaps (secrets zeroized, no secret logging, typed crypto errors).
- [ ] No incomplete implementations (no `todo!()`, `unimplemented!()`, stubs).
- [ ] No deviation from these standards.
- [ ] Doc comments on all public items.
- [ ] Tests for all new functionality (see [Testing Strategy](testing.md)).
- [ ] `cargo fmt`, `cargo clippy --workspace -- -D warnings`, and `cargo test --workspace` all pass.

---

## Cross-references

- [Testing Strategy](testing.md) -- test structure and conventions
- [Design Rationale](../design-rationale/overview.md) -- ADR index
- [Milestones](../roadmap/milestones.md) -- what each milestone delivers
- [Production Readiness WBS](../roadmap/production-readiness.md) -- governance and CI requirements