Skip to main content

max / synckit

Write the contributing guide The other repos carry their coding and test patterns in CONTRIBUTING.md; synckit had none, so the two things a newcomer gets wrong first were written down nowhere: which crate a change belongs in, and that the suite has two feature configurations that both have to pass. Covers the one-way crate boundary, the three feature flags and why the self dev-dependency needs default-features = false, both test commands, the hooks, and the parts of the shared test style guide this repo leans on hardest.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-05 16:36 UTC
Signed with PGP, not checked
Commit: fe2f94eee97ac785a39dfc9ae0c47420a32a18da
Parent: 5316d28
2 files changed, +148 insertions, -0 deletions
M README.md +3
@@ -15,6 +15,9 @@
15 15 sync adapter that turns a `ConfigSpec` into a `SyncTable` lives in
16 16 `synckit-client` behind its `store` feature.
17 17
18 + Working on the code: `CONTRIBUTING.md` covers the crate boundary, the feature
19 + flags, how to run each test configuration, and the test conventions.
20 +
18 21 The synckit **server** still lives in the MNW server. Splitting it into a
19 22 standalone service on its own VPS is a separate, later effort. See the wiki
20 23 note `mnw-strategy-synckit-vps-separation`, triggered by the first external
A CONTRIBUTING.md +145
@@ -1,0 +1,145 @@
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 + | Crate | Role | Depends on |
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: synckit-client was once entirely unbuildable for iOS
65 + while compiling cleanly on every host we build on.
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.** The integration suite was one 4,201-line file
112 + until it became `tests/integration/`; that is the failure the rule exists to
113 + prevent.
114 + - **A fixture earns a place in `common.rs` when a second module wants it.** One
115 + caller, one module: leave it where it is.
116 + - **Every test owns its world.** A fresh `MockServer` per test, a fresh
117 + in-memory database. Tests must pass in any order under any `--test-threads`,
118 + so anything process-global (the rustls provider) installs idempotently behind
119 + a `Once`, which is what `ensure_crypto_provider` is for.
120 + - **Match the error variant**, not `is_err()`. `assert!(matches!(err,
121 + SyncKitError::TokenExpired))` fails for the right reason.
122 + - **No network.** `wiremock` stands in for the server. Fixtures are validated
123 + against the server's OpenAPI schemas in `tests/openapi.json`, so a wire shape
124 + that has drifted from the server fails in `contract.rs` rather than in
125 + production.
126 +
127 + Two kinds of test carry more weight here than a plain example does, because the
128 + contracts they cover are universally quantified:
129 +
130 + - **Properties** (`proptest`). An order is an order for every pair; a round-trip
131 + round-trips for every input. `proptest-regressions/` is committed, so a shrunk
132 + counterexample becomes a permanent case.
133 + - **Metamorphic relations.** Relate two runs instead of judging one, which needs
134 + no table of expected values. Pagination must not change what a pull returns; a
135 + rotation must not change the plaintext a pull yields. Both live in the
136 + integration suite and both were written because the pre-existing tests counted
137 + requests without ever looking at the content.
138 +
139 + ## Writing
140 +
141 + House rules apply to comments, docs and commit messages: no emoji, no em
142 + dashes, no AI tells. `_private/docs/meta/brand.md` is the reference.
143 +
144 + A test comment explains why the assertion is what it is. It never narrates what
145 + the next line does.