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.
This commit is contained in:
+55
-16
@@ -55,33 +55,64 @@ public sealed class ConfigMcpTools
|
|||||||
: new TaskConfigResult(true, new TaskConfigDto(cfg.Model, cfg.SystemPrompt, cfg.AgentPath, cfg.MaxTurns));
|
: 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<string> NormalizeClearFields(IReadOnlyList<string>? clearFields)
|
||||||
|
{
|
||||||
|
var set = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
|
||||||
|
foreach (var field in clearFields ?? Array.Empty<string>())
|
||||||
|
{
|
||||||
|
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(
|
[McpServerTool, Description(
|
||||||
"Set a list's default model/system prompt/agent path/max turns — the fallback for tasks in this list " +
|
"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<SetListConfigResult> SetListConfig(
|
public async Task<SetListConfigResult> SetListConfig(
|
||||||
string listId, string? model = null, string? systemPrompt = null, string? agentPath = null,
|
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<string>? clearFields = null,
|
||||||
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
_ = await _lists.GetByIdAsync(listId, cancellationToken)
|
_ = await _lists.GetByIdAsync(listId, cancellationToken)
|
||||||
?? throw new InvalidOperationException($"List {listId} not found.");
|
?? throw new InvalidOperationException($"List {listId} not found.");
|
||||||
|
|
||||||
var m = model.NullIfBlank();
|
var clear = NormalizeClearFields(clearFields);
|
||||||
var sp = systemPrompt.NullIfBlank();
|
var existing = await _lists.GetConfigAsync(listId, cancellationToken);
|
||||||
var ap = agentPath.NullIfBlank();
|
|
||||||
|
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;
|
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);
|
await _lists.DeleteConfigAsync(listId, cancellationToken);
|
||||||
config = null;
|
config = null;
|
||||||
}
|
}
|
||||||
|
else if (m is null && sp is null && ap is null && mt is null && existing is null)
|
||||||
|
{
|
||||||
|
config = null;
|
||||||
|
}
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
await _lists.SetConfigAsync(new ListConfigEntity
|
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);
|
}, cancellationToken);
|
||||||
config = new TaskConfigDto(m, sp, ap, maxTurns);
|
config = new TaskConfigDto(m, sp, ap, mt);
|
||||||
}
|
}
|
||||||
|
|
||||||
await _broadcaster.ListUpdated(listId);
|
await _broadcaster.ListUpdated(listId);
|
||||||
@@ -90,21 +121,29 @@ public sealed class ConfigMcpTools
|
|||||||
|
|
||||||
[McpServerTool, Description(
|
[McpServerTool, Description(
|
||||||
"Set per-task overrides for model/system prompt/agent path/max turns; these take precedence over the " +
|
"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<SetTaskConfigResult> SetTaskConfig(
|
public async Task<SetTaskConfigResult> SetTaskConfig(
|
||||||
string taskId, string? model = null, string? systemPrompt = null, string? agentPath = null,
|
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<string>? clearFields = null,
|
||||||
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
_ = await _tasks.GetByIdAsync(taskId, cancellationToken)
|
var task = await _tasks.GetByIdAsync(taskId, cancellationToken)
|
||||||
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
||||||
|
|
||||||
var m = model.NullIfBlank();
|
var clear = NormalizeClearFields(clearFields);
|
||||||
var sp = systemPrompt.NullIfBlank();
|
|
||||||
var ap = agentPath.NullIfBlank();
|
|
||||||
|
|
||||||
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);
|
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.")]
|
[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.")]
|
||||||
|
|||||||
+100
-11
@@ -37,7 +37,7 @@ public sealed class ConfigMcpToolsTests : IDisposable
|
|||||||
{
|
{
|
||||||
var listId = await SeedListAsync();
|
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.True(setResult.Ok);
|
||||||
Assert.Equal(listId, setResult.ListId);
|
Assert.Equal(listId, setResult.ListId);
|
||||||
Assert.NotNull(setResult.Config);
|
Assert.NotNull(setResult.Config);
|
||||||
@@ -65,12 +65,41 @@ public sealed class ConfigMcpToolsTests : IDisposable
|
|||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task SetListConfig_AllNull_ClearsConfig()
|
public async Task SetListConfig_PartialUpdate_LeavesOtherFieldsUntouched()
|
||||||
{
|
{
|
||||||
var listId = await SeedListAsync();
|
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.True(clearResult.Ok);
|
||||||
Assert.Null(clearResult.Config);
|
Assert.Null(clearResult.Config);
|
||||||
@@ -78,6 +107,15 @@ public sealed class ConfigMcpToolsTests : IDisposable
|
|||||||
Assert.False(cfg.Found);
|
Assert.False(cfg.Found);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task SetListConfig_UnknownClearField_Throws()
|
||||||
|
{
|
||||||
|
var listId = await SeedListAsync();
|
||||||
|
|
||||||
|
await Assert.ThrowsAsync<InvalidOperationException>(() =>
|
||||||
|
_sut.SetListConfig(listId, clearFields: new[] { "bogus" }, cancellationToken: CancellationToken.None));
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task SetTaskConfig_PersistsOverrides()
|
public async Task SetTaskConfig_PersistsOverrides()
|
||||||
{
|
{
|
||||||
@@ -93,7 +131,7 @@ public sealed class ConfigMcpToolsTests : IDisposable
|
|||||||
};
|
};
|
||||||
await _tasks.AddAsync(task);
|
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.True(result.Ok);
|
||||||
Assert.Equal(task.Id, result.TaskId);
|
Assert.Equal(task.Id, result.TaskId);
|
||||||
@@ -106,7 +144,7 @@ public sealed class ConfigMcpToolsTests : IDisposable
|
|||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task SetTaskConfig_NullField_ClearsThatOverride()
|
public async Task SetTaskConfig_PartialUpdate_LeavesOtherOverridesUntouched()
|
||||||
{
|
{
|
||||||
var listId = await SeedListAsync();
|
var listId = await SeedListAsync();
|
||||||
var task = new TaskEntity
|
var task = new TaskEntity
|
||||||
@@ -119,19 +157,70 @@ public sealed class ConfigMcpToolsTests : IDisposable
|
|||||||
CommitType = "chore",
|
CommitType = "chore",
|
||||||
};
|
};
|
||||||
await _tasks.AddAsync(task);
|
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.True(result.Ok);
|
||||||
Assert.Equal("opus", result.Config!.Model);
|
Assert.Equal("opus", result.Config!.Model);
|
||||||
Assert.Null(result.Config.SystemPrompt);
|
Assert.Null(result.Config.SystemPrompt);
|
||||||
Assert.Equal("agent.md", result.Config.AgentPath);
|
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);
|
var loaded = await _tasks.GetByIdAsync(task.Id);
|
||||||
Assert.Null(loaded!.SystemPrompt);
|
Assert.Equal("opus", loaded!.Model);
|
||||||
Assert.Null(loaded.MaxTurns);
|
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<InvalidOperationException>(() =>
|
||||||
|
_sut.SetTaskConfig(task.Id, clearFields: new[] { "bogus" }, cancellationToken: CancellationToken.None));
|
||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
|
|||||||
@@ -81,7 +81,7 @@ public sealed class EffectiveRunConfigTests : IDisposable
|
|||||||
public async Task Task_override_beats_list_override_beats_preset()
|
public async Task Task_override_beats_list_override_beats_preset()
|
||||||
{
|
{
|
||||||
var listId = await SeedListAsync();
|
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 task = await SeedTaskAsync(listId, t => { t.Model = "haiku"; t.MaxTurns = 12; });
|
||||||
|
|
||||||
var result = await _sut.GetEffectiveRunConfig(task.Id, CancellationToken.None);
|
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()
|
public async Task List_override_wins_when_no_task_override()
|
||||||
{
|
{
|
||||||
var listId = await SeedListAsync();
|
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 task = await SeedTaskAsync(listId);
|
||||||
|
|
||||||
var result = await _sut.GetEffectiveRunConfig(task.Id, CancellationToken.None);
|
var result = await _sut.GetEffectiveRunConfig(task.Id, CancellationToken.None);
|
||||||
|
|||||||
Reference in New Issue
Block a user