# Contributing to proxxx

Thanks for considering a contribution. proxxx is a security-sensitive
infrastructure tool — a small bug can lock an admin out of their
cluster — so the bar is "demonstrate it works against a real PVE
node, not just in unit tests". This document explains how.

## TL;DR

1. Open an issue first for non-trivial work to align on scope.
2. Run the full local gate before opening a PR (`scripts/gate.sh`).
3. Live-cluster verification is required for changes that touch the
   API client, the mutation lifecycle, or any TUI that issues writes.
4. Security-impacting changes go through `SECURITY.md`, not a PR.
5. Be kind (see `CODE_OF_CONDUCT.md`).

## Before you start

- **Issue first**: open a GitHub issue describing what you want to
  change. For feature requests, mention which existing surface it
  would touch and whether it overlaps with any documented design
  boundary (`README.md` → "Honest non-goals"). For bug reports,
  include `proxxx version --json`, OS / arch, and a minimal repro.
- **No surprise mega-PRs**: a 2000-line PR with no preceding issue
  is unlikely to land. Break it down or open the design conversation
  first.
- **Existing roadmap**: the `pre-commit/01-feature-coverage.md`
  matrix tracks every feature with its verification status (✅ live /
  🟡 partial / ⚠️ wiremock-only / ❌ not verified). Pick something
  that's already on the matrix or propose a new row.

## Development environment

```bash
# Toolchain pinned in rust-toolchain.toml (1.95.0 + rustfmt + clippy
# + x86_64-unknown-linux-musl). rustup picks it up automatically
# on first cargo invocation.
git clone https://github.com/fabriziosalmi/proxxx.git
cd proxxx
cargo build --release   # ~1 minute cold; cached after that
./target/release/proxxx init
# Edit ~/.config/proxxx/config.toml (Linux) or
# ~/Library/Application Support/dev.proxxx.proxxx/config.toml (macOS)
./target/release/proxxx ls nodes
```

For VitePress docs:

```bash
cd docs && npm install && npx vitepress dev
# http://localhost:5173 — edit Markdown under docs/, hot reloads
```

## The gate (commit acceptance criteria)

Every commit on `main` passes an 8-stage local gate AND CI. Bypassing
either with `--no-verify` is owned by the bypasser. Stages:

| # | Stage | What | Roughly |
| - | --- | --- | --- |
| 0 | secret regression scan | Looks for tokens / passwords accidentally committed | <1 s |
| 1 | `cargo fmt --check` | Code style | ~3 s |
| 2 | `cargo clippy --release --all-targets -- -D warnings` | Lints, deny tier (`unwrap_used`, `expect_used`, `panic`, `todo`, `await_holding_lock`) | 10–60 s |
| 3 | `cargo audit --deny warnings` | Supply-chain CVEs from `Cargo.lock` | 3–5 s |
| 4 | `cargo deny check` | License whitelist + banned crates + crates.io-only sources + wildcard ban | 2–4 s |
| 5 | `cargo test --release --all-targets` | All unit + integration + wiremock + proptest cases (~25 properties × 256 random cases) | 10–90 s |
| 6 | `tests/live/test_run.sh` | 87 read-only probes against a real PVE cluster | ~30 s |
| 7 | `tests/live/test_mutation.sh` | LXC 9999 lifecycle + cluster-level CRUD + QEMU 9998; opt-in QGA via `PROXXX_E2E_QGA_VMID=<vmid>` | ~60 s |

End-to-end wall time against a reachable cluster: **~340–480 s** (a
release build dominates; stages 6+7 themselves are ~100 s combined).

Hook setup:

```bash
git config core.hooksPath .githooks
chmod +x scripts/gate.sh .githooks/pre-commit .githooks/pre-push
mkdir -p ~/.config/proxxx && chmod 700 ~/.config/proxxx
cp tests/live/env.local.example ~/.config/proxxx/live-env
chmod 600 ~/.config/proxxx/live-env
# fill in PROXXX_E2E_PVE_URL / TOKEN / NODE
```

Credentials live **outside the working tree**: a live PVE token inside a
checkout is one `git add -f` or one world-readable clone away from
leaking. The harnesses read `~/.config/proxxx/live-env` first and fall
back to the gitignored `tests/live/env.local` for existing setups;
`PROXXX_E2E_ENV_FILE` overrides both.

