chore: add fish env loader function and update rules submodule - #247
Conversation
- Introduce __hm_load_env_file.fish for Home Manager env loading - Modify fish/default.nix and .gitignore accordingly - Refresh rules submodule to latest commit bf5e1f9
WalkthroughAdds .env files to .gitignore. Introduces a Fish function (__hm_load_env_file) to load environment variables from .env files and invokes it during Fish interactive shell initialization. Updates a submodule pointer in rules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Fish as Fish (interactive)
participant Loader as __hm_load_env_file
participant FS as Filesystem
User->>Fish: Start interactive shell
Fish->>Loader: Call to load env
activate Loader
Loader->>FS: Check DOTFILES_ENV_FILE<br/>else probe $HOME/dotfiles/.env, $HOME/.env
FS-->>Loader: Return first existing file or none
alt Env file found
Loader->>FS: Read file line-by-line
loop For each parsed KEY=VALUE
Loader->>Fish: set -gx KEY VALUE
end
else No env file
Note over Loader: No-op
end
deactivate Loader
Fish->>Fish: Continue init (e.g., greeting settings)
Fish-->>User: Ready prompt
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the development environment's setup by automating the loading of environment variables from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| set trimmed (string replace -r '^export\\s+' '' -- $trimmed) | ||
| set -l parts (string split -m2 '=' -- $trimmed) | ||
| if test (count $parts) -lt 2 | ||
| continue | ||
| end | ||
|
|
||
| set -l key (string trim $parts[1]) | ||
| set -l value (string trim $parts[2]) | ||
|
|
There was a problem hiding this comment.
[P1] Strip export prefix before parsing keys
The regex used to drop an optional export prefix is '^export\\s+', which matches a literal \s string instead of whitespace because the backslash is escaped twice. Lines like export FOO=bar therefore leave trimmed as export FOO=bar, and the subsequent set -gx assigns a variable named export with values FOO and bar rather than exporting FOO. .env files that include the export keyword will silently fail to load the intended variables. Use '^export\s+' or '^export\s+' with a single backslash so the whitespace is removed before splitting.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)home-manager/programs/fish/default.nix(1 hunks)home-manager/programs/fish/functions/__hm_load_env_file.fish(1 hunks)rules(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
home-manager/**
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Keep home-manager configurations under home-manager/
Files:
home-manager/programs/fish/functions/__hm_load_env_file.fishhome-manager/programs/fish/default.nix
**/*.nix
📄 CodeRabbit inference engine (CLAUDE.md)
Format all Nix files with nixfmt
**/*.nix: Nix: Use 2 spaces for indentation
Nix: Keep line length under 100 characters
Nix: Sort attribute sets alphabetically
Nix: Use consistent spacing around operators
Nix: Format lists and sets consistentlyFollow the Nix expression language style guide
Files:
home-manager/programs/fish/default.nix
**/default.nix
📄 CodeRabbit inference engine (CLAUDE.md)
Use default.nix files for module exports
Files:
home-manager/programs/fish/default.nix
home-manager/programs/*/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
home-manager/programs/*/default.nix: Program configurations must be located in home-manager/programs// with configuration in default.nix
Program configurations should prefer Home Manager’s built-in modules when available
Program configurations should include all necessary dependencies
Program configurations should follow the provided template (programs..enable, package, and settings attrset)
Files:
home-manager/programs/fish/default.nix
home-manager/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Use proper indentation and formatting in Nix files
Files:
home-manager/programs/fish/default.nix
home-manager/programs/**/default.nix
📄 CodeRabbit inference engine (.cursor/rules/nix.mdc)
Program configurations in home-manager/programs should be organized by program name (one directory per program with a default.nix)
Files:
home-manager/programs/fish/default.nix
home-manager/programs/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/nix.mdc)
home-manager/programs/**/*.nix: Program configurations should include all necessary dependencies
Use home.packages for package installations in program configurations
Use programs. options provided by Home Manager when available
Files:
home-manager/programs/fish/default.nix
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: nix-linux
- GitHub Check: nix-nixos
- GitHub Check: nix-darwin
- GitHub Check: docker-build-push (linux/amd64, -amd64, amd64)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
🔇 Additional comments (1)
home-manager/programs/fish/default.nix (1)
16-16: Env loader hook looks good.Bringing
__hm_load_env_fileintointeractiveShellInitensures Fish sessions hydrate the environment before other setup runs. Nice touch.
| set trimmed (string replace -r '^export\\s+' '' -- $trimmed) | ||
| set -l parts (string split -m2 '=' -- $trimmed) | ||
| if test (count $parts) -lt 2 | ||
| continue | ||
| end |
There was a problem hiding this comment.
Fix regex stripping export.
^export\\s+ matches the literal string export\s instead of whitespace, so .env lines that start with export keep the prefix and later try to set a variable named export. Fixing the regex to ^export\s+ correctly removes the prefix.
Apply this diff:
- set trimmed (string replace -r '^export\\s+' '' -- $trimmed)
+ set trimmed (string replace -r '^export\s+' '' -- $trimmed)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set trimmed (string replace -r '^export\\s+' '' -- $trimmed) | |
| set -l parts (string split -m2 '=' -- $trimmed) | |
| if test (count $parts) -lt 2 | |
| continue | |
| end | |
| set trimmed (string replace -r '^export\s+' '' -- $trimmed) | |
| set -l parts (string split -m2 '=' -- $trimmed) | |
| if test (count $parts) -lt 2 | |
| continue | |
| end |
🤖 Prompt for AI Agents
home-manager/programs/fish/functions/__hm_load_env_file.fish around lines 29-33:
the regex used in string replace is escaping the backslash so it matches a
literal "\s" rather than whitespace; change the regex from '^export\\s+' to
'^export\s+' so the pattern correctly strips the "export" prefix followed by any
whitespace from the start of the line.
| set -gx $key $value | ||
| end < $env_file |
There was a problem hiding this comment.
Preserve whitespace in values when exporting.
Without quoting, any spaces in the value become separate list elements (e.g., FOO="hello world" becomes two words). Quoting the value and stopping option parsing keeps the original string intact.
Apply this diff:
- set -gx $key $value
+ set -gx -- $key "$value"🤖 Prompt for AI Agents
In home-manager/programs/fish/functions/__hm_load_env_file.fish around lines
44-45, the export uses set -gx $key $value which can split values containing
spaces; change it to use a stop-opts marker and quoted value: set -gx -- $key
"$value" so option parsing is disabled and the original string (including
whitespace) is preserved when exporting.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new fish function to load environment variables from .env files, which is a great addition for shell integration. I've reviewed the implementation and found a few critical and high-severity issues in the parsing logic that would cause the script to fail or behave incorrectly. My review includes suggestions to fix these issues to ensure the function is robust and works as expected.
| end | ||
|
|
||
| set trimmed (string replace -r '^export\\s+' '' -- $trimmed) | ||
| set -l parts (string split -m2 '=' -- $trimmed) |
There was a problem hiding this comment.
The option -m2 for string split is invalid syntax in fish shell and will cause the script to fail. To split the line only on the first occurrence of =, you should use the option -m 1. This will correctly separate the key from the value, which is crucial for parsing lines where the value itself might contain an equals sign (e.g., DATABASE_URL=postgres://...).
set -l parts (string split -m 1 '=' -- $trimmed)
| continue | ||
| end | ||
|
|
||
| set trimmed (string replace -r '^export\\s+' '' -- $trimmed) |
There was a problem hiding this comment.
The regular expression to remove the export prefix is incorrect. The double backslash in \\s+ will be treated as a literal backslash followed by s, so it won't match whitespace. To match one or more whitespace characters in a PCRE regex used by fish, you should use \s+.
set trimmed (string replace -r '^export\s+' '' -- $trimmed)
| if string match -qr "^'.*'\z" -- $value | ||
| set value (string trim --chars "'" -- $value) | ||
| else if string match -qr '^".*"$' -- $value | ||
| set value (string trim --chars '"' -- $value) | ||
| end |
There was a problem hiding this comment.
Using string trim --chars to remove quotes can be unreliable. For example, a value like ''foo'' would be incorrectly trimmed to foo instead of the intended 'foo'. A more robust approach is to use string sub --start=2 --end=-2, which removes only the first and last characters.
Additionally, the regex for matching double-quoted strings uses $ to match the end of the line. It's better practice to use \z to match the end of the string, which is more accurate if values span multiple lines.
if string match -qr "^'.*'\z" -- $value
set value (string sub --start=2 --end=-2 -- $value)
else if string match -qr '^\".*\"\z' -- $value
set value (string sub --start=2 --end=-2 -- $value)
end
1 participant
Summary
Summary by CodeRabbit
New Features
Chores