Skip to content

fix: escape installation_location in micromamba plugin (#3286) - #3287

Open
anushkagupta200615-jpg wants to merge 2 commits into
Netflix:masterfrom
anushkagupta200615-jpg:fix-micromamba-escaping
Open

fix: escape installation_location in micromamba plugin (#3286)#3287
anushkagupta200615-jpg wants to merge 2 commits into
Netflix:masterfrom
anushkagupta200615-jpg:fix-micromamba-escaping

Conversation

@anushkagupta200615-jpg

Copy link
Copy Markdown

This PR resolves issue #3286 by safely escaping the installation_location in the PyPI/Conda micromamba setup script.

Previously, if a user's installation directory path contained spaces (e.g. /Users/My Name/), the underlying tar subprocess executed via shell=True would incorrectly parse the path as multiple arguments, causing a fatal error and breaking the installation. By applying Python's standard shlex.quote(), we guarantee the path is passed safely to the shell, fixing the issue without needing to restructure the underlying subprocess piping.

@greptile-apps

greptile-apps Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates the micromamba installation command. The main changes are:

  • Adds shlex for shell-safe quoting.
  • Quotes the micromamba installation directory in the tar command.
  • Adjusts the tar extraction flags in the installer path.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
metaflow/plugins/pypi/micromamba.py Updates the micromamba install command to quote the destination path used by tar.

Reviews (2): Last reviewed commit: "Remove redundant j flag in tar extractio..." | Re-trigger Greptile

@allenjamesvinoy

Copy link
Copy Markdown

From my understanding, the 'j' switch in tar -xvj is redundant in this extraction use case. What do you think about removing it? @anushkagupta200615-jpg

@anushkagupta200615-jpg

Copy link
Copy Markdown
Author

@allenjamesvinoy Thanks for catching that! You're absolutely right—the j switch isn't strictly necessary since tar can automatically detect the compression format. I've just pushed a commit removing it . Thanks for the suggestion!

@Shriprasad-P Shriprasad-P left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified that the shlex.quote(installation_location) change correctly fixes the original path parsing issue on the supported POSIX platforms, including paths containing spaces and shell metacharacters.

There is one blocking regression in the current command, however.

The micromamba archive is bzip2-compressed and is streamed directly into tar via:

curl ... | tar ...

This PR changes tar -xvj to tar -xv. GNU tar does not automatically detect compression when reading a compressed archive from a pipe; it requires the corresponding decompression option. For this bzip2 stream that means -j.

As a result, the new command can work with BSD/macOS tar while failing on supported Linux systems using GNU tar before bin/micromamba is extracted.

Please keep the quoting fix but restore explicit bzip2 decompression, e.g. retain -j, or change the implementation to download the archive to a named file before extraction.

A regression test covering the streamed archive path on Linux would also be valuable.

The shlex.quote() portion of the change itself looks correct.

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.

3 participants