Skip to content

antidote / oh-my-zsh: improve escaping of shell arguments#9042

Merged
khaneliman merged 2 commits intonix-community:masterfrom
Zocker1999NET:fix-shell-escaping
Apr 12, 2026
Merged

antidote / oh-my-zsh: improve escaping of shell arguments#9042
khaneliman merged 2 commits intonix-community:masterfrom
Zocker1999NET:fix-shell-escaping

Conversation

@Zocker1999NET
Copy link
Copy Markdown
Contributor

Description

This pull request improves the handling of shell arguments in the Antidote and Oh My Zsh Home Manager modules by ensuring all relevant file paths and arguments are properly shell-escaped.

Checklist

  • Change is backwards compatible.

  • Code formatted with nix fmt or
    nix-shell -p treefmt nixfmt deadnix keep-sorted nixf-diagnose --run treefmt.

  • Code tested through nix run .#tests -- test-all or
    nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.
    • Generate a news entry. See News
    • Basic tests added. See Tests
  • If this PR adds an exciting new feature or contains a breaking change.

    • Generate a news entry. See News

Copy link
Copy Markdown
Collaborator

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

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

Some of these don't look like the placement is correct. It appears it would create escaped strings in the middle of stuff. Would be nice to have some tests around some of the generated values.

@Zocker1999NET
Copy link
Copy Markdown
Contributor Author

It appears it would create escaped strings in the middle of stuff.

Do you mean stuff like ZSH=${escapeShellArg cfg.oh-my-zsh.package}/share/oh-my-zsh; which could be converted to ZSH="/nix/store/...-oh-my-zsh"/share/oh-my-zsh? That is intentional because most shells (including zsh) support that.

Or do you mean something else?

@khaneliman
Copy link
Copy Markdown
Collaborator

Yeah, just feels wrong to not escape the full path / variable and only part of it.

@rycee
Copy link
Copy Markdown
Member

rycee commented Apr 11, 2026

I'm inclined to say that it looks fine to me and maybe it should be considered idiomatic since it simplifies the code a bit.

@khaneliman khaneliman merged commit 89b2d57 into nix-community:master Apr 12, 2026
7 checks passed
@ellingtonjp
Copy link
Copy Markdown

This change breaks oh-my-zsh plugins. For example, this was previously working, but now breaks for me:

        oh-my-zsh = {
          enable = true;
          plugins = [
            "git"
            "vi-mode"
          ];
        };

With the following:

error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:37:12:
           36|
           37|   strict = derivationStrict drvAttrs;
             |            ^
           38|

       … while evaluating derivation 'darwin-system-26.05.06648f4'
         whose name attribute is located at /nix/store/f9zw9nsp96n5zlfr2cwl8msrd1mfbzm1-source/pkgs/stdenv/generic/make-derivation.nix:541:11

       … while evaluating attribute 'activationScript' of derivation 'darwin-system-26.05.06648f4'
         at /nix/store/jz2wwz18fnladl35z2bc4406fl9w5ifw-source/modules/system/default.nix:89:7:
           88|
           89|       activationScript = cfg.activationScripts.script.text;
             |       ^
           90|

       … while evaluating the option `system.activationScripts.script.text':

       … while evaluating definitions from `/nix/store/jz2wwz18fnladl35z2bc4406fl9w5ifw-source/modules/system/activation-scripts.nix':

       … while evaluating the option `system.activationScripts.postActivation.text':

       … while evaluating definitions from `/nix/store/hd2d4d2bw34ay6js0w4yp7vz8q2b6b16-source/nix-darwin':

       … while evaluating the option `home-manager.users.jon.home.file."./.zshrc".source':

       … while evaluating definitions from `/nix/store/hd2d4d2bw34ay6js0w4yp7vz8q2b6b16-source/modules/files.nix':

       … while evaluating the option `home-manager.users.jon.home.file."./.zshrc".text':

       … while evaluating definitions from `/nix/store/hd2d4d2bw34ay6js0w4yp7vz8q2b6b16-source/modules/programs/zsh':

       … while evaluating the option `home-manager.users.jon.programs.zsh.initContent':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: expected a list but found a string: "git vi-mode"
       ```

@Zocker1999NET
Copy link
Copy Markdown
Contributor Author

@ellingtonjp Oh, sorry. Thanks for giving the error message, I tracked the issue down & will immediately create a 2nd PR

ooojustin added a commit to ooojustin/home-manager that referenced this pull request Apr 13, 2026
escapeShellArg single-quotes values, which prevents shell variable
expansion. Setting custom = "$HOME/.config/shell" generates
ZSH_CUSTOM='$HOME/.config/shell' instead of the expected
ZSH_CUSTOM="$HOME/.config/shell", breaking custom themes and plugins.

Use double-quoted assignments instead. This allows $HOME and other
shell variables to expand while still handling paths with spaces.

Regression introduced in PR nix-community#9042.
@ooojustin
Copy link
Copy Markdown

This change also stopped env vars like $HOME from working in oh-my-zsh.custom / theme paths, I opened a follow-up in #9095

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants