From 5eea4391290da92f8ae020515245c60a3bbc1ca0 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Mon, 10 Aug 2026 13:55:58 +0200 Subject: [PATCH] fix(mcp): make set_task_config/set_list_config patch instead of overwrite Omitted fields on set_task_config/set_list_config used to be indistinguishable from an explicit clear, so any single-field update silently wiped every other override (including SessionSkills, which wasn't even a tool parameter). Now only the fields you pass are changed; clearing an override requires naming it in the new clearFields array (a sentinel string doesn't work uniformly since maxTurns is an int). set_list_config's "all four null deletes the config" case is preserved but only reachable via clearFields. Return values now reflect the full resulting config. --- .../External/ConfigMcpTools.cs | 71 ++++++++--- .../External/ConfigMcpToolsTests.cs | 111 ++++++++++++++++-- .../External/EffectiveRunConfigTests.cs | 4 +- 3 files changed, 157 insertions(+), 29 deletions(-) diff --git a/src/ClaudeDo.Worker/External/ConfigMcpTools.cs b/src/ClaudeDo.Worker/External/ConfigMcpTools.cs index 383a13a1..c65eda59 100644 --- a/src/ClaudeDo.Worker/External/ConfigMcpTools.cs +++ b/src/ClaudeDo.Worker/External/ConfigMcpTools.cs @@ -55,33 +55,64 @@ public sealed class ConfigMcpTools : new TaskConfigResult(true, new TaskConfigDto(cfg.Model, cfg.SystemPrompt, cfg.AgentPath, cfg.MaxTurns)); } + // clearFields (not a per-field sentinel like UpdateList's "" for workingDir) because maxTurns is an + // int? — there's no blank value to overload as "clear" for a number the way there is for a string. + private static readonly string[] ConfigFieldNames = { "model", "systemPrompt", "agentPath", "maxTurns" }; + + private static HashSet NormalizeClearFields(IReadOnlyList? clearFields) + { + var set = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var field in clearFields ?? Array.Empty()) + { + if (!ConfigFieldNames.Contains(field, StringComparer.OrdinalIgnoreCase)) + throw new InvalidOperationException( + $"Unknown clearFields entry '{field}'. Valid values: {string.Join(", ", ConfigFieldNames)}."); + set.Add(field); + } + return set; + } + [McpServerTool, Description( "Set a list's default model/system prompt/agent path/max turns — the fallback for tasks in this list " + - "that don't override them. Passing all four as null clears the list config instead of setting one.")] + "that don't override them. Only the fields you pass are changed; omitted fields keep their current " + + "value. To clear a field instead, name it in clearFields; clearing all four deletes the list's config.")] public async Task SetListConfig( string listId, string? model = null, string? systemPrompt = null, string? agentPath = null, - int? maxTurns = null, CancellationToken cancellationToken = default) + int? maxTurns = null, + [Description("Field names to explicitly clear: 'model', 'systemPrompt', 'agentPath', 'maxTurns'. " + + "A field named here is cleared even if a value was also passed for it.")] + IReadOnlyList? clearFields = null, + CancellationToken cancellationToken = default) { _ = await _lists.GetByIdAsync(listId, cancellationToken) ?? throw new InvalidOperationException($"List {listId} not found."); - var m = model.NullIfBlank(); - var sp = systemPrompt.NullIfBlank(); - var ap = agentPath.NullIfBlank(); + var clear = NormalizeClearFields(clearFields); + var existing = await _lists.GetConfigAsync(listId, cancellationToken); + + var m = clear.Contains("model") ? null : model.NullIfBlank() ?? existing?.Model; + var sp = clear.Contains("systemPrompt") ? null : systemPrompt.NullIfBlank() ?? existing?.SystemPrompt; + var ap = clear.Contains("agentPath") ? null : agentPath.NullIfBlank() ?? existing?.AgentPath; + var mt = clear.Contains("maxTurns") ? null : maxTurns ?? existing?.MaxTurns; TaskConfigDto? config; - if (m is null && sp is null && ap is null && maxTurns is null) + if (m is null && sp is null && ap is null && mt is null && clear.Count > 0) { await _lists.DeleteConfigAsync(listId, cancellationToken); config = null; } + else if (m is null && sp is null && ap is null && mt is null && existing is null) + { + config = null; + } else { await _lists.SetConfigAsync(new ListConfigEntity { - ListId = listId, Model = m, SystemPrompt = sp, AgentPath = ap, MaxTurns = maxTurns, + ListId = listId, Model = m, SystemPrompt = sp, AgentPath = ap, MaxTurns = mt, + SessionSkills = existing?.SessionSkills, VerifyCommand = existing?.VerifyCommand, }, cancellationToken); - config = new TaskConfigDto(m, sp, ap, maxTurns); + config = new TaskConfigDto(m, sp, ap, mt); } await _broadcaster.ListUpdated(listId); @@ -90,21 +121,29 @@ public sealed class ConfigMcpTools [McpServerTool, Description( "Set per-task overrides for model/system prompt/agent path/max turns; these take precedence over the " + - "list's default config for this one task. Pass null for any field to clear that override.")] + "list's default config for this one task. Only the fields you pass are changed; omitted fields keep " + + "their current override. To clear a field instead, name it in clearFields.")] public async Task SetTaskConfig( string taskId, string? model = null, string? systemPrompt = null, string? agentPath = null, - int? maxTurns = null, CancellationToken cancellationToken = default) + int? maxTurns = null, + [Description("Field names to explicitly clear: 'model', 'systemPrompt', 'agentPath', 'maxTurns'. " + + "A field named here is cleared even if a value was also passed for it.")] + IReadOnlyList? clearFields = null, + CancellationToken cancellationToken = default) { - _ = await _tasks.GetByIdAsync(taskId, cancellationToken) + var task = await _tasks.GetByIdAsync(taskId, cancellationToken) ?? throw new InvalidOperationException($"Task {taskId} not found."); - var m = model.NullIfBlank(); - var sp = systemPrompt.NullIfBlank(); - var ap = agentPath.NullIfBlank(); + var clear = NormalizeClearFields(clearFields); - await _tasks.UpdateAgentSettingsAsync(taskId, m, sp, ap, maxTurns, ct: cancellationToken); + var m = clear.Contains("model") ? null : model.NullIfBlank() ?? task.Model; + var sp = clear.Contains("systemPrompt") ? null : systemPrompt.NullIfBlank() ?? task.SystemPrompt; + var ap = clear.Contains("agentPath") ? null : agentPath.NullIfBlank() ?? task.AgentPath; + var mt = clear.Contains("maxTurns") ? null : maxTurns ?? task.MaxTurns; + + await _tasks.UpdateAgentSettingsAsync(taskId, m, sp, ap, mt, task.SessionSkills, cancellationToken); await _broadcaster.TaskUpdated(taskId); - return new SetTaskConfigResult(true, taskId, new TaskConfigDto(m, sp, ap, maxTurns)); + return new SetTaskConfigResult(true, taskId, new TaskConfigDto(m, sp, ap, mt)); } [McpServerTool, Description("Read this task's per-task overrides (model/system prompt/agent path/max turns), which take precedence over the list's default config. Returns { found: false, config: null } if none is set.")] diff --git a/tests/ClaudeDo.Worker.Tests/External/ConfigMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/ConfigMcpToolsTests.cs index 7b80edf4..0bc84534 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ConfigMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ConfigMcpToolsTests.cs @@ -37,7 +37,7 @@ public sealed class ConfigMcpToolsTests : IDisposable { var listId = await SeedListAsync(); - var setResult = await _sut.SetListConfig(listId, "sonnet", "be terse", null, 42, CancellationToken.None); + var setResult = await _sut.SetListConfig(listId, "sonnet", "be terse", null, 42, cancellationToken: CancellationToken.None); Assert.True(setResult.Ok); Assert.Equal(listId, setResult.ListId); Assert.NotNull(setResult.Config); @@ -65,12 +65,41 @@ public sealed class ConfigMcpToolsTests : IDisposable } [Fact] - public async Task SetListConfig_AllNull_ClearsConfig() + public async Task SetListConfig_PartialUpdate_LeavesOtherFieldsUntouched() { var listId = await SeedListAsync(); - await _sut.SetListConfig(listId, "sonnet", null, null, null, CancellationToken.None); + await _sut.SetListConfig(listId, "opus", "be terse", "agent.md", 30, cancellationToken: CancellationToken.None); - var clearResult = await _sut.SetListConfig(listId, null, null, null, null, CancellationToken.None); + var result = await _sut.SetListConfig(listId, maxTurns: 40, cancellationToken: CancellationToken.None); + + Assert.Equal("opus", result.Config!.Model); + Assert.Equal("be terse", result.Config.SystemPrompt); + Assert.Equal("agent.md", result.Config.AgentPath); + Assert.Equal(40, result.Config.MaxTurns); + } + + [Fact] + public async Task SetListConfig_ClearFields_ClearsOnlyNamedField() + { + var listId = await SeedListAsync(); + await _sut.SetListConfig(listId, "opus", "be terse", "agent.md", 30, cancellationToken: CancellationToken.None); + + var result = await _sut.SetListConfig(listId, clearFields: new[] { "model" }, cancellationToken: CancellationToken.None); + + Assert.Null(result.Config!.Model); + Assert.Equal("be terse", result.Config.SystemPrompt); + Assert.Equal("agent.md", result.Config.AgentPath); + Assert.Equal(30, result.Config.MaxTurns); + } + + [Fact] + public async Task SetListConfig_AllFieldsCleared_DeletesConfig() + { + var listId = await SeedListAsync(); + await _sut.SetListConfig(listId, "sonnet", null, null, null, cancellationToken: CancellationToken.None); + + var clearResult = await _sut.SetListConfig( + listId, clearFields: new[] { "model", "systemPrompt", "agentPath", "maxTurns" }, cancellationToken: CancellationToken.None); Assert.True(clearResult.Ok); Assert.Null(clearResult.Config); @@ -78,6 +107,15 @@ public sealed class ConfigMcpToolsTests : IDisposable Assert.False(cfg.Found); } + [Fact] + public async Task SetListConfig_UnknownClearField_Throws() + { + var listId = await SeedListAsync(); + + await Assert.ThrowsAsync(() => + _sut.SetListConfig(listId, clearFields: new[] { "bogus" }, cancellationToken: CancellationToken.None)); + } + [Fact] public async Task SetTaskConfig_PersistsOverrides() { @@ -93,7 +131,7 @@ public sealed class ConfigMcpToolsTests : IDisposable }; await _tasks.AddAsync(task); - var result = await _sut.SetTaskConfig(task.Id, "opus", null, null, 15, CancellationToken.None); + var result = await _sut.SetTaskConfig(task.Id, "opus", null, null, 15, cancellationToken: CancellationToken.None); Assert.True(result.Ok); Assert.Equal(task.Id, result.TaskId); @@ -106,7 +144,7 @@ public sealed class ConfigMcpToolsTests : IDisposable } [Fact] - public async Task SetTaskConfig_NullField_ClearsThatOverride() + public async Task SetTaskConfig_PartialUpdate_LeavesOtherOverridesUntouched() { var listId = await SeedListAsync(); var task = new TaskEntity @@ -119,19 +157,70 @@ public sealed class ConfigMcpToolsTests : IDisposable CommitType = "chore", }; await _tasks.AddAsync(task); - await _sut.SetTaskConfig(task.Id, "opus", "be terse", "agent.md", 15, CancellationToken.None); + await _sut.SetTaskConfig(task.Id, "opus", "be terse", "agent.md", 15, cancellationToken: CancellationToken.None); - var result = await _sut.SetTaskConfig(task.Id, "opus", null, "agent.md", null, CancellationToken.None); + var result = await _sut.SetTaskConfig(task.Id, maxTurns: 40, cancellationToken: CancellationToken.None); + + Assert.True(result.Ok); + Assert.Equal("opus", result.Config!.Model); + Assert.Equal("be terse", result.Config.SystemPrompt); + Assert.Equal("agent.md", result.Config.AgentPath); + Assert.Equal(40, result.Config.MaxTurns); + + var loaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal("opus", loaded!.Model); + Assert.Equal("be terse", loaded.SystemPrompt); + Assert.Equal(40, loaded.MaxTurns); + } + + [Fact] + public async Task SetTaskConfig_ClearFields_ClearsOnlyNamedOverride() + { + var listId = await SeedListAsync(); + var task = new TaskEntity + { + Id = Guid.NewGuid().ToString(), + ListId = listId, + Title = "t", + Status = ClaudeDo.Data.Models.TaskStatus.Idle, + CreatedAt = DateTime.UtcNow, + CommitType = "chore", + }; + await _tasks.AddAsync(task); + await _sut.SetTaskConfig(task.Id, "opus", "be terse", "agent.md", 15, cancellationToken: CancellationToken.None); + + var result = await _sut.SetTaskConfig(task.Id, clearFields: new[] { "systemPrompt" }, cancellationToken: CancellationToken.None); Assert.True(result.Ok); Assert.Equal("opus", result.Config!.Model); Assert.Null(result.Config.SystemPrompt); Assert.Equal("agent.md", result.Config.AgentPath); - Assert.Null(result.Config.MaxTurns); + Assert.Equal(15, result.Config.MaxTurns); var loaded = await _tasks.GetByIdAsync(task.Id); - Assert.Null(loaded!.SystemPrompt); - Assert.Null(loaded.MaxTurns); + Assert.Equal("opus", loaded!.Model); + Assert.Null(loaded.SystemPrompt); + Assert.Equal("agent.md", loaded.AgentPath); + Assert.Equal(15, loaded.MaxTurns); + } + + [Fact] + public async Task SetTaskConfig_UnknownClearField_Throws() + { + var listId = await SeedListAsync(); + var task = new TaskEntity + { + Id = Guid.NewGuid().ToString(), + ListId = listId, + Title = "t", + Status = ClaudeDo.Data.Models.TaskStatus.Idle, + CreatedAt = DateTime.UtcNow, + CommitType = "chore", + }; + await _tasks.AddAsync(task); + + await Assert.ThrowsAsync(() => + _sut.SetTaskConfig(task.Id, clearFields: new[] { "bogus" }, cancellationToken: CancellationToken.None)); } [Fact] diff --git a/tests/ClaudeDo.Worker.Tests/External/EffectiveRunConfigTests.cs b/tests/ClaudeDo.Worker.Tests/External/EffectiveRunConfigTests.cs index c31b0f20..1fc79581 100644 --- a/tests/ClaudeDo.Worker.Tests/External/EffectiveRunConfigTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/EffectiveRunConfigTests.cs @@ -81,7 +81,7 @@ public sealed class EffectiveRunConfigTests : IDisposable public async Task Task_override_beats_list_override_beats_preset() { var listId = await SeedListAsync(); - await _sut.SetListConfig(listId, "opus", null, "list-agent.md", 50, CancellationToken.None); + await _sut.SetListConfig(listId, "opus", null, "list-agent.md", 50, cancellationToken: CancellationToken.None); var task = await SeedTaskAsync(listId, t => { t.Model = "haiku"; t.MaxTurns = 12; }); var result = await _sut.GetEffectiveRunConfig(task.Id, CancellationToken.None); @@ -98,7 +98,7 @@ public sealed class EffectiveRunConfigTests : IDisposable public async Task List_override_wins_when_no_task_override() { var listId = await SeedListAsync(); - await _sut.SetListConfig(listId, "opus", null, null, 50, CancellationToken.None); + await _sut.SetListConfig(listId, "opus", null, null, 50, cancellationToken: CancellationToken.None); var task = await SeedTaskAsync(listId); var result = await _sut.GetEffectiveRunConfig(task.Id, CancellationToken.None);