8000 fix: validate manifest.name format when loading pipeline config by kokhlo · Pull Request #4470 · nf-core/tools · GitHub
[go: up one dir, main page]
Skip to content

fix: validate manifest.name format when loading pipeline config - #4470

Open
kokhlo wants to merge 2 commits into
nf-core:mainfrom
kokhlo:fix/4467-manifest-name-validation
Open

fix: validate manifest.name format when loading pipeline config#4470
kokhlo wants to merge 2 commits into
nf-core:mainfrom
kokhlo:fix/4467-manifest-name-validation

Conversation

@kokhlo
@kokhlo kokhlo commented Sep 15, 2026
Copy link
Copy Markdown

Summary

Both malformed manifest.name cases in #4467 trace 8000 back to one unconditional unpack in Pipeline.load_pipeline_config() (nf_core/utils.py):

self.pipeline_prefix, self.pipeline_name = manifest.get("name", "/").split("/")
  • name = 'My-Pipeline' (no /) → ValueError: not enough values to unpack (expected 2, got 1). The CLI's selective_traceback_hook suppresses tracebacks for ValueError, so nothing pointed at manifest.name, and the crash happened before Testing pipeline: was printed.
  • name missing → the "/" default unpacks to ("", ""), and the empty pipeline_name later raises IndexError in files_exist (the crash reported in IndexError: string index out of range #4464).

This PR validates the name at the source: if manifest.name is missing, or does not split into exactly two non-empty <repo>/<pipeline> parts, raise a UserWarning naming the field and the expected format. All CLI callers (pipelines lint, pipelines bump-version, pipelines rocrate) already catch UserWarning and print it as a clean one-line error, so both cases now report e.g.:

ERROR  manifest.name 'My-Pipeline' is not in the format '<repo>/<pipeline>'. Please set it to the full pipeline name, e.g. 'nf-core/mypipeline'.

Note: #4466 guards the files_exist.py symptom downstream — that stays compatible; this change just makes the guard unreachable in practice.

Testing

  • New regression tests for both cases on a copied fixture pipeline:
    • tests/test_utils.py::TestUtils::test_load_pipeline_config_malformed_manifest_name
    • tests/test_utils.py::TestUtils::test_load_pipeline_config_missing_manifest_name
  • Verified end-to-end with the real CLI (nf-core pipelines lint on a scaffolded pipeline): both malformed values now produce the one-line ERROR naming manifest.name (exit 1) instead of the bare ValueError / IndexError traceback.
  • uv run pytest tests/test_utils.py tests/pipelines/lint/ tests/pipelines/test_lint.py: 83 passed, 3 pre-existing failures on clean main on macOS (symlinked-tmpdir /var vs /private/var path asserts in test_list_files_git/test_list_files_no_git, and test_files_unchanged_pass/test_template_strings_ignored) — reproduced identical on unmodified main via git stash, unrelated to this change.
  • ruff check + ruff format --check clean on touched files.

Fixes #4467

@kokhlo
fix: validate manifest.name format when loading pipeline config
13767b0 
Raise a UserWarning naming manifest.name when it is missing or not in
the '<repo>/<pipeline>' format, instead of crashing with an opaque
'not enough values to unpack' ValueError (or an IndexError later in
files_exist) that gave no hint about which config field was at fault.

All CLI callers of Pipeline.load_pipeline_config() already catch
UserWarning and print it as a clean one-line error.

Signed-off-by: kokhlo <konstantin.khlopkov93@gmail.com>
mashehu
mashehu reviewed Oct 2, 2026
View reviewed changes
Comment thread nf_core/utils.py Resolved
Comment thread tests/test_utils.py Resolved
Comment thread tests/test_utils.py Resolved
@kokhlo
fix: name the manifest.name format placeholder <org> rather than <repo>
7735d01 
Applied the reviewer's three suggestions: the manifest.name error message,
its test docstring and the test's match pattern now say <org>/<pipeline>.
That is the placeholder the surrounding codebase already uses for the org
component (nf_core/pipelines/list.py).

tests/test_utils.py: 32 passed, 2 pre-existing failures
(test_list_files_git, test_list_files_no_git — the fixture copies the
pipeline to a tmp dir with no .git, so git init fails; both fail
identically without this change).
@kokhlo
kokhlo commented Oct 2, 2026
Copy link
Copy Markdown
Author

@/tmp/foss_4470_reply.md

@kokhlo
kokhlo commented Oct 2, 2026
Copy link
Copy Markdown
Author

@/tmp/foss_4470_correction.md

@mashehu
mashehu commented Oct 3, 2026
Copy link
Copy Markdown
Member

something seems to be off with your gh posting setup 😉

@kokhlo
kokhlo commented Oct 5, 2026
Copy link
Copy Markdown
Author

Good catch on all three, and they are in as of 7735d01c9 — I checked the placeholder convention on main before renaming rather than just swapping the word: nf_core/pipelines/list.py:43-44 already writes <org>/<pipeline>/clones/<sha>/ for the working tree and <org>/<pipeline>/bare/ for the bare repo, so <org> is what this codebase calls that component.

One correction while I am here: the commit message says the two test_utils.py failures are "git init fails in the tmp fixture". That is the wrong cause. Both are a macOS path-alias artifact — TMPDIR resolves to /var/folders/... while Path.resolve() returns /private/var/folders/..., so the membership assertion compares two spellings of the same directory. Verified on a separate worktree at the merge-base eb2f70909: the same two tests fail there by name, without my diff. On this branch the file is 2 failed, 32 passed.

And my previous two comments in this thread are the setup you flagged — they posted a local path instead of the text. That was a scripting slip on my side, not something to interpret. The substance those comments were meant to carry is the <org> rename above plus the 2 failed, 32 passed split, both of which are now in the commit itself.

One thing I deliberately did not touch: files_exist.py:127 and files_unchanged.py:77 still say <repo>/<pipeline>, so the three words are not consistent repo-wide yet. Renaming those is a two-line change but it is outside this PR's scope (it validates the name, it does not report it), so I left it alone rather than widening the diff — happy to do it as a separate one if you want the terminology settled in a single pass.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mashehu mashehu mashehu left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Malformed or missing manifest.name gives opaque error / IndexError instead of a lint failure

2 participants

@kokhlo @mashehu
0