Conversation
|
Warning Review limit reached
More reviews will be available in 38 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughVersion v0.7.0 adds end-to-end differential DamID binding analysis: a new ChangesdamidBind Differential Binding Feature
Sequence DiagramsequenceDiagram
participant Snakemake
participant general_functions as general_functions.smk
participant damidbind_rule as damidbind rule
participant damidbindR as damidbind.R
participant StagingDir as Symlink Staging
participant damidBind as damidBind pkg
participant Outputs as PDF/CSV Outputs
Snakemake->>general_functions: run_damidbind(BG_SAMPLES)
general_functions-->>Snakemake: True / False
Snakemake->>general_functions: targets()
general_functions-->>Snakemake: append damidbind result paths if True
Snakemake->>damidbind_rule: expand bedgraph + GFF inputs over DIRS/BG_SAMPLES
damidbind_rule->>damidbindR: params(genome, normalization, fdr, filter_occupancy)
damidbindR->>StagingDir: create dirs, symlink bedGraphs and GFFs
damidbindR->>damidBind: load_data_peaks(bg_dir, peaks_dir, normalization, organism)
damidBind-->>damidbindR: binding data
damidbindR->>damidBind: differential_binding(data, conditions, fdr, filter_occupancy)
damidBind-->>damidbindR: results or error
damidbindR->>Outputs: diagnostic PDF, Venn PDF, volcano PDF
damidbindR->>Outputs: analysisTable() → peaks.csv
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Resolved conflicts in favour of damidbind branch: - Dockerfile/README: new damidbind conda env, updated hashes, pinned v1.1.1 - Snakefile: keep VERSION = "v0.7.0" - damid.yaml: keep wget, drop r-biocmanager (moved to damidbind env) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
workflow/Snakefile (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid shadowing
run_damidbindwith a boolean flag.Line 60 rebinds the function name to
bool, which makes later callable use impossible and obscures intent.Suggested refactor
--- a/workflow/Snakefile +++ b/workflow/Snakefile @@ -run_damidbind = run_damidbind(BG_SAMPLES) +should_run_damidbind = run_damidbind(BG_SAMPLES)--- a/workflow/scripts/general_functions.smk +++ b/workflow/scripts/general_functions.smk @@ - if run_damidbind: + if should_run_damidbind:🤖 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 `@workflow/Snakefile` at line 60, The variable assignment on line 60 shadows the function `run_damidbind` by rebinding its name to the boolean result of calling `run_damidbind(BG_SAMPLES)`. Replace the left-hand side variable name with a more descriptive variable name that represents the boolean flag returned by the function call, such as something like `should_run_damidbind` or a similar meaningful name. This preserves the function name `run_damidbind` for any later callable uses and makes the intent of storing a boolean result explicit.
🤖 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 `@workflow/envs/damid.post-deploy.sh`:
- Line 2: The post-deploy script references BiocManager::install() to install
the damidBind package, but the r-biocmanager dependency is not declared in the
damid.yaml environment file, which will cause deployment failures. Add
r-biocmanager as a dependency in the damid.yaml file immediately after the
r-base dependency declaration to ensure the BiocManager package is available
when the post-deploy script executes.
In `@workflow/envs/damidbind.post-deploy.sh`:
- Around line 1-4: The post-deploy script lacks strict error handling which
allows command failures to be masked if subsequent commands succeed. Add the
bash option flags `set -euo pipefail` as a new line immediately after the
shebang (#!/bin/bash) to enable fail-fast behavior. This will ensure that if
either of the Rscript commands fails, the script will immediately exit with a
non-zero status rather than continuing execution and potentially exiting with a
success code.
In `@workflow/scripts/damidbind.R`:
- Around line 66-75: After the line that creates the unique conditions vector
(conditions <- unique(conditions)), add an explicit length validation check to
ensure the conditions vector contains exactly 2 elements. If the length is not
2, stop execution with a clear error message that explains the requirement. This
validation should occur before the line that converts conditions to a named
vector and before any call to differential_binding(), so that errors are caught
early with a meaningful message rather than failing deep within the damidBind
library code.
- Around line 29-53: The symlinks in outdir_bg and outdir_peaks directories are
created conditionally (only if they don't exist), which means stale symlinks
from previous runs persist when sample sets change. To fix this, after the
dir.create() calls for outdir_bg and outdir_peaks, add code to remove all
existing files and symlinks from these directories before the for loops that
create symlinks for bedgraphs and peaks_gff. This ensures a clean state on each
run and prevents stale files from affecting the analysis.
In `@workflow/Snakefile`:
- Line 13: The VERSION constant in workflow/Snakefile has been updated to
v0.7.0, but the corresponding version variable in
workflow/scripts/general_functions.smk is still set to v0.6.0, causing
inconsistent version reporting in logs. Update the version string in
general_functions.smk to match the new v0.7.0 version to ensure consistent
version reporting across the workflow.
---
Nitpick comments:
In `@workflow/Snakefile`:
- Line 60: The variable assignment on line 60 shadows the function
`run_damidbind` by rebinding its name to the boolean result of calling
`run_damidbind(BG_SAMPLES)`. Replace the left-hand side variable name with a
more descriptive variable name that represents the boolean flag returned by the
function call, such as something like `should_run_damidbind` or a similar
meaningful name. This preserves the function name `run_damidbind` for any later
callable uses and makes the intent of storing a boolean result explicit.
🪄 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: e311684e-c00e-4509-9f6a-e78aa9c4e6d5
📒 Files selected for processing (14)
.docker/Dockerfile.docker/README.md.docker/v0.5.0/Dockerfile.test_pe/config/config.yaml.test_se/config/config.yamlconfig/config.yamlworkflow/Snakefileworkflow/envs/damid.post-deploy.shworkflow/envs/damid.yamlworkflow/envs/damidbind.post-deploy.shworkflow/envs/damidbind.yamlworkflow/rules/damid.smkworkflow/scripts/damidbind.Rworkflow/scripts/general_functions.smk
💤 Files with no reviewable changes (1)
- .docker/v0.5.0/Dockerfile
| #!/bin/bash | ||
|
|
||
| Rscript -e 'install.packages("BioVenn", repos="https://cloud.r-project.org"); stopifnot(requireNamespace("BioVenn", quietly=TRUE))' | ||
| Rscript -e 'remotes::install_github("marshall-lab/damidBind", ref="v1.1.1", upgrade="never"); stopifnot(requireNamespace("damidBind", quietly=TRUE))' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
nl -ba workflow/envs/damidbind.post-deploy.sh
# Verify there are multiple install commands and no fail-fast guard.Repository: niekwit/damid-seq
Length of output: 195
🏁 Script executed:
cat -n workflow/envs/damidbind.post-deploy.shRepository: niekwit/damid-seq
Length of output: 478
🏁 Script executed:
# Also check if the file exists and get its size
ls -lh workflow/envs/damidbind.post-deploy.shRepository: niekwit/damid-seq
Length of output: 242
Enable fail-fast behavior in this post-deploy script.
The script lacks shell options to enforce strict error handling. If the first Rscript command on line 3 fails but the second one on line 4 succeeds, the script will exit with code 0, masking the failure. Add set -euo pipefail after the shebang to catch and propagate any command failures.
Proposed fix
#!/bin/bash
+set -euo pipefail
Rscript -e 'install.packages("BioVenn", repos="https://cloud.r-project.org"); stopifnot(requireNamespace("BioVenn", quietly=TRUE))'
Rscript -e 'remotes::install_github("marshall-lab/damidBind", ref="v1.1.1", upgrade="never"); stopifnot(requireNamespace("damidBind", quietly=TRUE))'📝 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.
| #!/bin/bash | |
| Rscript -e 'install.packages("BioVenn", repos="https://cloud.r-project.org"); stopifnot(requireNamespace("BioVenn", quietly=TRUE))' | |
| Rscript -e 'remotes::install_github("marshall-lab/damidBind", ref="v1.1.1", upgrade="never"); stopifnot(requireNamespace("damidBind", quietly=TRUE))' | |
| #!/bin/bash | |
| set -euo pipefail | |
| Rscript -e 'install.packages("BioVenn", repos="https://cloud.r-project.org"); stopifnot(requireNamespace("BioVenn", quietly=TRUE))' | |
| Rscript -e 'remotes::install_github("marshall-lab/damidBind", ref="v1.1.1", upgrade="never"); stopifnot(requireNamespace("damidBind", quietly=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 `@workflow/envs/damidbind.post-deploy.sh` around lines 1 - 4, The post-deploy
script lacks strict error handling which allows command failures to be masked if
subsequent commands succeed. Add the bash option flags `set -euo pipefail` as a
new line immediately after the shebang (#!/bin/bash) to enable fail-fast
behavior. This will ensure that if either of the Rscript commands fails, the
script will immediately exit with a non-zero status rather than continuing
execution and potentially exiting with a success code.
| dir.create(outdir_bg, recursive = TRUE, showWarnings = FALSE) | ||
| dir.create(outdir_peaks, recursive = TRUE, showWarnings = FALSE) | ||
|
|
||
| print("Creating simlinks to original data in directory struture for damidBind") | ||
| for (bg in bedgraphs) { | ||
| dir_name <- basename(dirname(bg)) | ||
| new_name <- sub( | ||
| "^(.*?)(-vs-Dam.*)", | ||
| paste0("\\1_", dir_name, "\\2"), | ||
| basename(bg) | ||
| ) | ||
| dest <- file.path(outdir_bg, new_name) | ||
| if (!file.exists(dest)) file.symlink(normalizePath(bg), dest) | ||
| } | ||
|
|
||
| for (gff in peaks_gff) { | ||
| dir_name <- basename(dirname(gff)) | ||
| new_name <- sub( | ||
| "\\.peaks\\.gff$", | ||
| paste0("_", dir_name, ".peaks.gff"), | ||
| basename(gff) | ||
| ) | ||
| dest <- file.path(outdir_peaks, new_name) | ||
| if (!file.exists(dest)) file.symlink(normalizePath(gff), dest) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
nl -ba workflow/scripts/damidbind.R | sed -n '26,60p'
# Verify staging dirs are reused and links are only conditionally created without cleanup.Repository: niekwit/damid-seq
Length of output: 191
🏁 Script executed:
sed -n '26,60p' workflow/scripts/damidbind.RRepository: niekwit/damid-seq
Length of output: 1233
🏁 Script executed:
grep -r "load_data_peaks" workflow/scripts/damidbind.RRepository: niekwit/damid-seq
Length of output: 180
🏁 Script executed:
find workflow -name "*.R" -type f | head -20Repository: niekwit/damid-seq
Length of output: 470
🏁 Script executed:
rg -t R "^\s*load_data_peaks\s*(<|=)" --max-count 5Repository: niekwit/damid-seq
Length of output: 181
🏁 Script executed:
rg "load_data_peaks" --type-list | grep -i rRepository: niekwit/damid-seq
Length of output: 545
🏁 Script executed:
rg "load_data_peaks" -A 10Repository: niekwit/damid-seq
Length of output: 155
🏁 Script executed:
rg "function.*load_data_peaks" --max-count 5Repository: niekwit/damid-seq
Length of output: 155
🏁 Script executed:
grep -r "load_data_peaks\s*<-\s*function\|load_data_peaks\s*=\s*function" --include="*.R"Repository: niekwit/damid-seq
Length of output: 155
🏁 Script executed:
grep -r "load_data_peaks" --include="*.R"Repository: niekwit/damid-seq
Length of output: 209
🏁 Script executed:
grep -r "load_data_peaks" --include="*.R" -B 2 -A 10Repository: niekwit/damid-seq
Length of output: 855
🏁 Script executed:
grep -r "library\|source" workflow/scripts/damidbind.R | head -20Repository: niekwit/damid-seq
Length of output: 193
🏁 Script executed:
cat workflow/scripts/damidbind.R | head -100Repository: niekwit/damid-seq
Length of output: 3064
🏁 Script executed:
grep -r "outdir_bg\|outdir_peaks" workflow/scripts/damidbind.R -B 2 -A 2Repository: niekwit/damid-seq
Length of output: 1198
🏁 Script executed:
web_search
damidBind R package load_data_peaks function documentationRepository: niekwit/damid-seq
Length of output: 246
🏁 Script executed:
grep -r "load_data_peaks" . --include="*.md" --include="*.txt" --include="*.yaml" -B 2 -A 2Repository: niekwit/damid-seq
Length of output: 155
🏁 Script executed:
grep -r "list.files\|dir\|list" workflow/scripts/damidbind.R | head -20Repository: niekwit/damid-seq
Length of output: 936
🏁 Script executed:
find . -name "Snakefile" -o -name "snakefile" -o -name "*.smk" | head -10Repository: niekwit/damid-seq
Length of output: 480
🏁 Script executed:
grep -r "outdir_bg\|outdir_peaks" . --include="Snakefile" --include="*.smk" -B 3 -A 3Repository: niekwit/damid-seq
Length of output: 804
🏁 Script executed:
grep -r "damidbind" . --include="Snakefile" --include="*.smk" -B 5 -A 10Repository: niekwit/damid-seq
Length of output: 6465
🏁 Script executed:
cat workflow/rules/damid.smk | grep -A 30 "rule damidbind"Repository: niekwit/damid-seq
Length of output: 1282
🏁 Script executed:
find . -name "*.md" -o -name "README*" | head -20Repository: niekwit/damid-seq
Length of output: 244
🏁 Script executed:
grep -r "how.*load.*files\|all.*files" workflow/scripts/damidbind.RRepository: niekwit/damid-seq
Length of output: 217
🏁 Script executed:
cat workflow/rules/damid.smk | grep -B 5 -A 40 "rule damidbind"Repository: niekwit/damid-seq
Length of output: 1569
🏁 Script executed:
grep -r "temp\|directory" workflow/rules/damid.smk | head -20Repository: niekwit/damid-seq
Length of output: 663
🏁 Script executed:
find . -name ".snakemake" -o -name "config.yaml" | head -10Repository: niekwit/damid-seq
Length of output: 236
🏁 Script executed:
cat config/config.yaml | head -50Repository: niekwit/damid-seq
Length of output: 2100
🏁 Script executed:
grep -r "workdir\|rmdirs\|cleanup" workflow/ --include="*.smk" | head -10Repository: niekwit/damid-seq
Length of output: 155
🏁 Script executed:
sed -n '29,53p' workflow/scripts/damidbind.RRepository: niekwit/damid-seq
Length of output: 911
🌐 Web query:
damidBind load_data_peaks function behavior consumes files
💡 Result:
The load_data_peaks function in the damidBind package is designed to load genome-wide binding data and associated peak regions into an R session for subsequent analysis [1][2]. It does not "consume" files in the sense of deleting them; rather, it reads and processes the information contained within them [1][3]. The function operates by accepting either file paths to data or pre-loaded GRanges objects [1][4]. Specifically, it performs the following behaviors: 1. Input Handling: It requires either path arguments (binding_profiles_path and peaks_path) to point to bedGraph/GFF/bed files, or the direct provision of named lists of GRanges objects via the binding_profiles and peaks arguments [1][3][4]. The function will stop with an error if both paths and objects are provided simultaneously, or if the necessary inputs are missing [1][5]. 2. Data Processing: It reads log2 ratio binding data (from bedGraph files) and peak regions (from GFF/bed files) [1][2]. It then processes these by calculating occupancy scores and annotating the regions with nearby genes [2][6]. 3. Configuration: Users can control aspects of the process, such as quantile normalization (via the quantile_norm parameter) and the distance threshold for gene-peak association (via the maxgap_loci parameter, which defaults to 1000 bp) [1][3][4]. 4. Diagnostics: By default, the function generates and displays diagnostic plots, such as PCA and correlation heatmaps, to assist in evaluating the input data [1][3][4]. In summary, the function imports external data into the R environment, enabling the downstream statistical analysis required by the damidBind pipeline [2][6].
Citations:
- 1: https://marshall-lab.org/damidBind/reference/load_data_peaks.html
- 2: https://bioconductor.statistik.tu-dortmund.de/packages/3.23/bioc/manuals/damidBind/man/damidBind.pdf
- 3: https://marshall-lab.org/damidBind/articles/damidBind_vignette.html
- 4: https://bioconductor.posit.co/packages/3.23/bioc/vignettes/damidBind/inst/doc/damidBind_vignette.html
- 5: https://github.com/marshall-lab/damidBind/blob/HEAD/R/load_data.R
- 6: https://marshall-lab.org/damidBind/
🏁 Script executed:
grep -r "--forceall\|--rerun" workflow/ config/ --include="*.yaml" --include="*.smk"Repository: niekwit/damid-seq
Length of output: 286
🏁 Script executed:
cat workflow/rules/damid.smk | tail -20Repository: niekwit/damid-seq
Length of output: 897
🏁 Script executed:
grep -A 5 "rule damidbind:" workflow/rules/damid.smk | grep -E "output|temp|directory"Repository: niekwit/damid-seq
Length of output: 155
Clear staging directories before creating symlinks.
load_data_peaks() processes all files in outdir_bg and outdir_peaks. Because symlinks are created only conditionally (if (!file.exists(dest))) and old links are never removed, reruns with changed sample sets can include stale files and alter analysis results.
Proposed fix
dir.create(outdir_bg, recursive = TRUE, showWarnings = FALSE)
dir.create(outdir_peaks, recursive = TRUE, showWarnings = FALSE)
+
+# Remove stale staged files from prior runs
+bg_existing <- list.files(outdir_bg, full.names = TRUE)
+if (length(bg_existing) > 0) unlink(bg_existing, force = TRUE)
+peaks_existing <- list.files(outdir_peaks, full.names = TRUE)
+if (length(peaks_existing) > 0) unlink(peaks_existing, force = TRUE)📝 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.
| dir.create(outdir_bg, recursive = TRUE, showWarnings = FALSE) | |
| dir.create(outdir_peaks, recursive = TRUE, showWarnings = FALSE) | |
| print("Creating simlinks to original data in directory struture for damidBind") | |
| for (bg in bedgraphs) { | |
| dir_name <- basename(dirname(bg)) | |
| new_name <- sub( | |
| "^(.*?)(-vs-Dam.*)", | |
| paste0("\\1_", dir_name, "\\2"), | |
| basename(bg) | |
| ) | |
| dest <- file.path(outdir_bg, new_name) | |
| if (!file.exists(dest)) file.symlink(normalizePath(bg), dest) | |
| } | |
| for (gff in peaks_gff) { | |
| dir_name <- basename(dirname(gff)) | |
| new_name <- sub( | |
| "\\.peaks\\.gff$", | |
| paste0("_", dir_name, ".peaks.gff"), | |
| basename(gff) | |
| ) | |
| dest <- file.path(outdir_peaks, new_name) | |
| if (!file.exists(dest)) file.symlink(normalizePath(gff), dest) | |
| } | |
| dir.create(outdir_bg, recursive = TRUE, showWarnings = FALSE) | |
| dir.create(outdir_peaks, recursive = TRUE, showWarnings = FALSE) | |
| # Remove stale staged files from prior runs | |
| bg_existing <- list.files(outdir_bg, full.names = TRUE) | |
| if (length(bg_existing) > 0) unlink(bg_existing, force = TRUE) | |
| peaks_existing <- list.files(outdir_peaks, full.names = TRUE) | |
| if (length(peaks_existing) > 0) unlink(peaks_existing, force = TRUE) | |
| print("Creating simlinks to original data in directory struture for damidBind") | |
| for (bg in bedgraphs) { | |
| dir_name <- basename(dirname(bg)) | |
| new_name <- sub( | |
| "^(.*?)(-vs-Dam.*)", | |
| paste0("\\1_", dir_name, "\\2"), | |
| basename(bg) | |
| ) | |
| dest <- file.path(outdir_bg, new_name) | |
| if (!file.exists(dest)) file.symlink(normalizePath(bg), dest) | |
| } | |
| for (gff in peaks_gff) { | |
| dir_name <- basename(dirname(gff)) | |
| new_name <- sub( | |
| "\\.peaks\\.gff$", | |
| paste0("_", dir_name, ".peaks.gff"), | |
| basename(gff) | |
| ) | |
| dest <- file.path(outdir_peaks, new_name) | |
| if (!file.exists(dest)) file.symlink(normalizePath(gff), dest) | |
| } |
🤖 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 `@workflow/scripts/damidbind.R` around lines 29 - 53, The symlinks in outdir_bg
and outdir_peaks directories are created conditionally (only if they don't
exist), which means stale symlinks from previous runs persist when sample sets
change. To fix this, after the dir.create() calls for outdir_bg and
outdir_peaks, add code to remove all existing files and symlinks from these
directories before the for loops that create symlinks for bedgraphs and
peaks_gff. This ensures a clean state on each run and prevents stale files from
affecting the analysis.
| conditions <- c() | ||
| for (bg in bedgraphs) { | ||
| condition <- sub("-vs-Dam.*", "", basename(bg)) | ||
| conditions <- c(conditions, condition) | ||
| } | ||
| conditions <- unique(conditions) | ||
|
|
||
| # Convert to named vector for damidBind | ||
| names(conditions) <- conditions | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What is the expected format and required length of the condargument fordamidBind::differential_binding?
💡 Result:
The cond argument for the damidBind::differential_binding function must be a character vector of exactly length two [1][2]. This vector specifies the two experimental conditions to be compared [3]. The elements of the vector represent the strings or regular expressions used to identify samples belonging to each respective condition within the input filenames [1][2]. The order of the elements within the vector determines the direction of the contrast (e.g., cond[1] is compared against cond[4]) [1][2]. You can provide either a named or an unnamed character vector [1][2]: - If named, the names are used as user-friendly labels for the conditions in generated plots and outputs [1][3][2]. - If unnamed, the match strings themselves are used as the display names [1][2]. Example usage: diff_results <- differential_binding( loaded_data, cond = c("Display Name 1" = "Condition 1 ID", "Display Name 2" = "Condition 2 ID")) [1][3]
Citations:
- 1: https://marshall-lab.org/damidBind/reference/differential_binding.html
- 2: https://bioconductor.statistik.tu-dortmund.de/packages/3.23/bioc/manuals/damidBind/man/damidBind.pdf
- 3: https://marshall-lab.org/damidBind/articles/damidBind_vignette.html
- 4: https://github.com/marshall-lab/damidBind/blob/HEAD/R/limma_functions.R
Add an explicit length check to validate exactly 2 conditions before differential_binding() call.
damidBind::differential_binding() requires a character vector of exactly two conditions. Invalid sample naming or configuration will currently fail deep in library code. Add early validation with a clear error message.
Proposed fix
conditions <- unique(conditions)
+if (length(conditions) != 2) {
+ stop(
+ sprintf(
+ "damidBind requires exactly 2 conditions for differential_binding(); got %d (%s).",
+ length(conditions),
+ paste(conditions, collapse = ", ")
+ ),
+ call. = FALSE
+ )
+}
# Convert to named vector for damidBind
names(conditions) <- conditions📝 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.
| conditions <- c() | |
| for (bg in bedgraphs) { | |
| condition <- sub("-vs-Dam.*", "", basename(bg)) | |
| conditions <- c(conditions, condition) | |
| } | |
| conditions <- unique(conditions) | |
| # Convert to named vector for damidBind | |
| names(conditions) <- conditions | |
| conditions <- c() | |
| for (bg in bedgraphs) { | |
| condition <- sub("-vs-Dam.*", "", basename(bg)) | |
| conditions <- c(conditions, condition) | |
| } | |
| conditions <- unique(conditions) | |
| if (length(conditions) != 2) { | |
| stop( | |
| sprintf( | |
| "damidBind requires exactly 2 conditions for differential_binding(); got %d (%s).", | |
| length(conditions), | |
| paste(conditions, collapse = ", ") | |
| ), | |
| call. = FALSE | |
| ) | |
| } | |
| # Convert to named vector for damidBind | |
| names(conditions) <- conditions |
🤖 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 `@workflow/scripts/damidbind.R` around lines 66 - 75, After the line that
creates the unique conditions vector (conditions <- unique(conditions)), add an
explicit length validation check to ensure the conditions vector contains
exactly 2 elements. If the length is not 2, stop execution with a clear error
message that explains the requirement. This validation should occur before the
line that converts conditions to a named vector and before any call to
differential_binding(), so that errors are caught early with a meaningful
message rather than failing deep within the damidBind library code.
The old script installed damidBind into the damid conda environment. damidBind now has its own environment (damidbind.yaml) with a dedicated post-deploy script, so this file is no longer needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.github/workflows/main.yml:
- Line 47: The cache key on line 47 only hashes workflow/envs/*.yaml files and
doesn't account for changes to workflow/envs/*.post-deploy.sh scripts, which can
mutate the environment contents and cause the cache to become stale. Modify the
key parameter in the hashFiles function to include both file patterns by
combining them with a pipe separator or hash multiple patterns, ensuring that
changes to either the YAML configuration files or the post-deploy shell scripts
will invalidate the cache and trigger a fresh environment setup.
- Around line 13-15: The checkout steps in the workflow are persisting
credentials unnecessarily, which increases security risk. Add
`persist-credentials: false` to the `with` block for each `actions/checkout@v4`
step (appearing at lines 13-15, 27, and 41) to disable credential persistence
since no authenticated Git operations like pushing are required in this
workflow.
- Line 27: The workflow uses an outdated and incompatible version of the
checkout action (actions/checkout@v2) which relies on Node.js 12 that is no
longer supported by GitHub's current runners (Node.js 20+). Replace all
instances of actions/checkout@v2 (found at line 27 and line 41) with
actions/checkout@v4, and pin it to a specific commit SHA instead of just the
version tag for improved security. This ensures compatibility with current
GitHub Actions infrastructure and follows security best practices.
- Line 13: Replace all tag-based GitHub Actions references with their full
commit SHA hashes in the workflow file. For the actions/checkout entries,
upgrade any instances of v2 to v4, then replace the version tags with their
corresponding commit SHAs. Apply this pattern consistently across all uses
directives mentioned in the affected lines (13, 17, 27, 29, 41, 44, 51, 62, 74,
85) to eliminate security risks from upstream tag retargeting and ensure all
actions reference immutable commit hashes.
In `@workflow/rules/damid.smk`:
- Line 20: The unsorted Bowtie2 BAM outputs at lines 20 and 72 are currently
materialized permanently, but since they are only consumed by downstream sort
steps, they should be marked as temporary to prevent unnecessary disk space
accumulation. Wrap the bam file path "results/bam/{dir}/{sample}.bam" with the
temp() function at both locations to allow Snakemake to automatically delete
these intermediate files after the downstream sorting rules have completed
processing them.
In `@workflow/Snakefile`:
- Line 56: The run_damidbind activation at line 56 is gated only on BG_SAMPLES,
but the rule damidbind requires peak calling input files
(results/peaks/fdr.../*.peaks.gff) that are only generated when peak calling is
enabled. When peak_calling_perl.run is false, these required input files will
not exist, causing the rule to fail with missing inputs. Modify the condition
for run_damidbind to also check that peak_calling_perl.run is enabled, ensuring
damidbind is only activated when both BG_SAMPLES exists and the peak calling
results are available.
🪄 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: 70eed196-732d-43f4-af75-f1b9ba5a808f
⛔ Files ignored due to path filters (19)
.test_pe/config/samples.csvis excluded by!**/*.csv.test_pe/reads/15917/Dam_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/Dam_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF1A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF1A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF2A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF2A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/Dam_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/Dam_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF1A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF1A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF2A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF2A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/Dam_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/Dam_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF1A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF1A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF2A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF2A_R2_001.fastq.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
.docker/Dockerfile.github/workflows/main.yml.test_pe/config/config.yaml.test_se/config/config.yamlworkflow/Snakefileworkflow/envs/damid.yamlworkflow/rules/damid.smkworkflow/scripts/filter_consensus_peaks.pyworkflow/scripts/general_functions.smk
🚧 Files skipped from review as they are similar to previous changes (1)
- .docker/Dockerfile
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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 @.github/workflows/main.yml:
- Line 47: The cache key on line 47 only hashes workflow/envs/*.yaml files and
doesn't account for changes to workflow/envs/*.post-deploy.sh scripts, which can
mutate the environment contents and cause the cache to become stale. Modify the
key parameter in the hashFiles function to include both file patterns by
combining them with a pipe separator or hash multiple patterns, ensuring that
changes to either the YAML configuration files or the post-deploy shell scripts
will invalidate the cache and trigger a fresh environment setup.
- Around line 13-15: The checkout steps in the workflow are persisting
credentials unnecessarily, which increases security risk. Add
`persist-credentials: false` to the `with` block for each `actions/checkout@v4`
step (appearing at lines 13-15, 27, and 41) to disable credential persistence
since no authenticated Git operations like pushing are required in this
workflow.
- Line 27: The workflow uses an outdated and incompatible version of the
checkout action (actions/checkout@v2) which relies on Node.js 12 that is no
longer supported by GitHub's current runners (Node.js 20+). Replace all
instances of actions/checkout@v2 (found at line 27 and line 41) with
actions/checkout@v4, and pin it to a specific commit SHA instead of just the
version tag for improved security. This ensures compatibility with current
GitHub Actions infrastructure and follows security best practices.
- Line 13: Replace all tag-based GitHub Actions references with their full
commit SHA hashes in the workflow file. For the actions/checkout entries,
upgrade any instances of v2 to v4, then replace the version tags with their
corresponding commit SHAs. Apply this pattern consistently across all uses
directives mentioned in the affected lines (13, 17, 27, 29, 41, 44, 51, 62, 74,
85) to eliminate security risks from upstream tag retargeting and ensure all
actions reference immutable commit hashes.
In `@workflow/rules/damid.smk`:
- Line 20: The unsorted Bowtie2 BAM outputs at lines 20 and 72 are currently
materialized permanently, but since they are only consumed by downstream sort
steps, they should be marked as temporary to prevent unnecessary disk space
accumulation. Wrap the bam file path "results/bam/{dir}/{sample}.bam" with the
temp() function at both locations to allow Snakemake to automatically delete
these intermediate files after the downstream sorting rules have completed
processing them.
In `@workflow/Snakefile`:
- Line 56: The run_damidbind activation at line 56 is gated only on BG_SAMPLES,
but the rule damidbind requires peak calling input files
(results/peaks/fdr.../*.peaks.gff) that are only generated when peak calling is
enabled. When peak_calling_perl.run is false, these required input files will
not exist, causing the rule to fail with missing inputs. Modify the condition
for run_damidbind to also check that peak_calling_perl.run is enabled, ensuring
damidbind is only activated when both BG_SAMPLES exists and the peak calling
results are available.
🪄 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: 70eed196-732d-43f4-af75-f1b9ba5a808f
⛔ Files ignored due to path filters (19)
.test_pe/config/samples.csvis excluded by!**/*.csv.test_pe/reads/15917/Dam_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/Dam_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF1A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF1A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF2A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/15917/HIF2A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/Dam_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/Dam_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF1A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF1A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF2A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/16197/HIF2A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/Dam_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/Dam_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF1A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF1A_R2_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF2A_R1_001.fastq.gzis excluded by!**/*.gz.test_pe/reads/2315/HIF2A_R2_001.fastq.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
.docker/Dockerfile.github/workflows/main.yml.test_pe/config/config.yaml.test_se/config/config.yamlworkflow/Snakefileworkflow/envs/damid.yamlworkflow/rules/damid.smkworkflow/scripts/filter_consensus_peaks.pyworkflow/scripts/general_functions.smk
🚧 Files skipped from review as they are similar to previous changes (1)
- .docker/Dockerfile
🛑 Comments failed to post (6)
.github/workflows/main.yml (4)
13-13: 🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/main.yml | head -100Repository: niekwit/damid-seq
Length of output: 3462
Pin all GitHub Actions to full commit SHAs.
These
uses:references are tag-based, not SHA-pinned (e.g., Line 13, 17, 29, 44), which leaves CI exposed to upstream tag retargeting. Additionally, Lines 27 and 41 use the deprecatedactions/checkout@v2, which no longer receives updates or security patches.Suggested hardening pattern
- uses: actions/checkout@v4 + uses: actions/checkout@<full_commit_sha>Apply the same pattern to each
uses:entry in this workflow. Foractions/checkout, upgrade to v4 if using v2.Also applies to: 17-17, 27-27, 29-29, 41-41, 44-44, 51-51, 62-62, 74-74, 85-85
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 13-15: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/main.yml at line 13, Replace all tag-based GitHub Actions references with their full commit SHA hashes in the workflow file. For the actions/checkout entries, upgrade any instances of v2 to v4, then replace the version tags with their corresponding commit SHAs. Apply this pattern consistently across all uses directives mentioned in the affected lines (13, 17, 27, 29, 41, 44, 51, 62, 74, 85) to eliminate security risks from upstream tag retargeting and ensure all actions reference immutable commit hashes.Source: Linters/SAST tools
13-15: 🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the workflow file find . -name "main.yml" -path ".github/workflows/*" | head -20Repository: niekwit/damid-seq
Length of output: 155
🏁 Script executed:
# Read the workflow file to see all checkout steps cat -n .github/workflows/main.ymlRepository: niekwit/damid-seq
Length of output: 3462
🌐 Web query:
GitHub Actions checkout action persist-credentials security best practice💡 Result:
In GitHub Actions, the persist-credentials input for the actions/checkout action determines whether the repository's authentication token (typically GITHUB_TOKEN) is stored in the local Git configuration of the runner [1][2][3]. Security Best Practice The established security best practice is to set persist-credentials to false unless your workflow specifically requires authenticated Git operations (such as git push) during the job [4][3]. By setting persist-credentials: false, you prevent the credentials from being stored in the runner's Git configuration, which minimizes the risk of unauthorized access to these credentials by subsequent steps or third-party actions in your workflow [4][3]. Key Considerations: 1. Default Behavior: By default, persist-credentials is set to true [3][5]. This means that if you do not explicitly set it to false, the credentials will be persisted for all subsequent steps in the job [3]. 2. When to Use True: Only use persist-credentials: true when your workflow must perform authenticated Git commands, such as pushing commits back to the repository [3]. Even in these cases, ensure your GITHUB_TOKEN has the least-privilege permissions necessary for the required tasks [1][3]. 3. Risk Mitigation: If the credentials are persisted (the default), any malicious code or compromised third-party action running in subsequent steps could potentially access or misuse these credentials [3][6]. While newer versions of actions/checkout have improved the security of this storage (e.g., storing credentials in a temporary file under $RUNNER_TEMP rather than.git/config), setting it to false remains the most robust defense-in-depth approach when the functionality is not strictly required [1][2][5]. 4. Alternative for Pushing: If you need to perform Git operations, consider whether you can use the GitHub API instead, or strictly isolate the steps that require authentication to ensure they are the only ones with access [3]. Summary To adhere to the principle of least privilege, explicitly include persist-credentials: false in all checkout steps where git push or similar authenticated Git operations are not required [4][3]. - step: checkout uses: actions/checkout@v4 with: persist-credentials: false
Citations:
- 1: https://github.com/actions/checkout
- 2: https://github.com/actions/checkout/?tab=readme-ov-file
- 3: https://actsense.dev/vulnerabilities/unsafe_checkout/
- 4: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
- 5: actions/checkout#2312
- 6: actions/checkout#485
Add
persist-credentials: falseto all checkout steps.The checkout steps currently allow persisted credentials. Set
persist-credentials: falseto reduce token exposure to subsequent steps, unless authenticated Git operations (e.g.,git push) are required. None are needed in this workflow.Proposed fix
- uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: falseFor single-line checkout steps without a
withblock:- - uses: actions/checkout@v2 + - uses: actions/checkout@v2 + with: + persist-credentials: falseAlso applies to: lines 27 and 41.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 13-15: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/main.yml around lines 13 - 15, The checkout steps in the workflow are persisting credentials unnecessarily, which increases security risk. Add `persist-credentials: false` to the `with` block for each `actions/checkout@v4` step (appearing at lines 13-15, 27, and 41) to disable credential persistence since no authenticated Git operations like pushing are required in this workflow.Source: Linters/SAST tools
27-27: 🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -name "main.yml" -path ".github/workflows/*" | head -5Repository: niekwit/damid-seq
Length of output: 155
🏁 Script executed:
cat -n .github/workflows/main.yml | sed -n '20,50p'Repository: niekwit/damid-seq
Length of output: 1158
🌐 Web query:
actions/checkout v2 deprecated incompatible GitHub Actions runners 2024 2025💡 Result:
The actions/checkout@v2 action is effectively deprecated due to its reliance on outdated Node.js runtimes, specifically Node.js 12 [1]. As GitHub Actions has transitioned to newer Node.js runtimes—first to Node.js 16, then Node.js 20, and most recently Node.js 24—older versions of actions like checkout@v2 have become incompatible with modern GitHub Actions runner environments [2][3][4]. Key points regarding the incompatibility and deprecation include: 1. Runtime Incompatibility: GitHub Actions frequently updates its runner images to newer Node.js versions to maintain security and performance [4][1]. The actions/checkout@v2 action, which was built for the Node.js 12 era, lacks support for the newer Node.js environments (v20 and v24) required by current GitHub-hosted runners [2][3]. 2. Minimum Runner Version Requirements: Newer major versions of actions/checkout, such as v5 and v6, which utilize the Node.js 24 runtime, require specific minimum versions of the Actions Runner software (e.g., v2.327.1 or later) [2][5]. Self-hosted runners that are not updated to these minimum versions will fail to execute these newer actions, as they cannot support the required Node.js runtime [2]. 3. Recommended Action: If you encounter issues or deprecation warnings related to actions/checkout, the recommended course of action is to update to the latest major version (e.g., v4, v5, or v6) [2][6]. This ensures compatibility with current GitHub Actions infrastructure, including the latest runner images and Node.js runtimes [2][3]. 4. Self-Hosted Runner Maintenance: For users maintaining self-hosted runners, it is critical to ensure that the runner software is kept up to date [2]. GitHub documentation specifies that self-hosted runners should be updated within 30 days of a new version release to maintain compatibility with updated action runtimes [2]. If a self-hosted runner is not updated, it may fail to run actions that have been upgraded to newer Node.js runtimes [2].
Citations:
- 1: https://github.com/actions/checkout/pull/689/commits
- 2: actions/checkout#2240
- 3: https://github.com/actions/checkout/blob/main/README.md
- 4: actions/checkout#1596
- 5: https://github.com/actions/checkout/releases
- 6: https://stackoverflow.com/questions/77897660/github-actions-node-js-16-actions-are-deprecated-warning
Upgrade
actions/checkout@v2to v4; v2 is incompatible with current GitHub Actions runners.
actions/checkout@v2relies on Node.js 12, which is no longer supported by GitHub-hosted runners (which now use Node.js 20+). This will cause workflow failures. Upgrade to at least v4 and pin to a specific commit SHA for security.Proposed fix
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 + with: + ref: <commit-sha>Also applies to: Line 41
🧰 Tools
🪛 actionlint (1.7.12)
[error] 27-27: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 zizmor (1.26.1)
[warning] 27-27: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/main.yml at line 27, The workflow uses an outdated and incompatible version of the checkout action (actions/checkout@v2) which relies on Node.js 12 that is no longer supported by GitHub's current runners (Node.js 20+). Replace all instances of actions/checkout@v2 (found at line 27 and line 41) with actions/checkout@v4, and pin it to a specific commit SHA instead of just the version tag for improved security. This ensures compatibility with current GitHub Actions infrastructure and follows security best practices.Source: Linters/SAST tools
47-47: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cache key misses post-deploy installers, so conda cache can go stale.
Line 47 keys only on
workflow/envs/*.yaml. This won’t invalidate cache whenworkflow/envs/*.post-deploy.shchanges, even though those scripts mutate environment contents.Proposed key expansion
- key: conda-${{ hashFiles('workflow/envs/*.yaml') }} + key: conda-${{ hashFiles('workflow/envs/*.yaml', 'workflow/envs/*.post-deploy.sh') }}📝 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.key: conda-${{ hashFiles('workflow/envs/*.yaml', 'workflow/envs/*.post-deploy.sh') }}🤖 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 @.github/workflows/main.yml at line 47, The cache key on line 47 only hashes workflow/envs/*.yaml files and doesn't account for changes to workflow/envs/*.post-deploy.sh scripts, which can mutate the environment contents and cause the cache to become stale. Modify the key parameter in the hashFiles function to include both file patterns by combining them with a pipe separator or hash multiple patterns, ensuring that changes to either the YAML configuration files or the post-deploy shell scripts will invalidate the cache and trigger a fresh environment setup.workflow/rules/damid.smk (1)
20-20: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make unsorted Bowtie2 BAMs temporary to prevent disk-pressure failures.
Line 20 and Line 72 now materialize large intermediate BAMs permanently. These are only consumed by downstream sort steps, so keeping them can cause avoidable storage growth and runtime failures on bigger datasets.
Suggested fix
- bam="results/bam/{dir}/{sample}.bam", + bam=temp("results/bam/{dir}/{sample}.bam"), ... - bam="results/bam/{dir}/{sample}.bt2.bam", + bam=temp("results/bam/{dir}/{sample}.bt2.bam"),Also applies to: 72-72
🤖 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 `@workflow/rules/damid.smk` at line 20, The unsorted Bowtie2 BAM outputs at lines 20 and 72 are currently materialized permanently, but since they are only consumed by downstream sort steps, they should be marked as temporary to prevent unnecessary disk space accumulation. Wrap the bam file path "results/bam/{dir}/{sample}.bam" with the temp() function at both locations to allow Snakemake to automatically delete these intermediate files after the downstream sorting rules have completed processing them.workflow/Snakefile (1)
56-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate damidbind activation on peak-caller availability.
At Line 56, activation is based only on
BG_SAMPLES. Butrule damidbindrequiresresults/peaks/fdr.../*.peaks.gffinputs; withpeak_calling_perl.run: false, this can still target damidbind outputs and fail with missing inputs.Suggested fix
-run_damidbind = run_damidbind(BG_SAMPLES) +run_damidbind = config["peak_calling_perl"]["run"] and run_damidbind(BG_SAMPLES)🤖 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 `@workflow/Snakefile` at line 56, The run_damidbind activation at line 56 is gated only on BG_SAMPLES, but the rule damidbind requires peak calling input files (results/peaks/fdr.../*.peaks.gff) that are only generated when peak calling is enabled. When peak_calling_perl.run is false, these required input files will not exist, causing the rule to fail with missing inputs. Modify the condition for run_damidbind to also check that peak_calling_perl.run is enabled, ensuring damidbind is only activated when both BG_SAMPLES exists and the peak calling results are available.
1 participant
Workflow can now run damidBind (https://github.com/marshall-lab/damidBind), when multiple non-Dam samples are run to find differential peaks.
Summary by CodeRabbit
Release Notes
New Features
peaks.csv).Bug Fixes
Chores