packfs/CONTRIBUTING.md

# Contributing to PackFS

## Before changing anything

Read [`concept.md`](concept.md) in full. It is the project's specification,
not background reading — every backend, lock, and on-disk field in `src/`
exists because a section of `concept.md` requires it, and most functions'
comments cite the section they implement rather than re-explaining it.
`concept.md` is frozen (see [`CLAUDE.md`](CLAUDE.md)): if you believe the
design itself needs to change, that belongs in a new document that amends
or supersedes it, not in an edit to `concept.md`.

Then read [`CLAUDE.md`](CLAUDE.md), which states the load-bearing constraints
a change must respect (static linking, the write model, the concurrency
model, path containment, pack integrity) and the documentation register this
project is written in.

## Workflow

```sh
make test                              # must pass before any PR
cc ... -fsanitize=address,undefined    # ASan/UBSan: see .gitea/workflows/ci.yml for exact flags
cc ... -fsanitize=thread               # TSan, for anything touching src/upper.c, src/overlay.c, or src/vfs.c
```

Any change to the concurrency-sensitive files (`upper.c`, `overlay.c`,
`vfs.c`) must be run under ThreadSanitizer, not just the plain test suite —
a data race there is exactly the class of bug Section 5 of `concept.md`
exists to prevent, and the plain build will not surface it.

**Known environment limitation, stated plainly rather than glossed over:**
ThreadSanitizer cannot run at all in some sandboxed/containerized
development environments — including the one this project's own commit
history was largely developed in — because TSan requires disabling ASLR
for itself via `personality(ADDR_NO_RANDOMIZE)`, and some sandboxes block
that syscall outright (confirmed here: `personality()` returns `EPERM`,
and even a trivial unrelated pthread program fails identically with
`FATAL: ThreadSanitizer: unexpected memory mapping`, not just PackFS code).
If your environment can't run TSan, say so rather than silently skipping
it or claiming verification that didn't happen — ASan/UBSan still catch
real bugs (they found and fixed a genuine heap-use-after-free during this
project's development, see the `reclaim_gate` note in `internal.h`) but do
not do TSan's happens-before race analysis, so they are not a substitute
for it. CI (`.gitea/workflows/ci.yml`, Gitea Actions — this project is
hosted on Gitea, not GitHub) runs on this project's own self-hosted
runner. **Confirmed, not just anticipated: that runner cannot run TSan
either.** The first real CI run on it failed the ThreadSanitizer step with
the exact same signature described above (`FATAL: ThreadSanitizer:
unexpected memory mapping`), consistent with the runner's job containers
using a default seccomp profile that blocks `personality()` the same way
some local sandboxes do. The CI step now detects this specific failure
signature and does not fail the build over it (while still failing hard on
any *other* TSan outcome — a real race, a crash, anything without that
exact signature) — see the step's own comment in `ci.yml` for the
detection logic. This means **CI's ThreadSanitizer step passing is not
evidence that TSan actually ran** on a given push; it may just mean the
runner couldn't start it and the step correctly didn't treat that as a
failure. Treat a change as TSan-verified only once it has actually passed
on a machine confirmed able to run it (a local machine or container with
`personality(ADDR_NO_RANDOMIZE)` available), not merely because CI is
green.

**A second, separate environment quirk, also observed directly rather than
assumed:** in the same kind of sandboxed environment, an ASan/UBSan-built
test binary occasionally (non-deterministically, roughly 1 run in 5–10 in
this project's own experience) fails to start at all, printing
`AddressSanitizer:DEADLYSIGNAL` — and, in its worse form, printing that same
line in an unbounded loop rather than printing it once and exiting, which
can run for minutes and consume unbounded CPU/memory if left unattended.
This has been observed hitting different, unrelated test binaries from run
to run (in one session: `test_dir` and `test_mem`, in another: nothing at
all, in another: `test_pack_overlay`), which — together with the fact that
every one of those binaries passes cleanly on a repeat run — indicates a
sandbox startup race (plausibly in the same family as the ASLR/mapping
restrictions behind the TSan limitation above), not a bug in the binary
being tested. **`tests/test_crash_consistency.c` hits this flake noticeably
more often than the rest of the suite** (observed 2 failures in 5 runs,
versus roughly 1-in-5–10 elsewhere) — expected, not a sign anything is
wrong with that test specifically: it forks 60+ subprocesses per run
(one per fault-injection trial), and each fork is an independent chance to
hit the same startup race, so a test that forks this much will trip it
proportionally more often. Re-run it in isolation before treating a
failure there as a real finding, same as any other `DEADLYSIGNAL` exit. **Always wrap sanitizer-build test runs in `timeout` in such
an environment** (e.g. `timeout 30 ./build/san/test_name`) so a stuck run
fails loudly and bounded instead of hanging; treat a `DEADLYSIGNAL` exit as
inconclusive, re-run, and only treat the run as a real finding if the output
contains an actual `ERROR: AddressSanitizer` or `runtime error:` string —
`DEADLYSIGNAL` alone, with neither of those strings present, is this flake,
not a memory-safety bug in PackFS. As with the TSan limitation, the correct
response is to say this plainly and re-run until a clean pass is obtained
(or hand off to CI, which runs on an unrestricted VM where this has not been
observed), not to suppress or silently ignore a `DEADLYSIGNAL` exit.

**A real gap this project's own process had, found the hard way: local
sanitizer runs had been using `ASAN_OPTIONS=detect_leaks=0`, which masked
a genuine memory leak that Gitea CI (which does not set that option) then
caught.** The override wasn't unreasonable on its own terms — a
`SIGKILL`ed forked child (as in `tests/test_crash_consistency.c`) never
runs its own exit-time leak check, so its allocations were never actually
the concern — but setting it for the *entire* test run also suppressed
LeakSanitizer for the parent process's own code, where a real bug
(`tests/test_journal_failure.c` was missing a `vfs_free(v2)` call on its
normal, expected code path, in two separate reopen blocks) went
undetected locally across many runs. **Do not add `detect_leaks=0` (or
any other blanket sanitizer-weakening option) to a local verification
habit without it also being in `.gitea/workflows/ci.yml`** — if CI and a
local run check different things, a real finding can pass locally and
only surface once it reaches CI, which is what happened here. If a fork
+`SIGKILL` test's own allocations genuinely need excluding, scope the
exclusion narrowly (e.g. a leak-suppression file naming the specific
allocation site) rather than disabling leak detection for the whole
binary.

A change to the index structure in `upper.c` (the `TreapNode`/`treap_*`/
`node_*` functions, `snapshot_upsert`, `snapshot_remove`, or the
`UpperSnapshot`/`UpperEntry` representation) should be run through
`make bench` before and after, not just `make test`: `BENCH.md` records a
real, measured O(n²) cost the flat array this index used to be had in bulk
sequential create/unlink, and the persistent treap that replaced it exists
specifically to avoid regressing back into it — a correctness-preserving
change to this code can still reintroduce that regression (e.g. an
implementation that silently degrades to a linked list under adversarial
insertion order) in a way `make test` alone cannot detect, only `make bench`
plus `tests/test_index_stress.c`'s randomized-order correctness check can.

## Scope

Changes that add functionality `concept.md` Section 10 lists as explicitly
out of scope for v0 (full POSIX semantics, enforced permissions/symlinks/hard
links, cross-process concurrency, content-defined chunking/delta
compression) should discuss the tradeoff with a maintainer first — those
exclusions were deliberate design decisions, not gaps waiting to be filled.

**`zip` and `tar` import/export backends will not be accepted, full stop —
not discussed, not behind a flag.** `concept.md` Section 11 recommends them,
but that recommendation is permanently superseded; see `CLAUDE.md`, "Project
decisions that supersede concept.md." This is a harder line than the v0
exclusions above, which are open to future discussion — this one is not.

## Static-linking constraint

`concept.md` Section 11.1 is a hard constraint: the core must build with
zero required third-party libraries and zero dynamic loading. In practice
this project has no optional-dependency boundary at all: `concept.md`
described one at the `zip`/`tar` backend for `miniz`/`libarchive`, but per
the decision above, that backend will never exist, so there is no path by
which a third-party dependency enters this codebase — a change proposing
one, for any backend, will not be accepted.

## Style

Match the register already in the file you're editing — precise,
constraint-labeled comments citing the `concept.md` section they implement,
no filler. See the "Documentation standard" section of `CLAUDE.md` for the
full rule; it applies to code comments and commit messages, not only to
`.md` files.