Vendor dotenvy .env parser to remove git dependency - #1351
Merged
Conversation
The `dotenvy` crate was pulled as a git dependency pinned to an unreleased upstream commit (the EnvLoader/EnvMap/EnvSequence API never shipped to crates.io). Cargo clones it from GitHub on every cache-cold build — the release matrix runs uncached on purpose — which exhausts GitHub's git egress and breaks compilation. Only two call sites used it, both via `EnvSequence::InputOnly`: parse a .env file into a map without reading or mutating the process env. Vendor just that path into utils::dotenv (EnvMap + parser + line iterator), dropping the builder, env-merging sequences, unsafe env-modifying loaders, CLI, and macros. Upstream parser/substitution tests are ported verbatim to lock in behavioral parity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HEJGDhrXJQx1mvtdTwqUz7
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI replaces ChangesCLI dotenv loader migration
Sequence Diagram(s)sequenceDiagram
participant EnvState_var as "EnvState::var"
participant dotenv_from_path as "dotenv::from_path"
participant Iter_load as "Iter::load"
participant Lines_next as "Lines::next"
participant LineParser as "LineParser"
participant EnvMap as "EnvMap"
EnvState_var->>dotenv_from_path: load `.env` from project_root
dotenv_from_path->>Iter_load: stream parsed lines
Iter_load->>Lines_next: read logical line
Lines_next->>LineParser: parse key and value
LineParser->>EnvMap: insert parsed entry
EnvState_var->>EnvMap: look up requested key
EnvMap-->>EnvState_var: return value or NotPresent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli/src/docker_env.rs (1)
286-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
.ok()silently swallows.envparse errors.
from_path(...).ok()discardsdotenv::Error::LineParsethe same as a missing file, so a malformed.envsilently falls back to the hardcoded defaults (default DB credentials, ports, etc.) with no signal to the user. This diverges fromEnvState::varinsystem_config.rs, which prints a warning for non-IO errors. Consider matching that behavior and only ignoringError::Io.♻️ Surface non-IO errors instead of swallowing them
- let dotenv = dotenv::from_path(project_root.join(".env")).ok(); + let dotenv = match dotenv::from_path(project_root.join(".env")) { + Ok(map) => Some(map), + Err(dotenv::Error::Io(_, _)) => None, + Err(err) => { + println!("Warning: Failed loading .env file with unexpected error: {err}"); + None + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/docker_env.rs` at line 286, The dotenv load in docker_env::from_path is swallowing parse failures by converting all errors with .ok(), which makes malformed .env files look like missing files. Update the dotenv handling to distinguish dotenv::Error::Io from other error kinds, ignore only the I/O case, and surface non-IO parse errors with a warning similar to EnvState::var in system_config.rs. Keep the fix localized around the existing dotenv::from_path(project_root.join(".env")) call so malformed .env inputs are visible instead of silently falling back to defaults.packages/cli/src/utils/dotenv.rs (1)
582-582: 📐 Maintainability & Code Quality | 🔵 TrivialSwitch the test helpers to
tempfile
packages/cli/Cargo.tomlstill depends on deprecatedtempdir, and this file is one of several test helpers usingTempDir::new(...). Migrate these tests totempfile::tempdir()and droptempdirfrom dev-dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/dotenv.rs` at line 582, The test helper in dotenv.rs still uses the deprecated tempdir::TempDir::new pattern, so migrate it to tempfile::tempdir() and update any related test setup in the same helper block to use the new API. Make sure the test code references the tempfile crate consistently, and then remove tempdir from the dev-dependencies so the CLI tests no longer depend on the deprecated crate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/docker_env.rs`:
- Line 286: The dotenv load in docker_env::from_path is swallowing parse
failures by converting all errors with .ok(), which makes malformed .env files
look like missing files. Update the dotenv handling to distinguish
dotenv::Error::Io from other error kinds, ignore only the I/O case, and surface
non-IO parse errors with a warning similar to EnvState::var in system_config.rs.
Keep the fix localized around the existing
dotenv::from_path(project_root.join(".env")) call so malformed .env inputs are
visible instead of silently falling back to defaults.
In `@packages/cli/src/utils/dotenv.rs`:
- Line 582: The test helper in dotenv.rs still uses the deprecated
tempdir::TempDir::new pattern, so migrate it to tempfile::tempdir() and update
any related test setup in the same helper block to use the new API. Make sure
the test code references the tempfile crate consistently, and then remove
tempdir from the dev-dependencies so the CLI tests no longer depend on the
deprecated crate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a2e55a7-6670-47c4-a80e-603c50fbb472
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/cli/Cargo.tomlpackages/cli/src/config_parsing/system_config.rspackages/cli/src/docker_env.rspackages/cli/src/utils/dotenv.rspackages/cli/src/utils/mod.rs
💤 Files with no reviewable changes (1)
- packages/cli/Cargo.toml
`.ok()` discarded LineParse errors the same as a missing file, silently falling back to default DB credentials/ports on a malformed .env. Match EnvState::var: ignore only Io errors, warn on the rest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HEJGDhrXJQx1mvtdTwqUz7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vendors a minimal
.envfile parser from the unreleased dotenvy 0.16 rewrite to eliminate the git dependency ongithub.com/enviodev/dotenvy. The vendored implementation includes only the parsing functionality needed by the CLI, dropping the builder, environment-merging sequences, unsafe env-modifying loaders, and CLI/macros.Key changes:
packages/cli/src/utils/dotenv.rswith a complete.envparser implementation ported from dotenvy 0.16.envfiles into anEnvMap(HashMap-backed) without modifying process environment$VAR/${VAR}) against process environment and earlier file entriespackages/cli/src/config_parsing/system_config.rsto use vendored parser instead of dotenvy cratepackages/cli/src/docker_env.rsto use vendored parser instead of dotenvy cratedotenvygit dependency frompackages/cli/Cargo.tomldotenvmodule frompackages/cli/src/utils/mod.rsThe vendored code is MIT-licensed (© the dotenvy authors) and maintains API compatibility with the previous dotenvy usage while reducing build-time dependency resolution overhead.
https://claude.ai/code/session_01HEJGDhrXJQx1mvtdTwqUz7
Summary by CodeRabbit
.envfiles..envparsing supports comments, quoted strings, escaping, and$VAR/${VAR}variable substitution..envfiles (e.g., UTF-8 BOMs and edge-case formats) by warning and safely continuing as if no.envexists..envvalues.