Contributing & code review

This is the canonical guide for what review measures your PR against — the rules of the game, why each rule exists, and what is explicitly not a reason to block your PR.

If you’re looking for how to set up, run, test, and debug the project, that’s the Development guide. The two pages are deliberately split: that one is workflow, this one is policy.

The root CONTRIBUTING.md on GitHub is a short pointer (clone, build, commit format, license) that links into both. Everything substantive lives across these two pages.

We want outl to be a project where you can show up, read this page, and know exactly what you’re walking into. No tribal knowledge, no hidden quality bar.

The same priorities are encoded for automated review in .github/copilot-instructions.md. If you ever feel a reviewer comment came out of nowhere, it almost certainly traces back to this page or that file.

---

Philosophy

outl is a sync engine first and a notes app second. The CRDT layer underneath has to be correct, because the cost of a sync bug is silently-corrupted user data — exactly the failure mode Roam and Logseq ship and exactly the one we exist to not repeat.

That shapes how we review. We are strict about the things that touch correctness, scalability, and the public contract. We are deliberately relaxed about the things that don’t.

Concretely:

• Correctness > cleverness. The CRDT follows Kleppmann et al. 2022 literally. A “smarter” version is a regression.
• Simple > abstract. We invoke the Rule of Three before introducing a trait or generic. One implementation gets a concrete type.
• Real problems > hypothetical ones. A refactor needs to unblock something or pay down a named debt. “Cleaner code” alone is not a merge reason.
• The user’s .md is sacred. It belongs to the user. We never write metadata into it, even when it would make our life easier.

If you read this and think “they’re going to be brutal in review” — not the goal. The goal is for you to read this before writing the patch, so the review goes “ship it” instead of “back to draft”.

---

Before opening a PR

Reviewers (human and automated) check the PR description before the diff. If the description doesn’t answer these, the PR gets a top-level comment requesting changes and the line-level review is deferred.

1. Link an issue. Closes #N, Fixes #N, or Related to #N.
   • For typo fixes, doc-only changes, or dependency bumps with a changelog link, you can skip this.
   • For everything else, an issue is the place to debate scope and approach. PRs are the place to debate implementation.
2. State the problem in plain language. One paragraph. The user-facing problem first, then the technical approach.
3. For a refactor, answer “why now?”. “The code is cleaner” is not enough — name the concrete thing this unblocks or the debt it pays down.
4. For a bug fix, describe the bug. Steps to reproduce, observed behaviour, expected behaviour. Ideally a failing test that the patch turns green.
5. For a feature, point at an approved issue. If a feature isn’t in an accepted issue, open the issue first.

The template at .github/PULL_REQUEST_TEMPLATE.md walks you through this. Use it.

---

Non-negotiable invariants

These are blockers in review. A PR that violates one will not merge regardless of how clean the code looks. You’re welcome to disagree with any of them — the path for that is an issue, not a PR.

1. The op log is the source of truth

Every mutation flows through Op → Workspace::apply → log. The materialized tree and the .md files are projections of the log.

Why: sync replays the log. If a mutation skips the log, the second device never sees it. If you write to .md to “fix” state, you’ve created a divergence the next sync round won’t catch.

2. Markdown stays 100% clean

No id:: lines. No inline UUIDs. No HTML comments carrying state. Stable IDs live in the .outl sidecar (a JSON file next to the .md).

Why: the .md belongs to the user. They open it in vim, ship it to a wiki, paste it into a chat. The day we make their file ugly to serve our internal needs is the day we became Logseq.

The sidecar isn’t a dotfile, by the way — iCloud silently drops dotted paths during cross-device sync.

3. The CRDT matches the paper

do_op, undo_op, apply_op, and creates_cycle in outl-core/src/tree.rs follow Kleppmann et al. 2022 literally. These four functions carry a 100% line and branch coverage rule.

Why: the paper has a formal correctness proof. Any deviation — even one that looks like a perf win — moves us off the proof, and we have no other way to argue we converge. A new branch without a test is a blocker.

4. A cycle-creating move is a deterministic no-op

If a Move op would create a cycle, the materialized tree ignores it. But the op still goes into the log.

