# Contributing

Thanks for considering a contribution to Rutster. This file is the short
form — read [`AGENTS.md`](AGENTS.md) for the full orientation any agent
(human, AI, hybrid) working in the repo should have.

---

## Trunk-based development

1. Branch from `main` for any change.
2. Open a PR targeting `main`.
3. CI gates (below) must pass before merge.
4. Squash-merge to keep `main` linear.

**Never push directly to `main`.** Branch protection is planned to
enforce this; until then, self-discipline.

### Branch naming

Single-purpose branches named after the change:

- `slice-1-webrtc-loopback` — long-running build-target branch
- `docs/quickstart` — documentation additions
- `media/opus-roundtrip-fix` — bug fix with crate scope
- `ci/deny-toml` — CI config

No strict scheme — short, descriptive, hyphen-separated.

---

## CI gates

All four must pass before merge to `main`:

```bash
cargo fmt --check                  # formatting
cargo clippy -- -D warnings        # lints (warnings = failures)
cargo test --all                   # all unit + integration tests
cargo deny check                    # licenses, advisories, bans, sources
```

CI runs on push + PR to `main`, matrix: latest stable + the MSRV pinned
in `rust-toolchain.toml`. The `clippy` + `test` jobs install `libopus-dev`
(the one system dependency, per PORT_PLAN §7).

If your change adds a new dependency, run `cargo deny check` locally
before pushing — a license conflict or duplicate-version ban will fail
CI, and it's faster to catch locally.

---

## Commit messages

- **Imperative mood**: "Add X" / "Fix Y", not "Added X" / "Fixes Y".
- **Subject ≤ 72 chars**.
- **Body wraps at 72**, blank line between subject and body.
- **Reference ADRs / specs** by number when relevant: `ADR-0002`,
  `slice-1 spec §3.4`.
- Match the style of recent commits:
  ```bash
  git log --oneline -10
  ```

Example shape (from the repo history):

```
media: PcmFrame + AudioSource/Sink + Opus codec pair

PcmFrame is the canonical tap format (16-bit mono @ 24 kHz, 480 samples
per 20 ms frame — ARCHITECTURE.md). AudioSource/AudioSink are the seam
step 2 splices the tap client into (spec §3.3); EchoAudioPipe is the
slice-1 wiring of that seam. OpusDecoder/OpusEncoder wrap the opus
crate's libopus FFI with hot-path match-and-continue.
```

### Atomic commits

One logical change per commit. Doc ratifications, code, and tests each
land as separate commits when practical. Don't bundle unrelated work.

A new feature + its tests can land in one commit if the test is part of
the feature's correctness story (TDD). A refactor + a new feature should
be two commits.

### Never commit secrets

`.gitignore` covers `.env*`, `*.pem`, `*.key`. If a new secret pattern
appears in your work, extend `.gitignore` in the same commit.

---

## Code style

See [`AGENTS.md`](AGENTS.md) "Code style" for the full guide. Highlights:

- `cargo fmt` is the source of truth for formatting. Don't hand-format.
- `clippy -D warnings` is the lint bar. Fix the code, don't suppress
  with `#[allow]` unless the rationale is documented inline.
- `snake_case` (functions, variables, modules, crates), `PascalCase`
  (types), `UPPER_SNAKE_CASE` (constants).
- Newtype wrappers over primitives for type-safety — e.g. `ChannelId(Uuid)`,
  not bare `Uuid`.
- Hot path (the 20 ms media loop): **never** `?`-propagate.
  Match-and-continue. Dropped packet must not terminate the peer.
- Cold path: `thiserror`-derived error enums + `?`, converted to HTTP
  status codes at the axum boundary.
- Never `unwrap()` / `expect()` outside tests or const-init contexts.

### Documentation comments (learner-facing — important)

**This project overrides the default "no comments" convention.** The
user is learning Rust from this codebase. Code in slice 1 (and the
spearhead steps) carries thorough educational comments:

