Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .claude/development-notes/brainstorming.md
Original file line number Diff line number Diff line change
Expand Up @@ -256,3 +256,33 @@ The catalogue's two headers already separate layout from content — `#!index-fo
After 3.0.0, and nothing about it is urgent while Z is the only person developing modules.

**One thing gets harder by waiting, and the cost of getting it wrong is bounded.** `MODULE_INDEX_FORMAT="1"` compiles into every released copy and `require_module_index_format` is an exact-match refusal, so catalogue columns added later — `frame` and `environment`, which is what would let the repository hand an older pipeline a version that still runs on it — are unreadable by 3.0.0. The consequence is not data loss or a broken install: a 3.0.0 user who wants a module published later is told to upgrade. That may simply be acceptable for the first release of the module system, and it is Z's call rather than a deadline anyone has to meet.

---

## The catalogue's columns are matched by name and read by position — Z, 2026-09-10

Added when the `kind` column landed and cost thirteen failing cases. **Z deferred it explicitly: "We can look at the name-position thing later. Add it to the list of post v3.1.0 tasks."**

`index.tsv` was designed so that adding a column is free. Its own header says so, at length: *"THE COLUMNS ARE READ BY NAME, out of the header row below, so their order here does not matter and a column a release has never heard of is ignored rather than misread. Adding one later is therefore safe and needs no layout bump."* That is true, and it is the reason `#!index-format` is not bumped for an addition.

**It is true of every release except the one doing the adding.** `module_index_rows()` reorders each row into `MODULE_INDEX_COLUMNS` order and joins it with `$'\037'`; every consumer then reads it back with a positional `IFS="$MODULE_INDEX_SEP" read -r name version contract frame environment url sha summary`. So the by-name matching happens once, at the top, and everything downstream is positional. Inserting `kind` second shifted six `read` destructurings, an `awk` filter keyed on `$2`, and a `sort -t"$SEP" -k2,2Vr` that silently stopped sorting by version and started sorting by kind.

### What it would buy

A column addition that costs one line instead of thirteen cases. More to the point, **the failure mode is silent where it matters most**: the `sort` did not error, it just returned the wrong newest version. Two of the thirteen failures were assertions going vacuous rather than red, and one of those only announced itself because it carried a `rows > 0` guard. A wrong column is not a crash, it is a plausible answer.

### The shape it probably wants

An accessor rather than a destructuring — `row_field "$row" version` reading the same `MODULE_INDEX_COLUMNS` list that built the row. Then a column's position is knowledge held in exactly one place instead of at every call site, and `MODULE_INDEX_COLUMNS` becomes the single declaration it was always meant to be.

The cost is a subshell per field per row where there is now one `read` per row. `available` renders every row in the catalogue, so this is measurable and worth measuring before committing to it; a catalogue is tens of rows today and the answer may simply be that it does not matter.

### What it would break

Nothing published. The wire format is unchanged — this is entirely how the wrapper reads a row it already has. It does not touch `#!index-format`, so no released copy notices.

**It does touch every consumer at once**, which is exactly the change that wants a full suite behind it rather than a `--case` run. That is the argument for after a release rather than during one.

### Where it sits

**After v3.1.0.** Nothing is wrong today: the columns and the destructurings agree, and `00_static` checks every row against the file it advertises. This is paying down the next addition's cost, not fixing a defect — and the next addition is not scheduled.
58 changes: 58 additions & 0 deletions .claude/development-notes/check-split-and-layout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# `check install` / `check project`, and the directory move

**Written 2026-09-10 against the working tree on top of `6d38c88`.** Z drove this one directly, in a series of short instructions; the reasoning below is what each of them settled.

## The shape problem Z named

`install/check_install.sh` had grown to check three things: the tools, the `bin/` helpers, and whether `parameters.config` parses — resolving the tool list out of `params.software` when a project was present and falling back to a canonical list when it was not. Taking `parameters.config` out of it (Z: *"We changed how install worked entirely. Checking parameters.config is just going to cause confusion"*) left a hardcoded `CANONICAL` list that duplicated the template's `software` block, and left nothing checking a project at all.

