HTML report/nfcore addons - #4
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds nf-core QC modules, TrESFlow HTML and JSON reports, revised DNA processing, uncompressed computational FASTQs with compressed publication copies, launch-path resolution, workflow wiring, configuration, tests, and documentation. ChangesTrESFlow pipeline updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (5)
bin/render_tres_report.py (1)
127-154: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding
strict=Truetozip()for data integrity.At line 138,
zip(header, parts)withoutstrict=Truewill silently truncate if the header and data row have different column counts. While GATK metrics files have a consistent format, addingstrict=Truewould catch malformed input early rather than producing incorrect metrics silently.Suggested fix
- values = dict(zip(header, parts)) + values = dict(zip(header, parts, strict=True))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/render_tres_report.py` around lines 127 - 154, The `zip(header, parts)` call in the read_duplicate_metrics function will silently truncate if the header and data row have different column counts, potentially causing incorrect metrics without any warning. Add `strict=True` as a parameter to the zip call where `values = dict(zip(header, parts))` is assigned to ensure that mismatched column counts are detected and caught early, rather than silently producing incomplete or incorrect data.Source: Linters/SAST tools
modules/nf-core/multiqc/tests/main.nf.test (1)
106-107: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid network-dependent fixtures in module tests.
Line 106-107 and Line 149-150 fetch config files from GitHub at test time. This makes tests susceptible to network/transient failures. Prefer committed local fixture files under the module test assets to keep CI deterministic.
Also applies to: 149-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/nf-core/multiqc/tests/main.nf.test` around lines 106 - 107, Remove network dependencies from the MultiQC module tests by replacing the GitHub URL references in the multiqc_config.yml file access with local fixture files. Download the multiqc_config.yml file from the GitHub URL and commit it to the module test assets directory, then update both occurrences (around lines 106-107 and 149-150) to reference the local file path instead of the remote GitHub URL using the file() function with the local asset path. This ensures tests remain deterministic and do not depend on network availability or transient GitHub connectivity issues.modules/nf-core/samtools/idxstats/meta.yml (1)
29-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAlign declared input contract with the actual process requirements.
Line 31-37 currently documents SAM/SAI support, but
modules/nf-core/samtools/idxstats/main.nf(Line 11) requires an index path in all cases. Please either narrow the documented patterns to indexed BAM/CRAM inputs or make the index optional in the process contract so docs and runtime behavior match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/nf-core/samtools/idxstats/meta.yml` around lines 29 - 38, The input patterns declared in the meta.yml file do not match the actual process requirements in main.nf, which mandates an index file in all cases. Narrow the pattern specifications for both the bam and bai input definitions to remove unsupported formats: remove SAM (sam) from the bam pattern on line 33 to include only "*.{bam,cram}", and remove SAI (sai) from the bai pattern on line 37 to include only "*.{bai,crai}". This aligns the documented input contract with the actual runtime behavior that requires indexed BAM or CRAM files.modules/nf-core/gatk4/markduplicates/main.nf (2)
29-32: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding
defkeyword for local variables.While Nextflow allows variable assignments without
def, usingdeffor local variables (prefixandprefix_bam) is more idiomatic and makes scope explicit.♻️ Suggested style improvement
- prefix = task.ext.prefix ?: "${meta.id}.bam" + def prefix = task.ext.prefix ?: "${meta.id}.bam" // If the extension is CRAM, then change it to BAM - prefix_bam = prefix.tokenize('.')[-1] == 'cram' ? "${prefix.substring(0, prefix.lastIndexOf('.'))}.bam" : prefix + def prefix_bam = prefix.tokenize('.')[-1] == 'cram' ? "${prefix.substring(0, prefix.lastIndexOf('.'))}.bam" : prefix🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/nf-core/gatk4/markduplicates/main.nf` around lines 29 - 32, Add the `def` keyword before both local variable declarations for `prefix` and `prefix_bam`. This makes the variable scope explicit and follows Nextflow idioms. Change the assignments to use `def prefix = ...` and `def prefix_bam = ...` instead of declaring them without the keyword.
65-74: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueStub creates both output formats, while script creates only one.
The stub implementation creates both
.bamand.cramoutputs (plus both index types), whereas the real script produces only the format specified byprefix. This inconsistency could cause confusion when tests run in stub mode versus real mode, though it won't cause failures since outputs are marked optional.Consider making the stub match the real behavior more closely by checking the prefix and creating only the expected output format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/nf-core/gatk4/markduplicates/main.nf` around lines 65 - 74, The stub block unconditionally creates both BAM and CRAM output files with their indices, while the real script creates only the format specified by the prefix variable. Modify the stub implementation to conditionally create only the expected output format based on the prefix. Check whether the prefix indicates BAM or CRAM format (by examining the prefix string or the actual script's logic) and use conditional statements in the stub block to create only the corresponding output files and their appropriate index, making the stub behavior consistent with the actual script behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conf/modules.config`:
- Around line 47-50: In the GATK4_MARKDUPLICATES configuration block, the
ext.prefix parameter currently includes the .bam file extension. Remove the .bam
extension from the ext.prefix value so that it only contains
"${meta.id}_MarkedDup" instead of "${meta.id}_MarkedDup.bam". The nf-core module
will automatically append the appropriate file extension, and removing the
hardcoded extension will prevent double extensions in the output file names.
In `@conf/test.config`:
- Around line 95-98: The DEEPTOOLS_BAMCOVERAGE process block is missing the
ext.mock = true configuration that other test tasks have. Add the line ext.mock
= true inside the withName: DEEPTOOLS_BAMCOVERAGE block so that the real module
is not executed during test runs, keeping tests fast and reliable.
In `@modules/nf-core/deeptools/bamcoverage/main.nf`:
- Around line 61-64: The stub block uses the args variable without declaring it
first, while the script block properly declares args. Add a declaration of args
(likely from task.ext.args with an empty string as default) at the beginning of
the stub block, before the line where args.contains() is called. This ensures
the variable is defined when checking for bedgraph format options, matching the
pattern used in the script block.
- Around line 33-34: The CRAM detection and output naming logic uses incorrect
Nextflow Path property names with uppercase letters. Replace `input.Extension`
with `input.extension` and replace `input.BaseName` with `input.baseName` in the
lines where `is_cram` and `input_out` variables are defined, as Nextflow Path
API requires lowercase property names or method calls like `getExtension()` and
`getBaseName()`.
In `@modules/nf-core/deeptools/bamcoverage/tests/main.nf.test`:
- Line 43: The test name contains a typo where "homo_sampiens" is misspelled.
Change "homo_sampiens" to "homo_sapiens" in the test function name to correct
the species name spelling in the test method declaration.
- Line 105: Fix the typo in the test name string for the test function at this
location. The word "homo_sampiens" in the test name is misspelled and should be
corrected to "homo_sapiens" to match the correct scientific name of the human
species. Update the string passed to the test function to use the correct
spelling.
- Line 12: The test name contains a typo: "homo_sampiens" should be corrected to
"homo_sapiens" (the correct scientific name for humans). Locate the test
function and update the test name string from "homo_sampiens - bam" to
"homo_sapiens - bam" to fix the spelling error.
- Line 74: Fix the typo in the test name where "homo_sampiens" should be
corrected to "homo_sapiens" in the test function. Change the string in the
test() call from "homo_sampiens - cram - fasta" to "homo_sapiens - cram - fasta"
to match the correct species name spelling.
In `@modules/nf-core/fastqc/main.nf`:
- Around line 24-27: The `old_new_pairs` assignment has a logic error in the
single-element list handling. When `reads.size() == 1` is true, `reads` is a
list, not a Path, so accessing `reads.extension` directly will fail. Fix this by
extracting the first element from the list (using `reads[0]` or `reads.first()`)
before accessing the `.extension` property when handling the single-element case
in the ternary operator.
In `@modules/nf-core/fastqc/tests/main.nf.test`:
- Around line 133-140: The assertions in the test are checking for output
filenames that do not match the input files. The 3rd and 4th outputs in the html
and zip assertions (at indices [0][1][2] and [0][1][3]) expect test_3 and
test_4, but the actual input fixtures are test2_1.fastq.gz and test2_2.fastq.gz.
Update all four assertions in both the html and zip output blocks to expect
test2_1_fastqc and test2_2_fastqc respectively instead of test_3 and test_4 to
align with the input file names.
In `@modules/nf-core/gatk4/markduplicates/main.nf`:
- Around line 58-62: The CRAM conversion block (when prefix ends with .cram)
uses the fasta reference file with the samtools view command but lacks
validation that the fasta file exists. Add a validation check inside the if
block that tests whether the fasta variable points to an existing file, and only
execute the samtools view -Ch -T command and subsequent operations if the fasta
file is present. If fasta is missing, the block should either error out with a
clear message or skip the CRAM conversion.
In `@modules/nf-core/gatk4/markduplicates/meta.yml`:
- Around line 1-99: The metrics output section declares a pattern of
`*.{metrics.txt}` but the actual process outputs files with the pattern
`*.{metrics}` without the `.txt` suffix. Update the pattern field in the metrics
output section to match the actual file output by changing it from
`*.{metrics.txt}` to `*.{metrics}`.
In `@modules/nf-core/multiqc/meta.yml`:
- Around line 64-68: Update the pattern field for the HTML output in the MultiQC
module metadata to use a valid glob pattern. Change the pattern value from
".html" to "*.html" to correctly match actual emitted filenames like
multiqc_report.html. This ensures the pattern field accurately reflects the
output contract of the module.
- Around line 85-88: The type declaration for the `*_plots` output does not
match the actual emission type from the Nextflow process. Change the `type:
file` declaration for the `*_plots` output to `type: directory` to correctly
reflect that the process emits a directory path (via `path("*_plots")` in
main.nf) rather than a single file, ensuring the schema aligns with the actual
process contract.
In `@modules/nf-core/samtools/quickcheck/tests/main.nf.test`:
- Around line 151-152: The remote fixture URLs at lines 151-152, 172-173, and
193-194 use mutable branch references (refs/heads/develop) which can cause
non-deterministic CI behavior. Replace all occurrences of the github.com URLs
pointing to refs/heads/develop with immutable commit-SHA based URLs (e.g.,
refs/heads/develop replaced with a specific commit hash) or vendor the test
fixtures locally to ensure snapshots and expected exit codes remain stable over
time.
---
Nitpick comments:
In `@bin/render_tres_report.py`:
- Around line 127-154: The `zip(header, parts)` call in the
read_duplicate_metrics function will silently truncate if the header and data
row have different column counts, potentially causing incorrect metrics without
any warning. Add `strict=True` as a parameter to the zip call where `values =
dict(zip(header, parts))` is assigned to ensure that mismatched column counts
are detected and caught early, rather than silently producing incomplete or
incorrect data.
In `@modules/nf-core/gatk4/markduplicates/main.nf`:
- Around line 29-32: Add the `def` keyword before both local variable
declarations for `prefix` and `prefix_bam`. This makes the variable scope
explicit and follows Nextflow idioms. Change the assignments to use `def prefix
= ...` and `def prefix_bam = ...` instead of declaring them without the keyword.
- Around line 65-74: The stub block unconditionally creates both BAM and CRAM
output files with their indices, while the real script creates only the format
specified by the prefix variable. Modify the stub implementation to
conditionally create only the expected output format based on the prefix. Check
whether the prefix indicates BAM or CRAM format (by examining the prefix string
or the actual script's logic) and use conditional statements in the stub block
to create only the corresponding output files and their appropriate index,
making the stub behavior consistent with the actual script behavior.
In `@modules/nf-core/multiqc/tests/main.nf.test`:
- Around line 106-107: Remove network dependencies from the MultiQC module tests
by replacing the GitHub URL references in the multiqc_config.yml file access
with local fixture files. Download the multiqc_config.yml file from the GitHub
URL and commit it to the module test assets directory, then update both
occurrences (around lines 106-107 and 149-150) to reference the local file path
instead of the remote GitHub URL using the file() function with the local asset
path. This ensures tests remain deterministic and do not depend on network
availability or transient GitHub connectivity issues.
In `@modules/nf-core/samtools/idxstats/meta.yml`:
- Around line 29-38: The input patterns declared in the meta.yml file do not
match the actual process requirements in main.nf, which mandates an index file
in all cases. Narrow the pattern specifications for both the bam and bai input
definitions to remove unsupported formats: remove SAM (sam) from the bam pattern
on line 33 to include only "*.{bam,cram}", and remove SAI (sai) from the bai
pattern on line 37 to include only "*.{bai,crai}". This aligns the documented
input contract with the actual runtime behavior that requires indexed BAM or
CRAM files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a845e072-2d59-4e8e-b9b3-881191005d1c
⛔ Files ignored due to path filters (8)
modules/nf-core/deeptools/bamcoverage/tests/main.nf.test.snapis excluded by!**/*.snapmodules/nf-core/fastqc/tests/main.nf.test.snapis excluded by!**/*.snapmodules/nf-core/gatk4/markduplicates/tests/main.nf.test.snapis excluded by!**/*.snapmodules/nf-core/multiqc/tests/main.nf.test.snapis excluded by!**/*.snapmodules/nf-core/samtools/flagstat/tests/main.nf.test.snapis excluded by!**/*.snapmodules/nf-core/samtools/idxstats/tests/main.nf.test.snapis excluded by!**/*.snapmodules/nf-core/samtools/quickcheck/tests/main.nf.test.snapis excluded by!**/*.snapmodules/nf-core/samtools/stats/tests/main.nf.test.snapis excluded by!**/*.snap
📒 Files selected for processing (61)
README.mdassets/test_realdata/ligation_barcode_whitelist.txtbin/render_tres_report.pyconf/base.configconf/modules.configconf/test.configdocs/architecture/implemented_pipeline.mddocs/output.mddocs/usage.mdmodules.jsonmodules/local/check_dna_nodup_bam/main.nfmodules/local/normalize_dna_bamcoverage/main.nfmodules/local/normalize_dna_markduplicates/main.nfmodules/local/rna_starsolo_align/main.nfmodules/local/samtools_quickcheck_report/main.nfmodules/local/tres_report_html/main.nfmodules/nf-core/deeptools/bamcoverage/environment.ymlmodules/nf-core/deeptools/bamcoverage/main.nfmodules/nf-core/deeptools/bamcoverage/meta.ymlmodules/nf-core/deeptools/bamcoverage/tests/main.nf.testmodules/nf-core/fastqc/.conda-lock/linux_amd64-bd-5cb1a2fa2f18c7c2_1.txtmodules/nf-core/fastqc/.conda-lock/linux_arm64-bd-e455e32f745abe68_1.txtmodules/nf-core/fastqc/environment.ymlmodules/nf-core/fastqc/main.nfmodules/nf-core/fastqc/meta.ymlmodules/nf-core/fastqc/tests/main.nf.testmodules/nf-core/gatk4/markduplicates/environment.ymlmodules/nf-core/gatk4/markduplicates/main.nfmodules/nf-core/gatk4/markduplicates/meta.ymlmodules/nf-core/gatk4/markduplicates/tests/bam.configmodules/nf-core/gatk4/markduplicates/tests/cram.configmodules/nf-core/gatk4/markduplicates/tests/main.nf.testmodules/nf-core/multiqc/.conda-lock/linux_amd64-bd-c17fb751507e9dfc_1.txtmodules/nf-core/multiqc/.conda-lock/linux_arm64-bd-5c84a5000a226ab5_1.txtmodules/nf-core/multiqc/environment.ymlmodules/nf-core/multiqc/main.nfmodules/nf-core/multiqc/meta.ymlmodules/nf-core/multiqc/tests/custom_prefix.configmodules/nf-core/multiqc/tests/main.nf.testmodules/nf-core/multiqc/tests/nextflow.configmodules/nf-core/samtools/flagstat/environment.ymlmodules/nf-core/samtools/flagstat/main.nfmodules/nf-core/samtools/flagstat/meta.ymlmodules/nf-core/samtools/flagstat/tests/main.nf.testmodules/nf-core/samtools/idxstats/environment.ymlmodules/nf-core/samtools/idxstats/main.nfmodules/nf-core/samtools/idxstats/meta.ymlmodules/nf-core/samtools/idxstats/tests/main.nf.testmodules/nf-core/samtools/quickcheck/environment.ymlmodules/nf-core/samtools/quickcheck/main.nfmodules/nf-core/samtools/quickcheck/meta.ymlmodules/nf-core/samtools/quickcheck/tests/main.nf.testmodules/nf-core/samtools/quickcheck/tests/nextflow.configmodules/nf-core/samtools/stats/environment.ymlmodules/nf-core/samtools/stats/main.nfmodules/nf-core/samtools/stats/meta.ymlmodules/nf-core/samtools/stats/tests/main.nf.testsubworkflows/local/dna_core/main.nfsubworkflows/local/dna_core/meta.ymlsubworkflows/local/rna_core/main.nfworkflows/treseq.nf
| withName: GATK4_MARKDUPLICATES { | ||
| ext.prefix = { "${meta.id}_MarkedDup.bam" } | ||
| ext.args = '--REMOVE_DUPLICATES false --BARCODE_TAG CB --CREATE_INDEX true --MAX_RECORDS_IN_RAM 10000000' | ||
| } |
There was a problem hiding this comment.
Remove the .bam extension from the ext.prefix.
The ext.prefix for GATK4_MARKDUPLICATES should not include the file extension. nf-core modules append their own extensions to the prefix, so including .bam here will produce output files with double extensions like *_MarkedDup.bam.bam.
🐛 Proposed fix
- ext.prefix = { "${meta.id}_MarkedDup.bam" }
+ ext.prefix = { "${meta.id}_MarkedDup" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@conf/modules.config` around lines 47 - 50, In the GATK4_MARKDUPLICATES
configuration block, the ext.prefix parameter currently includes the .bam file
extension. Remove the .bam extension from the ext.prefix value so that it only
contains "${meta.id}_MarkedDup" instead of "${meta.id}_MarkedDup.bam". The
nf-core module will automatically append the appropriate file extension, and
removing the hardcoded extension will prevent double extensions in the output
file names.
| withName: DEEPTOOLS_BAMCOVERAGE { | ||
| cpus = 1 | ||
| memory = '1 GB' | ||
| } |
There was a problem hiding this comment.
DEEPTOOLS_BAMCOVERAGE is not mocked in test profile.
Line 95 configures this process in conf/test.config, but unlike the surrounding test tasks it is missing ext.mock = true. That can execute the real module during tests and make test runs slower/flaky.
Suggested fix
withName: DEEPTOOLS_BAMCOVERAGE {
+ ext.mock = true
cpus = 1
memory = '1 GB'
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@conf/test.config` around lines 95 - 98, The DEEPTOOLS_BAMCOVERAGE process
block is missing the ext.mock = true configuration that other test tasks have.
Add the line ext.mock = true inside the withName: DEEPTOOLS_BAMCOVERAGE block so
that the real module is not executed during test runs, keeping tests fast and
reliable.
| def is_cram = input.Extension == "cram" ? true : false | ||
| def input_out = is_cram ? input.BaseName + ".bam" : "${input}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify path-property usage patterns in this module and nearby nf-core modules.
rg -n -C2 '\b(input\.Extension|input\.BaseName)\b|getExtension\(|getBaseName\(' modules/nf-coreRepository: CSOgroup/TrESFlow
Length of output: 672
🏁 Script executed:
# Read the full context of the main.nf file to understand the input variable type
head -50 modules/nf-core/deeptools/bamcoverage/main.nfRepository: CSOgroup/TrESFlow
Length of output: 2239
🏁 Script executed:
# Search for how other nf-core modules handle file extension detection and basename extraction
rg -n '\.(getExtension|getBaseName|extension|baseName)\(\)|\bext\b|\bstem\b' modules/nf-core --type groovy -A1 -B1 | head -100Repository: CSOgroup/TrESFlow
Length of output: 43
🌐 Web query:
Nextflow Path object properties Extension BaseName vs getExtension getBaseName Groovy
💡 Result:
In Nextflow, for Path objects, prefer the Groovy-style properties baseName and extension (e.g., path.baseName, path.extension). These correspond to the underlying methods getBaseName() and getExtension(). 1) Nextflow Path “properties” - path.baseName returns the path name without its extension; for /some/path/file.tar.gz it’s file.tar [1]. - path.extension returns the path extension without the dot; for /some/path/file.txt it’s txt [1]. - These work in Nextflow’s docs/examples as path.baseName, path.extension, and path.name [2]. 2) getBaseName() / getExtension() are the method forms - The Nextflow TaskPath type explicitly defines getBaseName() and getExtension() methods [3]. - Groovy property access like path.extension maps to the getter getExtension() (and similarly path.baseName → getBaseName()). This is consistent with how Groovy exposes extension-method properties for Path/File (via NioExtensions with getExtension(Path) and analogous base-name support) [4][5] and with Nextflow’s TaskPath.groovy implementing getBaseName() / getExtension() [6]. 3) What about “getExtension getBaseName Groovy”? - If you call the methods directly in Groovy, use parentheses: path.getExtension() and path.getBaseName(). - If you use properties (recommended/readable): path.extension and path.baseName. 4) Practical examples in Nextflow - From Nextflow’s working-with-files docs: - assert path.baseName == 'file' - assert path.extension == 'txt' - assert path.name == 'file.txt' - assert path.parent == '/some/path' [2]. So, “Extension BaseName vs getExtension getBaseName” is simply: extension/baseName are the property-style aliases for getExtension()/getBaseName() on Nextflow Path objects.
Citations:
- 1: https://github.com/nextflow-io/nextflow/blob/master/docs/reference/stdlib-types.md
- 2: https://github.com/nextflow-io/nextflow/blob/master/docs/working-with-files.md
- 3: https://javadoc.io/static/io.nextflow/nextflow/19.01.0-edge/nextflow/processor/TaskPath.html
- 4: https://docs.groovy-lang.org/next/html/api/org/apache/groovy/nio/extensions/NioExtensions.html
- 5: http://docs.groovy-lang.org/next/html/groovy-jdk/java/nio/file/Path.html
- 6: https://github.com/nextflow-io/nextflow/blob/v21.10.6/modules/nextflow/src/main/groovy/nextflow/processor/TaskPath.groovy
Use valid path API for CRAM detection and output naming.
input.Extension and input.BaseName are not valid Nextflow Path properties. In Nextflow, Path objects use lowercase extension and baseName properties, or the methods getExtension() and getBaseName(). The capitalized versions will cause script evaluation failure at runtime.
Proposed fix
- def is_cram = input.Extension == "cram" ? true : false
- def input_out = is_cram ? input.BaseName + ".bam" : "${input}"
+ def is_cram = input.getExtension() == 'cram'
+ def input_out = is_cram ? "${input.getBaseName()}.bam" : "${input}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def is_cram = input.Extension == "cram" ? true : false | |
| def input_out = is_cram ? input.BaseName + ".bam" : "${input}" | |
| def is_cram = input.getExtension() == 'cram' | |
| def input_out = is_cram ? "${input.getBaseName()}.bam" : "${input}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/deeptools/bamcoverage/main.nf` around lines 33 - 34, The CRAM
detection and output naming logic uses incorrect Nextflow Path property names
with uppercase letters. Replace `input.Extension` with `input.extension` and
replace `input.BaseName` with `input.baseName` in the lines where `is_cram` and
`input_out` variables are defined, as Nextflow Path API requires lowercase
property names or method calls like `getExtension()` and `getBaseName()`.
| stub: | ||
| def prefix = task.ext.prefix ?: "${meta.id}" | ||
| def extension = args.contains("--outFileFormat bedgraph") || args.contains("-of bedgraph") ? "bedgraph" : "bigWig" | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify undeclared use of `args` inside stub blocks in this file.
rg -n -C3 'stub:|def args|args\.contains' modules/nf-core/deeptools/bamcoverage/main.nfRepository: CSOgroup/TrESFlow
Length of output: 832
Define args inside stub block before using it.
The stub block uses args.contains() on line 63 without declaring it, while the parallel script block correctly declares args on line 26. This causes a runtime error in stub execution.
Proposed fix
stub:
def prefix = task.ext.prefix ?: "${meta.id}"
+ def args = task.ext.args ?: ''
def extension = args.contains("--outFileFormat bedgraph") || args.contains("-of bedgraph") ? "bedgraph" : "bigWig"
"""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/deeptools/bamcoverage/main.nf` around lines 61 - 64, The stub
block uses the args variable without declaring it first, while the script block
properly declares args. Add a declaration of args (likely from task.ext.args
with an empty string as default) at the beginning of the stub block, before the
line where args.contains() is called. This ensures the variable is defined when
checking for bedgraph format options, matching the pattern used in the script
block.
| tag "deeptools" | ||
| tag "deeptools/bamcoverage" | ||
|
|
||
| test("homo_sampiens - bam") { |
There was a problem hiding this comment.
Fix typo in test name.
Test name has "homo_sampiens" instead of "homo_sapiens".
✏️ Proposed fix
- test("homo_sampiens - bam") {
+ test("homo_sapiens - bam") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("homo_sampiens - bam") { | |
| test("homo_sapiens - bam") { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/deeptools/bamcoverage/tests/main.nf.test` at line 12, The
test name contains a typo: "homo_sampiens" should be corrected to "homo_sapiens"
(the correct scientific name for humans). Locate the test function and update
the test name string from "homo_sampiens - bam" to "homo_sapiens - bam" to fix
the spelling error.
| if [[ ${prefix} == *.cram ]]; then | ||
| samtools view -Ch -T ${fasta} -o ${prefix} ${prefix_bam} | ||
| rm ${prefix_bam} | ||
| samtools index ${prefix} | ||
| fi |
There was a problem hiding this comment.
Missing validation: CRAM output requires fasta reference.
The CRAM conversion logic uses ${fasta} without checking if it exists. If a user configures ext.prefix to end with .cram but doesn't provide a fasta reference, the samtools view -Ch -T ${fasta} command will fail because -T requires a valid reference file argument.
🛡️ Proposed fix to add validation
# If cram files are wished as output, the run samtools for conversion
if [[ ${prefix} == *.cram ]]; then
+ if [[ -z "${fasta}" ]]; then
+ echo "ERROR: CRAM output requires a reference fasta file" >&2
+ exit 1
+ fi
samtools view -Ch -T ${fasta} -o ${prefix} ${prefix_bam}
rm ${prefix_bam}
samtools index ${prefix}
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/gatk4/markduplicates/main.nf` around lines 58 - 62, The CRAM
conversion block (when prefix ends with .cram) uses the fasta reference file
with the samtools view command but lacks validation that the fasta file exists.
Add a validation check inside the if block that tests whether the fasta variable
points to an existing file, and only execute the samtools view -Ch -T command
and subsequent operations if the fasta file is present. If fasta is missing, the
block should either error out with a clear message or skip the CRAM conversion.
| name: gatk4_markduplicates | ||
| description: This tool locates and tags duplicate reads in a BAM or SAM file, | ||
| where duplicate reads are defined as originating from a single fragment of | ||
| DNA. | ||
| keywords: | ||
| - bam | ||
| - gatk4 | ||
| - markduplicates | ||
| - sort | ||
| tools: | ||
| - gatk4: | ||
| description: Developed in the Data Sciences Platform at the Broad Institute, | ||
| the toolkit offers a wide variety of tools with a primary focus on variant | ||
| discovery and genotyping. Its powerful processing engine and | ||
| high-performance computing features make it capable of taking on projects | ||
| of any size. | ||
| homepage: https://gatk.broadinstitute.org/hc/en-us | ||
| documentation: https://gatk.broadinstitute.org/hc/en-us/articles/360037052812-MarkDuplicates-Picard- | ||
| tool_dev_url: https://github.com/broadinstitute/gatk | ||
| doi: 10.1158/1538-7445.AM2017-3590 | ||
| licence: ["MIT"] | ||
| identifier: "" | ||
| input: | ||
| - - meta: | ||
| type: map | ||
| description: | | ||
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - bam: | ||
| type: file | ||
| description: Sorted BAM file | ||
| pattern: "*.{bam}" | ||
| ontologies: [] | ||
| - fasta: | ||
| type: file | ||
| description: Fasta file | ||
| pattern: "*.{fasta}" | ||
| ontologies: [] | ||
| - fasta_fai: | ||
| type: file | ||
| description: Fasta index file | ||
| pattern: "*.{fai}" | ||
| ontologies: [] | ||
| output: | ||
| cram: | ||
| - - meta: | ||
| type: map | ||
| description: | | ||
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - "*cram": | ||
| type: file | ||
| description: Marked duplicates CRAM file | ||
| pattern: "*.{cram}" | ||
| ontologies: [] | ||
| bam: | ||
| - - meta: | ||
| type: map | ||
| description: | | ||
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - "*bam": | ||
| type: file | ||
| description: Marked duplicates BAM file | ||
| pattern: "*.{bam}" | ||
| ontologies: [] | ||
| crai: | ||
| - - meta: | ||
| type: map | ||
| description: | | ||
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - "*.crai": | ||
| type: file | ||
| description: CRAM index file | ||
| pattern: "*.{cram.crai}" | ||
| ontologies: [] | ||
| bai: | ||
| - - meta: | ||
| type: map | ||
| description: | | ||
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - "*.bai": | ||
| type: file | ||
| description: BAM index file | ||
| pattern: "*.{bam.bai}" | ||
| ontologies: [] | ||
| metrics: | ||
| - - meta: | ||
| type: map | ||
| description: | | ||
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - "*.metrics": | ||
| type: file | ||
| description: Duplicate metrics file generated by GATK | ||
| pattern: "*.{metrics.txt}" | ||
| ontologies: [] |
There was a problem hiding this comment.
Pattern mismatch: metrics file extension.
Line 98 specifies the pattern as *.{metrics.txt}, but the actual process (main.nf line 52) outputs ${prefix}.metrics without the .txt suffix. The pattern should match the actual output.
📝 Proposed fix
- "*.metrics":
type: file
description: Duplicate metrics file generated by GATK
- pattern: "*.{metrics.txt}"
+ pattern: "*.metrics"
ontologies: []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: gatk4_markduplicates | |
| description: This tool locates and tags duplicate reads in a BAM or SAM file, | |
| where duplicate reads are defined as originating from a single fragment of | |
| DNA. | |
| keywords: | |
| - bam | |
| - gatk4 | |
| - markduplicates | |
| - sort | |
| tools: | |
| - gatk4: | |
| description: Developed in the Data Sciences Platform at the Broad Institute, | |
| the toolkit offers a wide variety of tools with a primary focus on variant | |
| discovery and genotyping. Its powerful processing engine and | |
| high-performance computing features make it capable of taking on projects | |
| of any size. | |
| homepage: https://gatk.broadinstitute.org/hc/en-us | |
| documentation: https://gatk.broadinstitute.org/hc/en-us/articles/360037052812-MarkDuplicates-Picard- | |
| tool_dev_url: https://github.com/broadinstitute/gatk | |
| doi: 10.1158/1538-7445.AM2017-3590 | |
| licence: ["MIT"] | |
| identifier: "" | |
| input: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - bam: | |
| type: file | |
| description: Sorted BAM file | |
| pattern: "*.{bam}" | |
| ontologies: [] | |
| - fasta: | |
| type: file | |
| description: Fasta file | |
| pattern: "*.{fasta}" | |
| ontologies: [] | |
| - fasta_fai: | |
| type: file | |
| description: Fasta index file | |
| pattern: "*.{fai}" | |
| ontologies: [] | |
| output: | |
| cram: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*cram": | |
| type: file | |
| description: Marked duplicates CRAM file | |
| pattern: "*.{cram}" | |
| ontologies: [] | |
| bam: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*bam": | |
| type: file | |
| description: Marked duplicates BAM file | |
| pattern: "*.{bam}" | |
| ontologies: [] | |
| crai: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*.crai": | |
| type: file | |
| description: CRAM index file | |
| pattern: "*.{cram.crai}" | |
| ontologies: [] | |
| bai: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*.bai": | |
| type: file | |
| description: BAM index file | |
| pattern: "*.{bam.bai}" | |
| ontologies: [] | |
| metrics: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*.metrics": | |
| type: file | |
| description: Duplicate metrics file generated by GATK | |
| pattern: "*.{metrics.txt}" | |
| ontologies: [] | |
| name: gatk4_markduplicates | |
| description: This tool locates and tags duplicate reads in a BAM or SAM file, | |
| where duplicate reads are defined as originating from a single fragment of | |
| DNA. | |
| keywords: | |
| - bam | |
| - gatk4 | |
| - markduplicates | |
| - sort | |
| tools: | |
| - gatk4: | |
| description: Developed in the Data Sciences Platform at the Broad Institute, | |
| the toolkit offers a wide variety of tools with a primary focus on variant | |
| discovery and genotyping. Its powerful processing engine and | |
| high-performance computing features make it capable of taking on projects | |
| of any size. | |
| homepage: https://gatk.broadinstitute.org/hc/en-us | |
| documentation: https://gatk.broadinstitute.org/hc/en-us/articles/360037052812-MarkDuplicates-Picard- | |
| tool_dev_url: https://github.com/broadinstitute/gatk | |
| doi: 10.1158/1538-7445.AM2017-3590 | |
| licence: ["MIT"] | |
| identifier: "" | |
| input: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - bam: | |
| type: file | |
| description: Sorted BAM file | |
| pattern: "*.{bam}" | |
| ontologies: [] | |
| - fasta: | |
| type: file | |
| description: Fasta file | |
| pattern: "*.{fasta}" | |
| ontologies: [] | |
| - fasta_fai: | |
| type: file | |
| description: Fasta index file | |
| pattern: "*.{fai}" | |
| ontologies: [] | |
| output: | |
| cram: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*cram": | |
| type: file | |
| description: Marked duplicates CRAM file | |
| pattern: "*.{cram}" | |
| ontologies: [] | |
| bam: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*bam": | |
| type: file | |
| description: Marked duplicates BAM file | |
| pattern: "*.{bam}" | |
| ontologies: [] | |
| crai: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*.crai": | |
| type: file | |
| description: CRAM index file | |
| pattern: "*.{cram.crai}" | |
| ontologies: [] | |
| bai: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*.bai": | |
| type: file | |
| description: BAM index file | |
| pattern: "*.{bam.bai}" | |
| ontologies: [] | |
| metrics: | |
| - - meta: | |
| type: map | |
| description: | | |
| Groovy Map containing sample information | |
| e.g. [ id:'test', single_end:false ] | |
| - "*.metrics": | |
| type: file | |
| description: Duplicate metrics file generated by GATK | |
| pattern: "*.metrics" | |
| ontologies: [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/gatk4/markduplicates/meta.yml` around lines 1 - 99, The
metrics output section declares a pattern of `*.{metrics.txt}` but the actual
process outputs files with the pattern `*.{metrics}` without the `.txt` suffix.
Update the pattern field in the metrics output section to match the actual file
output by changing it from `*.{metrics.txt}` to `*.{metrics}`.
| - "*.html": | ||
| type: file | ||
| description: MultiQC report file | ||
| pattern: ".html" | ||
| ontologies: [] |
There was a problem hiding this comment.
Use a valid glob pattern for report output.
Line 67 uses pattern: ".html", which does not match emitted filenames like multiqc_report.html. Use *.html to reflect the actual output contract.
Proposed fix
- pattern: ".html"
+ pattern: "*.html"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - "*.html": | |
| type: file | |
| description: MultiQC report file | |
| pattern: ".html" | |
| ontologies: [] | |
| - "*.html": | |
| type: file | |
| description: MultiQC report file | |
| pattern: "*.html" | |
| ontologies: [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/multiqc/meta.yml` around lines 64 - 68, Update the pattern
field for the HTML output in the MultiQC module metadata to use a valid glob
pattern. Change the pattern value from ".html" to "*.html" to correctly match
actual emitted filenames like multiqc_report.html. This ensures the pattern
field accurately reflects the output contract of the module.
| - "*_plots": | ||
| type: file | ||
| description: Plots created by MultiQC | ||
| pattern: "*_plots" |
There was a problem hiding this comment.
Align plots output type with the process contract.
Line 86 declares plots as type: file, but modules/nf-core/multiqc/main.nf emits path("*_plots"), which is a directory. This cross-file mismatch can break schema/lint contract checks.
Proposed fix
plots:
@@
- "*_plots":
- type: file
+ type: directory
description: Plots created by MultiQC
pattern: "*_plots"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - "*_plots": | |
| type: file | |
| description: Plots created by MultiQC | |
| pattern: "*_plots" | |
| - "*_plots": | |
| type: directory | |
| description: Plots created by MultiQC | |
| pattern: "*_plots" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/multiqc/meta.yml` around lines 85 - 88, The type declaration
for the `*_plots` output does not match the actual emission type from the
Nextflow process. Change the `type: file` declaration for the `*_plots` output
to `type: directory` to correctly reflect that the process emits a directory
path (via `path("*_plots")` in main.nf) rather than a single file, ensuring the
schema aligns with the actual process contract.
| file('https://github.com/samtools/samtools/raw/refs/heads/develop/test/quickcheck/1.quickcheck.badeof.bam', checkIfExists: true), | ||
| ] |
There was a problem hiding this comment.
Pin remote quickcheck fixtures to immutable revisions
Line 151, Line 172, and Line 193 fetch test data from refs/heads/develop, which is mutable and can make CI nondeterministic. Please switch to immutable commit-SHA URLs (or vendored local fixtures) so snapshots and expected exit codes stay stable over time.
Also applies to: 172-173, 193-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/nf-core/samtools/quickcheck/tests/main.nf.test` around lines 151 -
152, The remote fixture URLs at lines 151-152, 172-173, and 193-194 use mutable
branch references (refs/heads/develop) which can cause non-deterministic CI
behavior. Replace all occurrences of the github.com URLs pointing to
refs/heads/develop with immutable commit-SHA based URLs (e.g.,
refs/heads/develop replaced with a specific commit hash) or vendor the test
fixtures locally to ensure snapshots and expected exit codes remain stable over
time.
…tion # Conflicts: # conf/base.config # conf/test.config # docs/architecture/implemented_pipeline.md # docs/output.md # workflows/treseq.nf
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/output.md (1)
138-149: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument only the report fields that are actually rendered.
The current report schema does not include any UMI column or RNA UMI summary:
build_export_rows()andsequencing_table()only output read counts, mapping, barcode rates, and usable reads. The bullets for “UMI/no-UMI fields” and “RNA observed UMI count” therefore overstate the shipped report.Suggested fix
-- a detailed per-library sequencing QC table for barcode, read-count, and UMI/no-UMI fields +- a detailed per-library sequencing QC table for barcode, read-count, mapping, and usable-read fields ... -- RNA observed UMI count from UMI tagging counts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/output.md` around lines 138 - 149, The report description is listing fields that are not actually rendered by the current report output. Update the documentation text in the report summary so it matches the real schema produced by build_export_rows() and sequencing_table(), keeping only the fields that appear in the HTML report. Remove the references to UMI/no-UMI fields and RNA observed UMI count, and make sure the bullets stay aligned with the rendered RNA mapping, DNA mapped/unique-read, barcode rate, and usable-read metrics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/render_tres_report.py`:
- Around line 443-462: The aggregate RNA genome-mapping and DNA mapped-read
metrics are computed in build_metrics() but never exposed because
main_statistics only contains RNA transcriptome and DNA unique reads. Update the
main_statistics list in render_tres_report.py to include the overall RNA
genome-mapped card and the DNA mapped-reads card alongside the existing entries,
using the corresponding values, counts, denominators, and subtitles from
mapping_quality. This will ensure those aggregate metrics show up in both the
HTML cards and the CSV/Excel exports driven by main_statistics.
---
Outside diff comments:
In `@docs/output.md`:
- Around line 138-149: The report description is listing fields that are not
actually rendered by the current report output. Update the documentation text in
the report summary so it matches the real schema produced by build_export_rows()
and sequencing_table(), keeping only the fields that appear in the HTML report.
Remove the references to UMI/no-UMI fields and RNA observed UMI count, and make
sure the bullets stay aligned with the rendered RNA mapping, DNA
mapped/unique-read, barcode rate, and usable-read metrics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42e6af5f-a9a5-49d3-9c9e-4384ac5761be
📒 Files selected for processing (8)
README.mdbin/render_tres_report.pyconf/base.configconf/test.configdocs/architecture/implemented_pipeline.mddocs/output.mddocs/usage.mdworkflows/treseq.nf
💤 Files with no reviewable changes (4)
- conf/test.config
- conf/base.config
- workflows/treseq.nf
- docs/architecture/implemented_pipeline.md
✅ Files skipped from review due to trivial changes (1)
- docs/usage.md
| main_statistics = [ | ||
| { | ||
| "metric": "RNA confidently mapped to transcriptome", | ||
| "value": mapping_quality["rna_confidently_mapped_to_transcriptome_percent"], | ||
| "count": rna_transcriptome_count, | ||
| "denominator": rna_raw_reads, | ||
| "value_type": "percent", | ||
| "subtitle": "STARsolo GeneFull unique reads / raw RNA reads", | ||
| "modality": "RNA", | ||
| }, | ||
| { | ||
| "metric": "DNA unique reads", | ||
| "value": mapping_quality["dna_unique_reads_percent"], | ||
| "count": dna_usable_count, | ||
| "denominator": dna_raw_reads, | ||
| "value_type": "percent", | ||
| "subtitle": "Final NoDup BAM read pairs / raw DNA reads", | ||
| "modality": "DNA", | ||
| }, | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose the aggregate genome/mapped cards as main statistics.
build_metrics() already computes overall RNA genome-mapping and DNA mapped-read percentages, but main_statistics only includes RNA transcriptome and DNA unique reads. Because both the HTML cards and CSV/Excel export are driven from main_statistics, those two aggregate metrics never appear anywhere in the report.
Suggested fix
main_statistics = [
{
"metric": "RNA confidently mapped to transcriptome",
"value": mapping_quality["rna_confidently_mapped_to_transcriptome_percent"],
"count": rna_transcriptome_count,
"denominator": rna_raw_reads,
"value_type": "percent",
"subtitle": "STARsolo GeneFull unique reads / raw RNA reads",
"modality": "RNA",
},
+ {
+ "metric": "RNA confidently mapped to genome",
+ "value": mapping_quality["rna_confidently_mapped_to_genome_percent"],
+ "count": rna_genome_count,
+ "denominator": rna_raw_reads,
+ "value_type": "percent",
+ "subtitle": "STARsolo unique genome reads / raw RNA reads",
+ "modality": "RNA",
+ },
+ {
+ "metric": "DNA confidently mapped reads",
+ "value": mapping_quality["dna_confidently_mapped_percent"],
+ "count": dna_mapped_count,
+ "denominator": dna_raw_reads,
+ "value_type": "percent",
+ "subtitle": "Aligned DNA read pairs / raw DNA reads",
+ "modality": "DNA",
+ },
{
"metric": "DNA unique reads",
"value": mapping_quality["dna_unique_reads_percent"],
"count": dna_usable_count,
"denominator": dna_raw_reads,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| main_statistics = [ | |
| { | |
| "metric": "RNA confidently mapped to transcriptome", | |
| "value": mapping_quality["rna_confidently_mapped_to_transcriptome_percent"], | |
| "count": rna_transcriptome_count, | |
| "denominator": rna_raw_reads, | |
| "value_type": "percent", | |
| "subtitle": "STARsolo GeneFull unique reads / raw RNA reads", | |
| "modality": "RNA", | |
| }, | |
| { | |
| "metric": "DNA unique reads", | |
| "value": mapping_quality["dna_unique_reads_percent"], | |
| "count": dna_usable_count, | |
| "denominator": dna_raw_reads, | |
| "value_type": "percent", | |
| "subtitle": "Final NoDup BAM read pairs / raw DNA reads", | |
| "modality": "DNA", | |
| }, | |
| ] | |
| main_statistics = [ | |
| { | |
| "metric": "RNA confidently mapped to transcriptome", | |
| "value": mapping_quality["rna_confidently_mapped_to_transcriptome_percent"], | |
| "count": rna_transcriptome_count, | |
| "denominator": rna_raw_reads, | |
| "value_type": "percent", | |
| "subtitle": "STARsolo GeneFull unique reads / raw RNA reads", | |
| "modality": "RNA", | |
| }, | |
| { | |
| "metric": "RNA confidently mapped to genome", | |
| "value": mapping_quality["rna_confidently_mapped_to_genome_percent"], | |
| "count": rna_genome_count, | |
| "denominator": rna_raw_reads, | |
| "value_type": "percent", | |
| "subtitle": "STARsolo unique genome reads / raw RNA reads", | |
| "modality": "RNA", | |
| }, | |
| { | |
| "metric": "DNA confidently mapped reads", | |
| "value": mapping_quality["dna_confidently_mapped_percent"], | |
| "count": dna_mapped_count, | |
| "denominator": dna_raw_reads, | |
| "value_type": "percent", | |
| "subtitle": "Aligned DNA read pairs / raw DNA reads", | |
| "modality": "DNA", | |
| }, | |
| { | |
| "metric": "DNA unique reads", | |
| "value": mapping_quality["dna_unique_reads_percent"], | |
| "count": dna_usable_count, | |
| "denominator": dna_raw_reads, | |
| "value_type": "percent", | |
| "subtitle": "Final NoDup BAM read pairs / raw DNA reads", | |
| "modality": "DNA", | |
| }, | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/render_tres_report.py` around lines 443 - 462, The aggregate RNA
genome-mapping and DNA mapped-read metrics are computed in build_metrics() but
never exposed because main_statistics only contains RNA transcriptome and DNA
unique reads. Update the main_statistics list in render_tres_report.py to
include the overall RNA genome-mapped card and the DNA mapped-reads card
alongside the existing entries, using the corresponding values, counts,
denominators, and subtitles from mapping_quality. This will ensure those
aggregate metrics show up in both the HTML cards and the CSV/Excel exports
driven by main_statistics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_fastq_compression.py (1)
35-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both input encodings in the mock path.
This test covers only
.fastq.gzinputs. Add a plain.fqor.fastqcase, or parameterize the test over both suffixes. This protects the new uncompressed-input branch incopy_as_uncompressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_fastq_compression.py` around lines 35 - 55, Extend test_mock_trim_galore_outputs_uncompressed_fastqs to exercise both compressed and plain FASTQ inputs, preferably by parameterizing the source suffixes while preserving the existing uncompressed output assertions. Ensure the plain-input case uses matching .fq or .fastq paths and verifies copy_as_uncompressed produces the same record without gzip headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/run_trim_galore.py`:
- Around line 28-29: Validate the --output-r1 and --output-r2 Path values before
they reach copy_as_uncompressed or shutil.move, rejecting absolute paths and
traversal outside the task output root (or accepting only approved base-name
outputs). Apply the same constraint consistently to every trim-output write
destination while preserving valid output handling.
---
Nitpick comments:
In `@tests/test_fastq_compression.py`:
- Around line 35-55: Extend test_mock_trim_galore_outputs_uncompressed_fastqs to
exercise both compressed and plain FASTQ inputs, preferably by parameterizing
the source suffixes while preserving the existing uncompressed output
assertions. Ensure the plain-input case uses matching .fq or .fastq paths and
verifies copy_as_uncompressed produces the same record without gzip headers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10980a8b-2187-4b41-b4f9-288f50ec7e3f
📒 Files selected for processing (19)
.gitattributes.github/workflows/nf-test.yml.pre-commit-config.yamlREADME.mdbin/run_trim_galore.pymodules.jsonmodules/local/rna_starsolo_align/main.nfmodules/local/split_dna_reads/main.nfmodules/local/split_rna_reads/main.nfmodules/local/trim_dna_fastqs/main.nfmodules/local/trim_rna_fastqs/main.nfscripts/core_runtime/Split_ReadsV2.codonscripts/core_runtime/Tag.codonscripts/core_runtime/Tag_Lig3.codonscripts/core_runtime/Tag_UMI.codonscripts/core_runtime/utils.codonsubworkflows/local/dna_core/main.nfsubworkflows/local/rna_core/main.nftests/test_fastq_compression.py
🚧 Files skipped from review as they are similar to previous changes (5)
- modules.json
- subworkflows/local/rna_core/main.nf
- modules/local/rna_starsolo_align/main.nf
- README.md
- subworkflows/local/dna_core/main.nf
| def copy_as_uncompressed(source: Path, destination: Path): | ||
| with open_maybe_gzip_binary(source, "rb") as src, open(destination, "wb") as dst: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: output paths are fixed task-local paths or are explicitly validated.
rg -n -C 4 --glob '*.py' --glob '*.nf' \
-e 'output[-_]r[12]' \
-e 'run_trim_galore\.py' \
-e 'sampleId' .Repository: CSOgroup/TrESFlow
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run_trim_galore.py =="
cat -n bin/run_trim_galore.py | sed -n '1,140p'
echo "== trim dna/nf sections ========="
for f in modules/local/trim_dna_fastqs/main.nf modules/local/trim_rna_fastqs/main.nf; do
echo "--- $f ---"
cat -n "$f" | sed -n '1,70p'
done
echo "== sampleId occurrences relevant to trim =="
rg -n -C 2 '\${sampleId}\b' modules/local/trim_*_fastqs/main.nf .github tests || true
echo "== path traversal verifier for argparse Path argument =="
python3 - <<'PY'
from pathlib import Path
import textwrap, tempfile, os, shutil
src = Path(tempfile.gettempdir()) / "src.fastq.gz"
try:
src.symlink_to(Path(tempfile.gettempdir()) / "outside_target") if not src.exists() else src.touch(exist_ok=True)
for p in ["../outside_target", tmp_path := Path(tempfile.gettempdir())/Path(tempfile.gettempdir()).name]:
print("traversal resolves absolute?", Path(p).is_absolute(), "resolved:", Path(p).resolve(), "parents include task dir?", any(Path(p).resolve() == Path(".") for _ in [0]))
finally:
src.unlink(missing_ok=True)
PYRepository: CSOgroup/TrESFlow
Length of output: 9383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sampleId occurrences relevant to trim =="
rg -n -C 2 '\$\{sampleId\}' modules/local/trim_dna_fastqs/main.nf modules/local/trim_rna_fastqs/main.nf || true
echo "== sampleId validation/boundary helpers =="
rg -n -C 3 'sampleId|meta\.id|meta[.]id|sanitize|validate|TaskException|exitStatus|process exit|workDir|publishDir|params.outdir' --glob '*.nf' --glob '*.py' .github || true
echo "== Nextflow docs-like path expression shape in trim modules =="
rg -n -C 2 'path\(".*\$\{sampleId\}' modules/local/trim_dna_fastqs/main.nf modules/local/trim_rna_fastqs/main.nfRepository: CSOgroup/TrESFlow
Length of output: 3587
Validate trim output destinations before writing.
bin/run_trim_galore.py accepts --output-r1 and --output-r2 as Path values and passes them directly to open / shutil.move. If untrusted CLI inputs reach the wrapper, values such as ../target or absolute paths can write outside the task work directory. Constrain these paths to the task output root, or validate them as approved base-name outputs before use.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 28-28: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(destination, "wb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/run_trim_galore.py` around lines 28 - 29, Validate the --output-r1 and
--output-r2 Path values before they reach copy_as_uncompressed or shutil.move,
rejecting absolute paths and traversal outside the task output root (or
accepting only approved base-name outputs). Apply the same constraint
consistently to every trim-output write destination while preserving valid
output handling.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/core_runtime/AlignDNA.sh`:
- Line 55: Quote all variable expansions in AlignDNA.sh command arguments,
including RGID, PathSam_header, view_threads, sort_threads, sort_mem,
blacklist_bed, and PathOutputBam, across the BWA, samtools, and rm invocations
on the referenced command blocks. Preserve each command’s existing behavior
while preventing word splitting and wildcard expansion.
In `@scripts/core_runtime/RNA_FILTERED_BAM.sh`:
- Line 20: Update the indexing and cleanup commands in the RNA filtering flow to
target OUTBAM rather than INBAM. Preserve the generated filtered BAM and its
accompanying .bai for coverage and QC, and do not delete the input BAM index.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f085646e-ca9f-40bf-b07d-9ce1f4922a64
📒 Files selected for processing (28)
bin/run_split_reads_dna.pybin/run_split_reads_rna.pybin/tresflow_fastq_utils.pyconf/base.configconf/test.configdocs/architecture/implemented_pipeline.mddocs/output.mdlib/WorkflowSupport.groovymodules/local/align_dna/main.nfmodules/local/compress_rna_filtered_bam/main.nfmodules/local/compress_split_fastqs/main.nfmodules/local/fq_to_sam/main.nfmodules/local/rna_filtered_bam/main.nfmodules/local/split_dna_reads/main.nfmodules/local/split_duplicates_dna/main.nfmodules/local/split_rna_reads/main.nfmodules/local/tag_dna_cell_barcode/main.nfmodules/local/tag_rna_cell_barcode/main.nfscripts/core_runtime/AlignDNA.shscripts/core_runtime/RNA_FILTERED_BAM.shscripts/core_runtime/RNA_STARSOLO_ALIGN.shsubworkflows/local/dna_core/main.nfsubworkflows/local/dna_core/meta.ymlsubworkflows/local/rna_core/main.nfsubworkflows/local/rna_core/meta.ymltests/default.nf.testtests/test_fastq_compression.pyworkflows/treseq.nf
💤 Files with no reviewable changes (3)
- bin/run_split_reads_rna.py
- bin/tresflow_fastq_utils.py
- bin/run_split_reads_dna.py
🚧 Files skipped from review as they are similar to previous changes (2)
- conf/base.config
- docs/output.md
|
|
||
| # Append the new header to the RG-replaced SAM file and convert to BAM | ||
| { cat ${RGID}_TEMPHEADER1.sam ${PathSam_header} ${RGID}_TEMPHEADER2.sam; "${SAMTOOLS_BIN}" view --threads ${view_threads} ${RGID}_TEMP.sam; } | "${SAMTOOLS_BIN}" sort --threads ${sort_threads} -m ${sort_mem} -n -o ${RGID}_TEMP.bam - | ||
| { cat ${RGID}_TEMPHEADER1.sam ${PathSam_header} ${RGID}_TEMPHEADER2.sam; "${SAMTOOLS_BIN}" view --threads ${view_threads} ${RGID}_TEMP.sam; } | "${SAMTOOLS_BIN}" sort --threads ${sort_threads} -m ${sort_mem} -l 0 -n -o ${RGID}_TEMP.bam - |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
shellcheck scripts/core_runtime/AlignDNA.shRepository: CSOgroup/TrESFlow
Length of output: 8718
Quote unquoted variables throughout scripts/core_runtime/AlignDNA.sh.
Unquoted ${RGID}, ${PathSam_header}, ${view_threads}, ${sort_threads}, ${sort_mem}, ${blacklist_bed}, and ${PathOutputBam} expansions can split or expand wildcards, so the BWA, samtools, and rm commands may fail or operate on unintended files. Apply quoting in the path arguments on lines 43-47, 55-59, 62-63, 66, and 70.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 55-55: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/core_runtime/AlignDNA.sh` at line 55, Quote all variable expansions
in AlignDNA.sh command arguments, including RGID, PathSam_header, view_threads,
sort_threads, sort_mem, blacklist_bed, and PathOutputBam, across the BWA,
samtools, and rm invocations on the referenced command blocks. Preserve each
command’s existing behavior while preventing word splitting and wildcard
expansion.
Source: Linters/SAST tools
…located relative to the directory where launch Nextflow
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/local/tres_report_html/main.nf (1)
32-32: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winShell-escape
meta.library_namebefore interpolation.This task builds the process script by interpolating
meta.library_name, so a value containing",$(), or backticks breaks the command argument and can execute in the task shell. Pass the value through a staged file or use the existingRuntimeSupport.shellQuotehelper before interpolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/local/tres_report_html/main.nf` at line 32, Update the command construction around the library-name argument in the process script to shell-escape meta.library_name before interpolation, using the existing RuntimeSupport.shellQuote helper or a staged-file approach. Preserve the existing unknown library fallback while ensuring values containing quotes, command substitutions, or backticks remain a single literal argument.modules/local/check_dna_nodup_bam/main.nf (1)
18-22: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the
readyoutput disjoint from staged inputs.At line 18,
noDupBamis staged asinput_NoDup.bam, and the output globs at line 21 match the same file and BAI. If the BAM has zero mapped reads,readycan carry the original empty staged BAM intoDEEPTOOLS_BAMCOVERAGE; if reads exist,readycan carry both the staged input and copied output. Use exact output names.Proposed fix
- tuple val(splitName), val(meta), path("*_NoDup.bam"), path("*_NoDup.bam.bai"), val(effectiveGenomeSize), optional: true, emit: ready + tuple val(splitName), val(meta), path("${splitName}_NoDup.bam"), path("${splitName}_NoDup.bam.bai"), val(effectiveGenomeSize), optional: true, emit: ready🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/local/check_dna_nodup_bam/main.nf` around lines 18 - 22, Update the ready output declaration in the process around the staged inputs so it matches only the generated BAM and BAI filenames, not the staged input names input_NoDup.bam and input_NoDup.bam.bai. Use exact output paths based on splitName, preserving the existing tuple structure and optional behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/RuntimeSupport.groovy`:
- Around line 113-115: Add a bounded timeout to the preflight process handling
around processBuilder.start() in RuntimeSupport, avoiding blocking
inputStream.getText() before waiting. Capture or redirect output without waiting
indefinitely, call waitFor with a timeout, and terminate the process when it
exceeds that timeout while preserving normal output and exit-code handling.
In `@README.md`:
- Around line 8-10: Update the version-support wording in README.md lines 8-10
and docs/usage.md lines 45-46: state that parser v1 is supported from Nextflow
24.10, parser v2 is available from Nextflow 25.04, and parser v2 becomes the
default in Nextflow 26.04.
---
Outside diff comments:
In `@modules/local/check_dna_nodup_bam/main.nf`:
- Around line 18-22: Update the ready output declaration in the process around
the staged inputs so it matches only the generated BAM and BAI filenames, not
the staged input names input_NoDup.bam and input_NoDup.bam.bai. Use exact output
paths based on splitName, preserving the existing tuple structure and optional
behavior.
In `@modules/local/tres_report_html/main.nf`:
- Line 32: Update the command construction around the library-name argument in
the process script to shell-escape meta.library_name before interpolation, using
the existing RuntimeSupport.shellQuote helper or a staged-file approach.
Preserve the existing unknown library fallback while ensuring values containing
quotes, command substitutions, or backticks remain a single literal argument.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f44c9f1-c765-4fa3-802a-40a8087ad3ff
📒 Files selected for processing (44)
.github/workflows/linting.yml.github/workflows/nf-test.ymlREADME.mdconf/base.configconf/modules.configconf/test.configconf/test_full.configdocs/usage.mdlib/RuntimeSupport.groovymain.nfmodules/local/align_dna/main.nfmodules/local/bam_coverage_dna/main.nfmodules/local/check_dna_nodup_bam/main.nfmodules/local/compress_rna_filtered_bam/main.nfmodules/local/compress_split_fastqs/main.nfmodules/local/fq_to_sam/main.nfmodules/local/mark_duplicates_dna/main.nfmodules/local/normalize_dna_bamcoverage/main.nfmodules/local/normalize_dna_markduplicates/main.nfmodules/local/rna_coverage/main.nfmodules/local/rna_filtered_bam/main.nfmodules/local/rna_starsolo_align/main.nfmodules/local/runtime_support/main.nfmodules/local/samtools_quickcheck_report/main.nfmodules/local/split_dna_reads/main.nfmodules/local/split_duplicates_dna/main.nfmodules/local/split_rna_reads/main.nfmodules/local/tag_dna_cell_barcode/main.nfmodules/local/tag_dna_modality/main.nfmodules/local/tag_dna_sb/main.nfmodules/local/tag_rna_cell_barcode/main.nfmodules/local/tag_rna_sb/main.nfmodules/local/tag_rna_umi/main.nfmodules/local/tres_report_html/main.nfmodules/local/trim_dna_fastqs/main.nfmodules/local/trim_rna_fastqs/main.nfnextflow.confignextflow_schema.jsonnf-test.configsubworkflows/local/dna_core/main.nfsubworkflows/local/rna_core/main.nftests/nextflow.configtests/test_launch_paths.shworkflows/treseq.nf
💤 Files with no reviewable changes (2)
- conf/test_full.config
- conf/test.config
🚧 Files skipped from review as they are similar to previous changes (16)
- modules/local/fq_to_sam/main.nf
- modules/local/compress_split_fastqs/main.nf
- modules/local/normalize_dna_markduplicates/main.nf
- modules/local/compress_rna_filtered_bam/main.nf
- modules/local/split_rna_reads/main.nf
- conf/base.config
- modules/local/split_duplicates_dna/main.nf
- modules/local/split_dna_reads/main.nf
- subworkflows/local/rna_core/main.nf
- modules/local/samtools_quickcheck_report/main.nf
- modules/local/normalize_dna_bamcoverage/main.nf
- modules/local/trim_dna_fastqs/main.nf
- modules/local/rna_starsolo_align/main.nf
- subworkflows/local/dna_core/main.nf
- workflows/treseq.nf
- conf/modules.config
| final Process process = processBuilder.start() | ||
| final String output = process.inputStream.getText('UTF-8').trim() | ||
| final int exitCode = process.waitFor() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'runCodonSeqPreflight|check_codon_seq_host\.sh' \
lib main.nf tests 2>/dev/null || true
rg -n -C 3 'nextflow\.version|manifest\s*\{' \
nextflow.config nf-test.config 2>/dev/null || trueRepository: CSOgroup/TrESFlow
Length of output: 2185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RuntimeSupport outline =="
ast-grep outline lib/RuntimeSupport.groovy || true
echo "== RuntimeSupport relevant lines =="
sed -n '80,145p' lib/RuntimeSupport.groovy
echo "== ProcessBuilder usage =="
rg -n -C 4 'ProcessBuilder|start\(|waitFor|getText|inputStream\.' lib/RuntimeSupport.groovy
echo "== Config nextflow version =="
rg -n -C 8 'nextflow.version|manifest\s*\{' nextflow.configRepository: CSOgroup/TrESFlow
Length of output: 4165
Add a timeout to the Codon/Seq preflight process.
InputStream.getText() reads until EOF, so process.waitFor() runs afterward and cannot avoid a stalled check_codon_seq_host.sh blocking workflow startup. Capture the output concurrently or redirect it to a temporary file, use a bounded waitFor(timeout, TimeUnit), and terminate the process if the timeout is reached.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/RuntimeSupport.groovy` around lines 113 - 115, Add a bounded timeout to
the preflight process handling around processBuilder.start() in RuntimeSupport,
avoiding blocking inputStream.getText() before waiting. Capture or redirect
output without waiting indefinitely, call waitFor with a timeout, and terminate
the process when it exceeds that timeout while preserving normal output and
exit-code handling.
| The same source tree supports Nextflow's parser v1 and parser v2. Nextflow | ||
| 24.10 or later is required; parser v2 is the default in Nextflow 26.04. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "README excerpt:"
if [ -f README.md ]; then
sed -n '1,20p' README.md | cat -n
else
echo "README.md missing"
fi
echo
echo "usage excerpt:"
if [ -f docs/usage.md ]; then
sed -n '35,55p' docs/usage.md | cat -n
else
echo "docs/usage.md missing"
fi
echo
echo "Search for parser/version references:"
rg -n "Nextflow|parser v2|parser v1|NXF_SYNTAX_PARSER|24\.10|25\.04|26\.04" -S . -g '!node_modules' -g '!dist' -g '!build' || true
echo
echo "Nextflow config files:"
git ls-files | rg "(^|/)(nextflow\.config|\.nextflow)$|pipeline|manifest" || trueRepository: CSOgroup/TrESFlow
Length of output: 18791
🌐 Web query:
Nextflow NXF_SYNTAX_PARSER version introduced parser v2 default nextflow 26.04 25.04
💡 Result:
The Nextflow v2 syntax parser (also known as the strict syntax parser) became the default in Nextflow version 26.04 [1][2][3]. In previous versions, such as 25.04 and 25.10, the v2 parser was available but disabled by default [2][4][5]. Users could enable it by setting the environment variable NXF_SYNTAX_PARSER=v2 [1][6][5]. With the release of Nextflow 26.04, the v2 parser is enabled by default [3][7]. Users who wish to continue using the legacy parser can revert to it by setting the environment variable NXF_SYNTAX_PARSER=v1 [2][3][5].
Citations:
- 1: https://github.com/nextflow-io/nextflow/blob/master/docs/strict-syntax.md
- 2: https://github.com/nextflow-io/nextflow/blob/master/docs/strict-syntax.mdx
- 3: https://docs.seqera.io/nextflow/migrations/26-04
- 4: https://github.com/nextflow-io/nextflow/blob/master/docs/migrations/25-04.md
- 5: https://docs.seqera.io/nextflow/strict-syntax
- 6: https://training.nextflow.io/latest/info/nxf_versions/
- 7: https://docs.seqera.io/changelog/nextflow/v26.04.0
🌐 Web query:
Nextflow 25.04 NXF_SYNTAX_PARSER strict syntax parser v2 enabled by default
💡 Result:
In Nextflow 25.04, the strict syntax parser (v2) is not enabled by default [1][2]. It is an opt-in feature that can be enabled by setting the environment variable NXF_SYNTAX_PARSER=v2 [3][1]. The v2 strict syntax parser became the default starting in Nextflow 26.04 [4][5]. When running Nextflow 26.04 or later, users who need to maintain compatibility with legacy code can revert to the previous behavior by explicitly setting NXF_SYNTAX_PARSER=v1 [4][5].
Citations:
- 1: https://docs.seqera.io/nextflow/strict-syntax
- 2: https://docs.seqera.io/nextflow/migrations/25-04
- 3: https://github.com/nextflow-io/nextflow/blob/master/docs/migrations/25-04.md
- 4: https://github.com/nextflow-io/nextflow/blob/master/docs/strict-syntax.md
- 5: https://docs.seqera.io/platform-cloud/troubleshooting_and_faqs/nextflow
🌐 Web query:
Seqera Nextflow NXF_SYNTAX_PARSER introduced 25.04 documentation
💡 Result:
The NXF_SYNTAX_PARSER environment variable was introduced in Nextflow 25.04 to allow users to toggle between the legacy parser (v1) and the new strict syntax parser (v2) [1][2][3]. Key details regarding NXF_SYNTAX_PARSER include: Purpose: The strict syntax (v2) provides a strict implementation of Nextflow DSL2 based on the official language specification, aiming to provide more consistent parsing and better error messaging compared to the legacy (v1) parser [2][4][5]. Availability: It was first made available for use in the Nextflow CLI in version 25.04 [2][3]. Default Behavior: - In versions 25.04 and 25.10, the legacy parser (v1) is enabled by default, and the strict parser must be manually enabled by setting NXF_SYNTAX_PARSER=v2 [4][5]. - In version 26.04 and later, the strict parser (v2) is enabled by default, and users can revert to the legacy parser by setting NXF_SYNTAX_PARSER=v1 [1][4][5]. Pipeline developers are encouraged to test and migrate their code to be compatible with the v2 parser, as the legacy parser is intended to be deprecated and removed in the future [4][5]. Additionally, newer language features—such as static typing—require the use of the strict parser [4][6].
Citations:
- 1: https://github.com/nextflow-io/nextflow/blob/master/docs/strict-syntax.md
- 2: https://github.com/nextflow-io/nextflow/blob/master/docs/migrations/25-04.md
- 3: https://docs.seqera.io/nextflow/migrations/25-04
- 4: https://github.com/nextflow-io/nextflow/blob/master/docs/strict-syntax.mdx
- 5: https://docs.seqera.io/nextflow/strict-syntax
- 6: https://github.com/nextflow-io/nextflow/blob/master/docs/migrations/25-10.md
🌐 Web query:
seqera nextflow 26.04 NXF_SYNTAX_PARSER=v2 unavailable 26.04 default parser documentation
💡 Result:
In Nextflow 26.04 and later, the strict syntax parser (v2) is enabled by default [1][2][3]. Consequently, setting NXF_SYNTAX_PARSER=v2 is not required to enable this behavior, as it is the standard setting [2][4][3]. If you are experiencing issues with the strict parser or need to revert to the previous behavior to run existing pipelines unchanged, you can explicitly re-enable the legacy parser by setting the environment variable NXF_SYNTAX_PARSER=v1 [1][5][6]. The v2 parser enforces stricter validation of Nextflow DSL2 scripts and configuration files compared to the legacy (v1) parser [2][6][3]. If your pipeline fails under the default v2 parser, it is likely due to the use of Groovy syntax that is not compliant with the strict Nextflow language specification [2][6]. You can migrate your code to be compatible with the v2 parser by following the guidance provided in the official Seqera documentation, which includes details on addressing common unsupported patterns [6][3]. Note that some users have encountered configuration parsing incompatibilities when using the v2 parser with certain Seqera platform configurations, which can be addressed by ensuring environment variables are referenced correctly (e.g., using "${env('VAR')}" instead of "$VAR") [7].
Citations:
- 1: https://docs.seqera.io/nextflow/migrations/26-04
- 2: https://seqera.io/blog/nextflow-26-04/
- 3: https://docs.seqera.io/nextflow/strict-syntax
- 4: https://github.com/nextflow-io/nextflow/blob/master/docs/strict-syntax.md
- 5: https://docs.seqera.io/platform-cloud/troubleshooting_and_faqs/nextflow
- 6: https://github.com/nextflow-io/nextflow/blob/master/docs/strict-syntax.mdx
- 7: Seqera configuration incompatibilites with NXF_SYNTAX_PARSER=v2 nextflow-io/nextflow#6994
Split parser support into parser-specific version floors.
The current wording makes Nextflow 24.10 look sufficient for parser v2 and misses the parser-v2 minimum. Update both README.md lines 8-9 and docs/usage.md lines 45-46 to state parser v1 support from Nextflow 24.10, and parser v2 availability/default from Nextflow 25.04/26.04.
📍 Affects 2 files
README.md#L8-L10(this comment)docs/usage.md#L45-L46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 8 - 10, Update the version-support wording in
README.md lines 8-10 and docs/usage.md lines 45-46: state that parser v1 is
supported from Nextflow 24.10, parser v2 is available from Nextflow 25.04, and
parser v2 becomes the default in Nextflow 26.04.
Updated pipeline HTML report with fixed nfcore modules issues
Summary by CodeRabbit
qc/fastqc/andqc/samtools/.multiqc/.