- `//!` module docs on every `lib.rs` / `main.rs` / sub-module
- `///` item docs on every public struct / enum / fn / trait
- `//` inline comments on the *mechanism*, not the what — why this
  ownership pattern, why `Arc<Mutex<>>` vs `Arc<RwLock<>>`, why an
  `enum` over a struct with a `kind` field, etc. Aim: a Rust learner
  reads the comment and learns a specific Rust concept they wouldn't
  have inferred from the code alone.

`LEARNING.md` at the repo root indexes "to learn concept X, read file Y"
pointers. Add to it when you introduce a new pattern worth surfacing.

This verbosity is a deliberate trade-off: more tokens now, compound
educational value later. Once a pattern is established, later slices can
be sparser on the well-trodden patterns.

---

## Slice-1 boundaries — what NOT to add (yet)

If an agent (you, an AI pair, a contributor) proposes adding any of these
in slice 1, the right answer is "no, see slice-1 spec §1.2":

- Dedicated timing thread for the media loop (step 4)
- TLS on the HTTP signaling surface (step 5)
- Authn / authz / multi-tenancy (step 6)
- Trickle ICE (when NATs demand it)
- The tap itself (step 2 — slice 1 only *pre-paves* the seam)
- The brain / STT / LLM / TTS (step 3)
- Barge-in / VAD-driven playout kill (step 4)
- PSTN trunk / SIP client (step 5)
- Spend cap / abuse gate (step 6)
- Browser-based automated e2e tests / Selenium / Playwright (post-slice-1)
- Docker / compose (later rung)
- Event bus / Valkey / CDR emission (step 5)
- Transfer / park / pickup / barge features (escalation rung 2)

The spearhead depends on this sequencing. Adding things early breaks
the ordering each step is its own proof.

---

## Terminology policy

**Avoid authoritarian / exclusionary terms** in code, prose, identifiers,
and endpoint names. Use equally-descriptive alternatives:

| Avoid | Use instead |
|---|---|
| police / policing (the verb) | enforce / gate / guard |
| master / slave | primary / replica, leader / follower, controller / worker |
| blacklist / whitelist | denylist / allowlist, blocklist / safelist |
| officer | operator / handler / controller |
| censor | suppress / filter |

**Exception: protocol-convention names are kept verbatim** when they come
from upstream specs or libraries — replacing them would hurt the
educational mapping to upstream docs. **ICE** (Interactive Connectivity
Establishment, RFC 8445) stays: it's the protocol name in `str0m::ice`,
`RTCIceCandidate`, and the cargo ecosystem.

See [`AGENTS.md`](AGENTS.md) "Terminology policy" for the full table.

---

## Pull request workflow

1. Branch from `main`.
2. Make atomic commits per the guidance above.
3. Push the branch + open a PR targeting `main`.
4. CI runs: fmt, clippy, test, deny. All four must be green.
5. Reviewer checks the diff against the spec / ADRs cited in the commit
   message. New dependencies require scrutiny — `cargo deny check`
   enforces license + source bans, but reviewers should also sanity-check
   the dependency choice against the architecture.
6. Squash-merge once approved + green.
7. Delete the branch post-merge (keeps the branch list tidy).

### Reviewing

When reviewing a PR:

- Does it cite the relevant ADR / spec section? (For substantial changes.)
- Does it add anything from the slice-1 "what NOT to add" list? (Reject
  if so — refer to slice-1 spec §1.2.)
- Are educational comments present where a new pattern is introduced?
- Any `unwrap()` / `expect()` outside tests? (Reject unless justified.)
- Does the hot-path code use `?`? (Reject — it must match-and-continue.)
- Does the diff bundle unrelated work? (Ask for split commits.)

---

## License

By contributing, you agree your contributions are licensed under
**GPL-3.0-or-later** (ADR-0004). Strong copyleft in the Asterisk lineage.

If you contribute code with a different license header / SPDX expression
in a Cargo manifest, CI will reject it (`cargo deny check licenses`).
Don't introduce dependencies whose licenses conflict — check `deny.toml`
for the allow-list.