Skip to content

Conversation

aaronsteers
Copy link
Contributor

feat: implement dotenv-based secret management system

Summary

This PR implements a comprehensive dotenv-based secret management system for connector-builder-mcp that allows users to manage secrets in .env files without exposing actual secret values to the LLM. The system provides MCP tools for managing secrets and automatically hydrates connector configurations with secrets during execution.

Key Features:

  • File-based secret management: Uses .env files with configurable paths via environment variable or MCP tool
  • Naming convention mapping: Environment variables map to config paths (e.g., CREDENTIALS_PASSWORDcredentials.password)
  • MCP tools: set_dotenv_path, list_secrets, add_secret_stub, get_secrets_file_path_for_user
  • Automatic config hydration: Seamlessly injects secrets into connector configs before execution
  • Security-first: LLM never sees actual secret values, only metadata about what secrets exist

Implementation Details:

  • Added python-dotenv>=1.0.0 dependency
  • Created _secrets.py module with global state management for dotenv file path
  • Integrated config hydration into all three connector builder operations: validate_manifest, execute_stream_test_read, and get_resolved_manifest
  • Comprehensive test suite with 18 test cases covering all functionality

Review & Testing Checklist for Human

  • End-to-end workflow test: Use MCP client to set dotenv path, add secret stubs, populate with real values, and verify connector operations work correctly
  • Naming convention verification: Test various environment variable patterns (API_KEY, CREDENTIALS_PASSWORD, OAUTH_CLIENT_SECRET) map correctly to config paths
  • Config hydration integration: Verify existing connector operations still work and secrets are properly injected without breaking functionality
  • Tool registration: Confirm all four new MCP tools are accessible and work as expected
  • File handling edge cases: Test with various file paths, missing directories, permissions issues, and concurrent access scenarios

Recommended Test Plan:

  1. Start MCP server with poe server and connect with MCP client
  2. Use set_dotenv_path to configure a test .env file
  3. Use add_secret_stub to add placeholder secrets
  4. Manually edit the .env file to add real secret values
  5. Run connector validation/testing operations and verify secrets are hydrated correctly
  6. Test list_secrets shows secret keys without exposing values

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    Client["MCP Client"]
    Server["connector_builder_mcp/server.py"]:::context
    ConnectorBuilder["_connector_builder.py"]:::major-edit
    Secrets["_secrets.py"]:::major-edit
    EnvFile[".env file"]
    Tests["tests/test_secrets.py"]:::major-edit
    PyProject["pyproject.toml"]:::minor-edit
    Util["_util.py"]:::minor-edit
    
    Client --> Server
    Server --> ConnectorBuilder
    ConnectorBuilder --> Secrets
    Secrets --> EnvFile
    Tests --> Secrets
    
    ConnectorBuilder -.->|"hydrate_config()"| Secrets
    Secrets -.->|"load_secrets()"| EnvFile
    Client -.->|"set_dotenv_path<br/>list_secrets<br/>add_secret_stub"| Secrets

    subgraph Legend
        L1[Major Edit]:::major-edit
        L2[Minor Edit]:::minor-edit
        L3[Context/No Edit]:::context
    end

    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB
    classDef context fill:#FFFFFF
Loading

Notes

  • The _env_var_to_path function contains complex logic for mapping environment variable names to config paths - this is a critical piece that should be thoroughly tested
  • Global state management with _current_dotenv_path could potentially have concurrency issues in multi-threaded scenarios
  • Config hydration happens automatically before every connector operation, so any bugs here could break existing functionality
  • The implementation assumes users will follow the naming convention correctly - consider adding validation or better error messages for misnamed variables

Link to Devin run: https://app.devin.ai/sessions/eab82b57cce84fb8aeb33b11e2aabea0
Requested by: AJ Steers (@aaronsteers)

- Add python-dotenv dependency to pyproject.toml
- Create _secrets.py module with comprehensive secret management tools
- Implement naming convention mapping (CREDENTIALS_PASSWORD -> credentials.password)
- Add tools: set_dotenv_path, list_secrets, add_secret_stub, get_secrets_file_path_for_user
- Support both environment variable and tool-based file path configuration
- Integrate config hydration into existing connector builder tools
- Add comprehensive test suite with 18 test cases
- Update _util.py with reference to new hydration functionality
- Fix import issues in _connector_builder.py for better type compatibility

The system allows users to manage secrets in .env files without exposing
actual values to the LLM, while providing seamless config hydration for
connector operations.

Co-Authored-By: AJ Steers <[email protected]>
@Copilot Copilot AI review requested due to automatic review settings August 2, 2025 01:47
Copy link
Contributor

Original prompt from AJ Steers
@Devin - Add a dotenv implementation to connector-builder-mcp. Specifically, here are the details of how I want you to implement it. Instead of passing the environment variables to the server directly, we're going to send a file pointer as an environment variable. The reason for this is that we're not going to give the LLM access to the secrets, we are only going to have tools that list secrets in the file, add stubs for new secrets that are needed from the user, print the file path to the user, etc. 

Our tool will have access to those secrets via the Python dotenv library, and we'll hydrate connector config as needed, given the secrets that are provided. In this way the LM can build the connector, without ever getting access to the secrets. And the user is told exactly what they need to provide, and we make it easy for them to provide those secrets within the file that we give them the path to.

Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@github-actions github-actions bot added the enhancement New feature or request label Aug 2, 2025
Copy link

github-actions bot commented Aug 2, 2025

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

Testing This Branch via MCP

To test the changes in this specific branch with an MCP client like Claude Desktop, use the following configuration:

{
  "mcpServers": {
    "connector-builder-mcp-dev": {
      "command": "uvx",
      "args": ["--from", "git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1754097884-dotenv-implementation", "connector-builder-mcp"]
    }
  }
}

Testing This Branch via CLI

You can test this version of the MCP Server using the following CLI snippet:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1754097884-dotenv-implementation#egg=airbyte-connector-builder-mcp' --help

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poe <command> - Runs any poe command in the uv virtual environment

📝 Edit this welcome message.

Copy link

github-actions bot commented Aug 2, 2025

PyTest Results (Fast)

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit db0964e. ± Comparison against base commit 65b10d8.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 2, 2025

PyTest Results (Full)

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit db0964e. ± Comparison against base commit 65b10d8.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a comprehensive dotenv-based secret management system that allows users to manage secrets in .env files without exposing actual secret values to the LLM. The system provides MCP tools for managing secrets and automatically hydrates connector configurations with secrets during execution.

  • Adds file-based secret management using .env files with configurable paths
  • Implements naming convention mapping from environment variables to config paths (e.g., CREDENTIALS_PASSWORDcredentials.password)
  • Provides four MCP tools for secret management: set_dotenv_path, list_secrets, add_secret_stub, and get_secrets_file_path_for_user
  • Integrates automatic config hydration into all three connector builder operations

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
tests/test_secrets.py Comprehensive test suite with 18 test cases covering all secrets functionality
pyproject.toml Adds python-dotenv>=1.0.0 dependency
connector_builder_mcp/_util.py Adds documentation note referencing the new secrets hydration functionality
connector_builder_mcp/_secrets.py Core secrets management module with dotenv file handling, config hydration, and MCP tools
connector_builder_mcp/_connector_builder.py Integrates config hydration into connector operations and registers secrets tools

devin-ai-integration bot and others added 5 commits August 2, 2025 01:49
- list_secrets → list_dotenv_secrets
- add_secret_stub → populate_dotenv_missing_secrets_stubs
- get_secrets_file_path_for_user → get_dotenv_path

Updated all imports, function calls, and test cases to use new names.

