Skip to content

Conversation

aaronsteers
Copy link
Contributor

@aaronsteers aaronsteers commented Aug 6, 2025

fix: revert parameter names from manifest_input to manifest

Summary

This PR reverts parameter names from manifest_input back to manifest across all affected functions in the connector builder MCP server, as requested by @aaronsteers in PR feedback. The changes maintain the string input functionality that accepts either raw YAML content or file paths while restoring the original parameter naming convention.

Key Changes:

  • Reverted parameter names in validate_manifest(), execute_stream_test_read(), get_resolved_manifest(), and populate_dotenv_missing_secrets_stubs()
  • Updated function signatures, docstrings, and internal variable references
  • Fixed OSError handling in parse_manifest_input() to properly catch "File name too long" errors when YAML strings are mistaken for file paths
  • Updated test function calls and assertions to use the reverted parameter names

The core auto-detection logic that distinguishes between YAML strings and file paths remains unchanged, preserving the context window optimization benefits for LLMs.

Review & Testing Checklist for Human

  • Test auto-detection with edge cases: Verify the system correctly handles very long YAML strings, file paths that contain YAML-like content, and unusual file formats
  • Verify parameter consistency: Check that all function calls across the codebase use the correct manifest parameter name (no lingering manifest_input references)
  • Test error handling scenarios: Confirm that file system errors are properly reported and that the OSError catch doesn't mask legitimate issues
  • End-to-end workflow validation: Test the complete manifest validation, stream testing, and secrets management workflows with both YAML strings and file paths

Recommended Test Plan:

  1. Test with a real YAML file path to ensure file loading works
  2. Test with raw YAML string content to ensure parsing works
  3. Test with invalid file paths and malformed YAML to verify error handling
  4. Test the secrets management functions with manifest-based auto-detection

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    A["connector_builder_mcp/_connector_builder.py"]:::major-edit
    B["connector_builder_mcp/_secrets.py"]:::major-edit
    C["connector_builder_mcp/_util.py"]:::major-edit
    D["tests/test_secrets.py"]:::minor-edit
    E["tests/test_integration.py"]:::minor-edit
    F["parse_manifest_input()"]:::context
    
    A -->|calls| F
    B -->|calls| F
    C -->|contains| F
    D -->|tests| B
    E -->|tests| A
    
    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

Important

Auto-merge enabled.

This PR is set to merge automatically when all requirements are met.

- Replace dictionary/JSON manifest input with string-only input
- Add parse_manifest_input utility function with auto-detection logic
- Update validate_manifest, execute_stream_test_read, get_resolved_manifest, and populate_dotenv_missing_secrets_stubs functions
- Rename manifest parameter to manifest_input for clarity
- Support both raw YAML content and file path input
- Update all tests to use new string-based API
- Maintain backward compatibility through YAML conversion

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

Original prompt from AJ Steers
@Devin - please update the connector builder MCP server. Wherever it currently accepts a manifest JSON/dict, we want it to instead accept either a path or a YAML string. We night need to consider a rename of the arg. We might also consider whether it's better for us to detect whether we have a path vs a YAML string, vs just splitting into two mutually exclusive input args 

Goal is to not clog up the context by forcing the LLM to convert their YAML into JSON each time. And if they have a path pointer, that further reduces expansion of the context window

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 6, 2025
Copy link

github-actions bot commented Aug 6, 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/1754507432-yaml-path-manifest-support", "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/1754507432-yaml-path-manifest-support#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
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 updates the manifest parameters to accept string input (either raw YAML content or file path) instead of dictionary input, eliminating the need for JSON conversion and reducing context window usage.

  • Implements auto-detection logic to distinguish between file paths and raw YAML content
  • Updates four main functions to use manifest_input: str parameter instead of manifest: dict[str, Any]
  • Converts all test fixtures and assertions to use the new string-based API

Reviewed Changes

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

Show a summary per file
File Description
connector_builder_mcp/_util.py Adds parse_manifest_input() utility function with auto-detection logic
connector_builder_mcp/_connector_builder.py Updates three main functions to use string manifest input
connector_builder_mcp/_secrets.py Updates populate_dotenv_missing_secrets_stubs() to use string manifest input
tests/test_integration.py Converts test fixtures from YAML objects to file paths or YAML strings
tests/test_secrets.py Updates test calls to use YAML strings instead of dictionaries

Copy link

github-actions bot commented Aug 6, 2025

PyTest Results (Fast)

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

Results for commit 76bcbaf. ± Comparison against base commit beab29a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 6, 2025

PyTest Results (Full)

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

Results for commit 76bcbaf. ± Comparison against base commit beab29a.

♻️ This comment has been updated with latest results.

devin-ai-integration bot and others added 2 commits August 6, 2025 19:24
- Fix long line formatting in Field descriptions
- Update string quotes to double quotes for consistency
- Add proper line breaks for long error messages
- Add blank lines after import statements in tests

Co-Authored-By: AJ Steers <[email protected]>
- Move import yaml from inside test functions to module level
- Addresses @aaronsteers feedback on PR #18
- Follows Python best practices for import organization

Co-Authored-By: AJ Steers <[email protected]>
devin-ai-integration bot and others added 8 commits August 6, 2025 19:39
- Revert parameter names in all affected functions as requested by @aaronsteers
- Keep string input functionality and auto-detection logic intact
- Update function signatures, docstrings, and internal variable references
- Update test function calls and assertion messages
- Fix OSError handling in parse_manifest_input for YAML strings
- Addresses GitHub PR feedback on #18

Co-Authored-By: AJ Steers <[email protected]>
- Remove config parameter from all validate_manifest test calls
- Function now only accepts manifest parameter (YAML string or file path)
- Remove unused config variables that were causing linting errors
- All 63 tests now pass locally with clean linting checks
- Fixes TypeError: validate_manifest() takes 1 positional argument but 2 were given

Co-Authored-By: AJ Steers <[email protected]>
- Fix Field description formatting to use proper multi-line style
- Resolves Ruff Format Check CI failure
- All local checks now pass (ruff, mypy, pytest)

Co-Authored-By: AJ Steers <[email protected]>
@aaronsteers aaronsteers enabled auto-merge (squash) August 7, 2025 01:31
@aaronsteers aaronsteers merged commit 8954e30 into main Aug 7, 2025
14 checks passed
@aaronsteers aaronsteers deleted the devin/1754507432-yaml-path-manifest-support branch August 7, 2025 01:32
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