Docs/contributing and divergence

CONTRIBUTING.md

@@ -14,15 +14,148 @@ cargo fmt --check           # formatting check
 
 CI runs on Linux (x86_64, x86-64-v3, aarch64), macOS (aarch64), and Windows (x86_64). PRs must pass all CI checks before merging.
 
+A green `cargo test` is necessary but **not sufficient** — it is the floor, not the gate. See [Validating alignment, counting, and output changes](#validating-alignment-counting-and-output-changes) below.
+
+## Pull request standards
+
+These are hard requirements. A PR that does not meet them will be sent back rather than reviewed in detail.
+
+### You are the author
+
+Use whatever tools you like, including AI assistants — but **you** are the author, and you are accountable for every line and every claim in the PR. Before you open it, you must have read and understood the whole diff and be able to explain and defend it yourself.
+
+When a reviewer asks a question, answer it yourself, in your own words, from your own understanding of the change. You are welcome to go back to a tool to dig up specifics — file locations, exact numbers, a STAR source reference — and include them as supporting detail. What you should not do is make pasted tool output the substance of your reply: the review is a conversation between the contributor and the maintainer, not a relay for an assistant the contributor hasn't digested.
+
+### The description must match the code
+
+The PR description, CHANGELOG entry, and in-code comments are part of the contribution and are held to the same standard as the code:
+
+- **Every test named in the description must exist and pass.** Do not list a test that isn't in the diff.
+- **Every parameter, flag, or mechanism the description refers to must be present in the diff.** Do not describe behaviour the code does not implement.
+- **Do not describe dead code as if it runs.** If a function is not reached by any production code path, either wire it in or leave it out — don't ship it with a description implying it is active.
+- **Every benchmark number must be reproducible from the branch as submitted**, using a command anyone can run. Numbers produced by a locally-patched binary that isn't in the PR are not acceptable.
+- **Claims about what STAR does must be checked against the STAR source**, not asserted from memory. If a comment says "this is what STAR does", it must be verifiable in STAR's C++. A comment that misstates STAR's behaviour is a defect even if the code around it is fine.
+
+Inaccurate descriptions waste reviewer time and erode trust in the whole PR. If we find one fabricated claim, we will assume the rest of the description is unverified.
+
+### One theme per PR
+
+- One logical change per PR. Do not bundle unrelated features (a solo counting fix and a new RNG module; an index builder and two new genome types).
+- **No hidden changes.** If the diff introduces a new algorithm, a new dependency, or a change to a shared/hot code path, it must be named in the description — reviewers should never discover a 250-line stitcher the summary didn't mention.
+- No dead code. A module that changes no behaviour yet does not belong in a behaviour PR; land it with the change that uses it, behind its own validation.
+- Keep changes to shared files (e.g. `src/params/mod.rs`) minimal and additive, since most PRs touch them and will otherwise conflict.
+
+**Exception for maintainers landing their own reviewed work.** A maintainer pushing changes they have already reviewed may bundle related work into one PR to `main`, split cleanly by commit — the PR is the push, not the review, because the review already happened. This exception belongs to whoever carries that review responsibility; it is not a general licence to bundle. If you are contributing from a fork, the one-theme rule applies. A maintainer may bundle multiple PRs together when it would make sense to do so during review, and then submit a single bundled PR, closing out the included PRs. These bundled PRs should be linked to ensure tracability within github.
+
+### Divergence from STAR is allowed — but must be deliberate and flagged
+
+Diverging from STAR (including adding non-STAR flags, or choosing STAR's *documented* behaviour over its *actual binary* behaviour where they differ) is welcome, but it must be an explicit, signed-off decision — never an accident and never presented as faithfulness:
+
+- State the divergence plainly in the PR and add an entry to [`DIVERGENCE.md`](DIVERGENCE.md) with the rationale and the STAR behaviour it departs from.
+- Do not label a divergence "faithful", and do not invent a STAR flag/behaviour that does not exist and present it as parity.
+- A non-STAR flag or a change to default output behaviour needs maintainer sign-off before merge.
+
+### New dependencies need prior discussion
+
+Adding a dependency — **especially a non-Rust one** (a C library via a `-sys` crate, anything needing `bindgen`/`libclang` or a system library) — must be raised in an issue *before* the PR. This project is published to crates.io and builds on five platforms including Windows; a new C dependency is a maintenance and supply-chain decision, not an implementation detail.
+
+### Accepted-but-inert parameters
+
+If you add a CLI flag that parses but is not yet implemented, mark it as such in the parameter-surface test and document it — do not silently accept a flag that does nothing. A user passing a flag should never be quietly ignored.
+
 ## Test data
 
-Small synthetic and yeast test data lives in `test/`. Integration tests in `tests/` use the synthetic genome. Differential testing against STAR reference outputs is done via `test/compare_sam.py` and `test/compare_pe.py`.
+Integration tests in `tests/` use a bundled synthetic micro-genome and need no downloads. The differential benchmark below uses a small **public** yeast RNA-seq dataset that is not vendored; fetch it once and point `DATA` at wherever you keep it.
+
+- **Reference genome + annotation:** *Saccharomyces cerevisiae* R64-1-1, [Ensembl release 110](https://ftp.ensembl.org/pub/release-110/).
+- **Reads:** ENA run [ERR12389696](https://www.ebi.ac.uk/ena/browser/view/ERR12389696) — paired-end 150 bp yeast RNA-seq.
+
+Requires `seqtk` and a **STAR 2.7.11b** binary (built from <https://github.com/alexdobin/STAR>) on `PATH`.
+
+```bash
+export DATA=path/to/testdata          # your choice; used by every command below
+mkdir -p "$DATA"/{reference,reads}
+
+# 1. Reference genome + GTF
+cd "$DATA/reference"
+wget https://ftp.ensembl.org/pub/release-110/fasta/saccharomyces_cerevisiae/dna/Saccharomyces_cerevisiae.R64-1-1.dna.toplevel.fa.gz
+wget https://ftp.ensembl.org/pub/release-110/gtf/saccharomyces_cerevisiae/Saccharomyces_cerevisiae.R64-1-1.110.gtf.gz
+gunzip *.gz
+
+# 2. Full read pair from ENA
+cd "$DATA/reads"
+wget ftp://ftp.sra.ebi.ac.uk/vol1/fastq/ERR123/096/ERR12389696/ERR12389696_1.fastq.gz
+wget ftp://ftp.sra.ebi.ac.uk/vol1/fastq/ERR123/096/ERR12389696/ERR12389696_2.fastq.gz
+
+# 3. Deterministic subsamples. The SAME seed (-s100) on both mates keeps pairs aligned.
+#    The benchmark uses the 10k tier; test/run_tests.sh also uses the 100 and 1000 tiers.
+for tier in "100:100" "1000:1000" "10k:10000"; do
+  name=${tier%%:*}; n=${tier##*:}
+  seqtk sample -s100 ERR12389696_1.fastq.gz "$n" | gzip > "ERR12389696_sub_1_${name}.fastq.gz"
+  seqtk sample -s100 ERR12389696_2.fastq.gz "$n" | gzip > "ERR12389696_sub_2_${name}.fastq.gz"
+done
+cd -
+```
+
+Build both indices from that reference — STAR's for the reference alignments, and rustar-aligner's own. **Never reuse a STAR index for rustar-aligner**; each tool builds its own.
+
+```bash
+STAR --runMode genomeGenerate --genomeDir "$DATA/indices_star" \
+  --genomeFastaFiles "$DATA/reference/Saccharomyces_cerevisiae.R64-1-1.dna.toplevel.fa" \
+  --sjdbGTFfile "$DATA/reference/Saccharomyces_cerevisiae.R64-1-1.110.gtf" \
+  --sjdbOverhang 149 --genomeSAindexNbases 10 --runThreadN 4
+
+./target/release/rustar-aligner --runMode genomeGenerate --genomeDir "$DATA/indices_rustar" \
+  --genomeFastaFiles "$DATA/reference/Saccharomyces_cerevisiae.R64-1-1.dna.toplevel.fa" \
+  --sjdbGTFfile "$DATA/reference/Saccharomyces_cerevisiae.R64-1-1.110.gtf" \
+  --sjdbOverhang 149 --genomeSAindexNbases 10 --runThreadN 4
+```
+
+## Validating alignment, counting, and output changes
+
+`cargo test` does not catch faithfulness regressions. Anything that touches the aligner, counting, or record output **must** run the relevant differential harness against a reference produced by **STAR 2.7.11b** (<https://github.com/alexdobin/STAR>) and report before/after numbers in the PR. The dataset and both indices come from [Test data](#test-data) above (`$DATA`).
+
+**SE and PE alignment** (10k yeast reads) — this is the project's defining metric; a core-aligner PR without these numbers is incomplete.
+
+```bash
+# STAR reference alignments (the SAM the comparison diffs against)
+STAR --genomeDir "$DATA/indices_star" --readFilesCommand zcat --outSAMtype SAM --runThreadN 1 \
+  --readFilesIn "$DATA/reads/ERR12389696_sub_1_10k.fastq.gz" \
+  --outFileNamePrefix "$DATA/star_10k_/"
+STAR --genomeDir "$DATA/indices_star" --readFilesCommand zcat --outSAMtype SAM --runThreadN 1 \
+  --readFilesIn "$DATA/reads/ERR12389696_sub_1_10k.fastq.gz" "$DATA/reads/ERR12389696_sub_2_10k.fastq.gz" \
+  --outFileNamePrefix "$DATA/star_10k_pe_/"
+
+# rustar-aligner alignments (its own index)
+./target/release/rustar-aligner --runMode alignReads --genomeDir "$DATA/indices_rustar" \
+  --readFilesCommand zcat --outSAMtype SAM --runThreadN 1 \
+  --readFilesIn "$DATA/reads/ERR12389696_sub_1_10k.fastq.gz" \
+  --outFileNamePrefix "$DATA/rustar_10k_/"
+./target/release/rustar-aligner --runMode alignReads --genomeDir "$DATA/indices_rustar" \
+  --readFilesCommand zcat --outSAMtype SAM --runThreadN 1 \
+  --readFilesIn "$DATA/reads/ERR12389696_sub_1_10k.fastq.gz" "$DATA/reads/ERR12389696_sub_2_10k.fastq.gz" \
+  --outFileNamePrefix "$DATA/rustar_10k_pe_/"
+
+# compare. NOTE the arg styles differ: compare_sam.py is NAMED, compare_pe.py is POSITIONAL.
+python3 test/compare_sam.py \
+  --rustar-aligner "$DATA/rustar_10k_/Aligned.out.sam" \
+  --star           "$DATA/star_10k_/Aligned.out.sam"
+python3 test/compare_pe.py "$DATA/rustar_10k_pe_/Aligned.out.sam" "$DATA/star_10k_pe_/Aligned.out.sam"
+```
+
+Baseline to beat: SE ~99.8% and PE ~99.9% tie-adjusted faithfulness (see `CLAUDE.md`). Faithfulness must not regress; if a STAR-faithful change regresses a metric, the fix is more STAR-matching work, not a revert (a deliberate, signed-off divergence is the documented exception).
+
+- **STARsolo** counting changes: run `test/solo_diff_docker.sh` against a real STARsolo oracle. A change to default barcode/UMI counting is not validated by unit tests alone.
+- **STARlong / long-read** changes: validate against real STARlong output, not just synthetic unit fixtures.
+- **genomeGenerate / index** changes: correctness first — a suffix array is unique for a given text, so the builder's output must be shown **byte-identical** to the existing shipped path (the code path that actually ships, not a function only reached by unit tests). Any **speed or memory claim** must be measured across **multiple genome sizes** — at minimum yeast (~12 Mb), a chromosome-scale genome, and a mammalian-scale genome — with a **fair comparison**: same machine, same `--runThreadN`, same input, cold vs warm cache stated, and peak RSS measured the same way for both builders. One number on one genome is not evidence of a general improvement.
+
+Report the harness used and the raw before/after counts in the PR. "Tests pass" is not a validation result.
 
 ## Project history
 
 rustar-aligner was written as a faithful port of [STAR](https://github.com/alexdobin/STAR) by Alexander Dobin. Up to the initial release, the goal was behavioral parity with STAR — matching its algorithms, thresholds, and output formats as closely as possible. Notes from that development phase are in `docs-old/` (`docs-old/dev/` and the `phase*.md` files).
 
-Future development is not bound by that constraint. Adding STARsolo, new features, or diverging from STAR behavior is entirely welcome.
+Future development is not bound by that constraint. Adding STARsolo, new features, or diverging from STAR behavior is entirely welcome AFTER a parity version has been released. Additions that don't impact parity are welcome before that point.
 
 ## Documentation site
 

DIVERGENCE.md

@@ -0,0 +1,102 @@
+# Divergences from STAR
+
+rustar-aligner is a faithful port of [STAR](https://github.com/alexdobin/STAR) 2.7.11b: the goal is to match STAR's algorithms, thresholds, and output byte-for-byte wherever it is reasonable to do so. This file is the complete, authoritative list of the places where the two **do** differ, and why.
+
+Every entry here is a **deliberate, signed-off** decision or a **known, tracked** residual difference — not an accident. Per [CONTRIBUTING.md](CONTRIBUTING.md), a change that diverges from STAR must be recorded here, must never be presented as "faithful", and must not invent a STAR flag or behaviour that does not exist. If you find behaviour that differs from STAR and is *not* listed here, that is a bug — please open an issue.
+
+Divergences are grouped by kind:
+
+1. [Deliberate algorithmic divergences (affect alignment output)](#1-deliberate-algorithmic-divergences)
+2. [Cases where rustar-aligner produces a better alignment than STAR](#2-cases-where-rustar-aligner-outperforms-star)
+3. [Output-file metadata divergences (not alignments)](#3-output-file-metadata-divergences)
+4. [Implementation divergences with no intended output difference](#4-implementation-divergences-no-intended-output-difference)
+5. [Known residual single-read differences (tracked, not chosen)](#5-known-residual-single-read-differences)
+
+---
+
+## 1. Deliberate algorithmic divergences
+
+### 1.1 Multimapper tie-breaking / RNG
+
+**What STAR does.** STAR seeds a single `std::mt19937` per read-chunk/thread (`runRNGseed * (iChunk + 1)`) and advances that state sequentially as it processes reads. Under `--outMultimapperOrder Random`, the primary among equal-scoring loci is chosen from that per-thread stream, so the result depends on how reads are partitioned across threads.
+
+**What rustar-aligner does.** rustar-aligner parallelises **per read** via rayon, so a per-thread sequential RNG would make output depend on thread scheduling. Instead it derives a deterministic per-read seed by folding the read name into `--runRNGseed` (`per_read_seed` in `src/align/read_align.rs`), using an in-tree splitmix64 generator (`src/rng.rs`) rather than mt19937.
+
+**Why.** Determinism and thread-count invariance: the same read produces the same primary regardless of `--runThreadN`. STAR's exact mt19937 stream cannot be reproduced under per-read parallelism, and matching it would forfeit reproducibility.
+
+**Impact.** With the default `--outMultimapperOrder Old_2.4`, **no RNG is consulted at all** — the primary is the deterministic best alignment (max score → smaller genomic length → earliest discovered), which is STAR-faithful. The divergence is observable only under `--outMultimapperOrder Random`, and only in *which* equal-scoring locus is marked primary — never in the set of reported alignments.
+
+This is the reason faithfulness is reported **tie-adjusted**. On the 10k yeast benchmark, 299 SE and 475 PE primary-selection differences are all genuine ties: both tools find the identical alignment set, and differ only in which equal-scoring member is primary (from SA-iteration order or the RNG-seed difference above). Excluding those ties, SE is 99.815% and PE 99.883% exact.
+
+**Source.** `src/rng.rs`, `src/align/read_align.rs` (`per_read_seed`, `shuffle_tied_prefix`), `src/params/mod.rs` (`MultimapperOrder`). STAR: `ReadAlign_multMapSelect.cpp`, `ReadAlignChunk` RNG seeding.
+
+---
+
+## 2. Cases where rustar-aligner outperforms STAR
+
+These are not chosen divergences and not bugs: rustar-aligner reports a **higher-scoring, correct** alignment that STAR misses. They are listed here so the differential benchmark's non-exact reads are fully accounted for.
+
+### 2.1 Four PE alignments scored better than STAR
+
+On the 10k yeast PE benchmark, 4 reads differ in alignment score (AS) because STAR's combined-window stitching fails to place the pair at the better location:
+
+- `ERR12389696.844151` — rustar-aligner finds VIII:451791 with 0 mismatches; STAR reports VII:1001391 with 6 mismatches.
+- `ERR12389696.4972950` — rustar-aligner finds the correct **spliced** mate 2; STAR reports it unspliced.
+
+**Impact.** rustar-aligner's result is the better alignment in each case. These are counted against exact faithfulness in the raw metric but are improvements, not regressions.
+
+**Source.** See `CLAUDE.md` (PE status) and `STAR-RS-COMPARISON.md`.
+
+---
+
+## 3. Output-file metadata divergences
+
+### 3.1 `genomeParameters.txt` command-line header
+
+**What STAR does.** At `genomeGenerate`, STAR writes a `### <commandLineFull>` header line reproducing the full command line that built the index.
+
+**What rustar-aligner does.** rustar-aligner emits a fixed skeleton containing the parameters it knows at invocation (`--runMode`, `--runThreadN`, `--genomeDir`, `--genomeFastaFiles`, `--genomeSAindexNbases`, `--sjdbGTFfile`, `--sjdbOverhang`). The remaining value lines of `genomeParameters.txt` match STAR's `genomeParametersWrite.cpp` order and tab/space formatting.
+
+**Why.** The header is informational; reproducing an arbitrary STAR invocation's exact argv byte-for-byte serves no functional purpose and the index loads identically either way.
+
+**Impact.** The `###` header line will not byte-match an arbitrary STAR run. No effect on alignment, index loading, or any downstream tool.
+
+**Source.** `src/genome/mod.rs` (`genomeParameters.txt` writer).
+
+---
+
+## 4. Implementation divergences (no intended output difference)
+
+These differ in *how* a result is produced, not *what* is produced. They are documented so a reviewer chasing a discrepancy knows the mechanism differs by design.
+
+### 4.1 Transcriptome tables built on the fly
+
+For `--quantMode TranscriptomeSAM`, rustar-aligner builds the per-transcript exon map directly from the input GTF at run time, instead of loading STAR's persisted `transcriptInfo.tab` / `exonInfo.tab` files. The projection logic mirrors STAR's `Transcriptome_quantAlign.cpp`; the output (`Aligned.toTranscriptome.out.bam`) is intended to be equivalent.
+
+**Source.** `src/quant/transcriptome.rs`.
+
+### 4.2 In-tree RNG generator
+
+rustar-aligner uses an in-tree splitmix64 (`src/rng.rs`) rather than the `rand` crate, avoiding the `getrandom`/`zerocopy`/`ppv-lite86` dependency chain. This is the generator underlying §1.1; it is called out separately because it is a dependency/implementation choice independent of the tie-break policy.
+
+---
+
+## 5. Known residual single-read differences
+
+These are **not** deliberate divergences — they are tracked residual diffs on the 10k yeast benchmark, kept here for completeness. Each is a single read; none is a systematic behaviour difference.
+
+- **1 SE CIGAR-only diff** — `ERR12389696.13573895`: both tools align to XV:218357, MAPQ 255, identical score (AS=133), but place a 1-base insertion differently (`100M1I45M4S` vs STAR's `108M1I37M4S`). The 71-base seed is found at a different position within a long homopolymer run (a seed-level tie); resolving it requires reproducing STAR's exact Lmapped chain path.
+- **1 STAR-only PE mate** — `ERR12389696.18919121`: an SA-level difference.
+- **1 rustar-aligner-only PE mate** — `ERR12389696.6302610`: a pre-existing false positive.
+
+See `CLAUDE.md` ("Known Issues" / "PE Status") for the current status of these.
+
+---
+
+## Adding a new divergence
+
+When a change deliberately diverges from STAR (including adding a non-STAR flag, or choosing STAR's *documented* behaviour over its *actual binary* behaviour where they differ):
+
+1. Add an entry to the appropriate section above using the **What STAR does / What rustar-aligner does / Why / Impact / Source** format.
+2. Cite the STAR C++ source you checked, so the divergence can be re-verified.
+3. Get maintainer sign-off in the PR — see [CONTRIBUTING.md](CONTRIBUTING.md#divergence-from-star-is-allowed--but-must-be-deliberate-and-flagged).