From 3462ca1355092c5f2dcfcf70dad05f00791f266c Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 11:04:38 +0200 Subject: [PATCH] fix(installer): read the persisted MCP port on update instead of the wizard default RegisterMcpStep built the registration URL from ctx.ExternalMcpPort, which the Update pipeline never repopulates from the existing installation, so any update silently re-registered the wizard default (47822) even when worker.config.json had a different port configured. InstallerWorkerConfig was also missing external_mcp_port entirely, so the installer had no way to read it back. Port 0 (external listener disabled) now skips registration instead of pointing Claude at 127.0.0.1:0/mcp. --- src/ClaudeDo.Installer/Core/ConfigModels.cs | 3 + .../Steps/RegisterMcpStep.cs | 21 +++++- .../RegisterMcpStepTests.cs | 65 +++++++++++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 tests/ClaudeDo.Installer.Tests/RegisterMcpStepTests.cs diff --git a/src/ClaudeDo.Installer/Core/ConfigModels.cs b/src/ClaudeDo.Installer/Core/ConfigModels.cs index 462a0355..664b6299 100644 --- a/src/ClaudeDo.Installer/Core/ConfigModels.cs +++ b/src/ClaudeDo.Installer/Core/ConfigModels.cs @@ -52,6 +52,9 @@ public sealed class InstallerWorkerConfig [JsonPropertyName("claude_bin")] public string ClaudeBin { get; set; } = "claude"; + [JsonPropertyName("external_mcp_port")] + public int ExternalMcpPort { get; set; } = 47_822; + private static readonly JsonSerializerOptions ReadOpts = new() { ReadCommentHandling = JsonCommentHandling.Skip, diff --git a/src/ClaudeDo.Installer/Steps/RegisterMcpStep.cs b/src/ClaudeDo.Installer/Steps/RegisterMcpStep.cs index cb7817d6..b3f4cc25 100644 --- a/src/ClaudeDo.Installer/Steps/RegisterMcpStep.cs +++ b/src/ClaudeDo.Installer/Steps/RegisterMcpStep.cs @@ -5,9 +5,23 @@ namespace ClaudeDo.Installer.Steps; public sealed class RegisterMcpStep : IInstallStep { private const string ServerName = "claudedo"; + private readonly Func _loadWorkerConfig; + + public RegisterMcpStep(Func? loadWorkerConfig = null) + { + _loadWorkerConfig = loadWorkerConfig ?? InstallerWorkerConfig.Load; + } public string Name => "Register MCP with Claude"; + // Resolves the URL from the persisted worker.config.json rather than ctx.ExternalMcpPort: + // an Update run never repopulates ctx from the existing installation, so ctx would still + // hold the wizard default (47822) even when the installed config has a different port. + // Returns null when the port is 0 — WorkerConfig treats that as "external listener off", + // so registering a URL against it would just point Claude at nothing. + public static string? ResolveUrl(InstallerWorkerConfig cfg) => + cfg.ExternalMcpPort == 0 ? null : $"http://127.0.0.1:{cfg.ExternalMcpPort}/mcp"; + public async Task ExecuteAsync(InstallContext ctx, IProgress progress, CancellationToken ct) { if (!ctx.RegisterMcpWithClaude) @@ -16,7 +30,12 @@ public sealed class RegisterMcpStep : IInstallStep return StepResult.Ok(); } - var url = $"http://127.0.0.1:{ctx.ExternalMcpPort}/mcp"; + var url = ResolveUrl(_loadWorkerConfig()); + if (url is null) + { + progress.Report("Skipped (external_mcp_port is 0 — the external MCP listener is disabled)."); + return StepResult.Ok(); + } // Drop any prior registration first so a re-run (e.g. update, changed port) // overwrites cleanly instead of erroring on a duplicate name. diff --git a/tests/ClaudeDo.Installer.Tests/RegisterMcpStepTests.cs b/tests/ClaudeDo.Installer.Tests/RegisterMcpStepTests.cs new file mode 100644 index 00000000..cfccbf88 --- /dev/null +++ b/tests/ClaudeDo.Installer.Tests/RegisterMcpStepTests.cs @@ -0,0 +1,65 @@ +using ClaudeDo.Installer.Core; +using ClaudeDo.Installer.Steps; + +namespace ClaudeDo.Installer.Tests; + +public sealed class RegisterMcpStepTests +{ + // Progress posts to the SynchronizationContext (or the ThreadPool) asynchronously, + // so asserting on captured messages right after an await needs a synchronous reporter. + private sealed class SyncProgress(Action report) : IProgress + { + public void Report(T value) => report(value); + } + + + [Fact] + public void ResolveUrl_uses_the_configured_port() + { + var url = RegisterMcpStep.ResolveUrl(new InstallerWorkerConfig { ExternalMcpPort = 49999 }); + + Assert.Equal("http://127.0.0.1:49999/mcp", url); + } + + [Fact] + public void ResolveUrl_defaults_to_47822_without_a_configured_port() + { + var url = RegisterMcpStep.ResolveUrl(new InstallerWorkerConfig()); + + Assert.Equal("http://127.0.0.1:47822/mcp", url); + } + + [Fact] + public void ResolveUrl_is_null_when_the_external_listener_is_disabled() + { + var url = RegisterMcpStep.ResolveUrl(new InstallerWorkerConfig { ExternalMcpPort = 0 }); + + Assert.Null(url); + } + + [Fact] + public async Task Not_selected_skips_without_loading_the_worker_config() + { + var loaded = false; + var step = new RegisterMcpStep(() => { loaded = true; return new InstallerWorkerConfig(); }); + var ctx = new InstallContext { RegisterMcpWithClaude = false }; + + var result = await step.ExecuteAsync(ctx, new Progress(_ => { }), CancellationToken.None); + + Assert.True(result.Success); + Assert.False(loaded); + } + + [Fact] + public async Task Disabled_external_port_skips_without_running_the_claude_cli() + { + var step = new RegisterMcpStep(() => new InstallerWorkerConfig { ExternalMcpPort = 0 }); + var ctx = new InstallContext { RegisterMcpWithClaude = true }; + var messages = new List(); + + var result = await step.ExecuteAsync(ctx, new SyncProgress(messages.Add), CancellationToken.None); + + Assert.True(result.Success); + Assert.Contains(messages, m => m.Contains("external_mcp_port is 0", StringComparison.Ordinal)); + } +}