Z, on being shown that: ***"It is not the error I'm concerned about it is the shape. The tools were checked against the params.config."***

Moving `params.software` into `nextflow.config` was tried and **reverted** — Z: ***"Tools remain in params.config."*** The answer was two commands instead: ***"We need a separate check install and check project."***

## What each one is

| | |
|---|---|
| `check install` | the tools a release is built to run, **and that each comes from the release's own conda environment**; every helper in `bin/`. Reads no `parameters.config` and needs no project. |
| `check project` | `parameters.config` is current for this release and parses; `metadata.csv` and the run table parse, through the parsers step 0 uses; and every command **as `params.software` names it**. |

**A bare `check` is refused.** Z chose this over keeping it as an alias: whichever one it picked would leave the other unchecked while reporting success. `check` is the second subcommand after `analysis` to carry a word of its own, which the argument block at the top of the wrapper had to be taught — every other subcommand takes none, so `check install` was rejected before reaching its arm.

**`run_check()` is a function and not a nested `case`.** Two suite cases read the wrapper's own case arms and compare them against its usage line; a nested `install)` at the same indentation is read as a top-level subcommand. The top-level usage says `check <target>`, following the `analysis <command>` convention, because a nested `{install|project}` breaks a flat split on `|`.

## The environment check, which is the part that was silently wrong

`check_tool` asked `command -v` and accepted whatever `PATH` returned. **Every tool in `CANONICAL` is pinned in `install/environment.yml`** — verified, all fourteen including `gawk` and `python` — so one resolving from anywhere else means the environment is missing a package and the machine's own copy is standing in, at another version, on this machine only. The run works and reproduces nowhere.

It now compares each resolved path against `CONDA_PREFIX` and reports `OUTSIDE THE ENVIRONMENT` as a failure. When the environment is not active there is nothing to compare against, and the header says so rather than checking `PATH` and calling that an answer.

**`check project` deliberately does NOT apply that rule.** Repointing a tool at a system binary is a thing a project is allowed to do, and that check is where you see the result.

**A fixture that disabled the check under test.** The first version of these cases named the fake environment directory `env` while passing `ENV_NAME=check-install-env`; the script only compares paths when `basename $CONDA_PREFIX` matches `ENV_NAME`, so the comparison was off and both cases passed over nothing. The fixture directory is named for the environment now, and the passing case asserts the `from <prefix>` header line — which appears only when the script decided it knows which environment it is in, and is therefore what stops the case going vacuous again.

## The directory move

Z: ***"We also should move check_install, check_analysis_install and check_projects to bin"*** and ***"install only contains environment yml files. citations and bib file should move to another folder citations."***

```
bin/ every script that is RUN rather than sourced, the three checkers included
lib/ every file that is SOURCED
install/ the two pinned environment files, and nothing else
citations/ the pipeline's own references.bib and the citations.json generated from it
```

`analysis/` keeps its own `citations.json` and `references.bib`, and so does each module — **every separately publishable unit keeps its bib beside it**. The pipeline's pair had no home but `install/`, which is what `citations/` fixes.

Two things the move broke and how:

- **`check_install.sh` enumerates `bin/*` as pipeline helpers**, so it would have reported itself and its two siblings as helpers a run depends on. The three are skipped by name.
- **`07_analysis_frame`'s one-way-dependency case** greps `bin/` for any mention of the analysis layer, and `check_analysis_install.sh` is full of them. It is excluded **by name**, not by pattern, and a second case asserts exactly one file matches `bin/check_analysis*` — otherwise a rename or a second script widens the exclusion silently, since `--exclude` takes a glob and a name matching nothing is not an error.

`citations` was added to `PAYLOAD_ITEMS`, to the pipeline sandbox's copy list, and to `00_static`'s archive case — which now also asserts four **named files** and not only their directories, because a directory traveling empty satisfies every `assert_contains` on a path prefix.