Co-Authored-By: AJ Steers <[email protected]>
- Support manifest-based detection of secrets from airbyte_secret fields
- Support explicit config paths like ['credentials.password', 'oauth.client_secret']
- Support legacy single secret key mode for backward compatibility
- Create stubs for union of manifest-detected and explicitly provided secrets
- Add comprehensive tests covering all modes and edge cases

Co-Authored-By: AJ Steers <[email protected]>
…ames_from_manifest

- Address GitHub comment requesting more descriptive function name
- Function name now clearly indicates it extracts secret names, not actual secrets
- Maintains all existing functionality and test coverage

Co-Authored-By: AJ Steers <[email protected]>
devin-ai-integration bot and others added 2 commits August 2, 2025 02:42
- Remove global state variables (_current_dotenv_path, SECRETS_FILE_ENV_VAR)
- Update all function signatures to require explicit dotenv_path parameter
- Add allow_create parameter to populate_dotenv_missing_secrets_stubs
- Make dotenv file handling completely optional for connectors without secrets
- Update all hydrate_config calls to pass explicit dotenv_path=None
- Update all tests to pass explicit dotenv file paths
- Maintain backward compatibility while removing environment variable dependencies
- Support both secrets and semi-sensitive data (PII) through flexible mechanism

Co-Authored-By: AJ Steers <[email protected]>
- Fix formatting issues in connector_builder_mcp/_secrets.py and tests/test_secrets.py
- Resolve Ruff Format Check and Ruff Lint Check CI failures
- All local checks now pass: format, lint, typecheck, and tests

Co-Authored-By: AJ Steers <[email protected]>
…e secret hydration from manifest validation

- Add optional dotenv_path parameter to execute_stream_test_read function signature
- Remove secret hydration from validate_manifest and get_resolved_manifest functions
- Update function docstrings to reflect new parameter
- Addresses GitHub comments #18 and #19 on PR #11

Co-Authored-By: AJ Steers <[email protected]>
…dotenv keys

- Remove set_dotenv_path function as it's no longer needed in stateless design
- Remove function from MCP tool registration
- Remove associated test class and import
- Switch from uppercase underscore format (CREDENTIALS_PASSWORD) to dot notation (credentials.password) for dotenv keys
- Update hydrate_config to use dot notation directly for mapping dotenv keys to config paths
- Update all tests to reflect new dot notation naming convention
- Addresses GitHub comment from aaronsteers on PR #11 and implements requested naming simplification

Co-Authored-By: AJ Steers <[email protected]>
Copy link
Contributor

Test Implementation Audit - tests/test_secrets.py

I've completed the requested audit of the test implementation. Here are my findings:

Issues Identified

1. Test Classes vs Functions ✅ Action Required

All 5 test classes (TestLoadSecrets, TestHydrateConfig, TestListDotenvSecrets, TestPopulateDotenvMissingSecretsStubs, TestGetDotenvPath) are just organizational containers with no shared setup/teardown. As requested, these should be converted to standalone test functions for better readability.

2. Potentially Useless Tests ⚠️ 3 tests identified

  • test_hydrate_config_empty_config (lines 99-102) - Tests trivial case hydrate_config({}, "/path") == {}
  • test_hydrate_config_none_config (lines 104-107) - Tests trivial case hydrate_config(None, "/path") == None
  • test_get_dotenv_path (lines 277-281) - Tests trivial wrapper around Path().absolute()

These tests don't add meaningful validation beyond what the function signature already guarantees.

3. Wasteful Redundancies ⚠️ 1 test identified

  • test_hydrate_config_simple_keys (lines 77-86) overlaps significantly with test_hydrate_config_with_secrets (lines 56-75) - both test basic key injection functionality with similar patterns.

4. Suppressed Signals ✅ None found

All tests properly validate expected behavior and error conditions. No tests are inadvertently suppressing important error signals.

Summary

  • Total tests: 18
  • Tests to convert: All (classes → functions)
  • Tests to remove: 3 useless tests
  • Tests to consolidate: 1 redundant test
  • Final test count: ~14 focused tests

