Repository navigation
fix(ci): install pinned fastqc and multiqc so prefilter QC is exercised - #44
Merged
Merged
Conversation
CI installed four of the six tools the pipeline shells out to. FastQC and MultiQC were pinned in config/defaults/tool_versions.yml and declared required by src/core/validate.jl, but nothing on the runner ever installed them, so the prefilter QC stage, its flag plumbing out of pipeline.yml and its provenance capture had no automated coverage at all. Closes #30. Both pins are read at run time like every other tool. MultiQC comes from PyPI through the same pip path as cutadapt. FastQC is fetched from the pinned URL and refused unless it hashes to the pinned SHA256; it is unpacked to /opt rather than moved as a single file, because its launcher is a Perl script that resolves its jars relative to its own location, and reached through a symlink on PATH, which is what config/ci/tools.yml means by `path: "fastqc"`. The launcher runs `java` off PATH, so `fastqc --version` proves both that the install landed and that a JRE is present on the runner. Criterion 1 needed no change: both tools were already pinned correctly, FastQC with a SHA256 that was verified against a real download of the pinned URL. The skip is made loud rather than merely fixed. The live-probe loop in test_provenance.jl now gives each tool its own named testset, so the suite's own summary names all six and says per tool whether the probe ran; an absent tool is recorded broken on a bare checkout and fails, naming itself, when CI is set, because ci.yml is supposed to have installed it. The integration test runs the QC stage over the raw reads and asserts both logs exist, are non-empty, and carry the invocation marker that is the provenance record. The guard that asserts CI installs every tool went in vacuous and was caught by deleting the install step: matching on the tool's name passes with the step gone, because the pin-export block names every tool whether or not anything installs it. It now matches the dereference of each exported pin, which only an install step contains, and deleting either new step reddens it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
|
Warning Review limit reachedNext included review available in 34 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The guard added in e90af7f asserts that each exported pin is dereferenced by a step that consumes it, and wrote those references as "\$FASTQC_URL". config/ci/lint_source.jl rejects `\$` in an interpolating string, and that step is fail-fast at position 5 of the job -- so the whole Julia test job never ran and the new install steps were never exercised on CI at all. raw"$FASTQC_URL" carries identical bytes and says the right thing: this is a shell variable reference to be found verbatim in the YAML, never a Julia interpolation. The lint exempts raw strings for exactly that reason. Re-verified: source lint clean; install pins 113/113; and deleting the Install fastqc step still reddens it at lines 223 and 229, so the guard is still connected to what it guards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Closes #30.
What was wrong
ci.ymlinstalled four of the six tools the pipeline shells out to. FastQC andMultiQC were pinned in
config/defaults/tool_versions.ymland declared required bysrc/core/validate.jl:76, but nothing on the runner installed them, so the prefilterQC stage, its flag plumbing out of
pipeline.ymland its provenance capture had noautomated coverage at all.
Acceptance criteria
1 — pinned in
tool_versions.ymlwith a sha256. Already satisfied on disk beforethis branch; no change was needed and none was made. Reported rather than committed as
a no-op. The pin was not taken on trust: the pinned URL was downloaded
(
fastqc_v0.12.1.zip, 11,709,692 bytes) and its sha256 matches the pin exactly.MultiQC 1.33 was confirmed present on PyPI.
2 — the live probes run on CI, asserted by the testset's own output. The loop in
test_provenance.jlnow gives each tool its own named testset, so the summary namesall six and says per tool whether the probe ran:
Before, a skipped probe emitted an
@infoand contributed no row at all.3 — an end-to-end run produces non-empty QC logs with provenance captured. The
integration test now runs the QC stage over the raw reads before cutadapt and asserts
QC/multiqc_report.htmlexists and that bothQC/logs/fastqc.logandQC/logs/multiqc.logexist, are non-empty, and carry the[MetaManifold] cmd:marker — the same provenance record the DADA2 assertions below it grep for.
QCwasadded to the test's cleanup list, because a stale report from an earlier run would
both satisfy the new assertions and make
multiqc()short-circuit without running.4 — if pinning proves impractical, make the skip loud. Pinning did not prove
impractical, so both tools are installed. The skip was made loud anyway, because the
criterion describes the right behaviour regardless: on a bare checkout an absent tool
is recorded broken, which the summary prints in its own column; when
CIis setit fails, naming itself, because
ci.ymlis supposed to have installed it.How the tools are installed
MultiQC comes from PyPI through the same
pip install "$SPEC"path as cutadapt.FastQC is fetched from the pinned URL and refused unless it hashes to the pinned
sha256. It is unpacked to
/optrather than moved as a single file: the launcher is aPerl script that resolves its jars relative to its own location, so moving
FastQC/fastqcout of its directory breaks it. It is reached through a symlink onPATH, which is what
config/ci/tools.ymlmeans bypath: "fastqc". The launcher runsjavaoff PATH, sofastqc --versionproves both that the install landed and that aJRE is present on the runner.
Mutants
A passing suite proves nothing until a reintroduced defect reddens the right
assertion. Four were run.
Install fastqcstepInstall multiqcstep$MULTIQC_SPECdereferencemultiqc==1.33literally intoci.ymlCI=truewith all six tools absentThe first mutant survived, and that is the most useful thing in this PR. The guard
was written the obvious way — does the string
fastqcappear anywhere in the job'srun:blocks — and deleting the entire install step left the suite at 110/110 green,because the pin-export block mentions every tool by name whether or not anything
installs it. The guard asked a different question than its consumer. It now matches
the dereference of each exported pin (
$FASTQC_URL,$MULTIQC_SPEC, …), a stringthat appears only where a step consumes the pin, plus
/usr/local/bin/<tool>for thethree archive tools that must end up on PATH. Both deletion mutants now redden it.
Suite
1714 passed, 0 failed, 1 errored, 6 brokenlocally. The 6 broken are the intendednew named skips — none of the six tools is installed on this machine. The 1 error is
pre-existing and unrelated:
probe_rintest_provenance.jlis guarded by a callthat probes zero R packages while its consumer requires four, so the testset errors on
any bare checkout. That is filed separately as #43 and is deliberately left untouched
here, so the two changes stay disjoint.
Scope note
src/core/validate.jl's required-tool list is reached only fromvalidate_environment, not fromvalidate_project, so the integration test's stageswere genuinely running on CI before this change — it was the QC stage specifically
that had never run, not the pipeline as a whole.
🤖 Generated with Claude Code
https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm