fix: ensure proper wrapper prefix is passed to CWL and shown in wrapper error messages by johanneskoester · Pull Request #4121 · snakemake/snakemake

johanneskoester · 2026-03-19T15:25:19Z

Description

QC

The PR contains a test case for the changes or the changes are already covered by an existing test case.
The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Summary by CodeRabbit

Bug Fixes
- Fixed generation of the wrapper-prefix argument so it’s included only when set, preventing malformed command invocations.
- Improved error messages when wrapper scripts can’t be located — they now show a clearer URL to aid troubleshooting.
- Ensured consistent construction of wrapper URLs when a default prefix is used.

…er error messages

coderabbitai · 2026-03-19T15:25:39Z

📝 Walkthrough

Walkthrough

Adds a DEFAULT_WRAPPER_PREFIX constant and updates wrapper URL resolution and error messages; the CWL generator now emits a single combined --wrapper-prefix {value} argument only when a truthy wrapper prefix is present.

Changes

Cohort / File(s)	Summary
Wrapper module `src/snakemake/wrapper.py`	Add `DEFAULT_WRAPPER_PREFIX` constant; use it when `prefix` is None and the reference is not a version tag; construct error-message URLs with the default prefix when `prefix` is unset.
CWL argument generation `src/snakemake/cwl.py`	In `dag_to_cwl`, emit a single combined `--wrapper-prefix {wrapper_prefix}` argument only when `wrapper_prefix` is truthy; omit the argument entirely otherwise.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is incomplete with only QC checklist items filled but no substantive explanation of the changes, rationale, or impact.	Add a description section explaining what changes were made, why they were necessary, and any relevant context or testing details.
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main changes: ensuring proper wrapper prefix handling in CWL and wrapper error messages.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/wrapper-script-error-handling

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/snakemake/cwl.py`:
- Around line 205-211: The current argument construction uses *[
(f"--wrapper-prefix {dag.workflow.workflow_settings.wrapper_prefix}" if
dag.workflow.workflow_settings.wrapper_prefix else []) ], which when
wrapper_prefix is falsy inserts an empty list element into the arguments list;
change the expression to produce either a single-item list or an empty list
before unpacking—e.g. replace the outer list-wrapped conditional with (
[f"--wrapper-prefix {dag.workflow.workflow_settings.wrapper_prefix}"] if
dag.workflow.workflow_settings.wrapper_prefix else [] ) so that unpacking with *
yields no element when wrapper_prefix is unset (locate the snippet building the
CWL arguments around dag.workflow.workflow_settings.wrapper_prefix).

In `@src/snakemake/wrapper.py`:
- Around line 120-123: The WorkflowError message uses DEFAULT_WRAPPER_PREFIX
unconditionally which can misreport the actual wrapper location; update the
error to include the actual prefix used when locating the script (the variable
named prefix or the computed effective prefix) instead of
DEFAULT_WRAPPER_PREFIX, falling back to DEFAULT_WRAPPER_PREFIX only if no custom
prefix is present so the message correctly shows the URL/path used when
script_source is None (refer to script_source, DEFAULT_WRAPPER_PREFIX, prefix,
and path to locate and fix the message).

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d15679b4-0e63-47cc-9694-473859c9bdc4

📥 Commits

Reviewing files that changed from the base of the PR and between e249e79 and aa2b462.

📒 Files selected for processing (2)

src/snakemake/cwl.py
src/snakemake/wrapper.py

src/snakemake/cwl.py

src/snakemake/wrapper.py

coderabbitai

♻️ Duplicate comments (1)

src/snakemake/cwl.py (1)
205-209: ⚠️ Potential issue | 🟠 Major

The --wrapper-prefix argument format is incorrect for CWL.

The argument is emitted as a single combined string "--wrapper-prefix value" which CWL will pass as one argv entry. argparse expects these as two separate argv entries: --wrapper-prefix followed by the value.

Compare to --mode on lines 215-216 which correctly uses separate elements:
"--mode",
str(ExecMode.SUBPROCESS.item_to_choice()),
The past review already suggested this fix - split into two elements:
Proposed fix
             *(
-                [f"--wrapper-prefix {dag.workflow.workflow_settings.wrapper_prefix}"]
+                ["--wrapper-prefix", dag.workflow.workflow_settings.wrapper_prefix]
                 if dag.workflow.workflow_settings.wrapper_prefix
                 else []
             ),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/cwl.py` around lines 205 - 209, The CWL argument for wrapper