Stages 6 + 7 require a reachable PVE cluster. If you don't have one,
**explicitly skip stages 6 + 7** in your PR description — a maintainer
will run them. Don't silently bypass.

### Script gates (CI also runs these)

| # | Command | What it enforces | Time |
| :-- | :--- | :--- | :--- |
| 6 | `ruff check .` | Python lint (`scripts/`, `assets/`) | ~1 s |
| 7 | `shellcheck -S error tests/live/*.sh scripts/*.sh` | Shell errors in the live harnesses | ~1 s |
| 8 | `gitleaks detect` | Committed secrets (CI scans history too) | ~2 s |

Added by the 2026-09-09 audit (#274): these three surfaces shipped
unchecked. `shellcheck` starts at `-S error` deliberately — the tree
carries style and warning findings that are real but not worth blocking
on today, and a gate that is red by default is a gate people learn to
ignore.

### Coverage ratchet (CI)

The CI coverage job measures line coverage with `cargo llvm-cov` and
checks it against `.coverage-floor` with `scripts/coverage-ratchet.py`
(#298). The floor is the last measured value minus 0.5pp.

- **Coverage fell below the floor:** the job fails and lists the files
  whose uncovered lines grew. Add tests for them, or remove the untested
  code.
- **Coverage rose by more than 0.5pp:** the job fails and asks you to
  raise the floor in the same pull request. The run's `coverage-lcov`
  artifact holds the new file as `lcov.info.coverage-floor`; commit it as
  `.coverage-floor`. Do not write the floor from a local run: coverage
  measured on macOS differs from the Linux CI value.

## Live-cluster verification

The single most useful thing a contributor can do is run a change
against a real cluster and report results. The harness expects:

- A 1-3 node PVE cluster (lab is fine — `pve1`, `pve2`,
  `pve3`).
- VMID 9999 free for the LXC mutation lifecycle.
- VMID 9998 free for the QEMU mutation lifecycle (boots from an
  alpine ISO; auto-cleaned via `trap EXIT`).
- Optionally a QGA-enabled QEMU VM for the agent-required round-trip
  probes — set `PROXXX_E2E_QGA_VMID=<vmid>`. The env-file template
  carries the alpine apk one-liner to install qemu-guest-agent.

Findings format (paste in PR description):

```
Cluster: PVE 9.x.y, 3 nodes, ~16 GB RAM
Local gate: PASS=88 / FAIL=0 (read), PASS=53 / FAIL=0 (mutation+QGA)
Notes: <anything you saw that the harness didn't catch>
```

## Writing tests

- **Wiremock first**: every API client method gets at least one
  wiremock test covering the URL path + form encoding + at least
  one PVE-quirky shape (numerics-as-strings, hyphen-renamed fields,
  `null` where the schema implies a value, bool-from-int).
- **Live regression after**: when a live test surfaces a wire-shape
  bug, add a wiremock that pins the OBSERVED shape — not just the
  PVE-docs shape. Real PVE drifts from its docs.
- **Pre-flight risk variants**: any new destructive op MUST add at
  least one variant to `src/app/preflight.rs` if it raises a new
  class of risk; otherwise document why an existing variant covers it.
- **No `unwrap` in production paths**. `cargo clippy` enforces.
  Tests are relaxed via `cfg_attr(test, allow(...))`.
- **Concurrent code gets a concurrent test.** A bare `#[tokio::test]`
  runs on one thread and cannot observe a race. Code that several
  callers reach at once (the HITL approval store, the audit chain, the
  TUI queue writer, any `JoinSet`/`Semaphore` fan-out, anything shared
  between the CLI, TUI, MCP and daemon processes) needs a
  `#[tokio::test(flavor = "multi_thread")]` (or `std::thread`) test whose
  callers overlap for real, behind a `Barrier`, and whose assertion fails
  when the protection is removed (break it once to check). Run it a few
  dozen times in Linux too: fsync and scheduling differ from macOS, and
  the audit key race (#482) reproduced only there. Examples:
  `hitl::pending::concurrency_tests`, the fan-out test in
  `tui::refresh_cost_tests` (#294).

### Fuzzing

`fuzz/` holds cargo-fuzz targets for the code that reads untrusted
input. Each asserts an invariant, not only the absence of a panic:

| Target | Invariant |
| :--- | :--- |
| `mcp_shape` | A value an MCP `Name` or `Upid` parameter accepts is one path segment: no separator, no relative segment, no control character. |
| `sanitize` | `sanitize_display` and `safe_text` leave no C0 (except TAB), DEL or C1 control, drop nothing else, and are idempotent; `truncate_ellipsis` stays within its bound. |
| `guest_specs` | `--ipconfig`, `--net` and `--disk` values parse without panicking, and print/parse is stable. |
| `api_json` | Deserializing PVE responses into the API types never panics. |

They need a nightly toolchain and cargo-fuzz (`.github/workflows/fuzz.yml`
pins both and runs them weekly and on PRs that touch `fuzz/`):

```sh
cargo +stable install --locked --version 0.12.0 cargo-fuzz
cd fuzz
cargo +nightly-2026-10-15 fuzz run --dev -s none sanitize -- -max_total_time=60
```

Use the dev profile: a release build of the proxxx lib with coverage
instrumentation needs more than 6 GB for one `rustc`. A crashing input
lands in `fuzz/artifacts/<target>/`; turn it into a regular test before
fixing the bug.

## Pull requests

- Branch from `main`, name like `feat/<short>` / `fix/<short>` /
  `docs/<short>`.
- Commits in **conventional style** (`feat:`, `fix:`, `docs:`,
  `test:`, `chore:`, `refactor:`); the gate enforces nothing about
  the message format but reviewers will ask you to rewrite if it
  reads as a noise.
- One concept per PR. Mixed concerns get split.
- New functionality and bug fixes come with tests in the automated
  suite: a unit, integration, wiremock or property test that fails
  without the change (see [Writing tests](#writing-tests)). A PR
  without a new test says why, for example a dependency bump or a
  docs-only change.
- Update `CHANGELOG.md` under `[Unreleased]` describing what changes
  for users.
- If the change touches a row in `pre-commit/01-feature-coverage.md`,
  update the row's status emoji + cite the test or live evidence.

## What gets refused (saving you time)

- PRs that bypass the gate (`--no-verify`, deleted hooks, soften
  clippy lints, skip stages without a stated reason).
- PRs that introduce a new dep larger than ~200 KB without a clear
  load-bearing justification — proxxx is "single static binary" by
  design.
- PRs that re-implement something PVE already does (e.g. a Perl-side
  algorithm); we shell out instead, per the architectural review.
- "Improvements" to error messages that swallow detail. Errors
  should remain typed and forensically useful.

## Scope reminder

Out-of-scope features (already declared design boundaries — won't
land):

- A web UI for proxxx itself (proxxx is terminal-only by design).
- Re-implementation of `pve-access-control` (we shell out via
  `pveum`).
- WebAuthn enrolment from the TUI (browser cert ceremony required).
- Single-file extraction from PBS archives (full archive only).

The full list lives in `README.md` → "Honest non-goals".

### Surface freeze

No new resource family lands: no new group of `ProxmoxGateway` methods
and no new declarative state family. The freeze lifts for one family at
a time, when all three of these hold:

1. Line coverage is at or above the floor checked in as `.coverage-floor`
   (#298).
2. The gateway family it would join is under its size ceiling (#293).
3. A named person or project outside this repository has asked for it in
   an issue.

The floor exists since #298. Until the ceilings (#293) exist, condition 2
cannot be met, so the freeze holds. A request that does not meet them is
labelled `frozen: awaiting-demand` and stays open: that label is the
default answer, not the exception. Fixes, tests, documentation, and
changes inside an existing family are not affected.

### Documentation claims need a test

A claim about behaviour or structure, in any Markdown page or doc
comment, either has a test that fails when the claim stops being true,
or it is not written. A claim that describes a goal rather than the code
says so ("intended", "planned"). Removing a claim that has no test is
always acceptable.

What already holds this:

- The `--help` of every command is snapshotted
  (`src/snapshots/proxxx__tests__cli_help.snap`). A renamed flag or a
  removed subcommand fails the test; review and accept an intended change
  with `cargo insta review` (or `INSTA_UPDATE=always cargo test --bin
  proxxx`, then read the diff before committing it).
- The supported library surface (`api`, `config`, `app::preflight`,
  `state::model`) carries doctests that `cargo test` runs.
- The structural claims in `ARCHITECTURE.md` are checked by
  `tests/architecture_claims.rs`.

A pull request that changes documented behaviour updates the
documentation and the test that holds it in the same change.

## Questions?

Open a discussion on GitHub or an issue. For security findings,
do not open a public issue — see `SECURITY.md`.