| 1 |
# Contributing to synckit |
| 2 |
|
| 3 |
What you need to know before changing code here: how the two crates divide the |
| 4 |
work, which feature flags exist and why, how to run each test configuration, and |
| 5 |
the conventions a new test is expected to follow. |
| 6 |
|
| 7 |
Design and strategy are not in this file. `synckit-client/docs/architecture.md` |
| 8 |
covers the `SyncStore` engine and the wire format, |
| 9 |
`synckit-client/docs/integration_patterns.md` covers consuming the SDK from an |
| 10 |
app, and the maintainer wiki holds the rest (`synckit-overview`). |
| 11 |
|
| 12 |
## Two crates, no workspace |
| 13 |
|
| 14 |
Each crate builds on its own. There is no root `Cargo.toml`, matching the |
| 15 |
ecosystem convention, so `cargo` commands run from inside a crate directory or |
| 16 |
with an explicit `--manifest-path`. |
| 17 |
|
| 18 |
|
| 19 |
|
| 20 |
| `synckit-client` | The SDK: transport, crypto, the `SyncStore` engine | `synckit-config` (optional, behind `store`) | |
| 21 |
| `synckit-config` | Local key/value settings with per-key sync postures | Nothing internal | |
| 22 |
|
| 23 |
The dependency runs one way and must stay that way. `synckit-config` is usable |
| 24 |
with no network and no SDK, which is the point: a TUI that only wants to |
| 25 |
remember a theme links it alone. The adapter that turns a `ConfigSpec` into a |
| 26 |
`SyncTable` lives in `synckit-client/src/store/config.rs`, on the client side of |
| 27 |
the boundary. |
| 28 |
|
| 29 |
Inside `synckit-client`, `src/client/` is the HTTP surface split by domain |
| 30 |
(auth, sync, blob, groups, rotation, subscription), `src/store/` is the local |
| 31 |
SQLite engine, and `crypto.rs`, `conflict.rs`, `identity.rs` are the pure layers |
| 32 |
underneath both. Pure logic belongs in the pure layer, where it can be tested |
| 33 |
without a mock server. |
| 34 |
|
| 35 |
## Feature flags |
| 36 |
|
| 37 |
Three, and the interactions matter more than the flags do. |
| 38 |
|
| 39 |
- **`keychain`** (default on) stores the master key in the OS secret service. |
| 40 |
With it off, `keystore::store_key` is a no-op stub. That is what makes |
| 41 |
rotation testable on a headless host, so the flag is a real test axis rather |
| 42 |
than a build convenience. |
| 43 |
- **`store`** (default on) brings in the `SyncStore` engine and its bundled |
| 44 |
rusqlite. A consumer that only wants transport and crypto sets |
| 45 |
`default-features = false` and skips compiling SQLite; `mnw-cli` does exactly |
| 46 |
that, so it is a supported configuration and not a hypothetical one. |
| 47 |
- **`testing`** exposes constructors that bypass key derivation |
| 48 |
(`set_master_key_raw`, `with_http_client`). Never in `default`: a consumer |
| 49 |
build must have no chosen-key injection point. |
| 50 |
|
| 51 |
`testing` reaches this crate's own tests through a self dev-dependency: |
| 52 |
|
| 53 |
```toml |
| 54 |
synckit-client = { path = ".", default-features = false, features = ["testing"] } |
| 55 |
``` |
| 56 |
|
| 57 |
`default-features = false` there is load-bearing. It adds `testing` to whatever |
| 58 |
the run already selected instead of forcing the defaults back on, which is what |
| 59 |
lets `--no-default-features --features store,testing` genuinely turn `keychain` |
| 60 |
off. |
| 61 |
|
| 62 |
The Apple targets are checked, not built, by `scripts/check-mobile-targets.sh`. |
| 63 |
Run it after touching `keystore.rs`, the `keychain` feature, or any |
| 64 |
keyring-family dependency. A target can be broken there while every host we |
| 65 |
build on compiles cleanly. |
| 66 |
|
| 67 |
## Running the suites |
| 68 |
|
| 69 |
From `synckit-client/`: |
| 70 |
|
| 71 |
``` |
| 72 |
cargo test # defaults: keychain + store |
| 73 |
cargo test --no-default-features --features store,testing # keychain off |
| 74 |
``` |
| 75 |
|
| 76 |
Both configurations have to pass. They are not the same set of tests: the |
| 77 |
rotation orchestration module is gated `#[cfg(not(feature = "keychain"))]` |
| 78 |
because `rotate_key` finishes by caching the new key through the OS secret |
| 79 |
service, which a headless host does not have. |
| 80 |
|
| 81 |
Integration tests live in one binary, `tests/integration/main.rs`, with one |
| 82 |
module per domain and shared fixtures in `tests/integration/common.rs`. The |
| 83 |
`[[test]]` block in `Cargo.toml` names that path explicitly; without it, |
| 84 |
`mod common;` would resolve to `tests/common.rs` and miss the directory. |
| 85 |
|
| 86 |
`synckit-config` is a plain `cargo test`. |
| 87 |
|
| 88 |
Before committing, run `cargo fmt` and |
| 89 |
`cargo clippy --all-targets` (both feature configurations). Clippy is |
| 90 |
`pedantic`-with-an-allow-list, and the allow-list block is kept identical across |
| 91 |
repos, so extend it in all of them or in none. |
| 92 |
|
| 93 |
Activate the hooks once per clone: |
| 94 |
|
| 95 |
``` |
| 96 |
git config core.hooksPath scripts/githooks |
| 97 |
``` |
| 98 |
|
| 99 |
That gates commits on gitleaks and rustfmt. |
| 100 |
|
| 101 |
## Test conventions |
| 102 |
|
| 103 |
The full standard is `_private/docs/meta/test_style.md` and it applies here |
| 104 |
unchanged. The parts this repo leans on hardest: |
| 105 |
|
| 106 |
- **Names are prose, no `test_` prefix.** `push_retries_on_503`, not |
| 107 |
`test_push_retry`. Name the outcome, not the function. |
| 108 |
- **Unit tests go at the bottom of the file they test**, in one |
| 109 |
`#[cfg(test)] mod tests`. A unit test that reaches for a database or a mock |
| 110 |
server is an integration test in the wrong file. |
| 111 |
- **No file over ~800 lines.** Split a suite that outgrows it into a directory |
| 112 |
of per-domain modules. |
| 113 |
- **A fixture earns a place in `common.rs` when a second module wants it.** One |
| 114 |
caller, one module: leave it where it is. |
| 115 |
- **Every test owns its world.** A fresh `MockServer` per test, a fresh |
| 116 |
in-memory database. Tests must pass in any order under any `--test-threads`, |
| 117 |
so anything process-global (the rustls provider) installs idempotently behind |
| 118 |
a `Once`, which is what `ensure_crypto_provider` is for. |
| 119 |
- **Match the error variant**, not `is_err()`. `assert!(matches!(err, |
| 120 |
SyncKitError::TokenExpired))` fails for the right reason. |
| 121 |
- **No network.** `wiremock` stands in for the server. Fixtures are validated |
| 122 |
against the server's OpenAPI schemas in `tests/openapi.json`, so a wire shape |
| 123 |
that has drifted from the server fails in `contract.rs` rather than in |
| 124 |
production. |
| 125 |
|
| 126 |
Two kinds of test carry more weight here than a plain example does, because the |
| 127 |
contracts they cover are universally quantified: |
| 128 |
|
| 129 |
- **Properties** (`proptest`). An order is an order for every pair; a round-trip |
| 130 |
round-trips for every input. `proptest-regressions/` is committed, so a shrunk |
| 131 |
counterexample becomes a permanent case. |
| 132 |
- **Metamorphic relations.** Relate two runs instead of judging one, which needs |
| 133 |
no table of expected values. Pagination must not change what a pull returns; a |
| 134 |
rotation must not change the plaintext a pull yields. Both live in the |
| 135 |
integration suite. Assert on content, not on request counts. |
| 136 |
|
| 137 |
## Writing |
| 138 |
|
| 139 |
House rules apply to comments, docs and commit messages: no emoji, no em |
| 140 |
dashes, no AI tells. `_private/docs/meta/brand.md` is the reference. |
| 141 |
|
| 142 |
A test comment explains why the assertion is what it is. It never narrates what |
| 143 |
the next line does. |
| 144 |
|