From 5ca26142cc66c5b8119761e9a1bfcbae71b02a56 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 23 May 2026 21:11:57 +0000 Subject: [PATCH 1/4] feat(config): preserve operator overrides across model swaps via Models.Catalog (#1127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator overrides (ContextWindow, InputModalities, OutputModalities) hand-edited onto Models.Main / Fallback / Compaction used to get wiped by the next model picker swap, which only rewrites Provider/ModelId/ Provenance. Restoring those overrides meant re-editing netclaw.json every time. Adds an optional Models.Catalog map keyed by "{provider}/{modelId}" that holds per-model overrides separately from the role selection. On read, ModelSelection.ApplyCatalogOverlays folds matching catalog entries into role records, with inline values winning (most recent operator intent). On write (TUI picker + netclaw model set), the old role's override fields are promoted to the catalog before the new selection overwrites the inline record — so a subsequent switch back to that (provider, modelId) re-applies the saved values. The catalog is opt-in by design: auto-detected values (post-#987 fix landed in #1158) stay ephemeral, so a default install has zero catalog entries. The map only fills up when an operator has explicitly overridden something they want to survive across model changes. Wired through all five ModelSelection readers (daemon Program.cs x2, CLI Program.cs x2, ModelCommand.LoadModelSelection) and the IOptions binding via PostConfigure so the validator sees the effective view. Schema updated per the Configuration Schema Sync rule. 10 new tests cover overlay merge precedence, three-role coverage, no-op cases, and writer promotion (including JsonElement inputs from freshly-loaded configs). --- ...nfigFileHelperPromoteRoleOverridesTests.cs | 134 ++++++++++++++++++ src/Netclaw.Cli/Config/ConfigFileHelper.cs | 76 ++++++++++ src/Netclaw.Cli/Model/ModelCommand.cs | 21 ++- src/Netclaw.Cli/Program.cs | 2 + src/Netclaw.Cli/Tui/ModelManagerViewModel.cs | 5 + .../ModelSelectionCatalogOverlayTests.cs | 111 +++++++++++++++ src/Netclaw.Configuration/ModelOverride.cs | 26 ++++ src/Netclaw.Configuration/ModelSelection.cs | 46 +++++- .../Schemas/netclaw-config.v1.schema.json | 25 +++- src/Netclaw.Daemon/Program.cs | 6 + 10 files changed, 448 insertions(+), 4 deletions(-) create mode 100644 src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs create mode 100644 src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs create mode 100644 src/Netclaw.Configuration/ModelOverride.cs diff --git a/src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs b/src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs new file mode 100644 index 000000000..df35a3e7a --- /dev/null +++ b/src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs @@ -0,0 +1,134 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Text.Json; +using Netclaw.Cli.Config; +using Xunit; + +namespace Netclaw.Cli.Tests.Daemon; + +/// +/// Locks in the writer-side half of the #1127 contract: before overwriting a +/// role on a model swap, any operator overrides on the displaced role are +/// promoted to Models.Catalog["{oldProvider}/{oldModelId}"]. This is +/// what makes hand-edited ContextWindow / InputModalities / +/// OutputModalities survive a picker-driven model change. +/// +public sealed class ConfigFileHelperPromoteRoleOverridesTests +{ + [Fact] + public void Promote_MovesContextWindow_FromRole_ToCatalog() + { + var modelsSection = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "p", + ["ModelId"] = "m", + ["ContextWindow"] = 200_000L, + }, + }; + + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); + + var catalog = (Dictionary)modelsSection["Catalog"]; + var entry = (Dictionary)catalog["p/m"]; + Assert.Equal(200_000L, Assert.IsType(entry["ContextWindow"])); + } + + [Fact] + public void Promote_IsNoOp_WhenRoleHasNoOverrideFields() + { + // The common case: most operators rely on auto-detection and never + // hand-set ContextWindow / Modality fields. Promote must not seed an + // empty Catalog entry in that case. + var modelsSection = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "p", + ["ModelId"] = "m", + ["Provenance"] = "Live", + }, + }; + + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); + + Assert.False(modelsSection.ContainsKey("Catalog")); + } + + [Fact] + public void Promote_InlineWins_WhenCatalogEntryAlreadyExists() + { + // Inline value represents the most recent operator intent — it should + // overwrite a stale catalog entry from a previous switch. + var modelsSection = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "p", + ["ModelId"] = "m", + ["ContextWindow"] = 100L, + }, + ["Catalog"] = new Dictionary + { + ["p/m"] = new Dictionary + { + ["ContextWindow"] = 50L, + }, + }, + }; + + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); + + var catalog = (Dictionary)modelsSection["Catalog"]; + var entry = (Dictionary)catalog["p/m"]; + Assert.Equal(100L, Assert.IsType(entry["ContextWindow"])); + } + + [Fact] + public void Promote_HandlesJsonElement_FromFreshlyLoadedConfig() + { + // Simulates the path where ConfigFileHelper.LoadConfigFiles returned a + // dictionary whose nested values are JsonElement (un-materialized). + var json = """ + { + "Main": { "Provider": "p", "ModelId": "m", "InputModalities": "Text, Image" } + } + """; + var modelsSection = JsonSerializer.Deserialize>(json)!; + + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); + + var catalog = (Dictionary)modelsSection["Catalog"]; + var entry = (Dictionary)catalog["p/m"]; + Assert.Equal("Text, Image", Assert.IsType(entry["InputModalities"])); + } + + [Fact] + public void Promote_LeavesIdentityFields_OnOldRole() + { + // The role record itself isn't deleted by Promote — it's about to be + // overwritten by the caller. Verify Promote doesn't mutate the role's + // Provider/ModelId/Provenance, only reads them. + var modelsSection = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "p", + ["ModelId"] = "m", + ["Provenance"] = "Live", + ["ContextWindow"] = 1234L, + }, + }; + + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); + + var main = (Dictionary)modelsSection["Main"]; + Assert.Equal("p", main["Provider"]); + Assert.Equal("m", main["ModelId"]); + Assert.Equal("Live", main["Provenance"]); + } +} diff --git a/src/Netclaw.Cli/Config/ConfigFileHelper.cs b/src/Netclaw.Cli/Config/ConfigFileHelper.cs index 9cb7d36eb..50b5d0c73 100644 --- a/src/Netclaw.Cli/Config/ConfigFileHelper.cs +++ b/src/Netclaw.Cli/Config/ConfigFileHelper.cs @@ -121,4 +121,80 @@ internal static string DecryptIfEncrypted(Configuration.NetclawPaths paths, stri var protector = SecretsProtection.CreateProtector(paths); return protector.Unprotect(value); } + + /// + /// Override fields that survive a model swap: when the operator changes a + /// role's (Provider, ModelId), any of these fields present on the old role + /// record are moved into Models.Catalog["{provider}/{modelId}"] so + /// they re-apply if the operator switches back. Provider/ModelId/Provenance + /// are NOT overrides — they identify which model is selected, not how it + /// behaves. + /// + private static readonly string[] OverrideFieldNames = + ["ContextWindow", "InputModalities", "OutputModalities"]; + + /// + /// Before overwriting Models[roleKey] with a new selection, move + /// any override fields on the OLD role record into + /// Models.Catalog["{oldProvider}/{oldModelId}"]. Inline values from + /// the old role win over any existing catalog entry, because they + /// represent the most recent operator intent. No-op when the old role + /// record carries no override fields (the common case — most operators + /// rely on auto-detection). + /// + internal static void PromoteRoleOverridesToCatalog( + Dictionary modelsSection, string roleKey) + { + var oldRole = GetSectionOrNull(modelsSection, roleKey); + if (oldRole is null) + return; + + var oldProvider = TryGetString(oldRole, "Provider"); + var oldModelId = TryGetString(oldRole, "ModelId"); + if (string.IsNullOrEmpty(oldProvider) || string.IsNullOrEmpty(oldModelId)) + return; + + Dictionary? overrides = null; + foreach (var name in OverrideFieldNames) + { + if (!oldRole.TryGetValue(name, out var value) || value is null) + continue; + overrides ??= new Dictionary(); + overrides[name] = UnwrapJsonElement(value); + } + + if (overrides is null) + return; + + var catalog = GetOrCreateSection(modelsSection, "Catalog"); + var key = Configuration.ModelSelection.CatalogKey(oldProvider, oldModelId); + var entry = GetOrCreateSection(catalog, key); + foreach (var kvp in overrides) + entry[kvp.Key] = kvp.Value; + } + + private static string? TryGetString(Dictionary dict, string key) + { + if (!dict.TryGetValue(key, out var raw) || raw is null) + return null; + return raw switch + { + string s => s, + JsonElement je when je.ValueKind == JsonValueKind.String => je.GetString(), + _ => null, + }; + } + + private static object UnwrapJsonElement(object value) => value switch + { + JsonElement je => je.ValueKind switch + { + JsonValueKind.String => (object)(je.GetString() ?? string.Empty), + JsonValueKind.Number => je.TryGetInt64(out var l) ? l : je.GetDouble(), + JsonValueKind.True => true, + JsonValueKind.False => false, + _ => je.GetRawText(), + }, + _ => value, + }; } diff --git a/src/Netclaw.Cli/Model/ModelCommand.cs b/src/Netclaw.Cli/Model/ModelCommand.cs index fd12da153..91d7f65c5 100644 --- a/src/Netclaw.Cli/Model/ModelCommand.cs +++ b/src/Netclaw.Cli/Model/ModelCommand.cs @@ -142,6 +142,10 @@ private static int RunSet(string[] args, NetclawPaths paths, TextWriter writer) var (config, _) = ConfigFileHelper.LoadConfigFiles(paths); var modelsSection = ConfigFileHelper.GetOrCreateSection(config, "Models"); + // Preserve any operator overrides on the role being overwritten so + // switching back to the displaced (provider, modelId) re-applies them. + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); + var modelEntry = new Dictionary { ["Provider"] = providerName, @@ -149,8 +153,18 @@ private static int RunSet(string[] args, NetclawPaths paths, TextWriter writer) ["Provenance"] = ModelDiscoverySource.Manual.ToString() }; + // --context-window is an explicit operator override — persist it to + // the catalog (keyed by the new selection's identity) so it survives + // a subsequent picker swap. Inline on the role is also written so the + // shape stays consistent with hand-edited configs. if (contextWindow.HasValue) + { modelEntry["ContextWindow"] = contextWindow.Value; + var catalog = ConfigFileHelper.GetOrCreateSection(modelsSection, "Catalog"); + var entry = ConfigFileHelper.GetOrCreateSection( + catalog, ModelSelection.CatalogKey(providerName, modelId)); + entry["ContextWindow"] = contextWindow.Value; + } modelsSection[roleKey] = modelEntry; ConfigFileHelper.WriteConfigFile(paths.NetclawConfigPath, config); @@ -267,7 +281,8 @@ private static int RunClear(string[] args, NetclawPaths paths, TextWriter writer } /// - /// Load model selection from config file. + /// Load model selection from config file. Applies catalog overlays so + /// callers see the effective per-role view, not the raw on-disk shape. /// internal static ModelSelection? LoadModelSelection(NetclawPaths paths) { @@ -278,7 +293,9 @@ private static int RunClear(string[] args, NetclawPaths paths, TextWriter writer if (!doc.RootElement.TryGetProperty("Models", out var modelsElement)) return null; - return JsonSerializer.Deserialize(modelsElement.GetRawText(), JsonDefaults.EnumAware); + var models = JsonSerializer.Deserialize(modelsElement.GetRawText(), JsonDefaults.EnumAware); + models?.ApplyCatalogOverlays(); + return models; } private static int WriteHelp(TextWriter writer) diff --git a/src/Netclaw.Cli/Program.cs b/src/Netclaw.Cli/Program.cs index 9a22c1c55..3eba741df 100644 --- a/src/Netclaw.Cli/Program.cs +++ b/src/Netclaw.Cli/Program.cs @@ -187,6 +187,7 @@ static async Task RunAsync(string[] args) var models = initConfig.GetSection("Models") .Get() ?? new ModelSelection(); + models.ApplyCatalogOverlays(); var contextWindow = ContextWindowResolution.ResolveAsync( models.Main.ContextWindow, @@ -1802,6 +1803,7 @@ static void ConfigureCliChatServices(IServiceCollection services, IConfiguration // Resolve models for session config var models = configuration.GetSection("Models") .Get() ?? new ModelSelection(); + models.ApplyCatalogOverlays(); // Session config: bind operator-facing settings var sessionConfig = SessionConfig.BindFromConfiguration(configuration.GetSection("Session")); diff --git a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs index 0c2ee64f8..ac1d28f53 100644 --- a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs +++ b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs @@ -171,6 +171,11 @@ public void ConfirmAssignment() var (config, _) = ConfigFileHelper.LoadConfigFiles(_paths); var modelsSection = ConfigFileHelper.GetOrCreateSection(config, "Models"); + // Preserve any operator overrides (ContextWindow / modality forcing) + // on the role being overwritten — they survive in the catalog so + // switching back to the same (provider, modelId) re-applies them. + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); + var modelEntry = new Dictionary { ["Provider"] = SelectedProvider, diff --git a/src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs b/src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs new file mode 100644 index 000000000..a42dd9dd8 --- /dev/null +++ b/src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs @@ -0,0 +1,111 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Microsoft.Extensions.Configuration; +using Xunit; + +namespace Netclaw.Configuration.Tests; + +/// +/// Locks in the contract for #1127: operator overrides persisted in +/// overlay onto matching role records +/// (Main / Fallback / Compaction) when +/// runs, with inline values winning over the catalog entry. +/// +public sealed class ModelSelectionCatalogOverlayTests +{ + [Fact] + public void CatalogOverlay_AppliesContextWindow_WhenRoleHasNoInlineValue() + { + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Models:Main:Provider"] = "spark-362c", + ["Models:Main:ModelId"] = "Qwen/Qwen3.6-35B-A3B-FP8", + ["Models:Catalog:spark-362c/Qwen/Qwen3.6-35B-A3B-FP8:ContextWindow"] = "200000", + }) + .Build(); + + var selection = config.GetSection("Models").Get()!; + selection.ApplyCatalogOverlays(); + + Assert.Equal(200_000, selection.Main.ContextWindow); + } + + [Fact] + public void InlineRoleValue_WinsOver_CatalogOverlay() + { + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Models:Main:Provider"] = "p", + ["Models:Main:ModelId"] = "m", + ["Models:Main:ContextWindow"] = "1000", + ["Models:Catalog:p/m:ContextWindow"] = "9999", + }) + .Build(); + + var selection = config.GetSection("Models").Get()!; + selection.ApplyCatalogOverlays(); + + Assert.Equal(1000, selection.Main.ContextWindow); + } + + [Fact] + public void CatalogOverlay_AppliesIndependentlyTo_AllThreeRoles() + { + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Models:Main:Provider"] = "p", + ["Models:Main:ModelId"] = "main-model", + ["Models:Fallback:Provider"] = "p", + ["Models:Fallback:ModelId"] = "fallback-model", + ["Models:Compaction:Provider"] = "p", + ["Models:Compaction:ModelId"] = "compaction-model", + ["Models:Catalog:p/main-model:InputModalities"] = "Text, Image", + ["Models:Catalog:p/fallback-model:ContextWindow"] = "65536", + ["Models:Catalog:p/compaction-model:OutputModalities"] = "Text", + }) + .Build(); + + var selection = config.GetSection("Models").Get()!; + selection.ApplyCatalogOverlays(); + + Assert.Equal(ModelModality.Text | ModelModality.Image, selection.Main.InputModalities); + Assert.Equal(65_536, selection.Fallback!.ContextWindow); + Assert.Equal(ModelModality.Text, selection.Compaction!.OutputModalities); + } + + [Fact] + public void CatalogEntry_WithoutMatchingRole_IsIgnored() + { + // Operator switched Main from "old-model" to "new-model"; the old + // model's overrides remain in the catalog. They should NOT leak onto + // the unrelated new selection. + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Models:Main:Provider"] = "p", + ["Models:Main:ModelId"] = "new-model", + ["Models:Catalog:p/old-model:ContextWindow"] = "12345", + }) + .Build(); + + var selection = config.GetSection("Models").Get()!; + selection.ApplyCatalogOverlays(); + + Assert.Null(selection.Main.ContextWindow); + } + + [Fact] + public void ApplyCatalogOverlays_IsNoOp_WhenCatalogAbsent() + { + var selection = new ModelSelection { Main = new ModelReference { Provider = "p", ModelId = "m" } }; + selection.ApplyCatalogOverlays(); // does not throw, leaves fields null + Assert.Null(selection.Main.ContextWindow); + Assert.Null(selection.Main.InputModalities); + } +} diff --git a/src/Netclaw.Configuration/ModelOverride.cs b/src/Netclaw.Configuration/ModelOverride.cs new file mode 100644 index 000000000..514f96da2 --- /dev/null +++ b/src/Netclaw.Configuration/ModelOverride.cs @@ -0,0 +1,26 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +namespace Netclaw.Configuration; + +/// +/// Persisted operator overrides for a single (provider, modelId) pair. +/// Lives in ; merged into matching +/// role records at read time. Only fields the operator has explicitly +/// customized are stored — auto-detected values stay ephemeral and are +/// re-resolved at every daemon startup, so a default install has no +/// catalog entries at all. +/// +public sealed class ModelOverride +{ + /// Clamps the runtime session budget; same semantics as . + public int? ContextWindow { get; set; } + + /// Manual input-modality override; same semantics as . + public ModelModality? InputModalities { get; set; } + + /// Manual output-modality override; same semantics as . + public ModelModality? OutputModalities { get; set; } +} diff --git a/src/Netclaw.Configuration/ModelSelection.cs b/src/Netclaw.Configuration/ModelSelection.cs index f39ede885..63fc493f2 100644 --- a/src/Netclaw.Configuration/ModelSelection.cs +++ b/src/Netclaw.Configuration/ModelSelection.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -20,4 +20,48 @@ public sealed class ModelSelection /// Cheaper/faster model for compaction. Falls back to Main if not set. public ModelReference? Compaction { get; set; } + + /// + /// Optional map of operator overrides keyed by "{provider}/{modelId}". + /// Merged into matching role records by ; + /// inline values on a role win over the catalog entry. Typically empty — + /// auto-detection covers most operators' setups and only persists when an + /// override is explicitly set (e.g. a context-window cap or a forced + /// modality). + /// + public Dictionary? Catalog { get; set; } + + /// + /// Builds the catalog key for a (provider, modelId) pair. Provider keys + /// are user-defined identifiers and conventionally slash-free; model ids + /// may contain slashes (e.g. HF-shaped org/model), so the first + /// slash is the only meaningful delimiter. + /// + public static string CatalogKey(string provider, string modelId) => $"{provider}/{modelId}"; + + /// + /// Folds entries into matching role records, + /// preserving inline values (inline wins). Idempotent: a second call is + /// a no-op because inline already mirrors the catalog after the first + /// merge. Safe to call when is null. + /// + public void ApplyCatalogOverlays() + { + if (Catalog is null || Catalog.Count == 0) + return; + + Merge(Main); + if (Fallback is not null) Merge(Fallback); + if (Compaction is not null) Merge(Compaction); + } + + private void Merge(ModelReference role) + { + if (!Catalog!.TryGetValue(CatalogKey(role.Provider, role.ModelId), out var overlay)) + return; + + role.ContextWindow ??= overlay.ContextWindow; + role.InputModalities ??= overlay.InputModalities; + role.OutputModalities ??= overlay.OutputModalities; + } } diff --git a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json index 975cfa9d7..be0875492 100644 --- a/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json +++ b/src/Netclaw.Configuration/Schemas/netclaw-config.v1.schema.json @@ -335,7 +335,12 @@ "properties": { "Main": { "$ref": "#/$defs/ModelReference" }, "Fallback": { "$ref": "#/$defs/ModelReference" }, - "Compaction": { "$ref": "#/$defs/ModelReference" } + "Compaction": { "$ref": "#/$defs/ModelReference" }, + "Catalog": { + "type": "object", + "description": "Optional operator overrides keyed by \"{provider}/{modelId}\". Merged into matching role records at read time; inline values on a role win. Typically empty — present only when an operator has hand-set a value (e.g. a context-window cap) they want to survive model-picker swaps.", + "additionalProperties": { "$ref": "#/$defs/ModelOverride" } + } }, "additionalProperties": false }, @@ -854,6 +859,24 @@ }, "additionalProperties": false }, + "ModelOverride": { + "type": "object", + "description": "Persisted operator override for a single (provider, modelId). Subset of ModelReference fields: identity is encoded in the catalog key.", + "properties": { + "ContextWindow": { "type": "integer", "description": "Effective runtime context window in tokens. When set, clamps the detected provider value." }, + "InputModalities": { + "type": "string", + "description": "Manual override for input modalities. Comma-separated ModelModality flags.", + "pattern": "^(Text|Image|Audio|Video)(\\s*,\\s*(Text|Image|Audio|Video))*$" + }, + "OutputModalities": { + "type": "string", + "description": "Manual override for output modalities. Comma-separated ModelModality flags.", + "pattern": "^(Text|Image|Audio|Video)(\\s*,\\s*(Text|Image|Audio|Video))*$" + } + }, + "additionalProperties": false + }, "NotificationTarget": { "type": "object", "properties": { diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 2bc907cc4..0c4efcad4 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -331,6 +331,7 @@ static NetclawPaths ConfigureConfigServices(IServiceCollection services, IConfig ?? new() { ["local-ollama"] = new ProviderEntry() }; var models = configuration.GetSection("Models") .Get() ?? new ModelSelection(); + models.ApplyCatalogOverlays(); services.AddDaemonLlmProviders(providers, models); @@ -359,6 +360,10 @@ static void ConfigureDaemonServices( services .AddOptions() .Bind(configuration.GetSection("Models")) + // Fold catalog overlays into role records before validation runs, so + // a ContextWindow set only via Catalog still gets checked by + // ModelSelectionValidator's MinContextWindow gate. + .PostConfigure(models => models.ApplyCatalogOverlays()) .ValidateOnStart(); services.AddSingleton, ModelSelectionValidator>(); services @@ -378,6 +383,7 @@ static void ConfigureDaemonServices( // Resolve models for session config var models = configuration.GetSection("Models") .Get() ?? new ModelSelection(); + models.ApplyCatalogOverlays(); services.AddSingleton(models); // Auto-detect model capabilities via the runtime IModelCapabilityResolver From f48c134630b6593de0fd7a3ff635ed645f6a781a Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 23 May 2026 21:36:04 +0000 Subject: [PATCH 2/4] fix(config): address C1-C4 review findings on Models.Catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C1 — model set without --context-window now drops the override: RunSetAsync now skips PromoteRoleOverridesToCatalog when the operator re-runs `model set ROLE P M` against the same (provider, modelId) it already had. Promote would otherwise copy the inline override into the catalog and ApplyCatalogOverlays would silently restore it on the next read, leaving operators no CLI affordance to clear an override. The redundant explicit catalog write on --context-window is dropped — the inline value is enough; cross-model survival still works because the NEXT identity change promotes the inline value into the catalog automatically. C2 — ContextWindowDoctorCheck now sees catalog overlays: The doctor was reading Models.Main.ContextWindow directly from the JSON AST and ignoring Models.Catalog. An operator with an override that lived only in the catalog saw the doctor fall through to auto-detection and print a misleading "set Models.Main.ContextWindow to pin a specific value" tip. The check now mirrors ApplyCatalogOverlays' inline-wins precedence so doctor output stays consistent with the daemon's effective view. C3 — provider rename rewrites Catalog keys: ProviderRenamer.CascadeRenameModelRoles only touched Models.{Main,Fallback,Compaction}.Provider; catalog keys (which embed the provider name as the first segment of {provider}/{modelId}) were left pointing at the old name and silently orphaned. The cascade now also rewrites every Catalog key whose provider segment matches oldName (case-insensitive, mirroring the role cascade). C4 — clearing a role preserves its overrides via the catalog: Both ModelManagerViewModel.ClearRole and ModelCommand.RunClear now call PromoteRoleOverridesToCatalog before deleting the role record, matching the contract ConfirmAssignment / set already follow. Re-setting the same (provider, modelId) later restores the saved overrides via the catalog overlay. Helpers: TryGetString is now internal as TryReadString, and a new TryReadRoleIdentity helper centralizes the identity-equality check. Six new tests cover the regression cases end-to-end. --- .../Doctor/ContextWindowDoctorCheckTests.cs | 54 ++++++++++ .../Model/ModelCommandTests.cs | 99 +++++++++++++++++++ .../Provider/ProviderRenamerTests.cs | 44 +++++++++ src/Netclaw.Cli/Config/ConfigFileHelper.cs | 29 +++++- .../Doctor/ContextWindowDoctorCheck.cs | 24 ++++- src/Netclaw.Cli/Model/ModelCommand.cs | 42 +++++--- src/Netclaw.Cli/Provider/ProviderRenamer.cs | 43 ++++++++ src/Netclaw.Cli/Tui/ModelManagerViewModel.cs | 10 +- 8 files changed, 324 insertions(+), 21 deletions(-) diff --git a/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs b/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs index cdf89e8f8..26c6c9f2f 100644 --- a/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs +++ b/src/Netclaw.Cli.Tests/Doctor/ContextWindowDoctorCheckTests.cs @@ -159,6 +159,60 @@ public async Task NoExplicitContextWindow_DaemonOffline_ProviderReturnsNull_Retu Assert.Contains("provider returned no context window", result.Message); } + [Fact] + public async Task CatalogOverlay_ContextWindow_Passes() + { + // C2 regression: pre-fix the doctor only read Models.Main.ContextWindow + // directly from JSON, ignoring the catalog overlay that the daemon + // honors. An operator with Catalog['p/m'].ContextWindow set (and no + // inline value) saw the doctor fall through to auto-detection and + // print a misleading "set Models.Main.ContextWindow to pin a + // specific value" tip — for a value they had already pinned. + WriteConfig(new + { + configVersion = 1, + Models = new + { + Main = new { Provider = "p", ModelId = "m" }, + Catalog = new Dictionary + { + ["p/m"] = new { ContextWindow = 200000 } + } + } + }); + var check = CreateCheck(CreateOfflineDaemonApi()); + + var result = await check.RunAsync(TestContext.Current.CancellationToken); + + Assert.Equal(DoctorSeverity.Pass, result.Severity); + Assert.Contains("200,000", result.Message); + } + + [Fact] + public async Task InlineContextWindow_WinsOver_CatalogOverlay_InDoctor() + { + // Mirrors ApplyCatalogOverlays' inline-wins semantics so the doctor's + // diagnostic always matches what the daemon will actually use. + WriteConfig(new + { + configVersion = 1, + Models = new + { + Main = new { Provider = "p", ModelId = "m", ContextWindow = 100000 }, + Catalog = new Dictionary + { + ["p/m"] = new { ContextWindow = 200000 } + } + } + }); + var check = CreateCheck(CreateOfflineDaemonApi()); + + var result = await check.RunAsync(TestContext.Current.CancellationToken); + + Assert.Equal(DoctorSeverity.Pass, result.Severity); + Assert.Contains("100,000", result.Message); + } + // ── Helpers ────────────────────────────────────────────────────────── private ContextWindowDoctorCheck CreateCheck( diff --git a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs index 2d159bfa8..9cb187015 100644 --- a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs @@ -278,6 +278,105 @@ public async Task Set_FallbackModel_WritesCorrectRole() Assert.Equal("qwen3:8b", fallback.GetProperty("ModelId").GetString()); } + [Fact] + public async Task Set_SameIdentity_WithoutContextWindowFlag_DropsOverride() + { + // C1 regression: pre-fix, re-running `model set` for the same + // (provider, modelId) without --context-window silently restored + // the previously-set ContextWindow via the catalog overlay. The + // operator had no CLI affordance to drop the override. + WriteConfig(new Dictionary + { + ["configVersion"] = 1, + ["Providers"] = new Dictionary + { + ["p"] = new Dictionary { ["Type"] = "ollama" } + } + }); + + await ModelCommand.RunAsync( + ["model", "set", "main", "p", "m", "--context-window", "200000"], + _paths, output: _output); + + await ModelCommand.RunAsync( + ["model", "set", "main", "p", "m"], + _paths, output: _output); + + var selection = ModelCommand.LoadModelSelection(_paths)!; + Assert.Null(selection.Main.ContextWindow); + } + + [Fact] + public async Task Set_IdentityChange_PromotesPriorOverrideToCatalog() + { + // C1 sibling: changing the model preserves the displaced role's + // overrides — switching back later re-applies them via the catalog + // overlay. This is the cross-model survival contract. + WriteConfig(new Dictionary + { + ["configVersion"] = 1, + ["Providers"] = new Dictionary + { + ["p"] = new Dictionary { ["Type"] = "ollama" } + } + }); + + await ModelCommand.RunAsync( + ["model", "set", "main", "p", "m1", "--context-window", "200000"], + _paths, output: _output); + await ModelCommand.RunAsync( + ["model", "set", "main", "p", "m2"], + _paths, output: _output); + await ModelCommand.RunAsync( + ["model", "set", "main", "p", "m1"], + _paths, output: _output); + + var selection = ModelCommand.LoadModelSelection(_paths)!; + Assert.Equal(200_000, selection.Main.ContextWindow); + } + + [Fact] + public async Task Clear_PreservesOverridesToCatalog() + { + // C4 regression (CLI side): clearing a role previously deleted any + // inline ContextWindow / modality overrides without preserving + // them. Re-setting the same (provider, modelId) later must restore + // them via the catalog overlay. + WriteConfig(new Dictionary + { + ["configVersion"] = 1, + ["Providers"] = new Dictionary + { + ["p"] = new Dictionary { ["Type"] = "ollama" } + }, + ["Models"] = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "p", + ["ModelId"] = "main-model" + }, + ["Fallback"] = new Dictionary + { + ["Provider"] = "p", + ["ModelId"] = "fallback-model", + ["ContextWindow"] = 65536L + } + } + }); + + await ModelCommand.RunAsync( + ["model", "clear", "fallback"], + _paths, output: _output); + + await ModelCommand.RunAsync( + ["model", "set", "fallback", "p", "fallback-model"], + _paths, output: _output); + + var selection = ModelCommand.LoadModelSelection(_paths)!; + Assert.Equal(65_536, selection.Fallback!.ContextWindow); + } + private void WriteConfig(Dictionary data) { File.WriteAllText(_paths.NetclawConfigPath, diff --git a/src/Netclaw.Cli.Tests/Provider/ProviderRenamerTests.cs b/src/Netclaw.Cli.Tests/Provider/ProviderRenamerTests.cs index a1bd61020..6a6d46d44 100644 --- a/src/Netclaw.Cli.Tests/Provider/ProviderRenamerTests.cs +++ b/src/Netclaw.Cli.Tests/Provider/ProviderRenamerTests.cs @@ -295,6 +295,50 @@ public void Rename_TrimsWhitespaceOnNewName() Assert.True(providers.TryGetProperty("lab-a100", out _)); } + [Fact] + public void Rename_RewritesCatalogKeys_KeyedByOldProvider() + { + // C3 regression: pre-fix, Models.Catalog kept its keys keyed by the + // OLD provider name after a rename. The renamed role's + // ApplyCatalogOverlays lookup misses, and the operator's saved + // override silently disappears. + WriteConfig(new Dictionary + { + ["configVersion"] = 1, + ["Providers"] = new Dictionary + { + ["old-p"] = new Dictionary { ["Type"] = "openai-compatible" } + }, + ["Models"] = new Dictionary + { + ["Main"] = new Dictionary + { + ["Provider"] = "old-p", + ["ModelId"] = "m" + }, + ["Catalog"] = new Dictionary + { + ["old-p/m"] = new Dictionary { ["ContextWindow"] = 200000L }, + ["unrelated/k"] = new Dictionary { ["ContextWindow"] = 50000L } + } + } + }); + + var result = ProviderRenamer.Rename(_paths, "old-p", "new-p"); + + Assert.True(result.Success); + + using var config = JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)); + var catalog = config.RootElement.GetProperty("Models").GetProperty("Catalog"); + Assert.False(catalog.TryGetProperty("old-p/m", out _), + "old key should have been rewritten"); + Assert.True(catalog.TryGetProperty("new-p/m", out var renamedEntry), + "renamed key should be present"); + Assert.Equal(200_000, renamedEntry.GetProperty("ContextWindow").GetInt32()); + Assert.True(catalog.TryGetProperty("unrelated/k", out _), + "catalog entries for other providers should be untouched"); + } + private void WriteConfig(Dictionary data) { File.WriteAllText(_paths.NetclawConfigPath, diff --git a/src/Netclaw.Cli/Config/ConfigFileHelper.cs b/src/Netclaw.Cli/Config/ConfigFileHelper.cs index 50b5d0c73..8eafe2b6f 100644 --- a/src/Netclaw.Cli/Config/ConfigFileHelper.cs +++ b/src/Netclaw.Cli/Config/ConfigFileHelper.cs @@ -149,8 +149,8 @@ internal static void PromoteRoleOverridesToCatalog( if (oldRole is null) return; - var oldProvider = TryGetString(oldRole, "Provider"); - var oldModelId = TryGetString(oldRole, "ModelId"); + var oldProvider = TryReadString(oldRole, "Provider"); + var oldModelId = TryReadString(oldRole, "ModelId"); if (string.IsNullOrEmpty(oldProvider) || string.IsNullOrEmpty(oldModelId)) return; @@ -173,7 +173,30 @@ internal static void PromoteRoleOverridesToCatalog( entry[kvp.Key] = kvp.Value; } - private static string? TryGetString(Dictionary dict, string key) + /// + /// Returns the (Provider, ModelId) currently recorded at + /// modelsSection[roleKey], or null when the role is absent or + /// missing either identity field. Used by writers to detect identity + /// changes vs same-identity re-runs. + /// + internal static (string Provider, string ModelId)? TryReadRoleIdentity( + Dictionary modelsSection, string roleKey) + { + var role = GetSectionOrNull(modelsSection, roleKey); + if (role is null) return null; + var provider = TryReadString(role, "Provider"); + var modelId = TryReadString(role, "ModelId"); + return string.IsNullOrEmpty(provider) || string.IsNullOrEmpty(modelId) + ? null + : (provider, modelId); + } + + /// + /// Best-effort string read from a dictionary that may carry raw strings + /// (re-materialized values) or s (freshly loaded + /// from disk). Returns null for missing, null, or non-string values. + /// + internal static string? TryReadString(Dictionary dict, string key) { if (!dict.TryGetValue(key, out var raw) || raw is null) return null; diff --git a/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs b/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs index ceeb040a6..18f46562d 100644 --- a/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs +++ b/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs @@ -50,13 +50,20 @@ public async Task RunAsync(CancellationToken cancellationToke "Add a Models.Main section with ContextWindow to netclaw.json."); } + var modelId = main["ModelId"]?.GetValue() ?? "unknown"; + var providerName = main["Provider"]?.GetValue() ?? "local-ollama"; + + // Effective ContextWindow follows ModelSelection.ApplyCatalogOverlays: + // inline on the role wins; otherwise fall back to the catalog + // overlay keyed by "{provider}/{modelId}". The runtime daemon uses + // exactly this precedence, so the doctor must match it or it will + // report "no explicit setting" while the daemon honors an override. var contextWindow = main["ContextWindow"]; + if (contextWindow is null && models is not null) + contextWindow = TryReadCatalogContextWindow(models, providerName, modelId); + if (contextWindow is null) - { - var modelId = main["ModelId"]?.GetValue() ?? "unknown"; - var providerName = main["Provider"]?.GetValue() ?? "local-ollama"; return await ResolveEffectiveContextWindowAsync(modelId, providerName, cancellationToken); - } if (contextWindow.GetValue() is var cw and > 0) { @@ -71,6 +78,15 @@ public async Task RunAsync(CancellationToken cancellationToke "Set Models.Main.ContextWindow to the effective runtime context window size in tokens."); } + private static JsonNode? TryReadCatalogContextWindow( + JsonObject models, string providerName, string modelId) + { + var catalog = models["Catalog"] as JsonObject; + if (catalog is null) return null; + var entry = catalog[ModelSelection.CatalogKey(providerName, modelId)] as JsonObject; + return entry?["ContextWindow"]; + } + private async Task ResolveEffectiveContextWindowAsync( string modelId, string providerName, CancellationToken ct) { diff --git a/src/Netclaw.Cli/Model/ModelCommand.cs b/src/Netclaw.Cli/Model/ModelCommand.cs index 91d7f65c5..dcfa4b115 100644 --- a/src/Netclaw.Cli/Model/ModelCommand.cs +++ b/src/Netclaw.Cli/Model/ModelCommand.cs @@ -142,9 +142,30 @@ private static int RunSet(string[] args, NetclawPaths paths, TextWriter writer) var (config, _) = ConfigFileHelper.LoadConfigFiles(paths); var modelsSection = ConfigFileHelper.GetOrCreateSection(config, "Models"); - // Preserve any operator overrides on the role being overwritten so - // switching back to the displaced (provider, modelId) re-applies them. - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); + // CLI write semantics differ from the TUI picker: + // + // identity changed → Promote the displaced role's overrides into + // the catalog so a later switch back restores + // them. Cross-model survival. + // + // identity unchanged → DO NOT Promote. The operator running + // `model set ROLE P M` (without --context-window) + // is signaling "start fresh for this model"; + // promoting the inline override into the catalog + // would have ApplyCatalogOverlays silently + // restore it on the next read, leaving the + // operator no CLI affordance to clear it. + // + // Cross-model survival for explicit --context-window writes still + // works: the value lives inline on the role and the next identity + // change promotes it to the catalog automatically. + var existingIdentity = ConfigFileHelper.TryReadRoleIdentity(modelsSection, roleKey); + var identityChanged = existingIdentity is null + || existingIdentity.Value.Provider != providerName + || existingIdentity.Value.ModelId != modelId; + + if (identityChanged) + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); var modelEntry = new Dictionary { @@ -153,18 +174,8 @@ private static int RunSet(string[] args, NetclawPaths paths, TextWriter writer) ["Provenance"] = ModelDiscoverySource.Manual.ToString() }; - // --context-window is an explicit operator override — persist it to - // the catalog (keyed by the new selection's identity) so it survives - // a subsequent picker swap. Inline on the role is also written so the - // shape stays consistent with hand-edited configs. if (contextWindow.HasValue) - { modelEntry["ContextWindow"] = contextWindow.Value; - var catalog = ConfigFileHelper.GetOrCreateSection(modelsSection, "Catalog"); - var entry = ConfigFileHelper.GetOrCreateSection( - catalog, ModelSelection.CatalogKey(providerName, modelId)); - entry["ContextWindow"] = contextWindow.Value; - } modelsSection[roleKey] = modelEntry; ConfigFileHelper.WriteConfigFile(paths.NetclawConfigPath, config); @@ -273,6 +284,11 @@ private static int RunClear(string[] args, NetclawPaths paths, TextWriter writer return 0; } + // Preserve any operator overrides before the role record disappears, + // so re-setting this role to the same (provider, modelId) later + // restores them via the catalog overlay. + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); + modelsSection.Remove(roleKey); ConfigFileHelper.WriteConfigFile(paths.NetclawConfigPath, config); diff --git a/src/Netclaw.Cli/Provider/ProviderRenamer.cs b/src/Netclaw.Cli/Provider/ProviderRenamer.cs index 07741eb22..bbd5e1ef1 100644 --- a/src/Netclaw.Cli/Provider/ProviderRenamer.cs +++ b/src/Netclaw.Cli/Provider/ProviderRenamer.cs @@ -120,9 +120,52 @@ private static List CascadeRenameModelRoles( } } + // Catalog keys embed the provider name as the first segment of + // "{provider}/{modelId}". Rewrite any key whose provider segment + // matches oldName (case-insensitive, mirroring the role cascade) so + // the override remains reachable from the renamed Provider field. + RenameCatalogProviderSegment(models, oldName, newName); + return reassigned; } + private static void RenameCatalogProviderSegment( + Dictionary models, string oldName, string newName) + { + var catalog = ConfigFileHelper.GetSectionOrNull(models, "Catalog"); + if (catalog is null || catalog.Count == 0) return; + + // Two-pass: collect rewrites first, then mutate. Iterating and + // mutating a Dictionary's keys simultaneously throws. + List<(string OldKey, string NewKey)>? rewrites = null; + foreach (var key in catalog.Keys) + { + var slash = key.IndexOf('/'); + if (slash <= 0) continue; + var providerSegment = key.AsSpan(0, slash); + if (!providerSegment.Equals(oldName, StringComparison.OrdinalIgnoreCase)) + continue; + var modelIdSegment = key.Substring(slash + 1); + var newKey = Configuration.ModelSelection.CatalogKey(newName, modelIdSegment); + rewrites ??= new List<(string, string)>(); + rewrites.Add((key, newKey)); + } + + if (rewrites is null) return; + + foreach (var (oldKey, newKey) in rewrites) + { + var entry = catalog[oldKey]; + catalog.Remove(oldKey); + // If a same-spelled rename collides with an unrelated catalog + // entry under newKey (rare — would require a pre-existing + // duplicate), keep the freshly-renamed one — that's the entry + // the active role just rebound to, so it represents the + // operator's live intent. + catalog[newKey] = entry; + } + } + private static bool HasCollision( Dictionary section, string oldName, string newName) { diff --git a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs index ac1d28f53..79fc5cb93 100644 --- a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs +++ b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs @@ -214,7 +214,15 @@ public void ClearRole(string role) var (config, _) = ConfigFileHelper.LoadConfigFiles(_paths); var modelsSection = ConfigFileHelper.GetSectionOrNull(config, "Models"); - if (modelsSection?.Remove(roleKey) == true) + if (modelsSection is null || !modelsSection.ContainsKey(roleKey)) + return; + + // Preserve any operator overrides on the role being cleared so that + // re-setting it to the same (provider, modelId) later restores them + // via the catalog overlay — same contract as ConfirmAssignment. + ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); + + if (modelsSection.Remove(roleKey)) { ConfigFileHelper.WriteConfigFile(_paths.NetclawConfigPath, config); Refresh(); From 45ea8701a32127afe19d662c9e0c74320fc0b5d4 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 23 May 2026 22:00:09 +0000 Subject: [PATCH 3/4] refactor(config): catalog is hand-managed override layer; drop auto-promote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per #1127 the core requirement is that switching Main/Fallback must not blow away manual overrides. The cleanest realization of that is the split the issue actually proposed: role records are pure identity pointers (Provider, ModelId, Provenance), and per-model overrides (ContextWindow, InputModalities, OutputModalities) live independently in Models.Catalog keyed by "{provider}/{modelId}". Switching the pointer is decoupled from the catalog, so overrides survive trivially — no Promote logic needed. This simplifies the previous PromoteRoleOverridesToCatalog machinery out of existence. The catalog is now populated by exactly two paths: operator hand-edits, and `netclaw model set --context-window N` (which routes the explicit override directly to the catalog instead of inline). The CLI flag becomes the persistent-override affordance the prior C1 review finding asked for, without any same-vs-different identity gating or sticky-restore semantics. Inline override fields on role records remain supported at read time for backwards compatibility with hand-edited configs. Also addresses the second-round review findings against the auto-promote design: - F1 (TUI/CLI asymmetry): gone — neither path promotes. - F2 (doctor crashes on non-int catalog values): ContextWindowDoctorCheck now uses JsonValue.TryGetValue with a defensive catch and emits a clear error message naming the bad value, instead of letting InvalidOperationException abort DoctorRunner. - F3 (provider rename orphans catalog keys): already addressed in the previous commit's CascadeRenameModelRoles change; unchanged here. - F4 (case-sensitive Catalog lookup): ApplyCatalogOverlays now does a case-insensitive fallback scan so a role.Provider whose casing drifted from a catalog key still resolves. Doctor mirrors this. - F5 (Promote overwrites pre-existing catalog values): gone — no promote to overwrite anything. Tests rewritten to lock down the new contract directly: catalog is the destination for --context-window writes; switching Main between two model ids leaves the saved override on the displaced model intact and re-applies it on switch-back; clear is a pure pointer remove and never touches the catalog. --- ...nfigFileHelperPromoteRoleOverridesTests.cs | 134 ------------------ .../Model/ModelCommandTests.cs | 75 ++++++---- src/Netclaw.Cli/Config/ConfigFileHelper.cs | 107 ++------------ .../Doctor/ContextWindowDoctorCheck.cs | 64 +++++++-- src/Netclaw.Cli/Model/ModelCommand.cs | 52 +++---- src/Netclaw.Cli/Tui/ModelManagerViewModel.cs | 21 +-- src/Netclaw.Configuration/ModelSelection.cs | 22 ++- 7 files changed, 158 insertions(+), 317 deletions(-) delete mode 100644 src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs diff --git a/src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs b/src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs deleted file mode 100644 index df35a3e7a..000000000 --- a/src/Netclaw.Cli.Tests/Daemon/ConfigFileHelperPromoteRoleOverridesTests.cs +++ /dev/null @@ -1,134 +0,0 @@ -// ----------------------------------------------------------------------- -// -// Copyright (C) 2026 - 2026 Petabridge, LLC -// -// ----------------------------------------------------------------------- -using System.Text.Json; -using Netclaw.Cli.Config; -using Xunit; - -namespace Netclaw.Cli.Tests.Daemon; - -/// -/// Locks in the writer-side half of the #1127 contract: before overwriting a -/// role on a model swap, any operator overrides on the displaced role are -/// promoted to Models.Catalog["{oldProvider}/{oldModelId}"]. This is -/// what makes hand-edited ContextWindow / InputModalities / -/// OutputModalities survive a picker-driven model change. -/// -public sealed class ConfigFileHelperPromoteRoleOverridesTests -{ - [Fact] - public void Promote_MovesContextWindow_FromRole_ToCatalog() - { - var modelsSection = new Dictionary - { - ["Main"] = new Dictionary - { - ["Provider"] = "p", - ["ModelId"] = "m", - ["ContextWindow"] = 200_000L, - }, - }; - - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); - - var catalog = (Dictionary)modelsSection["Catalog"]; - var entry = (Dictionary)catalog["p/m"]; - Assert.Equal(200_000L, Assert.IsType(entry["ContextWindow"])); - } - - [Fact] - public void Promote_IsNoOp_WhenRoleHasNoOverrideFields() - { - // The common case: most operators rely on auto-detection and never - // hand-set ContextWindow / Modality fields. Promote must not seed an - // empty Catalog entry in that case. - var modelsSection = new Dictionary - { - ["Main"] = new Dictionary - { - ["Provider"] = "p", - ["ModelId"] = "m", - ["Provenance"] = "Live", - }, - }; - - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); - - Assert.False(modelsSection.ContainsKey("Catalog")); - } - - [Fact] - public void Promote_InlineWins_WhenCatalogEntryAlreadyExists() - { - // Inline value represents the most recent operator intent — it should - // overwrite a stale catalog entry from a previous switch. - var modelsSection = new Dictionary - { - ["Main"] = new Dictionary - { - ["Provider"] = "p", - ["ModelId"] = "m", - ["ContextWindow"] = 100L, - }, - ["Catalog"] = new Dictionary - { - ["p/m"] = new Dictionary - { - ["ContextWindow"] = 50L, - }, - }, - }; - - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); - - var catalog = (Dictionary)modelsSection["Catalog"]; - var entry = (Dictionary)catalog["p/m"]; - Assert.Equal(100L, Assert.IsType(entry["ContextWindow"])); - } - - [Fact] - public void Promote_HandlesJsonElement_FromFreshlyLoadedConfig() - { - // Simulates the path where ConfigFileHelper.LoadConfigFiles returned a - // dictionary whose nested values are JsonElement (un-materialized). - var json = """ - { - "Main": { "Provider": "p", "ModelId": "m", "InputModalities": "Text, Image" } - } - """; - var modelsSection = JsonSerializer.Deserialize>(json)!; - - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); - - var catalog = (Dictionary)modelsSection["Catalog"]; - var entry = (Dictionary)catalog["p/m"]; - Assert.Equal("Text, Image", Assert.IsType(entry["InputModalities"])); - } - - [Fact] - public void Promote_LeavesIdentityFields_OnOldRole() - { - // The role record itself isn't deleted by Promote — it's about to be - // overwritten by the caller. Verify Promote doesn't mutate the role's - // Provider/ModelId/Provenance, only reads them. - var modelsSection = new Dictionary - { - ["Main"] = new Dictionary - { - ["Provider"] = "p", - ["ModelId"] = "m", - ["Provenance"] = "Live", - ["ContextWindow"] = 1234L, - }, - }; - - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, "Main"); - - var main = (Dictionary)modelsSection["Main"]; - Assert.Equal("p", main["Provider"]); - Assert.Equal("m", main["ModelId"]); - Assert.Equal("Live", main["Provenance"]); - } -} diff --git a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs index 9cb187015..2f548ead5 100644 --- a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs @@ -279,12 +279,11 @@ public async Task Set_FallbackModel_WritesCorrectRole() } [Fact] - public async Task Set_SameIdentity_WithoutContextWindowFlag_DropsOverride() + public async Task Set_ContextWindowFlag_WritesToCatalog_NotInlineRole() { - // C1 regression: pre-fix, re-running `model set` for the same - // (provider, modelId) without --context-window silently restored - // the previously-set ContextWindow via the catalog overlay. The - // operator had no CLI affordance to drop the override. + // The role record stays a pure identity pointer; explicit override + // intent (--context-window) lands in Models.Catalog so it survives + // role-pointer changes (the #1127 contract). WriteConfig(new Dictionary { ["configVersion"] = 1, @@ -298,20 +297,26 @@ await ModelCommand.RunAsync( ["model", "set", "main", "p", "m", "--context-window", "200000"], _paths, output: _output); - await ModelCommand.RunAsync( - ["model", "set", "main", "p", "m"], - _paths, output: _output); + var config = ReadConfigFile(_paths.NetclawConfigPath); + var models = config.RootElement.GetProperty("Models"); + var main = models.GetProperty("Main"); + Assert.False(main.TryGetProperty("ContextWindow", out _), + "Role record should not carry inline ContextWindow — overrides live in Catalog."); + + var entry = models.GetProperty("Catalog").GetProperty("p/m"); + Assert.Equal(200_000, entry.GetProperty("ContextWindow").GetInt32()); + // Effective via overlay merge: the role sees ContextWindow=200000. var selection = ModelCommand.LoadModelSelection(_paths)!; - Assert.Null(selection.Main.ContextWindow); + Assert.Equal(200_000, selection.Main.ContextWindow); } [Fact] - public async Task Set_IdentityChange_PromotesPriorOverrideToCatalog() + public async Task Set_SwitchingRole_PointerDoesNotTouchCatalog() { - // C1 sibling: changing the model preserves the displaced role's - // overrides — switching back later re-applies them via the catalog - // overlay. This is the cross-model survival contract. + // #1127 core invariant: changing Main from p/m1 to p/m2 does NOT + // disturb Models.Catalog. The hand-set override on p/m1 stays in + // place and re-applies the next time Main points back at p/m1. WriteConfig(new Dictionary { ["configVersion"] = 1, @@ -321,27 +326,39 @@ public async Task Set_IdentityChange_PromotesPriorOverrideToCatalog() } }); + // Operator pins ContextWindow=200000 on p/m1. await ModelCommand.RunAsync( ["model", "set", "main", "p", "m1", "--context-window", "200000"], _paths, output: _output); + + // Operator switches Main to p/m2 (no override on m2). await ModelCommand.RunAsync( ["model", "set", "main", "p", "m2"], _paths, output: _output); + + var afterSwitch = ModelCommand.LoadModelSelection(_paths)!; + Assert.Equal("m2", afterSwitch.Main.ModelId); + Assert.Null(afterSwitch.Main.ContextWindow); // no override for m2 + + // Catalog still holds the m1 override untouched. + var config = ReadConfigFile(_paths.NetclawConfigPath); + var entry = config.RootElement + .GetProperty("Models").GetProperty("Catalog").GetProperty("p/m1"); + Assert.Equal(200_000, entry.GetProperty("ContextWindow").GetInt32()); + + // Switching Main back to p/m1 re-applies the saved override. await ModelCommand.RunAsync( ["model", "set", "main", "p", "m1"], _paths, output: _output); - var selection = ModelCommand.LoadModelSelection(_paths)!; - Assert.Equal(200_000, selection.Main.ContextWindow); + var afterReturn = ModelCommand.LoadModelSelection(_paths)!; + Assert.Equal(200_000, afterReturn.Main.ContextWindow); } [Fact] - public async Task Clear_PreservesOverridesToCatalog() + public async Task Clear_LeavesCatalogIntact() { - // C4 regression (CLI side): clearing a role previously deleted any - // inline ContextWindow / modality overrides without preserving - // them. Re-setting the same (provider, modelId) later must restore - // them via the catalog overlay. + // Clearing a role removes the pointer; saved overrides survive. WriteConfig(new Dictionary { ["configVersion"] = 1, @@ -359,19 +376,21 @@ public async Task Clear_PreservesOverridesToCatalog() ["Fallback"] = new Dictionary { ["Provider"] = "p", - ["ModelId"] = "fallback-model", - ["ContextWindow"] = 65536L + ["ModelId"] = "fallback-model" + }, + ["Catalog"] = new Dictionary + { + ["p/fallback-model"] = new Dictionary + { + ["ContextWindow"] = 65536L + } } } }); + await ModelCommand.RunAsync(["model", "clear", "fallback"], _paths, output: _output); await ModelCommand.RunAsync( - ["model", "clear", "fallback"], - _paths, output: _output); - - await ModelCommand.RunAsync( - ["model", "set", "fallback", "p", "fallback-model"], - _paths, output: _output); + ["model", "set", "fallback", "p", "fallback-model"], _paths, output: _output); var selection = ModelCommand.LoadModelSelection(_paths)!; Assert.Equal(65_536, selection.Fallback!.ContextWindow); diff --git a/src/Netclaw.Cli/Config/ConfigFileHelper.cs b/src/Netclaw.Cli/Config/ConfigFileHelper.cs index 8eafe2b6f..e5f75c42e 100644 --- a/src/Netclaw.Cli/Config/ConfigFileHelper.cs +++ b/src/Netclaw.Cli/Config/ConfigFileHelper.cs @@ -123,101 +123,24 @@ internal static string DecryptIfEncrypted(Configuration.NetclawPaths paths, stri } /// - /// Override fields that survive a model swap: when the operator changes a - /// role's (Provider, ModelId), any of these fields present on the old role - /// record are moved into Models.Catalog["{provider}/{modelId}"] so - /// they re-apply if the operator switches back. Provider/ModelId/Provenance - /// are NOT overrides — they identify which model is selected, not how it - /// behaves. + /// Write an operator-set field + /// into Models.Catalog["{provider}/{modelId}"]. Used by CLI flags + /// (e.g. --context-window) that represent explicit override intent. + /// The catalog is the persistent override layer — values written here + /// survive role-pointer changes (picker swaps, role clears, role + /// re-selection) because the catalog key is independent of which role + /// currently references the model. /// - private static readonly string[] OverrideFieldNames = - ["ContextWindow", "InputModalities", "OutputModalities"]; - - /// - /// Before overwriting Models[roleKey] with a new selection, move - /// any override fields on the OLD role record into - /// Models.Catalog["{oldProvider}/{oldModelId}"]. Inline values from - /// the old role win over any existing catalog entry, because they - /// represent the most recent operator intent. No-op when the old role - /// record carries no override fields (the common case — most operators - /// rely on auto-detection). - /// - internal static void PromoteRoleOverridesToCatalog( - Dictionary modelsSection, string roleKey) + internal static void SetCatalogOverride( + Dictionary modelsSection, + string provider, + string modelId, + string fieldName, + object value) { - var oldRole = GetSectionOrNull(modelsSection, roleKey); - if (oldRole is null) - return; - - var oldProvider = TryReadString(oldRole, "Provider"); - var oldModelId = TryReadString(oldRole, "ModelId"); - if (string.IsNullOrEmpty(oldProvider) || string.IsNullOrEmpty(oldModelId)) - return; - - Dictionary? overrides = null; - foreach (var name in OverrideFieldNames) - { - if (!oldRole.TryGetValue(name, out var value) || value is null) - continue; - overrides ??= new Dictionary(); - overrides[name] = UnwrapJsonElement(value); - } - - if (overrides is null) - return; - var catalog = GetOrCreateSection(modelsSection, "Catalog"); - var key = Configuration.ModelSelection.CatalogKey(oldProvider, oldModelId); + var key = Configuration.ModelSelection.CatalogKey(provider, modelId); var entry = GetOrCreateSection(catalog, key); - foreach (var kvp in overrides) - entry[kvp.Key] = kvp.Value; - } - - /// - /// Returns the (Provider, ModelId) currently recorded at - /// modelsSection[roleKey], or null when the role is absent or - /// missing either identity field. Used by writers to detect identity - /// changes vs same-identity re-runs. - /// - internal static (string Provider, string ModelId)? TryReadRoleIdentity( - Dictionary modelsSection, string roleKey) - { - var role = GetSectionOrNull(modelsSection, roleKey); - if (role is null) return null; - var provider = TryReadString(role, "Provider"); - var modelId = TryReadString(role, "ModelId"); - return string.IsNullOrEmpty(provider) || string.IsNullOrEmpty(modelId) - ? null - : (provider, modelId); - } - - /// - /// Best-effort string read from a dictionary that may carry raw strings - /// (re-materialized values) or s (freshly loaded - /// from disk). Returns null for missing, null, or non-string values. - /// - internal static string? TryReadString(Dictionary dict, string key) - { - if (!dict.TryGetValue(key, out var raw) || raw is null) - return null; - return raw switch - { - string s => s, - JsonElement je when je.ValueKind == JsonValueKind.String => je.GetString(), - _ => null, - }; + entry[fieldName] = value; } - - private static object UnwrapJsonElement(object value) => value switch - { - JsonElement je => je.ValueKind switch - { - JsonValueKind.String => (object)(je.GetString() ?? string.Empty), - JsonValueKind.Number => je.TryGetInt64(out var l) ? l : je.GetDouble(), - JsonValueKind.True => true, - JsonValueKind.False => false, - _ => je.GetRawText(), - }, - _ => value, - }; } diff --git a/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs b/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs index 18f46562d..2cd292a5e 100644 --- a/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs +++ b/src/Netclaw.Cli/Doctor/ContextWindowDoctorCheck.cs @@ -55,27 +55,27 @@ public async Task RunAsync(CancellationToken cancellationToke // Effective ContextWindow follows ModelSelection.ApplyCatalogOverlays: // inline on the role wins; otherwise fall back to the catalog - // overlay keyed by "{provider}/{modelId}". The runtime daemon uses - // exactly this precedence, so the doctor must match it or it will - // report "no explicit setting" while the daemon honors an override. - var contextWindow = main["ContextWindow"]; - if (contextWindow is null && models is not null) - contextWindow = TryReadCatalogContextWindow(models, providerName, modelId); + // overlay keyed by "{provider}/{modelId}" (case-insensitive). The + // runtime daemon uses exactly this precedence, so the doctor must + // match it or it will report "no explicit setting" while the + // daemon honors an override. + var contextWindow = main["ContextWindow"] + ?? TryReadCatalogContextWindow(models!, providerName, modelId); if (contextWindow is null) return await ResolveEffectiveContextWindowAsync(modelId, providerName, cancellationToken); - if (contextWindow.GetValue() is var cw and > 0) + if (!TryReadPositiveInt(contextWindow, out var cw)) { - return DoctorCheckResult.Pass( + return DoctorCheckResult.Error( "Context Window", - $"Context window explicitly set to {cw:N0} tokens."); + $"ContextWindow for {providerName}/{modelId} is not a positive integer (got {DescribeJsonValue(contextWindow)}).", + "Edit netclaw.json so the ContextWindow value is a positive integer literal (no quotes, no decimals)."); } - return DoctorCheckResult.Error( + return DoctorCheckResult.Pass( "Context Window", - "Models.Main.ContextWindow must be a positive integer.", - "Set Models.Main.ContextWindow to the effective runtime context window size in tokens."); + $"Context window explicitly set to {cw:N0} tokens."); } private static JsonNode? TryReadCatalogContextWindow( @@ -83,8 +83,44 @@ public async Task RunAsync(CancellationToken cancellationToke { var catalog = models["Catalog"] as JsonObject; if (catalog is null) return null; - var entry = catalog[ModelSelection.CatalogKey(providerName, modelId)] as JsonObject; - return entry?["ContextWindow"]; + var key = ModelSelection.CatalogKey(providerName, modelId); + // Case-insensitive scan: ApplyCatalogOverlays does the same when the + // exact-case lookup misses, so the doctor must agree. + if (catalog[key] is JsonObject direct) return direct["ContextWindow"]; + foreach (var kvp in catalog) + { + if (string.Equals(kvp.Key, key, StringComparison.OrdinalIgnoreCase) + && kvp.Value is JsonObject entry) + return entry["ContextWindow"]; + } + return null; + } + + private static bool TryReadPositiveInt(JsonNode node, out int value) + { + value = 0; + try + { + // JsonValue.TryGetValue avoids the InvalidOperationException + // GetValue() throws on strings, doubles, etc. — exactly the + // shapes a hand-edited Catalog override is prone to. + if (node is JsonValue jv && jv.TryGetValue(out var parsed) && parsed > 0) + { + value = parsed; + return true; + } + } + catch (Exception ex) when (ex is InvalidOperationException or FormatException) + { + // fall through to false + } + return false; + } + + private static string DescribeJsonValue(JsonNode node) + { + var raw = node.ToJsonString(); + return raw.Length > 40 ? raw[..37] + "..." : raw; } private async Task ResolveEffectiveContextWindowAsync( diff --git a/src/Netclaw.Cli/Model/ModelCommand.cs b/src/Netclaw.Cli/Model/ModelCommand.cs index dcfa4b115..df67a4f21 100644 --- a/src/Netclaw.Cli/Model/ModelCommand.cs +++ b/src/Netclaw.Cli/Model/ModelCommand.cs @@ -138,46 +138,33 @@ private static int RunSet(string[] args, NetclawPaths paths, TextWriter writer) } } - // Write to config + // Write to config. Role records are pure identity pointers + // (Provider, ModelId, Provenance). Operator-set overrides live + // independently in Models.Catalog, keyed by "{provider}/{modelId}", + // and survive every role-pointer change (this set, future sets, + // picker swaps, clears) because the catalog is decoupled from + // which role currently references the model. var (config, _) = ConfigFileHelper.LoadConfigFiles(paths); var modelsSection = ConfigFileHelper.GetOrCreateSection(config, "Models"); - // CLI write semantics differ from the TUI picker: - // - // identity changed → Promote the displaced role's overrides into - // the catalog so a later switch back restores - // them. Cross-model survival. - // - // identity unchanged → DO NOT Promote. The operator running - // `model set ROLE P M` (without --context-window) - // is signaling "start fresh for this model"; - // promoting the inline override into the catalog - // would have ApplyCatalogOverlays silently - // restore it on the next read, leaving the - // operator no CLI affordance to clear it. - // - // Cross-model survival for explicit --context-window writes still - // works: the value lives inline on the role and the next identity - // change promotes it to the catalog automatically. - var existingIdentity = ConfigFileHelper.TryReadRoleIdentity(modelsSection, roleKey); - var identityChanged = existingIdentity is null - || existingIdentity.Value.Provider != providerName - || existingIdentity.Value.ModelId != modelId; - - if (identityChanged) - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); - var modelEntry = new Dictionary { ["Provider"] = providerName, ["ModelId"] = modelId, ["Provenance"] = ModelDiscoverySource.Manual.ToString() }; + modelsSection[roleKey] = modelEntry; + // --context-window is explicit override intent → persist to catalog + // so it applies regardless of which role currently points at this + // (provider, modelId). To remove a previously-set override, the + // operator hand-edits Models.Catalog["{provider}/{modelId}"]. if (contextWindow.HasValue) - modelEntry["ContextWindow"] = contextWindow.Value; + { + ConfigFileHelper.SetCatalogOverride( + modelsSection, providerName, modelId, "ContextWindow", contextWindow.Value); + } - modelsSection[roleKey] = modelEntry; ConfigFileHelper.WriteConfigFile(paths.NetclawConfigPath, config); writer.WriteLine($"Set {role} model to {providerName}/{modelId}"); @@ -284,11 +271,10 @@ private static int RunClear(string[] args, NetclawPaths paths, TextWriter writer return 0; } - // Preserve any operator overrides before the role record disappears, - // so re-setting this role to the same (provider, modelId) later - // restores them via the catalog overlay. - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); - + // Clearing a role removes the identity pointer only. Any override + // entries in Models.Catalog stay in place, so re-binding this role + // to the same (provider, modelId) later still picks up the saved + // overrides via ApplyCatalogOverlays. modelsSection.Remove(roleKey); ConfigFileHelper.WriteConfigFile(paths.NetclawConfigPath, config); diff --git a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs index 79fc5cb93..298bdc3b1 100644 --- a/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs +++ b/src/Netclaw.Cli/Tui/ModelManagerViewModel.cs @@ -171,11 +171,9 @@ public void ConfirmAssignment() var (config, _) = ConfigFileHelper.LoadConfigFiles(_paths); var modelsSection = ConfigFileHelper.GetOrCreateSection(config, "Models"); - // Preserve any operator overrides (ContextWindow / modality forcing) - // on the role being overwritten — they survive in the catalog so - // switching back to the same (provider, modelId) re-applies them. - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); - + // Role records are pure identity pointers; operator overrides live + // in Models.Catalog keyed by "{provider}/{modelId}" and survive + // picker swaps because they are decoupled from the role record. var modelEntry = new Dictionary { ["Provider"] = SelectedProvider, @@ -214,16 +212,11 @@ public void ClearRole(string role) var (config, _) = ConfigFileHelper.LoadConfigFiles(_paths); var modelsSection = ConfigFileHelper.GetSectionOrNull(config, "Models"); - if (modelsSection is null || !modelsSection.ContainsKey(roleKey)) - return; - - // Preserve any operator overrides on the role being cleared so that - // re-setting it to the same (provider, modelId) later restores them - // via the catalog overlay — same contract as ConfirmAssignment. - ConfigFileHelper.PromoteRoleOverridesToCatalog(modelsSection, roleKey); - - if (modelsSection.Remove(roleKey)) + if (modelsSection?.Remove(roleKey) == true) { + // Clearing a role removes the identity pointer only. Any saved + // overrides in Models.Catalog stay in place and re-apply if the + // role is later re-bound to the same (provider, modelId). ConfigFileHelper.WriteConfigFile(_paths.NetclawConfigPath, config); Refresh(); StatusMessage.Value = $"Cleared {role} role. Restart daemon for changes to take effect."; diff --git a/src/Netclaw.Configuration/ModelSelection.cs b/src/Netclaw.Configuration/ModelSelection.cs index 63fc493f2..af1e4c3dc 100644 --- a/src/Netclaw.Configuration/ModelSelection.cs +++ b/src/Netclaw.Configuration/ModelSelection.cs @@ -57,11 +57,29 @@ public void ApplyCatalogOverlays() private void Merge(ModelReference role) { - if (!Catalog!.TryGetValue(CatalogKey(role.Provider, role.ModelId), out var overlay)) - return; + var key = CatalogKey(role.Provider, role.ModelId); + var overlay = FindOverlay(key); + if (overlay is null) return; role.ContextWindow ??= overlay.ContextWindow; role.InputModalities ??= overlay.InputModalities; role.OutputModalities ??= overlay.OutputModalities; } + + private ModelOverride? FindOverlay(string key) + { + // Fast path: exact-case match against the operator-written key. + if (Catalog!.TryGetValue(key, out var direct)) return direct; + // Tolerate provider-name casing drift between role.Provider and the + // catalog key (operator hand-edits, picker re-writes, the Providers + // dictionary which is case-sensitive vs. ProviderRenamer which is + // not). Catalog is typically empty or single-digit entries so the + // O(n) scan is negligible. + foreach (var kvp in Catalog) + { + if (string.Equals(kvp.Key, key, StringComparison.OrdinalIgnoreCase)) + return kvp.Value; + } + return null; + } } From 00ec19c6ab09644aa3d2764d7e22e87ebb28a034 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 23 May 2026 22:32:28 +0000 Subject: [PATCH 4/4] fix(config): preserve ':' in catalog keys; restore broken test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Microsoft.Extensions.Configuration uses ':' as its hierarchical path separator and applies that splitting to dictionary keys, so a catalog key like 'local-ollama/qwen3:30b' (the default Ollama ModelId) was bound as Catalog['local-ollama/qwen3'] with the '30b' suffix dropped as a stray sub-property — the ContextWindow override silently vanished on the daemon side. CLI and doctor (JsonSerializer and JsonNode paths) preserved the key correctly, so diagnostics reported the override was set while the daemon ignored it. Adds ModelSelection.LoadFromConfiguration as the single load entry point: binds Main/Fallback/Compaction via M.E.Configuration as before, then re-parses Catalog directly from the raw JSON via System.Text.Json (which treats keys as opaque strings), then applies overlays. All four caller sites (daemon ConfigureLlmServices + ConfigureDaemonServices, CLI init flow + ConfigureCliChatServices) now route through it. The IOptions PostConfigure does the same re-load before validation runs. Also fixes the pre-existing ModelCommandTests.Set_MainModel_WritesConfig which asserted on inline Main.ContextWindow that the new catalog-write path doesn't produce. Updated to assert against Catalog['my-ollama/qwen3:30b'].ContextWindow, which transitively also locks in the colon-key fix (the key contains ':'). Adds Configuration test LoadFromConfiguration_PreservesColonIn- CatalogKey_ForOllamaStyleModelIds that exercises the failure mode directly: writes a Catalog['local-ollama/qwen3:30b'] entry, binds via LoadFromConfiguration, asserts the key round-trips intact and the overlay applies to Main. --- .../Model/ModelCommandTests.cs | 14 ++++- src/Netclaw.Cli/Program.cs | 14 ++--- .../ModelSelectionCatalogOverlayTests.cs | 54 ++++++++++++++++++ src/Netclaw.Configuration/ModelSelection.cs | 57 +++++++++++++++++++ src/Netclaw.Daemon/Program.cs | 25 ++++---- 5 files changed, 145 insertions(+), 19 deletions(-) diff --git a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs index 2f548ead5..a2b93ff60 100644 --- a/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Model/ModelCommandTests.cs @@ -92,7 +92,19 @@ public async Task Set_MainModel_WritesConfig() Assert.Equal("my-ollama", main.GetProperty("Provider").GetString()); Assert.Equal("qwen3:30b", main.GetProperty("ModelId").GetString()); Assert.Equal("Manual", main.GetProperty("Provenance").GetString()); - Assert.Equal(32768, main.GetProperty("ContextWindow").GetInt32()); + + // Role records are identity-only; --context-window writes to Catalog + // keyed by "{provider}/{modelId}" so the override survives later role + // swaps. Catalog key contains ':' (Ollama's `qwen3:30b`) — this + // assertion also pins down the IConfiguration colon-split fix, since + // LoadModelSelection reads back via JsonSerializer. + Assert.False(main.TryGetProperty("ContextWindow", out _)); + var catalog = models.GetProperty("Catalog"); + var entry = catalog.GetProperty("my-ollama/qwen3:30b"); + Assert.Equal(32768, entry.GetProperty("ContextWindow").GetInt32()); + + var selection = ModelCommand.LoadModelSelection(_paths)!; + Assert.Equal(32768, selection.Main.ContextWindow); } [Fact] diff --git a/src/Netclaw.Cli/Program.cs b/src/Netclaw.Cli/Program.cs index 3eba741df..e5f5b2242 100644 --- a/src/Netclaw.Cli/Program.cs +++ b/src/Netclaw.Cli/Program.cs @@ -185,9 +185,8 @@ static async Task RunAsync(string[] args) .AddEnvironmentVariables("NETCLAW_"); var initConfig = configBuilder.Build(); - var models = initConfig.GetSection("Models") - .Get() ?? new ModelSelection(); - models.ApplyCatalogOverlays(); + var models = ModelSelection.LoadFromConfiguration( + initConfig.GetSection("Models"), initPaths.NetclawConfigPath); var contextWindow = ContextWindowResolution.ResolveAsync( models.Main.ContextWindow, @@ -982,7 +981,7 @@ static async Task RunAsync(string[] args) webBuilder.WebHost.UseUrls("http://127.0.0.1:0"); var sharedPaths = ConfigureConfigServices(webBuilder.Services, webBuilder.Configuration); - ConfigureCliChatServices(webBuilder.Services, webBuilder.Configuration); + ConfigureCliChatServices(webBuilder.Services, webBuilder.Configuration, sharedPaths); // Shared navigation state for passing resume session ID to ChatViewModel var navState = new ChatNavigationState { ResumeSessionId = resumeSessionId }; @@ -1798,12 +1797,11 @@ static IConfigurationRoot BuildCliConfig() // Daemon-backed CLI services (SignalR thin client) // ═══════════════════════════════════════════════════════════════════════ -static void ConfigureCliChatServices(IServiceCollection services, IConfigurationManager configuration) +static void ConfigureCliChatServices(IServiceCollection services, IConfigurationManager configuration, NetclawPaths paths) { // Resolve models for session config - var models = configuration.GetSection("Models") - .Get() ?? new ModelSelection(); - models.ApplyCatalogOverlays(); + var models = ModelSelection.LoadFromConfiguration( + configuration.GetSection("Models"), paths.NetclawConfigPath); // Session config: bind operator-facing settings var sessionConfig = SessionConfig.BindFromConfiguration(configuration.GetSection("Session")); diff --git a/src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs b/src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs index a42dd9dd8..dcac4a90f 100644 --- a/src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs +++ b/src/Netclaw.Configuration.Tests/ModelSelectionCatalogOverlayTests.cs @@ -108,4 +108,58 @@ public void ApplyCatalogOverlays_IsNoOp_WhenCatalogAbsent() Assert.Null(selection.Main.ContextWindow); Assert.Null(selection.Main.InputModalities); } + + [Fact] + public void LoadFromConfiguration_PreservesColonInCatalogKey_ForOllamaStyleModelIds() + { + // Microsoft.Extensions.Configuration treats ':' as a path separator + // and applies that splitting to dictionary keys, so a Catalog key + // like "local-ollama/qwen3:30b" gets bound as + // Catalog["local-ollama/qwen3"] with the "30b" suffix dropped as a + // stray sub-property. Default ModelId in ModelReference is qwen3:30b + // so this is the most common path. LoadFromConfiguration re-parses + // Catalog directly from the raw JSON via System.Text.Json (which + // treats keys as opaque strings) to side-step the binder. + var tempFile = Path.GetTempFileName(); + try + { + File.WriteAllText(tempFile, """ + { + "Models": { + "Main": { "Provider": "local-ollama", "ModelId": "qwen3:30b" }, + "Catalog": { + "local-ollama/qwen3:30b": { "ContextWindow": 200000 } + } + } + } + """); + + // Use InMemoryCollection for the Main pointer side (avoids + // depending on Microsoft.Extensions.Configuration.Json in the + // test project); LoadFromConfiguration reads Catalog directly + // from the file regardless of the IConfiguration source. + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Models:Main:Provider"] = "local-ollama", + ["Models:Main:ModelId"] = "qwen3:30b", + }) + .Build(); + + var selection = ModelSelection.LoadFromConfiguration( + config.GetSection("Models"), tempFile); + + // Catalog round-trips the colon-laden key intact. + Assert.NotNull(selection.Catalog); + Assert.True(selection.Catalog!.ContainsKey("local-ollama/qwen3:30b")); + Assert.Equal(200_000, selection.Catalog["local-ollama/qwen3:30b"].ContextWindow); + + // Overlay applies to Main, so the daemon sees the pinned value. + Assert.Equal(200_000, selection.Main.ContextWindow); + } + finally + { + File.Delete(tempFile); + } + } } diff --git a/src/Netclaw.Configuration/ModelSelection.cs b/src/Netclaw.Configuration/ModelSelection.cs index af1e4c3dc..2db9d54b4 100644 --- a/src/Netclaw.Configuration/ModelSelection.cs +++ b/src/Netclaw.Configuration/ModelSelection.cs @@ -3,6 +3,10 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Text.Json; +using System.Text.Json.Serialization; +using Microsoft.Extensions.Configuration; + namespace Netclaw.Configuration; /// @@ -82,4 +86,57 @@ private void Merge(ModelReference role) } return null; } + + /// + /// Bind a from configuration, then re-load + /// directly from the raw config file so dictionary + /// keys containing : survive intact, and apply overlays. Use this + /// instead of calling Get<ModelSelection>() directly. + /// + /// Microsoft.Extensions.Configuration uses : as its hierarchical + /// path separator and applies that splitting to dictionary keys too, so + /// the binder turns Catalog["local-ollama/qwen3:30b"] into + /// Catalog["local-ollama/qwen3"] with a stray 30b + /// sub-property — silently dropping the operator's override. Ollama's + /// default ModelId is qwen3:30b, so the broken path is the most + /// common one. Re-parsing the raw JSON via System.Text.Json (which + /// treats keys as opaque strings) is the cleanest fix that preserves + /// the rest of the M.E.Configuration pipeline for everything else. + /// + /// + public static ModelSelection LoadFromConfiguration( + IConfiguration modelsSection, string netclawConfigPath) + { + var selection = modelsSection.Get() ?? new ModelSelection(); + selection.Catalog = LoadCatalogFromFile(netclawConfigPath) ?? selection.Catalog; + selection.ApplyCatalogOverlays(); + return selection; + } + + /// + /// Parse Models.Catalog from + /// directly via System.Text.Json (bypassing + /// Microsoft.Extensions.Configuration's path-splitting binder). Returns + /// null when the file is absent, has no Models section, has no Catalog + /// property, or Catalog is not a JSON object — callers can keep + /// whatever binder-produced value they already have in that case. + /// + public static Dictionary? LoadCatalogFromFile(string netclawConfigPath) + { + if (!File.Exists(netclawConfigPath)) return null; + + using var doc = JsonDocument.Parse(File.ReadAllText(netclawConfigPath)); + if (!doc.RootElement.TryGetProperty("Models", out var models)) return null; + if (!models.TryGetProperty("Catalog", out var catalog)) return null; + if (catalog.ValueKind != JsonValueKind.Object) return null; + + return JsonSerializer.Deserialize>( + catalog.GetRawText(), + CatalogJsonOptions); + } + + private static readonly JsonSerializerOptions CatalogJsonOptions = new() + { + Converters = { new JsonStringEnumConverter() }, + }; } diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 0c4efcad4..386bd8aea 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -329,9 +329,8 @@ static NetclawPaths ConfigureConfigServices(IServiceCollection services, IConfig var providers = configuration.GetSection("Providers") .Get>() ?? new() { ["local-ollama"] = new ProviderEntry() }; - var models = configuration.GetSection("Models") - .Get() ?? new ModelSelection(); - models.ApplyCatalogOverlays(); + var models = ModelSelection.LoadFromConfiguration( + configuration.GetSection("Models"), paths.NetclawConfigPath); services.AddDaemonLlmProviders(providers, models); @@ -360,10 +359,17 @@ static void ConfigureDaemonServices( services .AddOptions() .Bind(configuration.GetSection("Models")) - // Fold catalog overlays into role records before validation runs, so - // a ContextWindow set only via Catalog still gets checked by - // ModelSelectionValidator's MinContextWindow gate. - .PostConfigure(models => models.ApplyCatalogOverlays()) + // Re-load Catalog directly from the config file (bypassing + // Microsoft.Extensions.Configuration's path-splitting binder, which + // mangles keys containing ':' such as the default Ollama + // `qwen3:30b`) and fold overlays into role records so a ContextWindow + // set only via Catalog still gets checked by ModelSelectionValidator. + .PostConfigure(models => + { + models.Catalog = ModelSelection.LoadCatalogFromFile(paths.NetclawConfigPath) + ?? models.Catalog; + models.ApplyCatalogOverlays(); + }) .ValidateOnStart(); services.AddSingleton, ModelSelectionValidator>(); services @@ -381,9 +387,8 @@ static void ConfigureDaemonServices( }); // Resolve models for session config - var models = configuration.GetSection("Models") - .Get() ?? new ModelSelection(); - models.ApplyCatalogOverlays(); + var models = ModelSelection.LoadFromConfiguration( + configuration.GetSection("Models"), paths.NetclawConfigPath); services.AddSingleton(models); // Auto-detect model capabilities via the runtime IModelCapabilityResolver