## State when this was written

`--cost static` 272 passing, `--fast` 322 passing, `nextflow lint` 32 files clean, manual 41 pages / 361 anchors, language sweep clean, every analysis version current.

**`dev/scripts/verify-archive.sh` defaults to `HEAD`**, so it passed against the old layout and proves nothing about this one until the move is committed. Re-run it then.
74 changes: 74 additions & 0 deletions .claude/development-notes/module-optionality.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# Modules are optional — the design change, and the audited blast radius

**Written 2026-09-10 against the tree at `91e027f` plus three uncommitted edits.** v3.0.0 was released earlier the same day, *with* the modules inside it. Nothing here is a defect in the released version: v3.0.0 works exactly as shipped. This is about what the NEXT release has to be, and why.

## Z's ruling

Z, 2026-09-10: *"The modules should not ship with the release. That's the whole idea."* And, when the analysis environment came up: *"The whole idea was to make the modules optional."*

Three parts follow from it:

1. **No module ships inside a release tarball.** The store starts empty; every module is installed from the catalogue.
2. **Each module declares its own conda pins** in its manifest, and `modules install` puts them into the shared analysis environment — the E8 machinery, which already exists.
3. **The baseline analysis environment slims** to what the FRAME needs, and stops carrying module dependencies.

## What was actually wrong, stated precisely

**The architecture was never wrong. The data was.**

The per-module conda machinery is built and was proven against real conda on 2026-09-10 by `dev/scripts/check-module-packages.sh` — every check `ok`: a fixture module's pins installed under `--freeze-installed`, R imported them, a second module sharing a pin moved nothing, uninstalling one left the other's packages alive, and the baseline never lost a package. `PoolSeqFlow:786-794` is the deferred install; `:1433` is the reconcile that `analysis install` performs over modules already in the store. Neither is missing anything.

**The three shipped modules declare `packages: []`.** That was TRUE while they shipped inside the release, because the same release shipped a 191-package environment holding everything they need. It became FALSE the moment they were published independently — which happened on 2026-09-09/10. They were never really modules; they were release components wearing a module's shape, and they are the one case that never exercised the machinery built for them.

## The measured package split

Evidence: every `library()`/`requireNamespace()`/`pkg::` reference across `analysis/lib/` and `analysis/modules/`.

| | |
|---|---|
| the frame uses | `jsonlite`, `knitr`, `rmarkdown` — plus `pandoc` and `typst` for the PDF report |
| the modules use | `doFuture`, `ggplot2`, `Rcpp`, and `data.table` (basicstats only) |
| used by nothing at all | `r-optparse`, `r-pheatmap` — pinned since the first guess at the E4c roster |

`foreach` and `future` arrive as `doFuture` dependencies. **The audit corrected two of my assumptions**: `mds` DOES need `ggplot2` unconditionally, and the compiled path needs the conda C++ toolchain, which `r-rcpp` does not pull — so the toolchain placement is part of the split, not incidental.

## The audited blast radius — seven seams, 2026-09-10

A seven-dimension audit with adversarial verification of every blocker and major finding. **Five verifier verdicts, zero refutations or corrections** — the findings below survived independent checking.

### Blockers

- **The clone-install route still ships all three modules.** `.gitattributes export-ignore` governs the TARBALL; `install` copies from a checkout tree where `analysis/modules/*` still exist. "The store starts empty" is false for every user following the manual's clone route. **This is the one I would have missed.**
- **The three published catalogue rows become install-clean, fail-at-first-run.** Their `environment=3.0.0` passes the `version_at_most` check on any later release, their manifests declare no packages, so they install cleanly onto a slim baseline and then die missing `ggplot2`/`doFuture`/`Rcpp`. The `environment` field's semantics assume baselines only ever GAIN packages.
- **`00_static`'s `no package leaves a shipped environment file` cannot legitimately pass a slimmed baseline** — and it is the guard added on 2026-09-09 for exactly the typst class of bug. It compares against the last release tag, so it stays red for the whole dev cycle. `export-environment.sh --allow-removals` is the sanctioned escape for the export; the test needs its own answer.
- **`00_static` asserts every repo manifest's `environment` EQUALS the release version.** With no module shipping, that coupling is backwards — it forces a bump nobody's needs justify.
- **The manual teaches the old design in at least three places**: "Four ship with the release", the `# Shipped Modules` section, and "No module shipped with this release names one \[a package\]".
- **`RELEASING.md` step 6**'s justification for automatic manifest rewriting — "travels in the same tarball as the analysis environment it names" — is exactly the premise being removed.

