From 325b2983fd2d915bca6857ddc0c761fa9adc58d9 Mon Sep 17 00:00:00 2001 From: Devis Lucato Date: Fri, 28 Nov 2025 15:00:35 +0100 Subject: [PATCH 1/2] Fix: km config now shows entire configuration, not just one node Problem: ConfigCommand called Initialize() which forced node selection, making it illogical to show only one node when querying the entire config. Solution: - ConfigCommand now uses GetConfig() directly (no node selection needed) - Default behavior shows all nodes + cache configuration - Added GetConfig() helper method in BaseCommand - Added tests to verify entire config is displayed Changes: - Add ConfigCommandTests with multi-node config verification - Update ConfigCommand to skip Initialize() for config queries - Add BaseCommand.GetConfig() protected method Test results: - Test written first (RED) - verified it failed - Code fixed (GREEN) - test now passes - All 262 tests passing (185 Main + 77 Core) - Zero regressions --- src/Main/CLI/Commands/BaseCommand.cs | 9 + src/Main/CLI/Commands/ConfigCommand.cs | 78 ++++---- .../Integration/ConfigCommandTests.cs | 166 ++++++++++++++++++ 3 files changed, 223 insertions(+), 30 deletions(-) create mode 100644 tests/Main.Tests/Integration/ConfigCommandTests.cs diff --git a/src/Main/CLI/Commands/BaseCommand.cs b/src/Main/CLI/Commands/BaseCommand.cs index e62012a32..52d4d74d7 100644 --- a/src/Main/CLI/Commands/BaseCommand.cs +++ b/src/Main/CLI/Commands/BaseCommand.cs @@ -31,6 +31,15 @@ protected BaseCommand(AppConfig config) this._config = config ?? throw new ArgumentNullException(nameof(config)); } + /// + /// Gets the injected application configuration. + /// + /// The application configuration. + protected AppConfig GetConfig() + { + return this._config; + } + /// /// Initializes command dependencies: node and formatter. /// Config is already injected via constructor (no file I/O). diff --git a/src/Main/CLI/Commands/ConfigCommand.cs b/src/Main/CLI/Commands/ConfigCommand.cs index 14706c96f..2f09fe96b 100644 --- a/src/Main/CLI/Commands/ConfigCommand.cs +++ b/src/Main/CLI/Commands/ConfigCommand.cs @@ -41,15 +41,17 @@ public override async Task ExecuteAsync( { try { - var (config, node, formatter) = this.Initialize(settings); + // ConfigCommand doesn't need node selection - it queries the entire configuration + // So we skip Initialize() and just use the injected config directly + var formatter = CLI.OutputFormatters.OutputFormatterFactory.Create(settings); // Determine what to show object output; if (settings.ShowNodes) { - // Show all nodes - output = config.Nodes.Select(kvp => new NodeSummaryDto + // Show all nodes summary + output = this.GetConfig().Nodes.Select(kvp => new NodeSummaryDto { Id = kvp.Key, Access = kvp.Value.Access.ToString(), @@ -64,44 +66,60 @@ public override async Task ExecuteAsync( // Show cache configuration output = new CacheInfoDto { - EmbeddingsCache = config.EmbeddingsCache != null ? new CacheConfigDto + EmbeddingsCache = this.GetConfig().EmbeddingsCache != null ? new CacheConfigDto { - Type = config.EmbeddingsCache.Type.ToString(), - Path = config.EmbeddingsCache.Path + Type = this.GetConfig().EmbeddingsCache.Type.ToString(), + Path = this.GetConfig().EmbeddingsCache.Path } : null, - LlmCache = config.LLMCache != null ? new CacheConfigDto + LlmCache = this.GetConfig().LLMCache != null ? new CacheConfigDto { - Type = config.LLMCache.Type.ToString(), - Path = config.LLMCache.Path + Type = this.GetConfig().LLMCache.Type.ToString(), + Path = this.GetConfig().LLMCache.Path } : null }; } else { - // Default: show current node details - output = new NodeDetailsDto + // Default: show entire configuration with all nodes + output = new { - NodeId = node.Id, - Access = node.Access.ToString(), - ContentIndex = new ContentIndexConfigDto - { - Type = node.ContentIndex.Type.ToString(), - Path = node.ContentIndex is KernelMemory.Core.Config.ContentIndex.SqliteContentIndexConfig sqlite - ? sqlite.Path - : null - }, - FileStorage = node.FileStorage != null ? new StorageConfigDto + Nodes = this.GetConfig().Nodes.Select(kvp => new NodeDetailsDto { - Type = node.FileStorage.Type.ToString() - } : null, - RepoStorage = node.RepoStorage != null ? new StorageConfigDto - { - Type = node.RepoStorage.Type.ToString() - } : null, - SearchIndexes = node.SearchIndexes.Select(si => new SearchIndexDto + NodeId = kvp.Key, + Access = kvp.Value.Access.ToString(), + ContentIndex = new ContentIndexConfigDto + { + Type = kvp.Value.ContentIndex.Type.ToString(), + Path = kvp.Value.ContentIndex is KernelMemory.Core.Config.ContentIndex.SqliteContentIndexConfig sqlite + ? sqlite.Path + : null + }, + FileStorage = kvp.Value.FileStorage != null ? new StorageConfigDto + { + Type = kvp.Value.FileStorage.Type.ToString() + } : null, + RepoStorage = kvp.Value.RepoStorage != null ? new StorageConfigDto + { + Type = kvp.Value.RepoStorage.Type.ToString() + } : null, + SearchIndexes = kvp.Value.SearchIndexes.Select(si => new SearchIndexDto + { + Type = si.Type.ToString() + }).ToList() + }).ToList(), + Cache = new CacheInfoDto { - Type = si.Type.ToString() - }).ToList() + EmbeddingsCache = this.GetConfig().EmbeddingsCache != null ? new CacheConfigDto + { + Type = this.GetConfig().EmbeddingsCache.Type.ToString(), + Path = this.GetConfig().EmbeddingsCache.Path + } : null, + LlmCache = this.GetConfig().LLMCache != null ? new CacheConfigDto + { + Type = this.GetConfig().LLMCache.Type.ToString(), + Path = this.GetConfig().LLMCache.Path + } : null + } }; } diff --git a/tests/Main.Tests/Integration/ConfigCommandTests.cs b/tests/Main.Tests/Integration/ConfigCommandTests.cs new file mode 100644 index 000000000..50eead491 --- /dev/null +++ b/tests/Main.Tests/Integration/ConfigCommandTests.cs @@ -0,0 +1,166 @@ +// Copyright (c) Microsoft. All rights reserved. + +using KernelMemory.Core.Config; +using KernelMemory.Main.CLI.Commands; +using Spectre.Console.Cli; +using Xunit; + +namespace KernelMemory.Main.Tests.Integration; + +/// +/// Tests for ConfigCommand behavior. +/// These tests verify that config command shows the entire configuration, +/// not just a single node. +/// +public sealed class ConfigCommandTests : IDisposable +{ + private readonly string _tempDir; + private readonly string _configPath; + + public ConfigCommandTests() + { + this._tempDir = Path.Combine(Path.GetTempPath(), $"km-config-test-{Guid.NewGuid():N}"); + Directory.CreateDirectory(this._tempDir); + + this._configPath = Path.Combine(this._tempDir, "config.json"); + + // Create test config with MULTIPLE nodes to verify entire config is shown + var config = new AppConfig + { + Nodes = new Dictionary + { + ["personal"] = NodeConfig.CreateDefaultPersonalNode(Path.Combine(this._tempDir, "nodes", "personal")), + ["work"] = NodeConfig.CreateDefaultPersonalNode(Path.Combine(this._tempDir, "nodes", "work")), + ["shared"] = NodeConfig.CreateDefaultPersonalNode(Path.Combine(this._tempDir, "nodes", "shared")) + } + }; + + var json = System.Text.Json.JsonSerializer.Serialize(config); + File.WriteAllText(this._configPath, json); + } + + public void Dispose() + { + try + { + if (Directory.Exists(this._tempDir)) + { + Directory.Delete(this._tempDir, true); + } + } + catch (IOException) + { + // Ignore cleanup errors + } + catch (UnauthorizedAccessException) + { + // Ignore cleanup errors + } + } + + [Fact] + public void ConfigCommand_WithoutFlags_ShouldShowEntireConfiguration() + { + // This test verifies the bug: km config should show ALL nodes, not just the selected one + + // Arrange + var config = ConfigParser.LoadFromFile(this._configPath); + + var settings = new ConfigCommandSettings + { + ConfigPath = this._configPath, + Format = "json" // Use JSON format for easier assertion + }; + + var command = new ConfigCommand(config); + var context = new CommandContext( + new[] { "--config", this._configPath }, + new EmptyRemainingArguments(), + "config", + null); + + // Capture stdout to verify output + using var outputCapture = new StringWriter(); + var originalOutput = Console.Out; + Console.SetOut(outputCapture); + + try + { + // Act + var exitCode = command.ExecuteAsync(context, settings).GetAwaiter().GetResult(); + + // Assert + Assert.Equal(Constants.ExitCodeSuccess, exitCode); + + var output = outputCapture.ToString(); + + // The output should contain ALL THREE nodes, not just one + Assert.Contains("personal", output); + Assert.Contains("work", output); + Assert.Contains("shared", output); + + // Verify it's showing the entire config structure + // Current bug: it only shows the first node's details + // Expected: it should show all nodes + } + finally + { + Console.SetOut(originalOutput); + } + } + + [Fact] + public void ConfigCommand_WithShowNodesFlag_ShouldShowAllNodesSummary() + { + // Arrange + var config = ConfigParser.LoadFromFile(this._configPath); + + var settings = new ConfigCommandSettings + { + ConfigPath = this._configPath, + Format = "json", + ShowNodes = true + }; + + var command = new ConfigCommand(config); + var context = new CommandContext( + new[] { "--config", this._configPath }, + new EmptyRemainingArguments(), + "config", + null); + + // Capture stdout + using var outputCapture = new StringWriter(); + var originalOutput = Console.Out; + Console.SetOut(outputCapture); + + try + { + // Act + var exitCode = command.ExecuteAsync(context, settings).GetAwaiter().GetResult(); + + // Assert + Assert.Equal(Constants.ExitCodeSuccess, exitCode); + + var output = outputCapture.ToString(); + + // Should show all three nodes + Assert.Contains("personal", output); + Assert.Contains("work", output); + Assert.Contains("shared", output); + } + finally + { + Console.SetOut(originalOutput); + } + } + + /// + /// Helper class to provide empty remaining arguments for CommandContext. + /// + private sealed class EmptyRemainingArguments : IRemainingArguments + { + public IReadOnlyList Raw => Array.Empty(); + public ILookup Parsed => Enumerable.Empty().ToLookup(x => x, x => (string?)null); + } +} From 424edc92a74fd443f8748671aeac80565b4927dd Mon Sep 17 00:00:00 2001 From: Devis Lucato Date: Fri, 28 Nov 2025 15:44:02 +0100 Subject: [PATCH 2/2] Update Definition of Done and add mandatory todo usage --- AGENTS.md | 11 +++++++--- src/Main/CLI/Commands/BaseCommand.cs | 6 +----- src/Main/CLI/Commands/ConfigCommand.cs | 30 ++++++++++++++------------ 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 54402a2b8..08aef0e4b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,11 +1,16 @@ - Follow @docs/AGENTS.md instructions without exceptions - Ignore the "archived" directory +- For any task with multiple steps, create a todo list FIRST, then execute. +- Avoid destructive operations like "get reset", "git clean", etc. # Definition of done -- `format.sh` is passing without errors or warnings -- `build.sh` is passing without errors or warnings -- `coverage.sh` is passing without errors or warnings, coverage > 80% (use coverage reports to find which code is not covered) +- [ ] instructions in `docs/AGENTS.md` have been followed without exceptions +- [ ] magic values and constants are centralized in `Constants.cs` +- [ ] `build.sh` runs successfully without warnings or errors +- [ ] `format.sh` runs successfully without warnings or errors +- [ ] `coverage.sh` runs successfully without warnings or errors +- [ ] there are zero skipped tests # C# Code Style diff --git a/src/Main/CLI/Commands/BaseCommand.cs b/src/Main/CLI/Commands/BaseCommand.cs index 52d4d74d7..6435c5341 100644 --- a/src/Main/CLI/Commands/BaseCommand.cs +++ b/src/Main/CLI/Commands/BaseCommand.cs @@ -34,11 +34,7 @@ protected BaseCommand(AppConfig config) /// /// Gets the injected application configuration. /// - /// The application configuration. - protected AppConfig GetConfig() - { - return this._config; - } + protected AppConfig Config => this._config; /// /// Initializes command dependencies: node and formatter. diff --git a/src/Main/CLI/Commands/ConfigCommand.cs b/src/Main/CLI/Commands/ConfigCommand.cs index 2f09fe96b..fb623fb61 100644 --- a/src/Main/CLI/Commands/ConfigCommand.cs +++ b/src/Main/CLI/Commands/ConfigCommand.cs @@ -51,7 +51,7 @@ public override async Task ExecuteAsync( if (settings.ShowNodes) { // Show all nodes summary - output = this.GetConfig().Nodes.Select(kvp => new NodeSummaryDto + output = this.Config.Nodes.Select(kvp => new NodeSummaryDto { Id = kvp.Key, Access = kvp.Value.Access.ToString(), @@ -64,26 +64,28 @@ public override async Task ExecuteAsync( else if (settings.ShowCache) { // Show cache configuration + var config = this.Config; output = new CacheInfoDto { - EmbeddingsCache = this.GetConfig().EmbeddingsCache != null ? new CacheConfigDto + EmbeddingsCache = config.EmbeddingsCache != null ? new CacheConfigDto { - Type = this.GetConfig().EmbeddingsCache.Type.ToString(), - Path = this.GetConfig().EmbeddingsCache.Path + Type = config.EmbeddingsCache.Type.ToString(), + Path = config.EmbeddingsCache.Path } : null, - LlmCache = this.GetConfig().LLMCache != null ? new CacheConfigDto + LlmCache = config.LLMCache != null ? new CacheConfigDto { - Type = this.GetConfig().LLMCache.Type.ToString(), - Path = this.GetConfig().LLMCache.Path + Type = config.LLMCache.Type.ToString(), + Path = config.LLMCache.Path } : null }; } else { // Default: show entire configuration with all nodes + var config = this.Config; output = new { - Nodes = this.GetConfig().Nodes.Select(kvp => new NodeDetailsDto + Nodes = config.Nodes.Select(kvp => new NodeDetailsDto { NodeId = kvp.Key, Access = kvp.Value.Access.ToString(), @@ -109,15 +111,15 @@ public override async Task ExecuteAsync( }).ToList(), Cache = new CacheInfoDto { - EmbeddingsCache = this.GetConfig().EmbeddingsCache != null ? new CacheConfigDto + EmbeddingsCache = config.EmbeddingsCache != null ? new CacheConfigDto { - Type = this.GetConfig().EmbeddingsCache.Type.ToString(), - Path = this.GetConfig().EmbeddingsCache.Path + Type = config.EmbeddingsCache.Type.ToString(), + Path = config.EmbeddingsCache.Path } : null, - LlmCache = this.GetConfig().LLMCache != null ? new CacheConfigDto + LlmCache = config.LLMCache != null ? new CacheConfigDto { - Type = this.GetConfig().LLMCache.Type.ToString(), - Path = this.GetConfig().LLMCache.Path + Type = config.LLMCache.Type.ToString(), + Path = config.LLMCache.Path } : null } };