refactor(mcp): migrate cloudflared/ngrok install config to fieldform - #430
Merged
Conversation
Both remaining tunnel providers still collected their install config through the legacy imperative Configurer (hand-rolled `if s.X == ""` prompts around provider-derived values). Migrate them onto the shared fieldform Fields + Finalize primitive like OpenAI, then remove the legacy Configurer path. - cloudflared -> cloudflaredFields(): Domain + TunnelName derive from the provisioned named-tunnel state (LoadCloudflareTunnelState) at precedence 0, with pinner-mcp as the TunnelName default. cloudflaredFinalize fails fast on a headless run when the required Domain is still unresolved. - ngrok -> ngrokFields(cfgMgr)/ngrokFinalize(): the authtoken derives from the ngrok config file / config-manager store; PublicURL derives from the account REST API then a stable SDK dev-domain. The legacy resolveNgrokURL's manual paste becomes PublicURL's Prompt. ngrokFinalize persists the token and fails fast on a headless run when the public URL is unresolved (the token is not independently required: an API/operator-resolved URL needs none). - TunnelProviderSpec.Fields now takes cfgMgr so a field's Derived hook can consult the provider's last-resort credential store (the openai/cloudflared registrations updated to the new signature). - Remove the now-dead cloudflaredConfigurer, ngrokConfigurer, and resolveNgrokURL plus their dedicated tests; the behaviors are covered by the new fieldform shape tests and the existing flattened/collect integration tests. - Also drops a dead ngrok.api_key config-manager persist (no such supported pair existed; the write was silently swallowed). Verification: go vet + go test clean on fieldform, tunnel, services, install, cli/wizard. Kodus: 0 HIGH/ERROR remaining (one low false-positive on a test fixture's literal "secret" key, which is required to parse CloudflareTunnelState).
This comment has been minimized.
This comment has been minimized.
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
Code Coverage ReportTotal Coverage: 52.0% Generated from commit: c5ab0d1 |
The ngrok public-URL Derived hook used context.Background() for the account-API query and the SDK tunnel lookup, dropping the install command's deadline/cancellation signal so an interrupted install hung until those calls' own timeouts. The legacy configurer threaded the caller ctx through both. Thread the step's ctx through TunnelProviderSpec.Fields into the ngrokFields closure so the Derived hook (whose signature carries no ctx) honors cancellation. Also aligns the flattened-test auth-token fixture values with the repo's test-auth-token convention to avoid trip...ting the secret scanner.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
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
1 participant
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.
Migrates the cloudflared and ngrok install configuration from the legacy
imperative Configurer onto the shared fieldform Fields + Finalize primitive,
then removes the legacy Configurer path.
named-tunnel state via Field.Derived, with pinner-mcp as the tunnel name
default; a headless run fails fast when the domain is unresolved.
the ngrok CLI config, and the account API with an SDK dev-domain fallback;
the manual URL paste is the field's prompt. Finalize persists the token and
fails fast on a headless run when the URL cannot be resolved.
read the provider's last-resort credential store.
plus a no-op ngrok.api_key config-manager persist (no such pair is supported).
This pull request migrates the cloudflared and ngrok tunnel providers from the legacy imperative
Configurerapproach to the shared declarativeFields + Finalizefield-resolution primitive, completing the migration that was previously only applied to the OpenAI provider.Key Changes:
Cloudflared provider migration: Replaced the imperative
cloudflaredConfigurerwith:cloudflaredFields(): Declarative field definitions forDomainandTunnelNamewithDerivedhooks that automatically populate values from provisioned named-tunnel state stored in the tunnel-state file, falling back to thepinner-mcpdefault for tunnel name.cloudflaredFinalize(): Post-gather hook that validates the domain is resolved on headless runs, failing fast with a clear error message instead of silently writing an incomplete env file.Ngrok provider migration: Replaced the imperative
ngrokConfigurerwith:ngrokFields(cfgMgr): Declarative fields for the authtoken/tunnel token and public base URL. TheDerivedhooks pre-resolve tokens from the config-manager store and existing ngrok config, then derive the public URL from three sources in order: operator-supplied value, account API query (when an API key is available), or a short-lived embedded tunnel's stable dev domain.ngrokFinalize(): Persists the resolved token to the config-manager last-resort store and validates that a public URL was resolved on headless runs, failing fast if not.Provider registry updates: Updated
Fieldssignatures to accept the config manager, enabling field hooks to consult the last-resort credential store. Removed the legacyConfigurerreferences for all three providers.Test coverage: Added new test files (
service_install_cloudflared_fields_test.go,service_install_ngrok_fields_test.go,tunnel_providers_registry_test.go) that validate field shapes, derivation logic, fail-fast contracts, and confirm no provider remains on the legacy path. Updated existing tests to align with the new field-based approach.Behavior preservation: All existing functionality is maintained—automatic resolution from provisioned state, API-based URL discovery, token pre-resolution, and interactive prompting fallbacks—now expressed through the declarative field framework with headless fail-fast validation.