Skip to content

Conversation

JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Aug 25, 2025

Overview

This PR introduces a new PreRunConfig system that cleanly separates the concern of 'what type of thing are we running' from 'how do we run it'. This provides type-safe input classification and improves code maintainability.

Key Changes

New Components

  • PreRunConfig types: Distinct types for different source categories:

    • `RegistrySource`: For registry names (e.g., "fetch", "github")
    • `ContainerImageSource`: For container images (e.g., "docker.io/library/alpine:latest")
    • `ProtocolSchemeSource`: For protocol schemes (e.g., "uvx://package", "npx://package")
    • `RemoteURLSource`: For remote MCP server URLs
    • `ConfigFileSource`: For configuration files
  • ParsePreRunConfig function: Comprehensive input classification with clear error messages

  • PreRunTransformer: Type-specific transformation logic from PreRunConfig to RunConfig

  • RunFlagsInterface: Clean interface abstraction to avoid circular dependencies

Refactoring

  • Updated `BuildRunnerConfig` in `cmd/thv/app/run_flags.go` to use the new PreRunConfig approach
  • Broke down complex functions to reduce cyclomatic complexity
  • Added proper documentation comments for all exported methods

Testing

  • Comprehensive test suite with table-driven tests covering all PreRunConfig types
  • Tests for edge cases, error conditions, and backward compatibility
  • All existing tests continue to pass

Benefits

  1. Type Safety: Input classification prevents runtime errors and provides clear error messages
  2. Extensibility: Easy to add new source types without modifying existing code
  3. Maintainability: Clear separation of parsing logic from transformation logic
  4. Testability: Each component can be tested independently
  5. Performance: Efficient parsing with early validation and clear error paths

Backward Compatibility

  • ✅ All existing functionality preserved
  • ✅ No breaking changes to public APIs
  • ✅ All existing tests pass
  • ✅ Zero linting issues

Testing

  • ✅ All unit tests pass (`task test`)
  • ✅ All linting checks pass (`task lint`)
  • ✅ Code coverage maintained

The new PreRunConfig system provides a solid foundation for handling different types of MCP server sources in a type-safe, maintainable way while maintaining full backward compatibility.

JAORMX added 3 commits August 26, 2025 10:48
Introduces a new PreRunConfig system that cleanly separates the concern of
'what type of thing are we running' from 'how do we run it'. This provides
type-safe input classification and improves code maintainability.

Key changes:
- Add PreRunConfig types for different source types (registry, container image,
  protocol scheme, remote URL, config file)
- Implement ParsePreRunConfig function with comprehensive input classification
- Create PreRunTransformer for type-specific RunConfig transformation
- Add RunFlagsInterface abstraction to avoid circular dependencies
- Refactor BuildRunnerConfig to use new PreRunConfig approach
- Add comprehensive test suite with table-driven tests
- Fix all linting issues and reduce cyclomatic complexity
- Maintain full backward compatibility

The new system provides better error messages, type safety, and extensibility
while preserving all existing functionality.
The e2e tests were failing because they expected to see 'fetch' in the output
when running 'thv run fetch', but the PreRunConfig changes had moved the
registry name logging from INFO to DEBUG level.

Changes:
- Change logger.Debugf to logger.Infof for registry server discovery messages
- Ensures both remote and container registry servers show their names in output
- Fixes failing e2e tests that check for registry names in command output

The output now includes:
'Found container server in registry: fetch -> ghcr.io/stackloklabs/gofetch/server:0.0.5'

This maintains backward compatibility with existing e2e test expectations.
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
@JAORMX JAORMX force-pushed the feat/prerun-config-abstraction branch from 74d673b to 321b520 Compare August 26, 2025 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant