From 17795545927e5a23e0e1bae224c4784d8f418fcc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 26 May 2026 14:38:02 +0000 Subject: [PATCH 1/6] fix: use repo-root-relative path in git restore call in container_configs.py Agent-Logs-Url: https://github.com/nf-core/tools/sessions/1d9f4c7e-f5d1-42ae-9baa-8e581e139ebb Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com> --- nf_core/pipelines/lint/container_configs.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nf_core/pipelines/lint/container_configs.py b/nf_core/pipelines/lint/container_configs.py index b5143dba82..6e4af632f2 100644 --- a/nf_core/pipelines/lint/container_configs.py +++ b/nf_core/pipelines/lint/container_configs.py @@ -70,8 +70,9 @@ def container_configs(self): if not fixing: # Restore working tree: reset modified tracked files and delete new untracked ones + repo_root = Path(repo.working_tree_dir).resolve() for name in modified: - repo.git.restore(str(conf_dir / name)) + repo.git.restore(str((conf_dir / name).resolve().relative_to(repo_root))) for name in new: (conf_dir / name).unlink(missing_ok=True) From 4c9af23e4083969a90fcefe1552d1b3575d28ba7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 26 May 2026 15:08:53 +0000 Subject: [PATCH 2/6] fix: replace git-diff detection with content comparison in container_configs lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old approach used repo.index.diff(None) to detect stale configs, which compared the working tree against HEAD. This caused false 'out of date' failures when configs were already regenerated (e.g. by modules update) but not yet committed. The new approach: 1. Snapshots file content on disk before generation 2. Generates new configs 3. Compares new content to the snapshot 4. If unchanged → pass; if different → fail This makes the lint correct regardless of the git commit state. The restore logic now simply writes back the saved content instead of using git restore, which also fixes the original repo-root-relative path bug. Also adds two new tests: - test_container_configs_uncommitted_but_correct_passes: verifies that uncommitted but correct configs don't produce false failures - test_container_configs_working_tree_restored_after_lint: verifies the working tree is properly restored after lint Agent-Logs-Url: https://github.com/nf-core/tools/sessions/e3bf8950-f65b-4a3d-9b94-e3a275b359eb Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com> --- nf_core/pipelines/lint/container_configs.py | 45 ++++++----- .../pipelines/lint/test_container_configs.py | 81 ++++++++++++++++--- 2 files changed, 95 insertions(+), 31 deletions(-) diff --git a/nf_core/pipelines/lint/container_configs.py b/nf_core/pipelines/lint/container_configs.py index 106909dac6..0b4db54d19 100644 --- a/nf_core/pipelines/lint/container_configs.py +++ b/nf_core/pipelines/lint/container_configs.py @@ -1,8 +1,6 @@ import logging from pathlib import Path -import git - from nf_core.pipelines.containers_utils import ContainerConfigs log = logging.getLogger(__name__) @@ -13,8 +11,8 @@ def container_configs(self): Scans all ``meta.yml`` files under ``modules/`` that contain a ``containers`` key, reads the process name from the sibling ``main.nf``, and regenerates - the container configuration files in ``conf/``. Uses ``git diff`` to detect - changes. If not in ``--fix`` mode the working tree is restored to its + the container configuration files in ``conf/``. Uses direct content comparison + to detect changes. If not in ``--fix`` mode the working tree is restored to its original state afterwards. Can be skipped by adding the following to the ``.nf-core.yml`` file: @@ -31,7 +29,11 @@ def container_configs(self): could_fix = False conf_dir = Path(self.wf_path) / "conf" - repo = git.Repo(self.wf_path) + + # Snapshot the content of existing container config files before generation + snapshot: dict[str, str | None] = {} + for path in conf_dir.glob("containers_*"): + snapshot[path.name] = path.read_text() try: generated = ContainerConfigs(self.wf_path).generate_container_configs() @@ -41,20 +43,20 @@ def container_configs(self): log.debug(f"Generated {len(generated)} container config file(s): {', '.join(sorted(generated)) or 'none'}") - # Files modified in the working tree (tracked and changed by generation) - modified = { - Path(d.a_path).name - for d in repo.index.diff(None) - if d.a_path and Path(d.a_path).parent.name == "conf" and Path(d.a_path).name.startswith("containers_") - } - # Newly created files (generated but not previously tracked) - new = { - Path(f).name - for f in repo.untracked_files - if Path(f).parent.name == "conf" and Path(f).name.startswith("containers_") - } - # Already-correct files: generated, tracked, and unchanged - correct = generated - modified - new + # Compare generated content to pre-generation snapshot + modified: set[str] = set() + new: set[str] = set() + correct: set[str] = set() + + for name in generated: + new_content = (conf_dir / name).read_text() if (conf_dir / name).exists() else "" + old_content = snapshot.get(name) + if old_content is None: + new.add(name) + elif new_content != old_content: + modified.add(name) + else: + correct.add(name) log.debug(f"Container config status — correct: {len(correct)}, modified: {len(modified)}, new: {len(new)}") @@ -76,11 +78,10 @@ def container_configs(self): could_fix = True if not fixing: - # Restore working tree: reset modified tracked files and delete new untracked ones + # Restore working tree: write back original content for modified files, remove new files log.debug(f"Restoring working tree: resetting {len(modified)} modified, removing {len(new)} new file(s)") - repo_root = Path(repo.working_tree_dir).resolve() for name in modified: - repo.git.restore(str((conf_dir / name).resolve().relative_to(repo_root))) + (conf_dir / name).write_text(snapshot[name]) for name in new: (conf_dir / name).unlink(missing_ok=True) diff --git a/tests/pipelines/lint/test_container_configs.py b/tests/pipelines/lint/test_container_configs.py index 4610980466..4a964dd036 100644 --- a/tests/pipelines/lint/test_container_configs.py +++ b/tests/pipelines/lint/test_container_configs.py @@ -42,7 +42,7 @@ def _commit_container_cfg(self, name: str, content: str) -> Path: def test_container_configs_up_to_date(self): """Linting passes when generated configs match what is on disk.""" content = "process { withName: 'FASTQC' { container = 'docker.io/biocontainers/fastqc:0.12.1' } }\n" - self._commit_container_cfg("containers_docker_amd64.config", content) + self._write_container_cfg("containers_docker_amd64.config", content) def generate(cc_self): (cc_self.workflow_directory / "conf" / "containers_docker_amd64.config").write_text(content) @@ -62,7 +62,7 @@ def test_container_configs_out_of_date(self): """Linting fails when generated configs differ from what is on disk.""" old = "process { withName: 'FASTQC' { container = 'old_image' } }\n" new = "process { withName: 'FASTQC' { container = 'new_image' } }\n" - self._commit_container_cfg("containers_docker_amd64.config", old) + self._write_container_cfg("containers_docker_amd64.config", old) def generate(cc_self): (cc_self.workflow_directory / "conf" / "containers_docker_amd64.config").write_text(new) @@ -78,16 +78,16 @@ def generate(cc_self): assert any("out of date" in f for f in result["failed"]) def test_container_configs_missing_file(self): - """Linting fails when generate produces a config that has never been committed to the repo.""" + """Linting fails when generate produces a config that did not exist on disk before.""" content = "process { withName: 'FASTQC' { container = 'docker.io/biocontainers/fastqc:0.12.1' } }\n" - repo = git.Repo(self.new_pipeline) - repo.index.remove(["conf/containers_docker_amd64.config"], working_tree=True) - repo.index.commit("remove container config to simulate missing file") + # Ensure the file doesn't exist before generation + target = Path(self.new_pipeline) / "conf" / "containers_new_platform.config" + target.unlink(missing_ok=True) def generate(cc_self): - (cc_self.workflow_directory / "conf" / "containers_docker_amd64.config").write_text(content) - return {"containers_docker_amd64.config"} + (cc_self.workflow_directory / "conf" / "containers_new_platform.config").write_text(content) + return {"containers_new_platform.config"} with patch( "nf_core.pipelines.containers_utils.ContainerConfigs.generate_container_configs", @@ -102,7 +102,7 @@ def test_container_configs_fix_overwrites_files(self): """--fix overwrites out-of-date container config files and reports them as fixed.""" old = "process { withName: 'FASTQC' { container = 'old_image' } }\n" new = "process { withName: 'FASTQC' { container = 'new_image' } }\n" - cfg = self._commit_container_cfg("containers_docker_amd64.config", old) + cfg = self._write_container_cfg("containers_docker_amd64.config", old) def generate(cc_self): (cc_self.workflow_directory / "conf" / "containers_docker_amd64.config").write_text(new) @@ -132,3 +132,66 @@ def test_container_configs_user_warning_warns(self): assert len(result["failed"]) == 0 assert any("Nextflow version too low" in w for w in result["warned"]) + + def test_container_configs_uncommitted_but_correct_passes(self): + """Lint passes when configs were already regenerated (e.g. by modules update) but not committed. + + This is the key scenario that broke with the old git-diff approach: if modules update + regenerated configs without committing, repo.index.diff(None) would show them as + modified vs HEAD, causing false 'out of date' failures. The content-comparison approach + correctly identifies these as up-to-date since the on-disk content matches what generation + would produce. + """ + content = "process { withName: 'FASTQC' { container = 'docker.io/biocontainers/fastqc:0.12.1' } }\n" + # Write the file but do NOT commit it — simulates modules update having regenerated it + self._write_container_cfg("containers_docker_amd64.config", content) + + def generate(cc_self): + # Generation writes the exact same content + (cc_self.workflow_directory / "conf" / "containers_docker_amd64.config").write_text(content) + return {"containers_docker_amd64.config"} + + with patch( + "nf_core.pipelines.containers_utils.ContainerConfigs.generate_container_configs", + autospec=True, + side_effect=generate, + ): + result = self._lint() + + assert len(result["failed"]) == 0 + assert any("up to date" in p for p in result["passed"]) + + def test_container_configs_working_tree_restored_after_lint(self): + """After lint (without --fix), the working tree is restored to its pre-lint state. + + This verifies the cleanup/restore logic works correctly: modified files are written + back to their original content, and newly created files are removed. + """ + old = "process { withName: 'FASTQC' { container = 'old_image' } }\n" + new = "process { withName: 'FASTQC' { container = 'new_image' } }\n" + cfg_path = self._write_container_cfg("containers_docker_amd64.config", old) + + # Also ensure a file that will be "new" doesn't exist + new_path = Path(self.new_pipeline) / "conf" / "containers_new_platform.config" + new_path.unlink(missing_ok=True) + + def generate(cc_self): + # Modify existing file + (cc_self.workflow_directory / "conf" / "containers_docker_amd64.config").write_text(new) + # Create a new file + (cc_self.workflow_directory / "conf" / "containers_new_platform.config").write_text("new platform\n") + return {"containers_docker_amd64.config", "containers_new_platform.config"} + + with patch( + "nf_core.pipelines.containers_utils.ContainerConfigs.generate_container_configs", + autospec=True, + side_effect=generate, + ): + result = self._lint() + + # Lint should detect issues + assert len(result["failed"]) > 0 + + # But the working tree must be restored + assert cfg_path.read_text() == old, "Modified file should be restored to original content" + assert not new_path.exists(), "Newly created file should be removed after lint" From 7f82f8bc143563c8f60c3a1bdb48b3e14cd2c921 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 26 May 2026 15:10:23 +0000 Subject: [PATCH 3/6] fix: correct type annotation for snapshot dict (str, not str | None) Agent-Logs-Url: https://github.com/nf-core/tools/sessions/e3bf8950-f65b-4a3d-9b94-e3a275b359eb Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com> --- nf_core/pipelines/lint/container_configs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nf_core/pipelines/lint/container_configs.py b/nf_core/pipelines/lint/container_configs.py index 0b4db54d19..a4ee0add4c 100644 --- a/nf_core/pipelines/lint/container_configs.py +++ b/nf_core/pipelines/lint/container_configs.py @@ -31,7 +31,7 @@ def container_configs(self): conf_dir = Path(self.wf_path) / "conf" # Snapshot the content of existing container config files before generation - snapshot: dict[str, str | None] = {} + snapshot: dict[str, str] = {} for path in conf_dir.glob("containers_*"): snapshot[path.name] = path.read_text() From e11140f82b5a1a1f73104be6fe40f6f0f93ca5c5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 26 May 2026 15:37:58 +0000 Subject: [PATCH 4/6] test: add relative wf-path test for container_configs lint Agent-Logs-Url: https://github.com/nf-core/tools/sessions/f898f098-3f2a-4e3f-8bbd-2fe45e5fed00 Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com> --- .../pipelines/lint/test_container_configs.py | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/pipelines/lint/test_container_configs.py b/tests/pipelines/lint/test_container_configs.py index 4a964dd036..7496acae59 100644 --- a/tests/pipelines/lint/test_container_configs.py +++ b/tests/pipelines/lint/test_container_configs.py @@ -195,3 +195,36 @@ def generate(cc_self): # But the working tree must be restored assert cfg_path.read_text() == old, "Modified file should be restored to original content" assert not new_path.exists(), "Newly created file should be removed after lint" + + def test_container_configs_restore_with_relative_wf_path(self): + """Verify working tree restore works correctly even with relative workflow paths. + + This tests the fix for the relative path bug: when git restore was called with + a relative path constructed from a potentially non-repo-root wf_path, it would fail. + The new content-comparison approach doesn't depend on git operations, so it correctly + handles any wf_path regardless of whether it's absolute, relative, or repo-relative. + """ + old = "process { withName: 'FASTQC' { container = 'old_image' } }\n" + new = "process { withName: 'FASTQC' { container = 'new_image' } }\n" + cfg_path = self._write_container_cfg("containers_docker_amd64.config", old) + + def generate(cc_self): + (cc_self.workflow_directory / "conf" / "containers_docker_amd64.config").write_text(new) + return {"containers_docker_amd64.config"} + + with patch( + "nf_core.pipelines.containers_utils.ContainerConfigs.generate_container_configs", + autospec=True, + side_effect=generate, + ): + # Lint with a relative path to the pipeline + # (self.new_pipeline is an absolute path, but PipelineLint should handle both) + lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) + lint_obj._load() + result = lint_obj.container_configs() + + # Lint should detect the modification as "out of date" + assert any("out of date" in f for f in result["failed"]) + + # But the working tree must still be restored correctly + assert cfg_path.read_text() == old, "Modified file should be restored despite path resolution" From cfae19d51eab937438648f53987841ee06168ddb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 26 May 2026 16:08:31 +0000 Subject: [PATCH 5/6] test: properly test relative wf_path in container_configs Agent-Logs-Url: https://github.com/nf-core/tools/sessions/0c47257c-1ebc-4639-95c8-377dc2159244 Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com> --- .../pipelines/lint/test_container_configs.py | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/tests/pipelines/lint/test_container_configs.py b/tests/pipelines/lint/test_container_configs.py index 7496acae59..3c4ead5a29 100644 --- a/tests/pipelines/lint/test_container_configs.py +++ b/tests/pipelines/lint/test_container_configs.py @@ -197,13 +197,15 @@ def generate(cc_self): assert not new_path.exists(), "Newly created file should be removed after lint" def test_container_configs_restore_with_relative_wf_path(self): - """Verify working tree restore works correctly even with relative workflow paths. + """Verify working tree restore works correctly when wf_path is relative. - This tests the fix for the relative path bug: when git restore was called with - a relative path constructed from a potentially non-repo-root wf_path, it would fail. - The new content-comparison approach doesn't depend on git operations, so it correctly - handles any wf_path regardless of whether it's absolute, relative, or repo-relative. + This tests the fix for the relative path bug: the old implementation called + git restore with paths relative to wf_path, which would fail when wf_path + wasn't the repo root. The new implementation directly writes back snapshotted + content, so it works regardless of whether wf_path is absolute or relative. """ + import os + old = "process { withName: 'FASTQC' { container = 'old_image' } }\n" new = "process { withName: 'FASTQC' { container = 'new_image' } }\n" cfg_path = self._write_container_cfg("containers_docker_amd64.config", old) @@ -217,14 +219,20 @@ def generate(cc_self): autospec=True, side_effect=generate, ): - # Lint with a relative path to the pipeline - # (self.new_pipeline is an absolute path, but PipelineLint should handle both) - lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) - lint_obj._load() - result = lint_obj.container_configs() + # Save original cwd and change to parent directory + original_cwd = os.getcwd() + try: + os.chdir(Path(self.new_pipeline).parent) + # Use relative path (just the basename) instead of absolute path + relative_pipeline_path = Path(self.new_pipeline).name + lint_obj = nf_core.pipelines.lint.PipelineLint(str(relative_pipeline_path)) + lint_obj._load() + result = lint_obj.container_configs() + finally: + os.chdir(original_cwd) # Lint should detect the modification as "out of date" assert any("out of date" in f for f in result["failed"]) - # But the working tree must still be restored correctly - assert cfg_path.read_text() == old, "Modified file should be restored despite path resolution" + # But the working tree must still be restored correctly even with relative path + assert cfg_path.read_text() == old, "Modified file should be restored even when using relative wf_path" From 2eebf84d8285abfd23c6d299651049b64364c0a0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 26 May 2026 16:34:04 +0000 Subject: [PATCH 6/6] fix: use Path.cwd() instead of os.getcwd() for lint compliance Agent-Logs-Url: https://github.com/nf-core/tools/sessions/660f217b-3360-4cc5-8f61-35616aadd65e Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com> --- tests/pipelines/lint/test_container_configs.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pipelines/lint/test_container_configs.py b/tests/pipelines/lint/test_container_configs.py index 3c4ead5a29..2c416f344e 100644 --- a/tests/pipelines/lint/test_container_configs.py +++ b/tests/pipelines/lint/test_container_configs.py @@ -205,7 +205,7 @@ def test_container_configs_restore_with_relative_wf_path(self): content, so it works regardless of whether wf_path is absolute or relative. """ import os - + old = "process { withName: 'FASTQC' { container = 'old_image' } }\n" new = "process { withName: 'FASTQC' { container = 'new_image' } }\n" cfg_path = self._write_container_cfg("containers_docker_amd64.config", old) @@ -220,7 +220,7 @@ def generate(cc_self): side_effect=generate, ): # Save original cwd and change to parent directory - original_cwd = os.getcwd() + original_cwd = Path.cwd() try: os.chdir(Path(self.new_pipeline).parent) # Use relative path (just the basename) instead of absolute path