prefix is emitted as a single combined string so it becomes one argv entry;
update the args construction that currently uses [f"--wrapper-prefix
{dag.workflow.workflow_settings.wrapper_prefix}"] to emit two separate argv
entries: "--wrapper-prefix" and the value
dag.workflow.workflow_settings.wrapper_prefix (mirroring how "--mode" is
produced with ExecMode.SUBPROCESS.item_to_choice()), ensuring the list contains
two elements instead of one combined string.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/snakemake/cwl.py`:
- Around line 205-209: The CWL argument for wrapper prefix is emitted as a
single combined string so it becomes one argv entry; update the args
construction that currently uses [f"--wrapper-prefix
{dag.workflow.workflow_settings.wrapper_prefix}"] to emit two separate argv
entries: "--wrapper-prefix" and the value
dag.workflow.workflow_settings.wrapper_prefix (mirroring how "--mode" is
produced with ExecMode.SUBPROCESS.item_to_choice()), ensuring the list contains
two elements instead of one combined string.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9f0dbda-58ff-45d6-994d-dda93dfa3f7f

📥 Commits

Reviewing files that changed from the base of the PR and between 65818f6 and 14f86b8.

📒 Files selected for processing (1)

src/snakemake/cwl.py

🤖 I have created a release *beep* *boop* --- ## [9.17.3](v9.17.2...v9.17.3) (2026-03-24) ### Bug Fixes * add curl when containerize with wrapper ([#4115](#4115)) ([44979e4](44979e4)) * ensure proper wrapper prefix is passed to CWL and shown in wrapper error messages ([#4121](#4121)) ([11b6f29](11b6f29)) * ensure that strings that purely contain integers or floats (e.g. "42") remain strings when parsing profiles ([#4119](#4119)) ([3ca08e1](3ca08e1)) * incorrect highlighting in HTML report ([#4120](#4120)) ([1ef224d](1ef224d)) ### Documentation * document an accidental (sorry) recent breaking change in type checking compatibility of Python scripts, in favor of a clean and robust new syntax ([#4116](#4116)) ([013bc43](013bc43)) * Rework tutorial ([#4068](#4068)) ([4bba4a9](4bba4a9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).  ## Summary by CodeRabbit * **Chores** * Released version 9.17.3

fix: ensure proper wrapper prefix is passed to CWL and shown in wrapp…

aa2b462

…er error messages

coderabbitai bot reviewed Mar 19, 2026

View reviewed changes

src/snakemake/cwl.py Outdated Show resolved Hide resolved

src/snakemake/wrapper.py Show resolved Hide resolved

johanneskoester and others added 3 commits March 19, 2026 16:35

fix prefix

65818f6

fix

6c88eb2

Merge branch 'main' into fix/wrapper-script-error-handling

14f86b8

coderabbitai bot reviewed Mar 24, 2026

View reviewed changes

johanneskoester merged commit 11b6f29 into main Mar 24, 2026
59 of 60 checks passed

johanneskoester deleted the fix/wrapper-script-error-handling branch March 24, 2026 11:53

snakemake-bot mentioned this pull request Mar 24, 2026

chore(main): release 9.17.3 #4112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure proper wrapper prefix is passed to CWL and shown in wrapper error messages#4121

fix: ensure proper wrapper prefix is passed to CWL and shown in wrapper error messages#4121
johanneskoester merged 4 commits intomainfrom
fix/wrapper-script-error-handling

johanneskoester commented Mar 19, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 19, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

johanneskoester commented Mar 19, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

QC

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

johanneskoester commented Mar 19, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 19, 2026 •

edited

Loading