Auto-parse .env and .env.mci files from project root and ./mci directory using python-dotenv#31
Conversation
- Add dotenv parsing utility with support for standard .env format - Implement automatic .env file discovery from project root and ./mci directory - Update MCIConfig and MCIClientWrapper to auto-load .env files - Add comprehensive tests for dotenv parsing and loading - Update README.md with .env auto-loading documentation - Maintain precedence order: ./mci/.env < root .env < system env < explicit env_vars Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
- Create comprehensive manual test for dotenv auto-loading - Test all precedence scenarios - Verify integration with CLI commands - All 352 automated tests passing - Linting passed successfully Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements automatic .env file loading for the MCI Python adapter, enabling seamless environment variable configuration without manual setup. When loading MCI schemas, the system now automatically discovers and parses .env files from both the project root and ./mci directory, merging them with defined precedence rules.
Key Changes:
- New dotenv parser: Implements standard .env format support (KEY=VALUE, comments, export keyword, quoted values) in
src/mci/utils/dotenv.py - Auto-loading integration: Modified
MCIConfigandMCIClientWrapperto automatically load .env files on initialization with configurableauto_load_dotenvparameter - Comprehensive testing: Added 23 new tests covering parsing, merging, precedence, and integration scenarios
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/mci/utils/dotenv.py |
New module providing dotenv file parsing and merging utilities with precedence support |
src/mci/core/config.py |
Enhanced MCIConfig.load() and validate_schema() to auto-load .env files with configurable behavior |
src/mci/core/mci_client.py |
Extended MCIClientWrapper initialization to support auto-loading .env files from project directories |
tests/unit/utils/test_dotenv.py |
Comprehensive unit tests for dotenv parsing, including edge cases like comments, quotes, and malformed lines |
tests/unit/core/test_config.py |
Integration tests verifying .env auto-loading in config loading and precedence rules |
tests/test_dotenv_loading.py |
Feature tests for end-to-end .env file loading behavior |
testsManual/test_dotenv_autoload.py |
Manual tests for visual verification of .env auto-loading with rich console output |
README.md |
Added extensive documentation on automatic .env file loading, precedence rules, and MCP client configuration |
Comments suppressed due to low confidence (2)
testsManual/test_dotenv_autoload.py:12
- Import of 'os' is not used.
import os
testsManual/test_dotenv_autoload.py:13
- Import of 'shutil' is not used.
import shutil
testsManual/test_dotenv_autoload.py
Outdated
| import os | ||
| import shutil |
There was a problem hiding this comment.
Unused imports detected. The os and shutil modules are imported but never used in this manual test file. These should be removed to keep the code clean.
| import os | |
| import shutil |
There was a problem hiding this comment.
Fixed in commit ebda5c0. Removed unused imports os and shutil.
src/mci/utils/dotenv.py
Outdated
| # If line doesn't match format, silently skip it | ||
| # (could be malformed or a directive we don't support) | ||
|
|
||
| except Exception: |
There was a problem hiding this comment.
Overly broad exception handler silently suppresses all errors. The bare except Exception: clause on line 81 catches all exceptions during file parsing, including unexpected errors like UnicodeDecodeError, MemoryError, or programming errors. This makes debugging difficult since all failures are silently ignored.
Consider catching only specific exceptions related to file reading (e.g., OSError, IOError) and letting other exceptions propagate, or at minimum log the error for debugging purposes.
| except Exception: | |
| except (OSError, IOError): |
There was a problem hiding this comment.
Fixed in commit ebda5c0. Updated exception handling to catch specific exceptions: OSError, IOError, and UnicodeDecodeError instead of bare Exception.
src/mci/utils/dotenv.py
Outdated
| if (value.startswith('"') and value.endswith('"')) or ( | ||
| value.startswith("'") and value.endswith("'") | ||
| ): | ||
| value = value[1:-1] |
There was a problem hiding this comment.
The quote removal logic has a potential bug when handling mismatched quotes. If a value starts with a double quote but ends with a single quote (e.g., KEY="value'), the current implementation will incorrectly strip both characters, resulting in value instead of preserving the original "value'.
Consider adding validation to ensure matching quote types:
if len(value) >= 2:
if value.startswith('"') and value.endswith('"'):
value = value[1:-1]
elif value.startswith("'") and value.endswith("'"):
value = value[1:-1]| if (value.startswith('"') and value.endswith('"')) or ( | |
| value.startswith("'") and value.endswith("'") | |
| ): | |
| value = value[1:-1] | |
| if value.startswith('"') and value.endswith('"'): | |
| value = value[1:-1] | |
| elif value.startswith("'") and value.endswith("'"): | |
| value = value[1:-1] |
There was a problem hiding this comment.
@copilot Why do we parse it manually? Isn't there any package as community standard in python for parsing .env files?
There was a problem hiding this comment.
Great question! I initially chose to implement a simple parser to:
- Avoid adding external dependencies (keep the package lightweight)
- Only support the minimal dotenv features needed for this use case
- Maintain full control over behavior (silent failures, specific error handling)
However, if you prefer using a standard library like python-dotenv, we can switch to that. It's the most popular dotenv library in Python with excellent community support. Would you like me to refactor to use python-dotenv instead?
For now, I've fixed the quote handling bug in commit ebda5c0 to properly validate matching quote types.
testsManual/test_dotenv_autoload.py
Outdated
|
|
||
| # Load config | ||
| config = MCIConfig() | ||
| client = config.load(str(schema_file)) |
There was a problem hiding this comment.
Variable client is not used.
| client = config.load(str(schema_file)) | |
| config.load(str(schema_file)) |
There was a problem hiding this comment.
Fixed in commit ebda5c0. Removed unused client variable.
testsManual/test_dotenv_autoload.py
Outdated
|
|
||
| # Load config | ||
| config = MCIConfig() | ||
| client = config.load(str(schema_file)) |
There was a problem hiding this comment.
Variable client is not used.
testsManual/test_dotenv_autoload.py
Outdated
| # Load config with explicit env vars | ||
| config = MCIConfig() | ||
| explicit_env = {"OVERRIDE_VAR": "from-explicit", "EXPLICIT_ONLY": "explicit-value"} | ||
| client = config.load(str(schema_file), env_vars=explicit_env) |
There was a problem hiding this comment.
Variable client is not used.
| client = config.load(str(schema_file), env_vars=explicit_env) | |
| config.load(str(schema_file), env_vars=explicit_env) |
tests/unit/utils/test_dotenv.py
Outdated
| import tempfile | ||
| from pathlib import Path | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
There was a problem hiding this comment.
Fixed in commit ebda5c0. Removed unused pytest import.
MaestroError
left a comment
There was a problem hiding this comment.
@copilot Please check comments and make fixes.
Also, let's add the .env.mci file parsing and make it a priority. So, at first, in both locations let's check the .env.mci file and if it doesn't exists, than check for .env file
Update documentation with change about ".env.mci" priority
src/mci/utils/dotenv.py
Outdated
| # If line doesn't match format, silently skip it | ||
| # (could be malformed or a directive we don't support) | ||
|
|
||
| except Exception: |
tests/unit/utils/test_dotenv.py
Outdated
| import tempfile | ||
| from pathlib import Path | ||
|
|
||
| import pytest |
testsManual/test_dotenv_autoload.py
Outdated
|
|
||
| # Load config | ||
| config = MCIConfig() | ||
| client = config.load(str(schema_file)) |
src/mci/utils/dotenv.py
Outdated
| if (value.startswith('"') and value.endswith('"')) or ( | ||
| value.startswith("'") and value.endswith("'") | ||
| ): | ||
| value = value[1:-1] |
There was a problem hiding this comment.
@copilot Why do we parse it manually? Isn't there any package as community standard in python for parsing .env files?
- Remove unused imports (os, shutil, pytest) - Fix exception handling to catch specific exceptions (OSError, IOError, UnicodeDecodeError) - Fix quote removal logic to ensure matching quote types - Remove unused client variables in manual tests - Add support for .env.mci files with priority over .env files - Update documentation with .env.mci precedence - Add 5 new tests for .env.mci functionality - All 357 tests passing Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
All feedback addressed in commit ebda5c0: ✅ Removed unused imports ✅ Added .env.mci file support with the following priority (lowest to highest):
✅ Updated documentation in README.md with comprehensive .env.mci coverage, including precedence rules and examples. ✅ Added 5 new tests for .env.mci functionality. All 357 tests passing. |
src/mci/utils/dotenv.py
Outdated
| # Remove quotes if present (ensure matching quote types) | ||
| if len(value) >= 2: | ||
| if value.startswith('"') and value.endswith('"'): | ||
| value = value[1:-1] | ||
| elif value.startswith("'") and value.endswith("'"): | ||
| value = value[1:-1] |
There was a problem hiding this comment.
The quote handling logic doesn't properly validate matching quotes. A value like \"hello' (starting with double quote, ending with single quote) will not have quotes stripped as expected, but it also won't raise an error. This could lead to unexpected behavior where mismatched quotes are silently kept in the value. Consider either: (1) validating that quotes match and raising an error if they don't, or (2) documenting this behavior as intentional. The PR description mentions 'quoted values with matching validation' but this validation is missing.
There was a problem hiding this comment.
@copilot Please refactor this method (parse_dotenv_file) with "python-dotenv", we don't need to reinvent the wheel.
There was a problem hiding this comment.
Refactored in commit d762f44 to use python-dotenv library instead of manual parsing. This provides robust quote handling and follows industry standards.
src/mci/core/mci_client.py
Outdated
| from .env files in the project root and ./mci directory. The precedence order is: | ||
| 1. ./mci/.env (lowest priority) | ||
| 2. Project root .env | ||
| 3. System environment variables | ||
| 4. env_vars argument (highest priority) | ||
|
|
There was a problem hiding this comment.
The precedence documentation is incomplete and doesn't match the actual implementation. According to the code in get_env_with_dotenv() and find_and_merge_dotenv_files(), there are actually 6 levels of precedence including .env.mci files. The correct order should be: 1. ./mci/.env, 2. ./mci/.env.mci, 3. Project root .env, 4. Project root .env.mci, 5. System environment variables, 6. env_vars argument. This discrepancy could confuse users about the actual behavior.
| from .env files in the project root and ./mci directory. The precedence order is: | |
| 1. ./mci/.env (lowest priority) | |
| 2. Project root .env | |
| 3. System environment variables | |
| 4. env_vars argument (highest priority) | |
| from .env and .env.mci files in both the project root and ./mci directory. | |
| The precedence order (lowest to highest priority) is: | |
| 1. ./mci/.env | |
| 2. ./mci/.env.mci | |
| 3. Project root .env | |
| 4. Project root .env.mci | |
| 5. System environment variables | |
| 6. env_vars argument |
There was a problem hiding this comment.
@copilot Please fix it and make sure it first checks for ".env.mci" files in both locations and if they doesn't exists, than check for .env file at first in ./mci directory than in root.
There was a problem hiding this comment.
Fixed in commit d762f44. Updated documentation to reflect the new exclusive loading behavior:
- If
.env.mcifiles exist: only./mci/.env.mciand root.env.mciare loaded - If NO
.env.mcifiles: only./mci/.envand root.envare loaded .envfiles are completely ignored when.env.mcifiles exist
src/mci/core/config.py
Outdated
| 1. ./mci/.env (lowest priority) | ||
| 2. Project root .env | ||
| 3. System environment variables | ||
| 4. env_vars argument (highest priority) |
There was a problem hiding this comment.
The precedence documentation is incomplete and doesn't match the actual implementation. According to the code in get_env_with_dotenv() and find_and_merge_dotenv_files(), there are actually 6 levels of precedence including .env.mci files. The correct order should be: 1. ./mci/.env, 2. ./mci/.env.mci, 3. Project root .env, 4. Project root .env.mci, 5. System environment variables, 6. env_vars argument. This discrepancy could confuse users about the actual behavior.
There was a problem hiding this comment.
@copilot Please fix it and make sure it first checks for ".env.mci" files in both locations and if they doesn't exists, than check for .env file at first in ./mci directory than in root.
There was a problem hiding this comment.
Fixed in commit d762f44. Updated config.py documentation with the new exclusive loading behavior and correct priority order.
src/mci/utils/dotenv.py
Outdated
| This function looks for .env files in two locations with the following priority: | ||
| 1. {project_root}/.env.mci (highest priority for MCI-specific configs) | ||
| 2. {project_root}/.env (project-level configs) | ||
| 3. {project_root}/mci/.env.mci (MCI library-level MCI-specific configs) | ||
| 4. {project_root}/mci/.env (lowest priority - MCI library defaults) | ||
|
|
||
| Files are checked in order at each location (.env.mci first, then .env). | ||
| Variables from higher priority files override those from lower priority files. |
There was a problem hiding this comment.
The documentation incorrectly states the priority order. The listed order shows root files as highest priority (1-2) and mci directory files as lower priority (3-4), but the actual implementation in lines 122-145 loads files in the opposite order: mci directory files first (lowest priority), then root files (higher priority). The implementation is correct, but the documentation should list them in the order they are applied (lowest to highest): 1. ./mci/.env, 2. ./mci/.env.mci, 3. root .env, 4. root .env.mci.
| This function looks for .env files in two locations with the following priority: | |
| 1. {project_root}/.env.mci (highest priority for MCI-specific configs) | |
| 2. {project_root}/.env (project-level configs) | |
| 3. {project_root}/mci/.env.mci (MCI library-level MCI-specific configs) | |
| 4. {project_root}/mci/.env (lowest priority - MCI library defaults) | |
| Files are checked in order at each location (.env.mci first, then .env). | |
| Variables from higher priority files override those from lower priority files. | |
| This function looks for .env files in two locations and merges them in the following order (lowest to highest priority): | |
| 1. {project_root}/mci/.env (lowest priority - MCI library defaults) | |
| 2. {project_root}/mci/.env.mci (MCI library-level MCI-specific configs) | |
| 3. {project_root}/.env (project-level configs) | |
| 4. {project_root}/.env.mci (highest priority for MCI-specific configs) | |
| Files are loaded in this order, and variables from higher priority files override those from lower priority files. |
There was a problem hiding this comment.
@copilot Please fix it and make sure it first checks for ".env.mci" files in both locations and if they doesn't exists, than check for .env file at first in ./mci directory than in root.
There was a problem hiding this comment.
Fixed in commit d762f44. Updated dotenv.py documentation to correctly describe the exclusive loading behavior:
- Checks for
.env.mcifiles first in both locations - If found, only loads
.env.mcifiles (ignores.env) - If not found, loads
.envfiles instead
MaestroError
left a comment
There was a problem hiding this comment.
@copilot Please fix the order for env files, current comments are mainly correct, but code doesn't follow the priority order.
Also, refactor env parsing with python-dotenv
src/mci/core/config.py
Outdated
| 1. ./mci/.env (lowest priority) | ||
| 2. Project root .env | ||
| 3. System environment variables | ||
| 4. env_vars argument (highest priority) |
There was a problem hiding this comment.
@copilot Please fix it and make sure it first checks for ".env.mci" files in both locations and if they doesn't exists, than check for .env file at first in ./mci directory than in root.
src/mci/core/mci_client.py
Outdated
| from .env files in the project root and ./mci directory. The precedence order is: | ||
| 1. ./mci/.env (lowest priority) | ||
| 2. Project root .env | ||
| 3. System environment variables | ||
| 4. env_vars argument (highest priority) | ||
|
|
There was a problem hiding this comment.
@copilot Please fix it and make sure it first checks for ".env.mci" files in both locations and if they doesn't exists, than check for .env file at first in ./mci directory than in root.
src/mci/utils/dotenv.py
Outdated
| # Remove quotes if present (ensure matching quote types) | ||
| if len(value) >= 2: | ||
| if value.startswith('"') and value.endswith('"'): | ||
| value = value[1:-1] | ||
| elif value.startswith("'") and value.endswith("'"): | ||
| value = value[1:-1] |
There was a problem hiding this comment.
@copilot Please refactor this method (parse_dotenv_file) with "python-dotenv", we don't need to reinvent the wheel.
src/mci/utils/dotenv.py
Outdated
| This function looks for .env files in two locations with the following priority: | ||
| 1. {project_root}/.env.mci (highest priority for MCI-specific configs) | ||
| 2. {project_root}/.env (project-level configs) | ||
| 3. {project_root}/mci/.env.mci (MCI library-level MCI-specific configs) | ||
| 4. {project_root}/mci/.env (lowest priority - MCI library defaults) | ||
|
|
||
| Files are checked in order at each location (.env.mci first, then .env). | ||
| Variables from higher priority files override those from lower priority files. |
There was a problem hiding this comment.
@copilot Please fix it and make sure it first checks for ".env.mci" files in both locations and if they doesn't exists, than check for .env file at first in ./mci directory than in root.
- Add python-dotenv dependency for robust .env parsing - Implement new priority logic: .env.mci files take precedence - When .env.mci files exist, .env files are NOT loaded (exclusive) - Priority: ./mci/.env.mci, then root .env.mci (if .env.mci exist) - OR: ./mci/.env, then root .env (if no .env.mci files) - Update all documentation to reflect new behavior - Update tests to match new exclusive loading logic - All 357 tests passing Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
All changes completed in commit d762f44: ✅ Refactored with
✅ Updated documentation in README.md, config.py, and mci_client.py |
Automatically loads environment variables from
.envand.env.mcifiles in the project root and./mcidirectory when initializing MCI configurations. Uses the industry-standardpython-dotenvlibrary for parsing. Implements exclusive loading behavior where.env.mcifiles take complete precedence over.envfiles for clean separation of MCI-specific configs.Changes
Core Implementation
src/mci/utils/dotenv.py: Refactored to usepython-dotenvlibrary for robust .env file parsingsrc/mci/core/config.py: Addedauto_load_dotenvparameter toMCIConfig.load()andvalidate_schema()src/mci/core/mci_client.py: ExtendedMCIClientWrapperto auto-load .env files on initializationpyproject.toml: Addedpython-dotenvdependencyFile Priority & Exclusive Loading Behavior
Important: MCI implements exclusive loading - when
.env.mcifiles exist,.envfiles are completely ignored.Loading Behavior:
.env.mcifiles exist: Only.env.mcifiles are loaded./mci/.env.mci- Library MCI-specific configs.env.mci- Project MCI-specific configs (overrides library).env.mcifiles exist: Only.envfiles are loaded./mci/.env- Library general defaults.env- Project-level configs (overrides library)Behavior
auto_load_dotenv=Falserun,list,validate) automaticallypython-dotenvfor industry-standard parsing with proper quote handlingExample
Scenario 1: Using .env.mci (MCI-specific)
Scenario 2: Using .env (no .env.mci)
Testing
.envfiles are ignored when.env.mciexistsDocumentation
config.pyandmci_client.pyFixes #30
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.