fix: keep discovered model capabilities out of config - #1761
Conversation
Aaronontheweb
added
bug
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
Author rationale review.
These comments explain why I made the key changes. This review does not request changes.
Provider discovery supplies temporary runtime facts. Config capability fields represent durable operator intent. The fix removes the path between those authority levels.
The proof covers the shared writer, CLI, model TUI, init wizard, runtime consumer, and native config artifact.
| Capability discovery does not write `ContextWindow`, `InputModalities`, or `OutputModalities`. | ||
| The daemon resolves those dynamic values at startup when the operator leaves them absent. | ||
|
|
||
| Older releases can contain discovery snapshots in these override fields. The stored shape |
There was a problem hiding this comment.
Rationale: I did not add an automatic migration. Older config does not record whether a field came from discovery or an operator. Automatic deletion could remove a real override. The existing clear flags provide a safe, explicit repair.
| ValueOverride<ModelModality> inputModalities, | ||
| ValueOverride<ModelModality> outputModalities, | ||
| DiscoveredModel? discovered) | ||
| ValueOverride<ModelModality> outputModalities) |
There was a problem hiding this comment.
Rationale: The DiscoveredModel parameter no longer exists. The type system now prevents this writer from persisting provider capabilities by accident.
| var resolvedOutput = ResolveModality(outputModalities, sameModelEntry, existing?.OutputModalities, discovered?.OutputModalities); | ||
| // Explicit operator input wins. Otherwise, the existing operator override remains. | ||
| // A new definition stays empty so runtime detection owns dynamic capabilities (#1756). | ||
| var resolvedWindow = contextWindow.Supplied ? contextWindow.Value : existing?.ContextWindow; |
There was a problem hiding this comment.
Rationale: Unset now means "preserve the stored override, or keep the field absent." It no longer means "copy the latest probe." An absent field lets daemon detection run at every startup.
| inputOverride, | ||
| outputOverride, | ||
| discoveredModel); | ||
| outputOverride); |
There was a problem hiding this comment.
Rationale: The CLI still uses the live model list to validate the model ID. It passes only explicit flags into persistence. This separates validation data from durable operator intent.
| ? ValueOverride<ModelModality>.Set(output) | ||
| : ValueOverride<ModelModality>.Unset, | ||
| discovered: null); | ||
| ValueOverride<int>.Unset, |
There was a problem hiding this comment.
Rationale: The wizard has no capability override controls. It must pass Unset for every capability field. Otherwise, provider results gain authority that the operator never gave them.
| var resolved = ModelCapabilityResolution.ResolveModelCapabilities( | ||
| new ModelSelection { Main = main }, detectedAtStartup); | ||
|
|
||
| Assert.Equal(100000, resolved.ContextWindowTokens); |
There was a problem hiding this comment.
Rationale: This test covers the complete producer-to-consumer contract. Selection sees 327680, startup sees 100000, and the runtime selects 100000. The modality change proves the same rule for all dynamic capability fields.
| Assert.Equal(512000, main.GetProperty("ContextWindow").GetInt32()); | ||
| Assert.Equal("Text, Image", main.GetProperty("InputModalities").GetString()); | ||
| Assert.Equal("Text", main.GetProperty("OutputModalities").GetString()); | ||
| Assert.False(main.TryGetProperty("ContextWindow", out _)); |
There was a problem hiding this comment.
Rationale: This test uses a successful OAuth probe with complete metadata. The saved definition still omits all capability keys. This proves that known provider data remains dynamic.
| assert_field '.Models.Roles.Main' 'ollama-qwen2-0-5b' "$config_json" || : | ||
| assert_field '.Models.Definitions[.Models.Roles.Main].Provider' 'ollama' "$config_json" || : | ||
| assert_field '.Models.Definitions[.Models.Roles.Main].ModelId' 'qwen2:0.5b' "$config_json" || : | ||
| assert_field '(.Models.Definitions[.Models.Roles.Main] | has("ContextWindow"))' 'false' "$config_json" || : |
There was a problem hiding this comment.
Rationale: Unit tests cover the writer, but the init wizard had its own data path. These assertions inspect the real config file. They catch any future wizard refactor that restores the bug.
1 participant
Summary
Producer and consumer
The CLI, model TUI, and init wizard produce named model definitions.
The daemon consumes absent capability fields through runtime provider detection.
Validation
dotnet test Netclaw.slnx --no-restore: 6,222 passed and 14 expected skips.dotnet slopwatch analyze: zero issues.pwsh ./scripts/Add-FileHeaders.ps1 -Verify: passed.git diff --check: passed.NETCLAW_EVAL_*credentials were absent.Fixes #1756.