Why: the log is total-ordered, and replaying it on every device must produce the same materialized tree. If device A drops the op and device B keeps it, a future re-parenting op that would have been valid now diverges. Keep the op, no-op the effect.

5. Storage is a trait, not a struct

outl-core does not import rusqlite, doesn’t write JSON directly, doesn’t know about files. Everything goes through dyn Storage. Today the only persistent implementation is JsonlStorage; MemoryStorage is used in tests.

Why: ChronDB (issue #1) is the next backend, and we’ve already paid the cost of removing SQLite in 0.5.0. Locking the core to a concrete backend reintroduces that cost. A second persistent backend doesn’t land without an RFC issue first.

6. Delete is Move(node, TRASH_ROOT)

There is no physical removal of a node. Delete moves it under a sentinel root.

Why: the algorithm in the paper is defined over a tree where nodes are never removed. Physical deletion would either break undo, break replay, or both.

7. Convergent state goes through the op log

If two actors can disagree about a value and you want them to reconcile, model it as an Op. Writing convergent state into a shared file — including the sidecar — bypasses the CRDT and loses concurrent writes silently.

Why: every shared file is last-write-wins under any file-system transport (iCloud, Syncthing, Dropbox). Per-actor ops-<actor>.jsonl files turn that into a non-issue: each device writes its own file, the CRDT merges on replay, with HLC ordering for determinism. The canonical example is Op::SetCollapsed for fold state.

The sidecar is reserved for structural matching metadata only — ids, position, content hash, ref handle. Not a sync surface.

8. Layering

• outl-core never imports a UI or CLI crate.
• outl-actions is the shared workspace-mutation surface. Every client (outl-tui, outl-mobile, future shells) calls into it.
• A client reimplementing logic that belongs in outl-actions is a blocker. Reviewers will point at the existing function and ask you to call or extend it.

Why: the mobile app and the TUI must behave identically for the same semantic operation. If toggling a TODO in the TUI emits different ops from toggling one in the mobile app, users see ghosts on sync. One source of truth per concept.

9. No reintroduction of SQLite, rusqlite, or binary log formats

Cross-device sync depends on per-actor append-only JSONL.

Why: 0.5.0 removed SQLite specifically to enable iCloud / Syncthing / shared-FS workflows. Binary formats and DB files don’t merge across those transports.

10. Settled decisions are off-limits in a PR

ULID for IDs, uhlc for time, MIT license, JSONL-per-actor, Tauri for mobile, iroh as the default sync transport (file/iCloud opt-in), comrak for markdown. These were debated and chosen at the start. The full table is in the root CLAUDE.md and CONTRIBUTING.md.

Why: these are foundational. Changing one ripples through every crate. The path to revisit is an issue with rationale, not an inline review comment.

---

What reviewers will look at

Rust quality

The bar is production code, not “it compiles”:

• No .unwrap() outside #[cfg(test)]. Use .expect("explicit reason") or propagate with ?. The expect message must name the invariant being asserted — “should not fail” is not a reason.
• No .unwrap_or_default() that masks an error path. If the default would be a silent data-loss bug, return the error.
• No unsafe in outl-core without a // SAFETY: comment naming the invariants the caller upholds.
• thiserror in libraries (outl-core, outl-md, outl-actions) so callers can match on variants. anyhow is for binary boundaries (outl-cli, outl-tui).
• Async hygiene. No blocking calls inside async fn. No Mutex / RwLock held across .await.
• API ergonomics. Public API prefers &str over String, &[T] over Vec<T> when ownership isn’t needed.
• Public API changes are documented. If you change a function or type that other crates use, update the doc-comment and the per-crate CLAUDE.md.

Performance — hot paths only

We care about performance in the paths that actually run a lot. Allocations in setup, error paths, or one-shot CLI commands are not worth a review comment.

The hot paths in outl:

• outl_core::tree — every op apply, every materialized-tree walk.
• outl_core::log — every append, every replay (boot, sync pull).
• outl_md::parse / outl_md::render — every .md read/write, every TUI buffer refresh.
• outl_md::index — backlink index rebuild, scales with workspace size.
• outl_tui render loop — runs on every keystroke.
• outl_actions::SyncEngine work loop — runs on every file event.

In those paths, reviewers will flag:

• .clone() on String, Vec, or large structs where a borrow works and the clone is per-call (not one-time setup).
• .to_string() / format!() when &str or deferred Display would do.
• Vec::new() + repeated push in a loop where capacity is knowable (Vec::with_capacity).
• Re-parsing the same markdown or re-walking the same subtree on every keystroke — propose caching with a clear invalidation story.
• Big-O regressions on tree ops or backlink computation.

If you’re unsure whether code is on a hot path, ask in the PR — we’d rather a question than a guess.

Architecture, scalability, extensibility

This is where review earns its keep:

• Reuse-first. Before adding a helper, scan the shared primitives catalog and grep upstream crates (outl-core → outl-md → outl-actions). Two implementations of “compute backlinks” eventually disagree, and the user is the one who notices. See the Reuse-first section below for the rule, past incidents, and what to do when the primitive doesn’t exist yet.
• New Op variants come with the full checklist. A new variant touches apply_op, undo_op (the inverse must be exact), the sidecar serializer, the markdown projection, replay tests, and per-crate docs. See /new-op for the walkthrough.
• Trait surface stays implementable. A Storage method that assumes file semantics (paths, flock) locks out ChronDB. Push back on that.
• Sidecar / op-log format changes need a migration story. Existing workspaces on disk must still load. Either the change is backward-compatible (new optional field) or the PR ships a versioned migration.
• File size discipline. Past 600 lines, plan a split by responsibility. Past 900, refactor before the next non-trivial edit. The refactor-architect agent will propose a split.
• Premature abstraction is rejected. A new trait or generic with one impl and no named second caller doesn’t merge. Rule of Three — concrete first, abstract on the third caller.

Simplicity

• A new dependency for two functions of std code needs a real justification — crate size, maintenance status, transitive deps, licence.
• A configuration knob without a concrete user asking for it doesn’t merge. Defaults that are right for the 90% case beat knobs nobody tunes.
• Cleverness loses to readability. If a reviewer has to run code in their head to understand it, the next maintainer pays.

Testing

• Bug fix without a regression test → blocker. The test must fail on main and pass with the patch.
• Critical path touched without coverage proof. Run /coverage outl-core (or the relevant crate) and paste the result. 100% coverage on the four CRDT functions and on outl_md::reconcile_md is non-negotiable.
• Tests assert behaviour, not implementation. A test that breaks on any refactor is a maintenance tax. Assert against the public surface (op log contents, materialized tree shape, rendered markdown), not internal helpers.
• Integration tests use the real backend. Mocked storage in a path that should hit JsonlStorage hides exactly the bugs that matter.

---

Reuse-first (no parallel implementations)

Before adding a helper, struct, or constant, scan the shared primitives catalog and grep the workspace for what already does the same thing. Duplication here is a real hazard: two implementations of the same logic drift apart over time, and the user is the one who hits the divergence.

Past incidents:

• outl_md::index::Backlink and outl_actions::Backlink were two parallel “backlinks” pipelines that started identical and ended up disagreeing on self-references — a bug the user had to spot because each surface looked fine in isolation. Collapsed into outl_actions::backlinks_for_page in 0.5.3.
• outl-mobile’s run_code_block Tauri shim was opened as a copy of outl-desktop/src-tauri/src/commands/exec.rs — same flat_index_for walk, same journals/<slug>.md || pages/<slug>.md probe (which already existed as outl_actions::page_md_path), same DTO shape. The catalog at the time did not list code execution as a cross-client primitive, and the desktop’s per-crate CLAUDE.md filed commands/exec.rs under “owned by the desktop”, so nothing pointed at the right home. Caught by the user mid-PR: “do que fizemos de rodar code block no mobile, não conseguimos compartilhar?” Collapsed into outl_actions::exec::run_code_block in 0.6.x — clients now own only the AppState lookup and the view wrapper. Lesson: if you’re staring at a Tauri shim that’s mostly parse_node_id → outline walk → outl-exec call → DTO, the walk and the DTO belong in outl-actions — every time.
• The Logseq importer’s crates/outl-cli/src/cmd/import/normalize.rs was opened reimplementing \r\n handling, id:: stripping, and long-form date rewriting — every one of which outl_actions::paste::normalize_external_syntax already owned. Caught in PR #47 review. Lesson: a “normalize markdown from outside” need always starts at paste::normalize_external_syntax; outline-level restructuring (headings → bullets, multi-paragraph merge, fence dedent) is the only thing the importer adds on top.

The rule:

1. Grep before writing. rg "fn foo" / rg "struct Foo" across crates/. Look in upstream crates first — outl-core, outl-md, outl-actions are where shared primitives live.
2. Prefer evolving the existing API over duplicating, even if that means a small refactor (rename, generalize a parameter, move into a sibling module). One owner per concept; many callers.
3. Duplication is OK only when the platforms are genuinely different. outl-tui::EditBuffer and the mobile <textarea> are both “cursor + text” — but one is a terminal widget Rust has to render itself, the other is a browser primitive. Same role, different runtime; not duplication. Recalculating (line, col) from cursor in both places, though, would be — extract to outl_md::view::char_to_line_col and let both wrap it.
4. Refactor into the shared crate, not around it. If a TUI helper feels like it could live in outl-actions, move it there now (the mobile client will need it soon). The flatten_subtree_paths migration is the canonical pattern.

When in doubt, name the would-be helper, search for it, then ask yourself: “is the existing thing one rename away?” If yes, rename.

---

Keep docs in sync with code

docs/development.md is the engineer onramp. It drifts the moment a CI workflow, a slash command, a hook, or a per-area toolchain step changes — and a stale onramp is worse than no onramp because a new contributor follows it confidently into a wall.

Treat the table below as a checklist. If your PR touches any row on the left, update the doc on the right in the same PR — not “later”, not “in a follow-up”. The doc-keeper agent runs at the end of a feature to catch what slipped through; the discipline is to not let it slip in the first place.

When in doubt: if a contributor’s first 30 minutes with the repo would land them on outdated guidance, update the doc. That’s the bar.

One owner per fact — link, don’t duplicate

Every user-facing fact lives in exactly one docs/*.md. CLAUDE.md files link to it instead of copying. A keybinding, a CLI subcommand, a slash command, a CRDT op variant, a config field, a sidecar field — each has one canonical home under docs/, and the matching CLAUDE.md (root or per-crate) links to it. It does not enumerate the same rows in its own table.

Why this matters: every duplicated table is a future drift incident. We’ve already hit it on docs/shortcuts.md ↔ per-crate vim tables (the desktop’s CLAUDE.md ended up listing chords that drifted from defaults.rs within one sprint) and on outl-tui/CLAUDE.md’s Navigation / Insert tables (rebound Ctrl+E for sidebar, doc kept showing \). When the same row lives in two places, half the time the second copy goes stale silently and the contributor following it walks into a wall.

The discipline:

• docs/*.md is the canonical surface for users + contributors. Tables, full chord lists, every config key, the full subcommand matrix — they live there.
• CLAUDE.md (root or per-crate) carries only what docs/ cannot: invariants, “architectural decisions you don’t get to revisit”, crate-specific contracts (which methods are the entrypoint, what the layering rule is), the reasoning behind a choice. Things a contributor needs before touching code — not user-facing reference.
• When the same fact would live in both, the CLAUDE.md writes a one-line link: > User-facing X lives in [docs/X.md](../docs/X.md) — don't duplicate it here. and stops.
• If you find yourself copying a table from docs/ into a CLAUDE.md, stop. Replace with a link.
• The other direction is fine. docs/*.md linking into CLAUDE.md for architectural context is welcome — that’s a one-way pointer toward depth, not duplication.

Map of canonical homes (extend as new ones are minted):

When you add a brand-new surface (a new CLI subcommand, a new Op variant, a new MCP tool, a new theme, a new client), it follows the same rule:

1. Document the surface in the right docs/*.md (create a new one if needed).
2. The per-crate CLAUDE.md adds a one-line link if it needs to point contributors at it, plus any architectural note that cannot live in docs/ (invariant, contract, “why this decision”).
3. Update the map above so the next contributor doesn’t have to rediscover where things live.

doc-sync-guard.sh is a backstop, not the discipline. The hook flags drift after the fact; the discipline is to link in the first place so drift can’t happen.

---

Markdown / documentation style

Never hard-wrap prose at an arbitrary column. We use semantic line breaks: one sentence per line, breaking after sentence-ending punctuation (., !, ?) and sometimes after : when followed by a substantial clause. Lines stay as long as the sentence is — no 70/80/100-column reflow.

Why: hard-wrapping at ~70 chars breaks lines mid-thought, makes diffs noisier on edits, and renders ugly in editors that already soft-wrap. Semantic line breaks keep diffs minimal (an edit touches one line, not a paragraph block) and read naturally on every surface (GitHub, mdBook, terminal pagers).

Rules:

• Prose: one sentence per line. Don’t break inside a sentence.
• Lists: each list item on its own line; if a single item contains multiple sentences, break those sentences too.
• Code fences, tables, YAML frontmatter, ASCII tree diagrams: preserve exactly. Tables especially must stay one row per line, no matter how wide.
• Headings, HRs, link references: one line, as always.
• Outline / .md content (anything under note-example/, real workspace pages, fixtures): do not touch. That markdown is data, not docs — it represents the outl dialect literally and indentation / line shape is structural.

This rule applies to every *.md in the repo except outline content (see exception above): root CLAUDE.md, per-crate CLAUDE.md, docs/*.md, README.md, CHANGELOG.md, CONTRIBUTING.md, SECURITY.md, .github/*.md, .claude/agents/*.md, .claude/commands/*.md.

---

What we will not block your PR for

These are noise. If a reviewer comments on one of these without a behavioural reason, push back politely:

• Style and formatting — cargo fmt, cargo clippy -D warnings, and rustdoc warnings handle them in CI.
• if let Some(x) = y vs match y { Some(x) => ... } with no behavioural difference.
• “I would have named this differently” without a concrete clarity win.
• Speculation about a future architecture that nobody asked for.
• Re-litigating settled decisions (ULID, MIT, JSONL, Tauri, etc.).
• Adding TODOs the author already acknowledged in “Out of scope”.

---

What we are not building yet

outl ships continuously; the CRDT core, TUI, CLI, desktop, iOS mobile, plugins, and code-block execution are all in. Reviewers will push back on PRs that try to introduce these without an explicit issue:

• Query DSL ({{query: ...}}).
• ChronDbStorage backend (issue #1).
• Android mobile build (iOS only today).
• Per-page op log shards (Per-page op log shards in docs/sync.md, only when the workspace hits 10k pages).

If your PR genuinely belongs to one of these, link the tracking issue.

---

Disagreeing with a review

We want this project to be inclusive, which means we want to hear when you think a reviewer is wrong. The expectation is:

• Disagree on the substance, in the thread. Cite the invariant or the decision you think the reviewer is misapplying.
• For settled decisions (“ULID is wrong for us”), open an issue with the rationale — those debates belong in design discussions, not in a code-review thread.
• For new opinions (“this trait surface is over-abstracted”), the PR itself is the right place. Reviewers will engage.

The job of a reviewer here is to protect the invariants and the user’s data, not to enforce taste. If a comment crosses that line, say so.

---

The agents that help

We use specialised review agents (configured in .claude/agents/):

• crdt-invariant-checker runs after any change in outl-core/src/{tree,log,op}.rs. Validates convergence, idempotency, cycle handling, coverage.
• paper-verifier compares the Rust against the paper line by line after edits to the four critical CRDT functions.
• markdown-roundtrip-tester runs after outl-md/ changes. Validates roundtrip stability and matching invariants.
• refactor-architect is invoked when a file crosses the 600-line threshold. Proposes a split by responsibility.
• doc-keeper runs at the end of every feature that changes public API, markdown syntax, TUI shortcut, sidecar format, or user-observable behaviour.

If you’re using Claude Code locally, these run automatically. If not, they run in CI on the PR.

---

Where to look next

• Development guide — how to clone, build, run, test, and debug. Pairs with this page.
• Root CLAUDE.md — project-wide invariants and conventions.
• Per-crate CLAUDE.md (e.g. crates/outl-core/CLAUDE.md) — invariants specific to that crate.
• docs/architecture.md — design decisions.
• docs/crdt.md — the algorithm.
• docs/markdown-format.md — the markdown dialect and sidecar spec.
• docs/storage.md — the Storage trait.

Welcome aboard.