### Major

- **`bump-version.sh` now writes a wrong claim.** Automatically setting `environment := new release` is right for a shipped module and wrong for an independent one, whose minimum should move only when its needs do. Landed 2026-09-10 at Z's request; the request was correct under the old design.
- **No gate checks that a module's declared packages cover what its R actually loads.** Under-declaration is silent — and the three published tarballs declare NONE. This is the gate that would have caught tonight.
- **The module suites SKIP rather than fail when `TEST_ANALYSIS_ENV` lacks a package**, so a slim environment silently retires the compiled and parallel coverage instead of reporting it.
- **`PoolSeqFlow analysis cite` cites zero R packages today** — a live bug, independent of any of this, in how the cite arm reads `analysis_r_packages`.
- **`analysis check` would report "All checks passed" on an environment missing every module-declared package**, because it only verifies the baseline.
- **The slim baseline list must add `python3` and `rsync`** (the frame invokes both) and probably drop `samtools`/`bcftools`/`htslib`, which the audit found nothing in the frame invoking — verify before acting.
- **Module package arrays must be name-disjoint from the baseline's set**, or `export-environment.sh`'s refusal fires permanently on a name the baseline owns.
- **Two tests fail against the uncommitted wrapper edit**: `02_launcher`'s modules-list case and `test_modules_list_reports_the_store` both plant a store module with no `.source`, which now prints `ships with this release`.
- **`test_an_unknown_module_refuses_before_any_task`** asserts the literal roster `Available here: association, basicstats, mds, verify`, read from the store at runtime — it only holds while the checkout's store has the three.
- **The store README** contradicts the new design in three places, including rule 6e: "Leave the field out when the release's own environment suffices — which is true of every module shipped here".
- **Offline/air-gapped installs regress**, which matters because this runs on HPC clusters: today a cluster user gets three working modules in the tarball; afterwards they need HTTPS to the catalogue AND to each tarball. A `file://` catalogue does not solve it — the rows carry `https` URLs independently of how the index was fetched.

## The uncommitted work, and what to do with it

Three files, all on `dev`, none committed:

- **`.gitattributes`** — `analysis/modules/*/ export-ignore`, keeping `README.md`. **Correct and verified**: `git archive --worktree-attributes` leaves only `analysis/modules.nf` and the README.
- **`dev/scripts/verify-archive.sh`** — `excluded` widened to `analysis/modules/*/`, and the positive assertion strengthened to "no module directory in the archive AND the README is". **Correct**; `00_static` passes at 46 against the working tree.
- **`PoolSeqFlow`** — `module_was_installed()` plus `ships with this release` labels and the uninstall warning. **This encodes the ABANDONED middle design** and should probably be reverted: once nothing ships, everything in the store was installed, and the label distinction is dead weight. It also breaks two tests. `.source` remains a useful marker; the labels built on it do not.

## Sequencing, when this is picked up

The blockers interlock, so order matters. The clone-install path has to be settled before "the store starts empty" is true for anyone. The published catalogue rows need a decision — remove them, or republish at bumped versions declaring their packages — before the baseline slims, or a next release strands anyone who installed one. The two `00_static` gates need their own answers before the slim can be committed at all, or the suite is red for the whole cycle.

Estimated at roughly one working day, about half of it unattended suite and environment runs.
Loading
Loading