Proposed Changes

  1. Convert all test classes to standalone functions
  2. Remove the 3 trivial tests that don't add value
  3. Consolidate the redundant hydration test
  4. Maintain all meaningful test coverage

The remaining tests provide good coverage of:

  • Edge cases (missing files, empty manifests, etc.)
  • Core functionality (secret loading, config hydration, stub generation)
  • Error conditions (invalid inputs, file creation failures)

Would you like me to proceed with implementing these improvements?

- Convert all test classes to standalone test functions for better readability
- Remove 3 trivial tests that don't add meaningful value:
  * test_hydrate_config_empty_config (trivial case)
  * test_hydrate_config_none_config (trivial case)
  * Kept test_get_dotenv_path but noted it tests trivial wrapper
- Consolidate test_hydrate_config_simple_keys into test_hydrate_config_with_secrets
- Addresses GitHub comment audit feedback from aaronsteers
- Reduced test count from 18 to 15 while maintaining full coverage

Co-Authored-By: AJ Steers <[email protected]>
Copy link
Contributor

Test Implementation Improvements Complete ✅

I've successfully implemented the audit recommendations:

Changes Made

  1. ✅ Converted all test classes to standalone functions - All 5 test classes (TestLoadSecrets, TestHydrateConfig, etc.) are now standalone functions for better readability
  2. ✅ Removed 3 useless tests that didn't add meaningful value:
    • test_hydrate_config_empty_config - trivial case testing hydrate_config({}, "/path") == {}
    • test_hydrate_config_none_config - trivial case testing hydrate_config(None, "/path") == None
    • Kept test_get_dotenv_path as requested (though it's a simple wrapper test)
  3. ✅ Consolidated redundant test - Merged test_hydrate_config_simple_keys into test_hydrate_config_with_secrets to eliminate duplication

Results

  • Test count: Reduced from 18 to 15 focused tests
  • All tests pass: ✅ 15/15 tests passing
  • Code quality: ✅ All formatting, linting, and type checks pass
  • Coverage maintained: All meaningful functionality still tested

Test Structure Now

tests/test_secrets.py (15 functions)
├── test_load_secrets_* (2 tests)
├── test_hydrate_config_* (5 tests) 
├── test_list_dotenv_secrets_* (2 tests)
├── test_populate_dotenv_missing_secrets_stubs_* (5 tests)
└── test_get_dotenv_path (1 test)

The tests are now more readable, focused, and eliminate the wasteful redundancies identified in the audit. No suppressed signals were found - all tests properly validate expected behavior and error conditions.

aaronsteers and others added 2 commits August 1, 2025 20:48
- Remove get_dotenv_path function call from register_secrets_tools
- Remove get_dotenv_path import from tests
- Remove test_get_dotenv_path test function
- Addresses GitHub comment from aaronsteers requesting deletion of this function

Co-Authored-By: AJ Steers <[email protected]>
devin-ai-integration bot and others added 5 commits August 2, 2025 03:52
- Update execute_stream_test_read function signature to use Path | None
- Add Path import from pathlib
- Convert Path to string when calling hydrate_config function
- Addresses GitHub comment feedback from aaronsteers

Co-Authored-By: AJ Steers <[email protected]>
- Change config_paths from list format to CSV string format in tests
- Fixes AttributeError: 'list' object has no attribute 'split'
- Updates test_populate_dotenv_missing_secrets_stubs_config_paths and test_populate_dotenv_missing_secrets_stubs_combined_mode
- Addresses pytest failures reported by aaronsteers

Co-Authored-By: AJ Steers <[email protected]>
@aaronsteers aaronsteers merged commit 36252a7 into main Aug 5, 2025
13 checks passed
@aaronsteers aaronsteers deleted the devin/1754097884-dotenv-implementation branch August 5, 2025 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant