Skip to content

Fix false-positive RequiredFilesExist lint in shed_lint (#1646)#1647

Merged
jmchilton merged 1 commit into
galaxyproject:masterfrom
mvdbeek:fix-shed-lint-required-files-symlink
May 26, 2026
Merged

Fix false-positive RequiredFilesExist lint in shed_lint (#1646)#1647
jmchilton merged 1 commit into
galaxyproject:masterfrom
mvdbeek:fix-shed-lint-required-files-symlink

Conversation

@mvdbeek

@mvdbeek mvdbeek commented May 25, 2026

Copy link
Copy Markdown
Member

Summary

Galaxy 26.0 added a RequiredFilesExist linter that walks the tool directory via galaxy.util.path.safe_walk to verify every <required_files> include resolves to a real file. safe_walk silently drops symlinks whose realpath points outside the walked prefix (a deliberate symlink-escape guard). shed_lint had been symlinking files into its temp "realized" repository, so every required-file check now returns empty and falsely reports the file as missing.

Fix: in RealizedFile.realize_to, copy files with shutil.copy2 instead of os.symlink. The realized directory is meant to mirror the eventual shed tarball, and tarfile.add dereferences symlinks anyway, so this brings the on-disk representation in line with both the tarball and the install dir.

Fixes #1646.

Why this rather than relaxing safe_walk?

The obvious alternative was to make planemo or the linter walk with symlinks followed (or pass an allowlist that whitelists the original source tree). I deliberately avoided that:

  • planemo runs Galaxy linters and other Galaxy code inline; we want the same safety invariants Galaxy enforces at install/runtime, not a planemo-only relaxed variant. safe_walk's symlink-escape guard exists precisely so a tool directory cannot reach files outside itself, and weakening that here would diverge planemo's view of a tool from Galaxy's.
  • Skipping RequiredFilesExist (or any other path-sensitive linter) would hide real bugs — the whole point of the new linter is to catch missing referenced files before they ship.
  • Reloading tool_source from the original source path (instead of the realized one) would bypass the implicit-ignore filtering that the realized repo is specifically there to enforce, and would duplicate tool-source resolution.

Copying instead of symlinking keeps Galaxy's safety model intact, keeps the linter active, and accurately reflects what will land in the tarball.

Test plan

  • New regression test tests/test_shed_lint.py::ShedLintTestCase::test_tool_linting_required_files + fixture tests/data/repos/single_tool_required_files/ (tool XML with <required_files><include path=\"helper.py\"/></required_files> next to the helper).
  • Test passes with the fix; fails on master with the exact issue-1646 message (ERROR (RequiredFilesExist): Required file [helper.py] does not exist).
  • Full tests/test_shed_lint.py, tests/test_shed_upload.py, tests/test_shed_create.py runs clean (the two *_from_git failures I saw locally are pre-existing GPG-signing env issues, identical on master).

galaxy-tool-util 26.0 added a `RequiredFilesExist` linter that walks
the tool directory via `safe_walk` to verify every `<required_files>`
include resolves to a real file. `safe_walk` silently drops symlinks
whose realpath points outside the walked directory, so every file that
`shed_lint` had symlinked into its temp "realized" repository was
filtered out and reported as missing (galaxyproject#1646).

The realized directory is meant to mirror the eventual shed tarball,
and `tarfile.add` dereferences symlinks anyway, so copying is the
semantically correct operation here.
@mvdbeek mvdbeek requested review from bgruening and jmchilton May 26, 2026 10:03
@mvdbeek

mvdbeek commented May 26, 2026

Copy link
Copy Markdown
Member Author

Alternative to galaxyproject/galaxy#22759

@jmchilton jmchilton enabled auto-merge May 26, 2026 15:52
@jmchilton jmchilton merged commit 40987e4 into galaxyproject:master May 26, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ERROR (RequiredFilesExist): Required file [create_yaml.py] does not exist

2 participants