From c8e50d7fb439222ea6ae6c059f70d01cac00654a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 21:31:32 +0000 Subject: [PATCH 01/11] Initial plan From 248a0aaf18e721bdd2db85be91a46981ad9a9a4b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 21:40:47 +0000 Subject: [PATCH 02/11] feat: Add --lint support for validating definition files Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/a75bab3c-44b6-4dcf-8da7-eb85c41f6ec3 --- requirements.yaml | 22 ++ src/DemaConsulting.ReviewMark/Context.cs | 15 ++ src/DemaConsulting.ReviewMark/Program.cs | 73 +++++- src/DemaConsulting.ReviewMark/Validation.cs | 33 +++ .../ContextTests.cs | 29 +++ .../ProgramTests.cs | 239 ++++++++++++++++++ 6 files changed, 410 insertions(+), 1 deletion(-) diff --git a/requirements.yaml b/requirements.yaml index 2536894..fb4022e 100644 --- a/requirements.yaml +++ b/requirements.yaml @@ -244,6 +244,22 @@ sections: - Program_Run_WithElaborateFlag_UnknownId_ReportsError - ReviewMark_Elaborate + - id: ReviewMark-Cmd-Lint + title: The tool shall support --lint flag to validate the definition file and report issues. + justification: | + Users need a way to verify that the .reviewmark.yaml configuration file is valid + before running the main tool, providing clear error messages about the cause and + location of any issues. + tests: + - Context_Create_LintFlag_SetsLintTrue + - Context_Create_NoArguments_LintIsFalse + - Program_Run_WithHelpFlag_IncludesLintOption + - Program_Run_WithLintFlag_ValidConfig_ReportsSuccess + - Program_Run_WithLintFlag_MissingConfig_ReportsError + - Program_Run_WithLintFlag_DuplicateIds_ReportsError + - Program_Run_WithLintFlag_UnknownSourceType_ReportsError + - ReviewMark_Lint + - title: Configuration Reading requirements: - id: ReviewMark-Config-Reading @@ -321,6 +337,7 @@ sections: - "windows@ReviewMark_Enforce" - "windows@ReviewMark_WorkingDirectoryOverride" - "windows@ReviewMark_Elaborate" + - "windows@ReviewMark_Lint" - id: ReviewMark-Platform-Linux title: The tool shall build and run on Linux platforms. @@ -336,6 +353,7 @@ sections: - "ubuntu@ReviewMark_Enforce" - "ubuntu@ReviewMark_WorkingDirectoryOverride" - "ubuntu@ReviewMark_Elaborate" + - "ubuntu@ReviewMark_Lint" - id: ReviewMark-Platform-MacOS title: The tool shall build and run on macOS platforms. @@ -351,6 +369,7 @@ sections: - "macos@ReviewMark_Enforce" - "macos@ReviewMark_WorkingDirectoryOverride" - "macos@ReviewMark_Elaborate" + - "macos@ReviewMark_Lint" - id: ReviewMark-Platform-Net8 title: The tool shall support .NET 8 runtime. @@ -365,6 +384,7 @@ sections: - "dotnet8.x@ReviewMark_Enforce" - "dotnet8.x@ReviewMark_WorkingDirectoryOverride" - "dotnet8.x@ReviewMark_Elaborate" + - "dotnet8.x@ReviewMark_Lint" - id: ReviewMark-Platform-Net9 title: The tool shall support .NET 9 runtime. @@ -379,6 +399,7 @@ sections: - "dotnet9.x@ReviewMark_Enforce" - "dotnet9.x@ReviewMark_WorkingDirectoryOverride" - "dotnet9.x@ReviewMark_Elaborate" + - "dotnet9.x@ReviewMark_Lint" - id: ReviewMark-Platform-Net10 title: The tool shall support .NET 10 runtime. @@ -393,6 +414,7 @@ sections: - "dotnet10.x@ReviewMark_Enforce" - "dotnet10.x@ReviewMark_WorkingDirectoryOverride" - "dotnet10.x@ReviewMark_Elaborate" + - "dotnet10.x@ReviewMark_Lint" - title: OTS Software requirements: diff --git a/src/DemaConsulting.ReviewMark/Context.cs b/src/DemaConsulting.ReviewMark/Context.cs index bf7fa11..e2315e7 100644 --- a/src/DemaConsulting.ReviewMark/Context.cs +++ b/src/DemaConsulting.ReviewMark/Context.cs @@ -60,6 +60,11 @@ internal sealed class Context : IDisposable /// public bool Validate { get; private init; } + /// + /// Gets a value indicating whether the lint flag was specified. + /// + public bool Lint { get; private init; } + /// /// Gets the validation results file path. /// @@ -159,6 +164,7 @@ public static Context Create(string[] args) Help = parser.Help, Silent = parser.Silent, Validate = parser.Validate, + Lint = parser.Lint, ResultsFile = parser.ResultsFile, DefinitionFile = parser.DefinitionFile, PlanFile = parser.PlanFile, @@ -226,6 +232,11 @@ private sealed class ArgumentParser /// public bool Validate { get; private set; } + /// + /// Gets a value indicating whether the lint flag was specified. + /// + public bool Lint { get; private set; } + /// /// Gets the log file path. /// @@ -328,6 +339,10 @@ private int ParseArgument(string arg, string[] args, int index) Validate = true; return index; + case "--lint": + Lint = true; + return index; + case "--log": LogFile = GetRequiredStringArgument(arg, args, index, FilenameArgument); return index + 1; diff --git a/src/DemaConsulting.ReviewMark/Program.cs b/src/DemaConsulting.ReviewMark/Program.cs index a87a942..392b93b 100644 --- a/src/DemaConsulting.ReviewMark/Program.cs +++ b/src/DemaConsulting.ReviewMark/Program.cs @@ -112,7 +112,14 @@ public static void Run(Context context) return; } - // Priority 4: Main tool functionality + // Priority 4: Lint + if (context.Lint) + { + RunLintLogic(context); + return; + } + + // Priority 5: Main tool functionality RunToolLogic(context); } @@ -140,6 +147,7 @@ private static void PrintHelp(Context context) context.WriteLine(" -?, -h, --help Display this help message"); context.WriteLine(" --silent Suppress console output"); context.WriteLine(" --validate Run self-validation"); + context.WriteLine(" --lint Lint the definition file and report issues"); context.WriteLine(" --results Write validation results to file (.trx or .xml)"); context.WriteLine(" --log Write output to log file"); context.WriteLine(" --definition Specify the definition YAML file (default: .reviewmark.yaml)"); @@ -154,6 +162,69 @@ private static void PrintHelp(Context context) context.WriteLine(" --elaborate Print a Markdown elaboration of the specified review set"); } + /// + /// Runs the lint logic to validate the definition file. + /// + /// The context containing command line arguments and program state. + private static void RunLintLogic(Context context) + { + // Determine the definition file path (explicit or default) + var directory = context.WorkingDirectory ?? Directory.GetCurrentDirectory(); + var definitionFile = context.DefinitionFile ?? PathHelpers.SafePathCombine(directory, ".reviewmark.yaml"); + + context.WriteLine($"Linting '{definitionFile}'..."); + + // Try to load the configuration file + ReviewMarkConfiguration config; + try + { + config = ReviewMarkConfiguration.Load(definitionFile); + } + catch (InvalidOperationException ex) + { + context.WriteError($"Error: {ex.Message}"); + return; + } + catch (ArgumentException ex) + { + context.WriteError($"Error: {ex.Message}"); + return; + } + + // Perform semantic validation: check for unknown evidence-source type + var hasErrors = false; + if (!string.Equals(config.EvidenceSource.Type, "url", StringComparison.OrdinalIgnoreCase) && + !string.Equals(config.EvidenceSource.Type, "fileshare", StringComparison.OrdinalIgnoreCase)) + { + context.WriteError( + $"Error: evidence-source type '{config.EvidenceSource.Type}' is not supported (must be 'url' or 'fileshare')."); + hasErrors = true; + } + + // Perform semantic validation: check for duplicate review set IDs + var seenIds = new Dictionary(StringComparer.Ordinal); + for (var i = 0; i < config.Reviews.Count; i++) + { + var review = config.Reviews[i]; + if (seenIds.TryGetValue(review.Id, out var firstIndex)) + { + context.WriteError( + $"Error: reviews[{i}] has duplicate ID '{review.Id}' (first defined at reviews[{firstIndex}])."); + hasErrors = true; + } + else + { + seenIds[review.Id] = i; + } + } + + // Report overall result + if (!hasErrors) + { + context.WriteLine($"'{definitionFile}' is valid."); + } + } + /// /// Runs the main tool logic. /// diff --git a/src/DemaConsulting.ReviewMark/Validation.cs b/src/DemaConsulting.ReviewMark/Validation.cs index ce93b16..41a2950 100644 --- a/src/DemaConsulting.ReviewMark/Validation.cs +++ b/src/DemaConsulting.ReviewMark/Validation.cs @@ -56,6 +56,7 @@ public static void Run(Context context) RunDirTest(context, testResults); RunEnforceTest(context, testResults); RunElaborateTest(context, testResults); + RunLintTest(context, testResults); // Calculate totals var totalTests = testResults.Results.Count; @@ -378,6 +379,38 @@ private static void RunElaborateTest(Context context, DemaConsulting.TestResults }); } + /// + /// Runs a test for lint functionality. + /// + /// The context for output. + /// The test results collection. + private static void RunLintTest(Context context, DemaConsulting.TestResults.TestResults testResults) + { + RunValidationTest(context, testResults, "ReviewMark_Lint", () => + { + using var tempDir = new TemporaryDirectory(); + var (definitionFile, _) = CreateTestDefinitionFixtures(tempDir.DirectoryPath); + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "lint-test.log"); + + // Run the program to lint the definition file + int exitCode; + using (var testContext = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile])) + { + Program.Run(testContext); + exitCode = testContext.ExitCode; + } + + if (exitCode != 0) + { + return $"Program exited with code {exitCode}"; + } + + // Verify the log contains a success message + var logContent = File.ReadAllText(logFile); + return logContent.Contains("is valid") ? null : "Lint output does not contain 'is valid'"; + }); + } + /// /// Runs a single validation test, recording the outcome in the test results collection. /// diff --git a/test/DemaConsulting.ReviewMark.Tests/ContextTests.cs b/test/DemaConsulting.ReviewMark.Tests/ContextTests.cs index 9df417d..e360756 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ContextTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ContextTests.cs @@ -712,5 +712,34 @@ public void Context_Create_ElaborateFlag_WithoutValue_ThrowsArgumentException() // Act & Assert - --elaborate without an ID argument should throw Assert.Throws(() => Context.Create(["--elaborate"])); } + + /// + /// Test that --lint flag sets Lint to true. + /// + [TestMethod] + public void Context_Create_LintFlag_SetsLintTrue() + { + // Act + using var context = Context.Create(["--lint"]); + + // Assert — Lint is true, other flags remain false, and exit code is zero + Assert.IsTrue(context.Lint); + Assert.IsFalse(context.Version); + Assert.IsFalse(context.Help); + Assert.AreEqual(0, context.ExitCode); + } + + /// + /// Test that Lint is false when --lint is not specified. + /// + [TestMethod] + public void Context_Create_NoArguments_LintIsFalse() + { + // Act + using var context = Context.Create([]); + + // Assert — Lint is false when --lint is not specified + Assert.IsFalse(context.Lint); + } } diff --git a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs index 18ab989..6e6dc18 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs @@ -306,4 +306,243 @@ public void Program_Run_WithElaborateFlag_UnknownId_ReportsError() } } } + + /// + /// Test that Run with --help flag includes --lint in the usage information. + /// + [TestMethod] + public void Program_Run_WithHelpFlag_IncludesLintOption() + { + // Arrange + var originalOut = Console.Out; + try + { + using var outWriter = new StringWriter(); + Console.SetOut(outWriter); + using var context = Context.Create(["--help"]); + + // Act + Program.Run(context); + + // Assert — help text includes the --lint option + var output = outWriter.ToString(); + Assert.Contains("--lint", output); + } + finally + { + Console.SetOut(originalOut); + } + } + + /// + /// Test that Run with --lint flag on a valid definition file reports success. + /// + [TestMethod] + public void Program_Run_WithLintFlag_ValidConfig_ReportsSuccess() + { + // Arrange — create temp directory with a valid definition file + var testDirectory = PathHelpers.SafePathCombine( + Path.GetTempPath(), $"ProgramTests_Lint_{Guid.NewGuid()}"); + try + { + Directory.CreateDirectory(testDirectory); + var indexFile = PathHelpers.SafePathCombine(testDirectory, "index.json"); + File.WriteAllText(indexFile, """{"reviews":[]}"""); + + var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); + File.WriteAllText(definitionFile, $""" + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: {indexFile} + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + var originalOut = Console.Out; + try + { + using var outWriter = new StringWriter(); + Console.SetOut(outWriter); + using var context = Context.Create(["--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — exit code is zero and output contains success message + var output = outWriter.ToString(); + Assert.AreEqual(0, context.ExitCode); + Assert.Contains("is valid", output); + } + finally + { + Console.SetOut(originalOut); + } + } + finally + { + if (Directory.Exists(testDirectory)) + { + Directory.Delete(testDirectory, recursive: true); + } + } + } + + /// + /// Test that Run with --lint flag on a missing definition file reports an error. + /// + [TestMethod] + public void Program_Run_WithLintFlag_MissingConfig_ReportsError() + { + // Arrange — use a non-existent definition file + var testDirectory = PathHelpers.SafePathCombine( + Path.GetTempPath(), $"ProgramTests_LintMissing_{Guid.NewGuid()}"); + Directory.CreateDirectory(testDirectory); + try + { + var nonExistentFile = PathHelpers.SafePathCombine(testDirectory, "nonexistent.yaml"); + + var originalError = Console.Error; + try + { + using var errWriter = new StringWriter(); + Console.SetError(errWriter); + using var context = Context.Create(["--silent", "--lint", "--definition", nonExistentFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code when the definition file does not exist + Assert.AreEqual(1, context.ExitCode); + } + finally + { + Console.SetError(originalError); + } + } + finally + { + if (Directory.Exists(testDirectory)) + { + Directory.Delete(testDirectory, recursive: true); + } + } + } + + /// + /// Test that Run with --lint flag detects duplicate review set IDs and reports an error. + /// + [TestMethod] + public void Program_Run_WithLintFlag_DuplicateIds_ReportsError() + { + // Arrange — create temp directory with a definition file containing duplicate IDs + var testDirectory = PathHelpers.SafePathCombine( + Path.GetTempPath(), $"ProgramTests_LintDuplicate_{Guid.NewGuid()}"); + try + { + Directory.CreateDirectory(testDirectory); + var indexFile = PathHelpers.SafePathCombine(testDirectory, "index.json"); + File.WriteAllText(indexFile, """{"reviews":[]}"""); + + var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); + File.WriteAllText(definitionFile, $""" + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: {indexFile} + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + - id: Core-Logic + title: Duplicate review set + paths: + - "src/**/*.cs" + """); + + var originalError = Console.Error; + try + { + using var errWriter = new StringWriter(); + Console.SetError(errWriter); + using var context = Context.Create(["--silent", "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code when duplicate IDs are found + Assert.AreEqual(1, context.ExitCode); + } + finally + { + Console.SetError(originalError); + } + } + finally + { + if (Directory.Exists(testDirectory)) + { + Directory.Delete(testDirectory, recursive: true); + } + } + } + + /// + /// Test that Run with --lint flag detects unknown evidence-source type and reports an error. + /// + [TestMethod] + public void Program_Run_WithLintFlag_UnknownSourceType_ReportsError() + { + // Arrange — create temp directory with a definition file having an unknown source type + var testDirectory = PathHelpers.SafePathCombine( + Path.GetTempPath(), $"ProgramTests_LintUnknownType_{Guid.NewGuid()}"); + try + { + Directory.CreateDirectory(testDirectory); + var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); + File.WriteAllText(definitionFile, """ + needs-review: + - "src/**/*.cs" + evidence-source: + type: ftp + location: ftp://example.com/index.json + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + var originalError = Console.Error; + try + { + using var errWriter = new StringWriter(); + Console.SetError(errWriter); + using var context = Context.Create(["--silent", "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code when source type is unknown + Assert.AreEqual(1, context.ExitCode); + } + finally + { + Console.SetError(originalError); + } + } + finally + { + if (Directory.Exists(testDirectory)) + { + Directory.Delete(testDirectory, recursive: true); + } + } + } } From 59ccfabb93326c79b50ac6acf6bce64f63f6320b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 03:20:51 +0000 Subject: [PATCH 03/11] refactor: address review feedback - TestDirectory helper, error message assertions, corrupted YAML tests Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/245f177b-039d-4058-a8eb-3fce0818cd47 --- requirements.yaml | 2 + .../ProgramTests.cs | 495 ++++++++---------- .../TestDirectory.cs | 52 ++ 3 files changed, 283 insertions(+), 266 deletions(-) create mode 100644 test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs diff --git a/requirements.yaml b/requirements.yaml index fb4022e..6f1649c 100644 --- a/requirements.yaml +++ b/requirements.yaml @@ -258,6 +258,8 @@ sections: - Program_Run_WithLintFlag_MissingConfig_ReportsError - Program_Run_WithLintFlag_DuplicateIds_ReportsError - Program_Run_WithLintFlag_UnknownSourceType_ReportsError + - Program_Run_WithLintFlag_CorruptedYaml_ReportsError + - Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError - ReviewMark_Lint - title: Configuration Reading diff --git a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs index 6e6dc18..e54de5c 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs @@ -26,6 +26,10 @@ namespace DemaConsulting.ReviewMark.Tests; [TestClass] public class ProgramTests { + /// + /// Log file name used across lint tests. + /// + private const string LintLogFile = "lint.log"; /// /// Test that Run with version flag displays version only. /// @@ -187,63 +191,51 @@ public void Program_Run_WithHelpFlag_IncludesElaborateOption() public void Program_Run_WithElaborateFlag_OutputsElaboration() { // Arrange — create temp directory with a definition file and source file - var testDirectory = PathHelpers.SafePathCombine( - Path.GetTempPath(), $"ProgramTests_Elaborate_{Guid.NewGuid()}"); + using var tempDir = new TestDirectory(); + var srcDir = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "src"); + Directory.CreateDirectory(srcDir); + File.WriteAllText(PathHelpers.SafePathCombine(srcDir, "A.cs"), "class A {}"); + + var indexFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "index.json"); + File.WriteAllText(indexFile, """{"reviews":[]}"""); + + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, $""" + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: {indexFile} + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + var originalOut = Console.Out; try { - Directory.CreateDirectory(testDirectory); - var srcDir = PathHelpers.SafePathCombine(testDirectory, "src"); - Directory.CreateDirectory(srcDir); - File.WriteAllText(PathHelpers.SafePathCombine(srcDir, "A.cs"), "class A {}"); + using var outWriter = new StringWriter(); + Console.SetOut(outWriter); + using var context = Context.Create([ + "--definition", definitionFile, + "--dir", tempDir.DirectoryPath, + "--elaborate", "Core-Logic"]); - var indexFile = PathHelpers.SafePathCombine(testDirectory, "index.json"); - File.WriteAllText(indexFile, """{"reviews":[]}"""); + // Act + Program.Run(context); - var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); - File.WriteAllText(definitionFile, $""" - needs-review: - - "src/**/*.cs" - evidence-source: - type: fileshare - location: {indexFile} - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - var originalOut = Console.Out; - try - { - using var outWriter = new StringWriter(); - Console.SetOut(outWriter); - using var context = Context.Create([ - "--definition", definitionFile, - "--dir", testDirectory, - "--elaborate", "Core-Logic"]); - - // Act - Program.Run(context); - - // Assert — output contains the review set ID and fingerprint heading - var output = outWriter.ToString(); - Assert.Contains("Core-Logic", output); - Assert.Contains("Fingerprint", output); - Assert.Contains("Files", output); - Assert.AreEqual(0, context.ExitCode); - } - finally - { - Console.SetOut(originalOut); - } + // Assert — output contains the review set ID and fingerprint heading + var output = outWriter.ToString(); + Assert.Contains("Core-Logic", output); + Assert.Contains("Fingerprint", output); + Assert.Contains("Files", output); + Assert.AreEqual(0, context.ExitCode); } finally { - if (Directory.Exists(testDirectory)) - { - Directory.Delete(testDirectory, recursive: true); - } + Console.SetOut(originalOut); } } @@ -254,56 +246,44 @@ public void Program_Run_WithElaborateFlag_OutputsElaboration() public void Program_Run_WithElaborateFlag_UnknownId_ReportsError() { // Arrange — create temp directory with a definition file - var testDirectory = PathHelpers.SafePathCombine( - Path.GetTempPath(), $"ProgramTests_ElaborateUnknown_{Guid.NewGuid()}"); + using var tempDir = new TestDirectory(); + + var indexFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "index.json"); + File.WriteAllText(indexFile, """{"reviews":[]}"""); + + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, $""" + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: {indexFile} + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + var originalError = Console.Error; try { - Directory.CreateDirectory(testDirectory); + using var errWriter = new StringWriter(); + Console.SetError(errWriter); + using var context = Context.Create([ + "--silent", + "--definition", definitionFile, + "--elaborate", "Unknown-Id"]); - var indexFile = PathHelpers.SafePathCombine(testDirectory, "index.json"); - File.WriteAllText(indexFile, """{"reviews":[]}"""); + // Act + Program.Run(context); - var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); - File.WriteAllText(definitionFile, $""" - needs-review: - - "src/**/*.cs" - evidence-source: - type: fileshare - location: {indexFile} - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - var originalError = Console.Error; - try - { - using var errWriter = new StringWriter(); - Console.SetError(errWriter); - using var context = Context.Create([ - "--silent", - "--definition", definitionFile, - "--elaborate", "Unknown-Id"]); - - // Act - Program.Run(context); - - // Assert — non-zero exit code when the review-set ID is not found - Assert.AreEqual(1, context.ExitCode); - } - finally - { - Console.SetError(originalError); - } + // Assert — non-zero exit code when the review-set ID is not found + Assert.AreEqual(1, context.ExitCode); } finally { - if (Directory.Exists(testDirectory)) - { - Directory.Delete(testDirectory, recursive: true); - } + Console.SetError(originalError); } } @@ -341,55 +321,34 @@ public void Program_Run_WithHelpFlag_IncludesLintOption() public void Program_Run_WithLintFlag_ValidConfig_ReportsSuccess() { // Arrange — create temp directory with a valid definition file - var testDirectory = PathHelpers.SafePathCombine( - Path.GetTempPath(), $"ProgramTests_Lint_{Guid.NewGuid()}"); - try - { - Directory.CreateDirectory(testDirectory); - var indexFile = PathHelpers.SafePathCombine(testDirectory, "index.json"); - File.WriteAllText(indexFile, """{"reviews":[]}"""); - - var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); - File.WriteAllText(definitionFile, $""" - needs-review: + using var tempDir = new TestDirectory(); + var indexFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "index.json"); + File.WriteAllText(indexFile, """{"reviews":[]}"""); + + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, $""" + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: {indexFile} + reviews: + - id: Core-Logic + title: Review of core business logic + paths: - "src/**/*.cs" - evidence-source: - type: fileshare - location: {indexFile} - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - var originalOut = Console.Out; - try - { - using var outWriter = new StringWriter(); - Console.SetOut(outWriter); - using var context = Context.Create(["--lint", "--definition", definitionFile]); - - // Act - Program.Run(context); - - // Assert — exit code is zero and output contains success message - var output = outWriter.ToString(); - Assert.AreEqual(0, context.ExitCode); - Assert.Contains("is valid", output); - } - finally - { - Console.SetOut(originalOut); - } - } - finally - { - if (Directory.Exists(testDirectory)) - { - Directory.Delete(testDirectory, recursive: true); - } - } + """); + + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — exit code is zero and log contains success message + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(0, context.ExitCode); + Assert.Contains("is valid", logContent); } /// @@ -399,38 +358,19 @@ public void Program_Run_WithLintFlag_ValidConfig_ReportsSuccess() public void Program_Run_WithLintFlag_MissingConfig_ReportsError() { // Arrange — use a non-existent definition file - var testDirectory = PathHelpers.SafePathCombine( - Path.GetTempPath(), $"ProgramTests_LintMissing_{Guid.NewGuid()}"); - Directory.CreateDirectory(testDirectory); - try - { - var nonExistentFile = PathHelpers.SafePathCombine(testDirectory, "nonexistent.yaml"); - - var originalError = Console.Error; - try - { - using var errWriter = new StringWriter(); - Console.SetError(errWriter); - using var context = Context.Create(["--silent", "--lint", "--definition", nonExistentFile]); - - // Act - Program.Run(context); - - // Assert — non-zero exit code when the definition file does not exist - Assert.AreEqual(1, context.ExitCode); - } - finally - { - Console.SetError(originalError); - } - } - finally - { - if (Directory.Exists(testDirectory)) - { - Directory.Delete(testDirectory, recursive: true); - } - } + using var tempDir = new TestDirectory(); + var nonExistentFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "nonexistent.yaml"); + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", nonExistentFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code and log contains an error mentioning the missing file + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(1, context.ExitCode); + Assert.Contains("Error:", logContent); + Assert.Contains("nonexistent.yaml", logContent); } /// @@ -440,57 +380,40 @@ public void Program_Run_WithLintFlag_MissingConfig_ReportsError() public void Program_Run_WithLintFlag_DuplicateIds_ReportsError() { // Arrange — create temp directory with a definition file containing duplicate IDs - var testDirectory = PathHelpers.SafePathCombine( - Path.GetTempPath(), $"ProgramTests_LintDuplicate_{Guid.NewGuid()}"); - try - { - Directory.CreateDirectory(testDirectory); - var indexFile = PathHelpers.SafePathCombine(testDirectory, "index.json"); - File.WriteAllText(indexFile, """{"reviews":[]}"""); - - var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); - File.WriteAllText(definitionFile, $""" - needs-review: + using var tempDir = new TestDirectory(); + var indexFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "index.json"); + File.WriteAllText(indexFile, """{"reviews":[]}"""); + + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, $""" + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: {indexFile} + reviews: + - id: Core-Logic + title: Review of core business logic + paths: - "src/**/*.cs" - evidence-source: - type: fileshare - location: {indexFile} - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - - id: Core-Logic - title: Duplicate review set - paths: - - "src/**/*.cs" - """); - - var originalError = Console.Error; - try - { - using var errWriter = new StringWriter(); - Console.SetError(errWriter); - using var context = Context.Create(["--silent", "--lint", "--definition", definitionFile]); - - // Act - Program.Run(context); - - // Assert — non-zero exit code when duplicate IDs are found - Assert.AreEqual(1, context.ExitCode); - } - finally - { - Console.SetError(originalError); - } - } - finally - { - if (Directory.Exists(testDirectory)) - { - Directory.Delete(testDirectory, recursive: true); - } - } + - id: Core-Logic + title: Duplicate review set + paths: + - "src/**/*.cs" + """); + + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code and log contains a clear duplicate-ID error message + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(1, context.ExitCode); + Assert.Contains("Error:", logContent); + Assert.Contains("duplicate ID", logContent); + Assert.Contains("Core-Logic", logContent); } /// @@ -500,49 +423,89 @@ public void Program_Run_WithLintFlag_DuplicateIds_ReportsError() public void Program_Run_WithLintFlag_UnknownSourceType_ReportsError() { // Arrange — create temp directory with a definition file having an unknown source type - var testDirectory = PathHelpers.SafePathCombine( - Path.GetTempPath(), $"ProgramTests_LintUnknownType_{Guid.NewGuid()}"); - try - { - Directory.CreateDirectory(testDirectory); - var definitionFile = PathHelpers.SafePathCombine(testDirectory, "definition.yaml"); - File.WriteAllText(definitionFile, """ - needs-review: + using var tempDir = new TestDirectory(); + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, """ + needs-review: + - "src/**/*.cs" + evidence-source: + type: ftp + location: ftp://example.com/index.json + reviews: + - id: Core-Logic + title: Review of core business logic + paths: - "src/**/*.cs" - evidence-source: - type: ftp - location: ftp://example.com/index.json - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - var originalError = Console.Error; - try - { - using var errWriter = new StringWriter(); - Console.SetError(errWriter); - using var context = Context.Create(["--silent", "--lint", "--definition", definitionFile]); - - // Act - Program.Run(context); - - // Assert — non-zero exit code when source type is unknown - Assert.AreEqual(1, context.ExitCode); - } - finally - { - Console.SetError(originalError); - } - } - finally - { - if (Directory.Exists(testDirectory)) - { - Directory.Delete(testDirectory, recursive: true); - } - } + """); + + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code and log contains a clear unsupported-type error message + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(1, context.ExitCode); + Assert.Contains("Error:", logContent); + Assert.Contains("ftp", logContent); + Assert.Contains("not supported", logContent); + } + + /// + /// Test that Run with --lint flag reports a clear error for corrupted (invalid) YAML. + /// + [TestMethod] + public void Program_Run_WithLintFlag_CorruptedYaml_ReportsError() + { + // Arrange — create a definition file with invalid YAML syntax + using var tempDir = new TestDirectory(); + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, """ + {{{this is not valid yaml + """); + + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code and log contains an error about invalid content + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(1, context.ExitCode); + Assert.Contains("Error:", logContent); + } + + /// + /// Test that Run with --lint flag reports a clear error when required fields are missing. + /// + [TestMethod] + public void Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError() + { + // Arrange — create a definition file with no evidence-source block + using var tempDir = new TestDirectory(); + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, """ + needs-review: + - "src/**/*.cs" + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code and log contains an error about the missing field + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(1, context.ExitCode); + Assert.Contains("Error:", logContent); + Assert.Contains("evidence-source", logContent); } } diff --git a/test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs b/test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs new file mode 100644 index 0000000..926a38d --- /dev/null +++ b/test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs @@ -0,0 +1,52 @@ +// Copyright (c) DEMA Consulting +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +namespace DemaConsulting.ReviewMark.Tests; + +/// +/// Represents a temporary directory that is automatically deleted when disposed. +/// +internal sealed class TestDirectory : IDisposable +{ + /// + /// Gets the path to the temporary directory. + /// + public string DirectoryPath { get; } + + /// + /// Initializes a new instance of the class. + /// + public TestDirectory() + { + DirectoryPath = PathHelpers.SafePathCombine(Path.GetTempPath(), $"reviewmark_test_{Guid.NewGuid()}"); + Directory.CreateDirectory(DirectoryPath); + } + + /// + /// Deletes the temporary directory and all its contents. + /// + public void Dispose() + { + if (Directory.Exists(DirectoryPath)) + { + Directory.Delete(DirectoryPath, recursive: true); + } + } +} From 72a7c70d1d7fb8886862979512ee98658c91de66 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 04:14:24 +0000 Subject: [PATCH 04/11] feat: improve lint error quality - filename/line numbers, includes support, and stronger test assertions Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/7318deb4-3700-47ba-b4b8-b7c3e37c4b4e --- requirements.yaml | 6 + src/DemaConsulting.ReviewMark/Program.cs | 5 - .../ReviewMarkConfiguration.cs | 278 +++++++++++++----- .../ProgramTests.cs | 54 +++- .../ReviewMarkConfigurationTests.cs | 145 +++++++++ 5 files changed, 405 insertions(+), 83 deletions(-) diff --git a/requirements.yaml b/requirements.yaml index 6f1649c..e450064 100644 --- a/requirements.yaml +++ b/requirements.yaml @@ -260,6 +260,12 @@ sections: - Program_Run_WithLintFlag_UnknownSourceType_ReportsError - Program_Run_WithLintFlag_CorruptedYaml_ReportsError - Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError + - Program_Run_WithLintFlag_IncludedFileCorrupted_ReportsChildFilename + - ReviewMarkConfiguration_Load_InvalidYaml_ErrorIncludesFilenameAndLine + - ReviewMarkConfiguration_Load_MissingEvidenceSource_ErrorIncludesFilename + - ReviewMarkConfiguration_Load_WithIncludes_MergesReviews + - ReviewMarkConfiguration_Load_MissingIncludedFile_ErrorNamesIncludedFile + - ReviewMarkConfiguration_Load_CorruptedIncludedFile_ErrorNamesIncludedFileAndLine - ReviewMark_Lint - title: Configuration Reading diff --git a/src/DemaConsulting.ReviewMark/Program.cs b/src/DemaConsulting.ReviewMark/Program.cs index 392b93b..f9cd003 100644 --- a/src/DemaConsulting.ReviewMark/Program.cs +++ b/src/DemaConsulting.ReviewMark/Program.cs @@ -185,11 +185,6 @@ private static void RunLintLogic(Context context) context.WriteError($"Error: {ex.Message}"); return; } - catch (ArgumentException ex) - { - context.WriteError($"Error: {ex.Message}"); - return; - } // Perform semantic validation: check for unknown evidence-source type var hasErrors = false; diff --git a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs index 6800b5a..04ea01c 100644 --- a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs @@ -49,6 +49,12 @@ file sealed class ReviewMarkYaml [YamlMember(Alias = "evidence-source")] public EvidenceSourceYaml? EvidenceSource { get; set; } + /// + /// Gets or sets the list of relative paths to included fragment files. + /// + [YamlMember(Alias = "includes")] + public List? Includes { get; set; } + /// /// Gets or sets the list of review set definitions. /// @@ -123,6 +129,136 @@ file sealed class ReviewSetYaml public List? Paths { get; set; } } +// --------------------------------------------------------------------------- +// File-local helpers — use file-local YAML types +// --------------------------------------------------------------------------- + +/// +/// File-local static helper that encapsulates YAML deserialization and model validation +/// on behalf of . Because both this class and +/// are file-local, C# allows them to appear in the +/// method signatures here. +/// +file static class ReviewMarkConfigurationHelpers +{ + /// + /// Deserializes a YAML string into the raw model. + /// + /// YAML content to parse. + /// + /// Optional file path used to produce actionable error messages. When null, + /// YAML errors are thrown as (preserving the + /// contract). When non-null, + /// they are thrown as and include the + /// file name, line, and column. + /// + /// The deserialized . + /// + /// Thrown when is null and the YAML is invalid. + /// + /// + /// Thrown when is set and the YAML is invalid. + /// + public static ReviewMarkYaml DeserializeRaw(string yaml, string? filePath) + { + var deserializer = new DeserializerBuilder() + .WithNamingConvention(NullNamingConvention.Instance) + .IgnoreUnmatchedProperties() + .Build(); + + try + { + if (filePath != null) + { + return deserializer.Deserialize(yaml) + ?? throw new InvalidOperationException( + $"Configuration file '{filePath}' is empty or null."); + } + + return deserializer.Deserialize(yaml) + ?? throw new ArgumentException("YAML content is empty or invalid.", nameof(yaml)); + } + catch (YamlException ex) + { + if (filePath != null) + { + throw new InvalidOperationException( + $"Failed to parse '{filePath}' at line {ex.Start.Line}, column {ex.Start.Column}: {ex.Message}", + ex); + } + + throw new ArgumentException($"Invalid YAML content: {ex.Message}", nameof(yaml), ex); + } + } + + /// + /// Validates a raw model and builds a + /// from it. + /// + /// The deserialized raw model to validate. + /// A validated . + /// + /// Thrown when required fields are absent or malformed. + /// + public static ReviewMarkConfiguration BuildConfiguration(ReviewMarkYaml raw) + { + // Map needs-review patterns (default to empty list if absent) + var needsReviewPatterns = (IReadOnlyList)(raw.NeedsReview ?? []); + + // Map evidence-source (required: evidence-source block, type, and location) + if (raw.EvidenceSource is not { } es) + { + throw new ArgumentException("Configuration is missing required 'evidence-source' block."); + } + + if (string.IsNullOrWhiteSpace(es.Type)) + { + throw new ArgumentException("Configuration 'evidence-source' is missing a required 'type' field."); + } + + if (string.IsNullOrWhiteSpace(es.Location)) + { + throw new ArgumentException("Configuration 'evidence-source' is missing a required 'location' field."); + } + + var evidenceSource = new EvidenceSource( + Type: es.Type, + Location: es.Location, + UsernameEnv: es.Credentials?.UsernameEnv, + PasswordEnv: es.Credentials?.PasswordEnv); + + // Map review sets, requiring id, title, and paths for each entry + var reviews = (raw.Reviews ?? []) + .Select((r, i) => + { + // Each review set must have an id + if (string.IsNullOrWhiteSpace(r.Id)) + { + throw new ArgumentException($"Review set at index {i} is missing a required 'id' field."); + } + + // Each review set must have a title + if (string.IsNullOrWhiteSpace(r.Title)) + { + throw new ArgumentException($"Review set '{r.Id}' is missing a required 'title' field."); + } + + // Each review set must have at least one non-empty path pattern + var paths = r.Paths; + if (paths is null || !paths.Any(p => !string.IsNullOrWhiteSpace(p))) + { + throw new ArgumentException( + $"Review set '{r.Id}' at index {i} is missing required 'paths' entries."); + } + + return new ReviewSet(r.Id, r.Title, paths); + }) + .ToList(); + + return new ReviewMarkConfiguration(needsReviewPatterns, evidenceSource, reviews); + } +} + // --------------------------------------------------------------------------- // Public API — internal to the assembly // --------------------------------------------------------------------------- @@ -281,7 +417,7 @@ internal sealed class ReviewMarkConfiguration /// Glob patterns for files requiring review. /// Evidence-source configuration. /// Review set definitions. - private ReviewMarkConfiguration( + internal ReviewMarkConfiguration( IReadOnlyList needsReviewPatterns, EvidenceSource evidenceSource, IReadOnlyList reviews) @@ -297,7 +433,11 @@ private ReviewMarkConfiguration( /// Absolute or relative path to the configuration file. /// A populated instance. /// Thrown when is null or empty. - /// Thrown when the file cannot be read. + /// + /// Thrown when the file cannot be read, the YAML is invalid, an included file cannot be read or + /// parsed, or required configuration fields are missing. The exception message always identifies + /// the problematic file and, for YAML syntax errors, the line and column number. + /// internal static ReviewMarkConfiguration Load(string filePath) { // Validate the file path argument @@ -321,8 +461,62 @@ internal static ReviewMarkConfiguration Load(string filePath) throw new InvalidOperationException($"Failed to read configuration file '{filePath}': {ex.Message}", ex); } - // Delegate to Parse for deserialization and apply path resolution - var config = Parse(yaml); + // Deserialize the raw YAML model, embedding the file path and line number in any parse error. + var raw = ReviewMarkConfigurationHelpers.DeserializeRaw(yaml, filePath); + + // Determine the base directory for resolving included file paths. + var baseDirectory = Path.GetDirectoryName(Path.GetFullPath(filePath)) + ?? throw new InvalidOperationException($"Cannot determine base directory for configuration file '{filePath}'."); + + // Process includes: load each referenced fragment and merge its content. + foreach (var include in raw.Includes ?? []) + { + if (string.IsNullOrWhiteSpace(include)) + { + continue; + } + + var includePath = Path.GetFullPath(include, baseDirectory); + + // Read the included file, embedding the included path in any file-system error. + string includeYaml; + try + { + includeYaml = File.ReadAllText(includePath); + } + catch (Exception ex) when (ex is not InvalidOperationException) + { + throw new InvalidOperationException( + $"Failed to read included file '{includePath}': {ex.Message}", ex); + } + + // Parse the included fragment; parse errors will name the included file and line. + var includeRaw = ReviewMarkConfigurationHelpers.DeserializeRaw(includeYaml, includePath); + + // Merge fragment's needs-review patterns and reviews into the parent raw model. + if (includeRaw.NeedsReview?.Count > 0) + { + raw.NeedsReview ??= []; + raw.NeedsReview.AddRange(includeRaw.NeedsReview); + } + + if (includeRaw.Reviews?.Count > 0) + { + raw.Reviews ??= []; + raw.Reviews.AddRange(includeRaw.Reviews); + } + } + + // Validate the merged raw model, embedding the file path in any semantic error. + ReviewMarkConfiguration config; + try + { + config = ReviewMarkConfigurationHelpers.BuildConfiguration(raw); + } + catch (ArgumentException ex) + { + throw new InvalidOperationException($"Invalid configuration in '{filePath}': {ex.Message}", ex); + } // Resolve relative fileshare locations against the config file's directory so that // a relative location (e.g., "index.json") works correctly regardless of the process @@ -330,8 +524,6 @@ internal static ReviewMarkConfiguration Load(string filePath) if (string.Equals(config.EvidenceSource.Type, "fileshare", StringComparison.OrdinalIgnoreCase) && !Path.IsPathRooted(config.EvidenceSource.Location)) { - var baseDirectory = Path.GetDirectoryName(Path.GetFullPath(filePath)) - ?? throw new InvalidOperationException($"Cannot determine base directory for configuration file '{filePath}'."); var absoluteLocation = Path.GetFullPath(config.EvidenceSource.Location, baseDirectory); return new ReviewMarkConfiguration( config.NeedsReviewPatterns, @@ -354,77 +546,11 @@ internal static ReviewMarkConfiguration Parse(string yaml) // Validate the yaml input ArgumentNullException.ThrowIfNull(yaml); - // Build a YamlDotNet deserializer that ignores unmatched fields - var deserializer = new DeserializerBuilder() - .WithNamingConvention(NullNamingConvention.Instance) - .IgnoreUnmatchedProperties() - .Build(); + // Deserialize without a file path so YAML errors are wrapped as ArgumentException (not + // InvalidOperationException) which is what callers of Parse (unit tests) expect. + var raw = ReviewMarkConfigurationHelpers.DeserializeRaw(yaml, filePath: null); - // Deserialize the raw YAML into the internal model - ReviewMarkYaml raw; - try - { - raw = deserializer.Deserialize(yaml) - ?? throw new ArgumentException("YAML content is empty or invalid.", nameof(yaml)); - } - catch (YamlException ex) - { - throw new ArgumentException($"Invalid YAML content: {ex.Message}", nameof(yaml), ex); - } - - // Map needs-review patterns (default to empty list if absent) - var needsReviewPatterns = (IReadOnlyList)(raw.NeedsReview ?? []); - - // Map evidence-source (required: evidence-source block, type, and location) - if (raw.EvidenceSource is not { } es) - { - throw new ArgumentException("Configuration is missing required 'evidence-source' block.", nameof(yaml)); - } - - if (string.IsNullOrWhiteSpace(es.Type)) - { - throw new ArgumentException("Configuration 'evidence-source' is missing a required 'type' field.", nameof(yaml)); - } - - if (string.IsNullOrWhiteSpace(es.Location)) - { - throw new ArgumentException("Configuration 'evidence-source' is missing a required 'location' field.", nameof(yaml)); - } - - var evidenceSource = new EvidenceSource( - Type: es.Type, - Location: es.Location, - UsernameEnv: es.Credentials?.UsernameEnv, - PasswordEnv: es.Credentials?.PasswordEnv); - // Map review sets, requiring id, title, and paths for each entry - var reviews = (raw.Reviews ?? []) - .Select((r, i) => - { - // Each review set must have an id - if (string.IsNullOrWhiteSpace(r.Id)) - { - throw new ArgumentException($"Review set at index {i} is missing a required 'id' field."); - } - - // Each review set must have a title - if (string.IsNullOrWhiteSpace(r.Title)) - { - throw new ArgumentException($"Review set '{r.Id}' is missing a required 'title' field."); - } - - // Each review set must have at least one non-empty path pattern - var paths = r.Paths; - if (paths is null || !paths.Any(p => !string.IsNullOrWhiteSpace(p))) - { - throw new ArgumentException( - $"Review set '{r.Id}' at index {i} is missing required 'paths' entries."); - } - - return new ReviewSet(r.Id, r.Title, paths); - }) - .ToList(); - - return new ReviewMarkConfiguration(needsReviewPatterns, evidenceSource, reviews); + return ReviewMarkConfigurationHelpers.BuildConfiguration(raw); } /// diff --git a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs index e54de5c..c8c0810 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs @@ -471,10 +471,12 @@ public void Program_Run_WithLintFlag_CorruptedYaml_ReportsError() // Act Program.Run(context); - // Assert — non-zero exit code and log contains an error about invalid content + // Assert — non-zero exit code and log contains an error naming the definition file and a line number var logContent = File.ReadAllText(logFile); Assert.AreEqual(1, context.ExitCode); Assert.Contains("Error:", logContent); + Assert.Contains("definition.yaml", logContent); + Assert.Contains("at line", logContent); } /// @@ -502,10 +504,58 @@ public void Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError() // Act Program.Run(context); - // Assert — non-zero exit code and log contains an error about the missing field + // Assert — non-zero exit code and log names the file and the missing field var logContent = File.ReadAllText(logFile); Assert.AreEqual(1, context.ExitCode); Assert.Contains("Error:", logContent); + Assert.Contains("definition.yaml", logContent); Assert.Contains("evidence-source", logContent); } + + /// + /// Test that Run with --lint flag on a parent that includes a corrupted child file + /// reports an error naming the child file and its line number. + /// + [TestMethod] + public void Program_Run_WithLintFlag_IncludedFileCorrupted_ReportsChildFilename() + { + // Arrange — create a valid parent definition and a corrupted child file + using var tempDir = new TestDirectory(); + var indexFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "index.json"); + File.WriteAllText(indexFile, """{"reviews":[]}"""); + + var childFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "extra-reviews.yaml"); + File.WriteAllText(childFile, """ + {{{this is not valid yaml + """); + + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, $""" + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: {indexFile} + includes: + - extra-reviews.yaml + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code and error names the child file (not just the parent) + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(1, context.ExitCode); + Assert.Contains("Error:", logContent); + Assert.Contains("extra-reviews.yaml", logContent); + Assert.Contains("at line", logContent); + } } diff --git a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs index 1836039..4e88892 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs @@ -301,6 +301,151 @@ public void ReviewMarkConfiguration_Load_NonExistentFile_ThrowsException() ReviewMarkConfiguration.Load(nonExistentPath)); } + /// + /// Test that Load includes the file name in the error message when YAML is invalid. + /// + [TestMethod] + public void ReviewMarkConfiguration_Load_InvalidYaml_ErrorIncludesFilenameAndLine() + { + // Arrange — write a configuration file with invalid YAML syntax + var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); + File.WriteAllText(configPath, "{{{invalid yaml"); + + // Act & Assert + var ex = Assert.Throws(() => + ReviewMarkConfiguration.Load(configPath)); + Assert.Contains(".reviewmark.yaml", ex.Message); + Assert.Contains("at line", ex.Message); + } + + /// + /// Test that Load includes the file name in the error message when required fields are missing. + /// + [TestMethod] + public void ReviewMarkConfiguration_Load_MissingEvidenceSource_ErrorIncludesFilename() + { + // Arrange — write a valid YAML file that is missing the required evidence-source block + var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); + File.WriteAllText(configPath, """ + needs-review: + - "src/**/*.cs" + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + // Act & Assert + var ex = Assert.Throws(() => + ReviewMarkConfiguration.Load(configPath)); + Assert.Contains(".reviewmark.yaml", ex.Message); + Assert.Contains("evidence-source", ex.Message); + } + + /// + /// Test that Load with includes merges reviews from the included file. + /// + [TestMethod] + public void ReviewMarkConfiguration_Load_WithIncludes_MergesReviews() + { + // Arrange — parent file references a child file + var childPath = PathHelpers.SafePathCombine(_testDirectory, "extra.yaml"); + File.WriteAllText(childPath, """ + reviews: + - id: Extra-Logic + title: Extra review set + paths: + - "extra/**/*.cs" + """); + + var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); + File.WriteAllText(configPath, """ + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: index.json + includes: + - extra.yaml + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + // Act + var config = ReviewMarkConfiguration.Load(configPath); + + // Assert — both review sets are present + Assert.AreEqual(2, config.Reviews.Count); + Assert.AreEqual("Core-Logic", config.Reviews[0].Id); + Assert.AreEqual("Extra-Logic", config.Reviews[1].Id); + } + + /// + /// Test that Load reports an error that names the missing included file. + /// + [TestMethod] + public void ReviewMarkConfiguration_Load_MissingIncludedFile_ErrorNamesIncludedFile() + { + // Arrange — parent file references a non-existent child file + var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); + File.WriteAllText(configPath, """ + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: index.json + includes: + - nonexistent.yaml + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + // Act & Assert + var ex = Assert.Throws(() => + ReviewMarkConfiguration.Load(configPath)); + Assert.Contains("nonexistent.yaml", ex.Message); + } + + /// + /// Test that Load reports a parse error that names the corrupted included file and its line number. + /// + [TestMethod] + public void ReviewMarkConfiguration_Load_CorruptedIncludedFile_ErrorNamesIncludedFileAndLine() + { + // Arrange — parent file references a child file with invalid YAML + var childPath = PathHelpers.SafePathCombine(_testDirectory, "extra.yaml"); + File.WriteAllText(childPath, "{{{invalid yaml content"); + + var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); + File.WriteAllText(configPath, """ + needs-review: + - "src/**/*.cs" + evidence-source: + type: fileshare + location: index.json + includes: + - extra.yaml + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + """); + + // Act & Assert — error names the child file (not just the parent) and includes a line number + var ex = Assert.Throws(() => + ReviewMarkConfiguration.Load(configPath)); + Assert.Contains("extra.yaml", ex.Message); + Assert.Contains("at line", ex.Message); + } + /// /// Test that Load resolves a relative fileshare location against the config file's directory. /// From cf4b38517e1baa0f9c0e9f41ec01655a9ea7ac0e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 12:19:50 +0000 Subject: [PATCH 05/11] revert: remove includes feature - reviewmark uses a single definition file Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/25f4d0e6-fdf3-493b-803c-d19cfb0967fd --- requirements.yaml | 4 - .../ReviewMarkConfiguration.cs | 55 +--------- .../ProgramTests.cs | 47 -------- .../ReviewMarkConfigurationTests.cs | 103 ------------------ 4 files changed, 5 insertions(+), 204 deletions(-) diff --git a/requirements.yaml b/requirements.yaml index e450064..179e822 100644 --- a/requirements.yaml +++ b/requirements.yaml @@ -260,12 +260,8 @@ sections: - Program_Run_WithLintFlag_UnknownSourceType_ReportsError - Program_Run_WithLintFlag_CorruptedYaml_ReportsError - Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError - - Program_Run_WithLintFlag_IncludedFileCorrupted_ReportsChildFilename - ReviewMarkConfiguration_Load_InvalidYaml_ErrorIncludesFilenameAndLine - ReviewMarkConfiguration_Load_MissingEvidenceSource_ErrorIncludesFilename - - ReviewMarkConfiguration_Load_WithIncludes_MergesReviews - - ReviewMarkConfiguration_Load_MissingIncludedFile_ErrorNamesIncludedFile - - ReviewMarkConfiguration_Load_CorruptedIncludedFile_ErrorNamesIncludedFileAndLine - ReviewMark_Lint - title: Configuration Reading diff --git a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs index 04ea01c..f3457e6 100644 --- a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs @@ -49,12 +49,6 @@ file sealed class ReviewMarkYaml [YamlMember(Alias = "evidence-source")] public EvidenceSourceYaml? EvidenceSource { get; set; } - /// - /// Gets or sets the list of relative paths to included fragment files. - /// - [YamlMember(Alias = "includes")] - public List? Includes { get; set; } - /// /// Gets or sets the list of review set definitions. /// @@ -434,9 +428,9 @@ internal ReviewMarkConfiguration( /// A populated instance. /// Thrown when is null or empty. /// - /// Thrown when the file cannot be read, the YAML is invalid, an included file cannot be read or - /// parsed, or required configuration fields are missing. The exception message always identifies - /// the problematic file and, for YAML syntax errors, the line and column number. + /// Thrown when the file cannot be read, the YAML is invalid, or required configuration fields are + /// missing. The exception message always identifies the problematic file and, for YAML syntax + /// errors, the line and column number. /// internal static ReviewMarkConfiguration Load(string filePath) { @@ -464,50 +458,11 @@ internal static ReviewMarkConfiguration Load(string filePath) // Deserialize the raw YAML model, embedding the file path and line number in any parse error. var raw = ReviewMarkConfigurationHelpers.DeserializeRaw(yaml, filePath); - // Determine the base directory for resolving included file paths. + // Determine the base directory for resolving relative fileshare locations. var baseDirectory = Path.GetDirectoryName(Path.GetFullPath(filePath)) ?? throw new InvalidOperationException($"Cannot determine base directory for configuration file '{filePath}'."); - // Process includes: load each referenced fragment and merge its content. - foreach (var include in raw.Includes ?? []) - { - if (string.IsNullOrWhiteSpace(include)) - { - continue; - } - - var includePath = Path.GetFullPath(include, baseDirectory); - - // Read the included file, embedding the included path in any file-system error. - string includeYaml; - try - { - includeYaml = File.ReadAllText(includePath); - } - catch (Exception ex) when (ex is not InvalidOperationException) - { - throw new InvalidOperationException( - $"Failed to read included file '{includePath}': {ex.Message}", ex); - } - - // Parse the included fragment; parse errors will name the included file and line. - var includeRaw = ReviewMarkConfigurationHelpers.DeserializeRaw(includeYaml, includePath); - - // Merge fragment's needs-review patterns and reviews into the parent raw model. - if (includeRaw.NeedsReview?.Count > 0) - { - raw.NeedsReview ??= []; - raw.NeedsReview.AddRange(includeRaw.NeedsReview); - } - - if (includeRaw.Reviews?.Count > 0) - { - raw.Reviews ??= []; - raw.Reviews.AddRange(includeRaw.Reviews); - } - } - - // Validate the merged raw model, embedding the file path in any semantic error. + // Validate the raw model, embedding the file path in any semantic error. ReviewMarkConfiguration config; try { diff --git a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs index c8c0810..c3748e4 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs @@ -511,51 +511,4 @@ public void Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError() Assert.Contains("definition.yaml", logContent); Assert.Contains("evidence-source", logContent); } - - /// - /// Test that Run with --lint flag on a parent that includes a corrupted child file - /// reports an error naming the child file and its line number. - /// - [TestMethod] - public void Program_Run_WithLintFlag_IncludedFileCorrupted_ReportsChildFilename() - { - // Arrange — create a valid parent definition and a corrupted child file - using var tempDir = new TestDirectory(); - var indexFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "index.json"); - File.WriteAllText(indexFile, """{"reviews":[]}"""); - - var childFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "extra-reviews.yaml"); - File.WriteAllText(childFile, """ - {{{this is not valid yaml - """); - - var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); - File.WriteAllText(definitionFile, $""" - needs-review: - - "src/**/*.cs" - evidence-source: - type: fileshare - location: {indexFile} - includes: - - extra-reviews.yaml - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); - - // Act - Program.Run(context); - - // Assert — non-zero exit code and error names the child file (not just the parent) - var logContent = File.ReadAllText(logFile); - Assert.AreEqual(1, context.ExitCode); - Assert.Contains("Error:", logContent); - Assert.Contains("extra-reviews.yaml", logContent); - Assert.Contains("at line", logContent); - } } diff --git a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs index 4e88892..eeafea1 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs @@ -343,109 +343,6 @@ public void ReviewMarkConfiguration_Load_MissingEvidenceSource_ErrorIncludesFile Assert.Contains("evidence-source", ex.Message); } - /// - /// Test that Load with includes merges reviews from the included file. - /// - [TestMethod] - public void ReviewMarkConfiguration_Load_WithIncludes_MergesReviews() - { - // Arrange — parent file references a child file - var childPath = PathHelpers.SafePathCombine(_testDirectory, "extra.yaml"); - File.WriteAllText(childPath, """ - reviews: - - id: Extra-Logic - title: Extra review set - paths: - - "extra/**/*.cs" - """); - - var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); - File.WriteAllText(configPath, """ - needs-review: - - "src/**/*.cs" - evidence-source: - type: fileshare - location: index.json - includes: - - extra.yaml - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - // Act - var config = ReviewMarkConfiguration.Load(configPath); - - // Assert — both review sets are present - Assert.AreEqual(2, config.Reviews.Count); - Assert.AreEqual("Core-Logic", config.Reviews[0].Id); - Assert.AreEqual("Extra-Logic", config.Reviews[1].Id); - } - - /// - /// Test that Load reports an error that names the missing included file. - /// - [TestMethod] - public void ReviewMarkConfiguration_Load_MissingIncludedFile_ErrorNamesIncludedFile() - { - // Arrange — parent file references a non-existent child file - var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); - File.WriteAllText(configPath, """ - needs-review: - - "src/**/*.cs" - evidence-source: - type: fileshare - location: index.json - includes: - - nonexistent.yaml - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - // Act & Assert - var ex = Assert.Throws(() => - ReviewMarkConfiguration.Load(configPath)); - Assert.Contains("nonexistent.yaml", ex.Message); - } - - /// - /// Test that Load reports a parse error that names the corrupted included file and its line number. - /// - [TestMethod] - public void ReviewMarkConfiguration_Load_CorruptedIncludedFile_ErrorNamesIncludedFileAndLine() - { - // Arrange — parent file references a child file with invalid YAML - var childPath = PathHelpers.SafePathCombine(_testDirectory, "extra.yaml"); - File.WriteAllText(childPath, "{{{invalid yaml content"); - - var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); - File.WriteAllText(configPath, """ - needs-review: - - "src/**/*.cs" - evidence-source: - type: fileshare - location: index.json - includes: - - extra.yaml - reviews: - - id: Core-Logic - title: Review of core business logic - paths: - - "src/**/*.cs" - """); - - // Act & Assert — error names the child file (not just the parent) and includes a line number - var ex = Assert.Throws(() => - ReviewMarkConfiguration.Load(configPath)); - Assert.Contains("extra.yaml", ex.Message); - Assert.Contains("at line", ex.Message); - } - /// /// Test that Load resolves a relative fileshare location against the config file's directory. /// From 705b6dc96d0fbd177a64b742c646fabdd4f2f2c6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 12:40:48 +0000 Subject: [PATCH 06/11] refactor: accumulate all lint errors in one pass via ReviewMarkConfiguration.Lint Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/0745bda8-94ca-4460-a7b2-c361d77474c0 --- requirements.yaml | 2 + src/DemaConsulting.ReviewMark/Program.cs | 42 +------ .../ReviewMarkConfiguration.cs | 112 ++++++++++++++++++ .../ProgramTests.cs | 39 ++++++ .../ReviewMarkConfigurationTests.cs | 34 ++++++ 5 files changed, 192 insertions(+), 37 deletions(-) diff --git a/requirements.yaml b/requirements.yaml index 179e822..06e361a 100644 --- a/requirements.yaml +++ b/requirements.yaml @@ -260,8 +260,10 @@ sections: - Program_Run_WithLintFlag_UnknownSourceType_ReportsError - Program_Run_WithLintFlag_CorruptedYaml_ReportsError - Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError + - Program_Run_WithLintFlag_MultipleErrors_ReportsAll - ReviewMarkConfiguration_Load_InvalidYaml_ErrorIncludesFilenameAndLine - ReviewMarkConfiguration_Load_MissingEvidenceSource_ErrorIncludesFilename + - ReviewMarkConfiguration_Lint_MultipleErrors_ReturnsAll - ReviewMark_Lint - title: Configuration Reading diff --git a/src/DemaConsulting.ReviewMark/Program.cs b/src/DemaConsulting.ReviewMark/Program.cs index f9cd003..5276a11 100644 --- a/src/DemaConsulting.ReviewMark/Program.cs +++ b/src/DemaConsulting.ReviewMark/Program.cs @@ -174,47 +174,15 @@ private static void RunLintLogic(Context context) context.WriteLine($"Linting '{definitionFile}'..."); - // Try to load the configuration file - ReviewMarkConfiguration config; - try - { - config = ReviewMarkConfiguration.Load(definitionFile); - } - catch (InvalidOperationException ex) - { - context.WriteError($"Error: {ex.Message}"); - return; - } - - // Perform semantic validation: check for unknown evidence-source type - var hasErrors = false; - if (!string.Equals(config.EvidenceSource.Type, "url", StringComparison.OrdinalIgnoreCase) && - !string.Equals(config.EvidenceSource.Type, "fileshare", StringComparison.OrdinalIgnoreCase)) + // Lint the file, collecting all detectable errors in one pass. + var errors = ReviewMarkConfiguration.Lint(definitionFile); + foreach (var error in errors) { - context.WriteError( - $"Error: evidence-source type '{config.EvidenceSource.Type}' is not supported (must be 'url' or 'fileshare')."); - hasErrors = true; - } - - // Perform semantic validation: check for duplicate review set IDs - var seenIds = new Dictionary(StringComparer.Ordinal); - for (var i = 0; i < config.Reviews.Count; i++) - { - var review = config.Reviews[i]; - if (seenIds.TryGetValue(review.Id, out var firstIndex)) - { - context.WriteError( - $"Error: reviews[{i}] has duplicate ID '{review.Id}' (first defined at reviews[{firstIndex}])."); - hasErrors = true; - } - else - { - seenIds[review.Id] = i; - } + context.WriteError($"Error: {error}"); } // Report overall result - if (!hasErrors) + if (errors.Count == 0) { context.WriteLine($"'{definitionFile}' is valid."); } diff --git a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs index f3457e6..e1761eb 100644 --- a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs @@ -489,6 +489,118 @@ internal static ReviewMarkConfiguration Load(string filePath) return config; } + /// + /// Lints a .reviewmark.yaml file and returns all detected issues. + /// Unlike , this method does not stop at the first error; + /// it accumulates every detectable problem and returns them all so the caller + /// can report a complete list in a single pass. + /// + /// Absolute or relative path to the configuration file. + /// + /// A read-only list of error messages. The list is empty when the file is + /// structurally and semantically valid. + /// + /// Thrown when is null or empty. + internal static IReadOnlyList Lint(string filePath) + { + // Validate the file path argument + if (string.IsNullOrWhiteSpace(filePath)) + { + throw new ArgumentException("File path must not be null or empty.", nameof(filePath)); + } + + var errors = new List(); + + // Try to read the file; if this fails we cannot continue. + string yaml; + try + { + yaml = File.ReadAllText(filePath); + } + catch (Exception ex) when (ex is not InvalidOperationException) + { + errors.Add($"Failed to read configuration file '{filePath}': {ex.Message}"); + return errors; + } + + // Try to parse the raw YAML model; if this fails we cannot do semantic checks. + ReviewMarkYaml raw; + try + { + raw = ReviewMarkConfigurationHelpers.DeserializeRaw(yaml, filePath); + } + catch (InvalidOperationException ex) + { + errors.Add(ex.Message); + return errors; + } + + // Validate the evidence-source block, collecting all field-level errors. + var es = raw.EvidenceSource; + if (es == null) + { + errors.Add( + $"Invalid configuration in '{filePath}': Configuration is missing required 'evidence-source' block."); + } + else + { + if (string.IsNullOrWhiteSpace(es.Type)) + { + errors.Add( + $"Invalid configuration in '{filePath}': 'evidence-source' is missing a required 'type' field."); + } + else if (!string.Equals(es.Type, "url", StringComparison.OrdinalIgnoreCase) && + !string.Equals(es.Type, "fileshare", StringComparison.OrdinalIgnoreCase)) + { + errors.Add( + $"evidence-source type '{es.Type}' is not supported (must be 'url' or 'fileshare')."); + } + + if (string.IsNullOrWhiteSpace(es.Location)) + { + errors.Add( + $"Invalid configuration in '{filePath}': 'evidence-source' is missing a required 'location' field."); + } + } + + // Validate each review set, accumulating all structural and uniqueness errors. + var seenIds = new Dictionary(StringComparer.Ordinal); + var reviews = raw.Reviews ?? []; + for (var i = 0; i < reviews.Count; i++) + { + var r = reviews[i]; + + if (string.IsNullOrWhiteSpace(r.Id)) + { + errors.Add( + $"Invalid configuration in '{filePath}': Review set at index {i} is missing a required 'id' field."); + } + else if (seenIds.TryGetValue(r.Id, out var firstIndex)) + { + errors.Add( + $"reviews[{i}] has duplicate ID '{r.Id}' (first defined at reviews[{firstIndex}])."); + } + else + { + seenIds[r.Id] = i; + } + + if (string.IsNullOrWhiteSpace(r.Title)) + { + errors.Add( + $"Invalid configuration in '{filePath}': Review set at index {i} is missing a required 'title' field."); + } + + if (r.Paths == null || !r.Paths.Any(p => !string.IsNullOrWhiteSpace(p))) + { + errors.Add( + $"Invalid configuration in '{filePath}': Review set at index {i} is missing required 'paths' entries."); + } + } + + return errors; + } + /// /// Parses a YAML string into a . /// diff --git a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs index c3748e4..692e80d 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs @@ -511,4 +511,43 @@ public void Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError() Assert.Contains("definition.yaml", logContent); Assert.Contains("evidence-source", logContent); } + + /// + /// Test that Run with --lint flag reports ALL errors in one pass when the file has + /// multiple detectable issues (missing evidence-source AND duplicate review IDs). + /// + [TestMethod] + public void Program_Run_WithLintFlag_MultipleErrors_ReportsAll() + { + // Arrange — create a definition file that is missing evidence-source AND has duplicate IDs + using var tempDir = new TestDirectory(); + var definitionFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "definition.yaml"); + File.WriteAllText(definitionFile, """ + needs-review: + - "src/**/*.cs" + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + - id: Core-Logic + title: Duplicate review set + paths: + - "src/**/*.cs" + """); + + var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); + using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); + + // Act + Program.Run(context); + + // Assert — non-zero exit code and log contains BOTH the missing evidence-source error + // AND the duplicate ID error, proving all errors are accumulated in one pass. + var logContent = File.ReadAllText(logFile); + Assert.AreEqual(1, context.ExitCode); + Assert.Contains("evidence-source", logContent); + Assert.Contains("duplicate ID", logContent); + Assert.Contains("Core-Logic", logContent); + } } diff --git a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs index eeafea1..b6c068b 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs @@ -343,6 +343,40 @@ public void ReviewMarkConfiguration_Load_MissingEvidenceSource_ErrorIncludesFile Assert.Contains("evidence-source", ex.Message); } + /// + /// Test that Lint returns all errors from a file with multiple detectable issues + /// (missing evidence-source AND duplicate review IDs) without stopping at the first. + /// + [TestMethod] + public void ReviewMarkConfiguration_Lint_MultipleErrors_ReturnsAll() + { + // Arrange — write a YAML file missing evidence-source and containing duplicate IDs + var configPath = PathHelpers.SafePathCombine(_testDirectory, ".reviewmark.yaml"); + File.WriteAllText(configPath, """ + needs-review: + - "src/**/*.cs" + reviews: + - id: Core-Logic + title: Review of core business logic + paths: + - "src/**/*.cs" + - id: Core-Logic + title: Duplicate review set + paths: + - "src/**/*.cs" + """); + + // Act + var errors = ReviewMarkConfiguration.Lint(configPath); + + // Assert — both the missing evidence-source error and the duplicate ID error are returned + Assert.IsTrue(errors.Count >= 2, $"Expected at least 2 errors, got {errors.Count}."); + Assert.IsTrue(errors.Any(e => e.Contains("evidence-source")), + "Expected an error about missing evidence-source."); + Assert.IsTrue(errors.Any(e => e.Contains("duplicate ID") && e.Contains("Core-Logic")), + "Expected an error about duplicate ID 'Core-Logic'."); + } + /// /// Test that Load resolves a relative fileshare location against the config file's directory. /// From 18959d3d9912c475c4dc03ac2e9ce4fa57daeb99 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 12:44:04 +0000 Subject: [PATCH 07/11] fix: clarify case-sensitivity intent and tighten Lint unit test count assertion Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/0745bda8-94ca-4460-a7b2-c361d77474c0 --- src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs | 3 +++ .../ReviewMarkConfigurationTests.cs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs index e1761eb..b47e2ec 100644 --- a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs @@ -564,6 +564,9 @@ internal static IReadOnlyList Lint(string filePath) } // Validate each review set, accumulating all structural and uniqueness errors. + // Review IDs are treated as case-sensitive identifiers (Ordinal), which is intentional: + // "Core-Logic" and "core-logic" are distinct IDs. Evidence-source type uses OrdinalIgnoreCase + // because YAML convention allows any casing for keyword values like "url" or "fileshare". var seenIds = new Dictionary(StringComparer.Ordinal); var reviews = raw.Reviews ?? []; for (var i = 0; i < reviews.Count; i++) diff --git a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs index b6c068b..26ab57a 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ReviewMarkConfigurationTests.cs @@ -370,7 +370,7 @@ public void ReviewMarkConfiguration_Lint_MultipleErrors_ReturnsAll() var errors = ReviewMarkConfiguration.Lint(configPath); // Assert — both the missing evidence-source error and the duplicate ID error are returned - Assert.IsTrue(errors.Count >= 2, $"Expected at least 2 errors, got {errors.Count}."); + Assert.AreEqual(2, errors.Count); Assert.IsTrue(errors.Any(e => e.Contains("evidence-source")), "Expected an error about missing evidence-source."); Assert.IsTrue(errors.Any(e => e.Contains("duplicate ID") && e.Contains("Core-Logic")), From 9db09b0dc66017f88cd319bc5c04e6c62f1d1c26 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 15:43:21 +0000 Subject: [PATCH 08/11] fix: dispose Context before reading log file, consistent error messages, TestDirectory safety, shared type validator Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/206afe15-d999-4902-a3d6-381cc23061cb --- .../ReviewMarkConfiguration.cs | 23 ++++- .../ProgramTests.cs | 84 ++++++++++++------- .../TestDirectory.cs | 15 +++- 3 files changed, 89 insertions(+), 33 deletions(-) diff --git a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs index b47e2ec..b886802 100644 --- a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs @@ -135,6 +135,16 @@ file sealed class ReviewSetYaml /// file static class ReviewMarkConfigurationHelpers { + /// + /// Returns true when is a recognised evidence-source + /// type (url or fileshare, case-insensitive). + /// + /// The type string to test. + /// true if the type is supported; false otherwise. + public static bool IsSupportedEvidenceSourceType(string type) => + string.Equals(type, "url", StringComparison.OrdinalIgnoreCase) || + string.Equals(type, "fileshare", StringComparison.OrdinalIgnoreCase); + /// /// Deserializes a YAML string into the raw model. /// @@ -210,6 +220,12 @@ public static ReviewMarkConfiguration BuildConfiguration(ReviewMarkYaml raw) throw new ArgumentException("Configuration 'evidence-source' is missing a required 'type' field."); } + if (!IsSupportedEvidenceSourceType(es.Type)) + { + throw new ArgumentException( + $"Configuration 'evidence-source' type '{es.Type}' is not supported (must be 'url' or 'fileshare')."); + } + if (string.IsNullOrWhiteSpace(es.Location)) { throw new ArgumentException("Configuration 'evidence-source' is missing a required 'location' field."); @@ -549,11 +565,10 @@ internal static IReadOnlyList Lint(string filePath) errors.Add( $"Invalid configuration in '{filePath}': 'evidence-source' is missing a required 'type' field."); } - else if (!string.Equals(es.Type, "url", StringComparison.OrdinalIgnoreCase) && - !string.Equals(es.Type, "fileshare", StringComparison.OrdinalIgnoreCase)) + else if (!ReviewMarkConfigurationHelpers.IsSupportedEvidenceSourceType(es.Type)) { errors.Add( - $"evidence-source type '{es.Type}' is not supported (must be 'url' or 'fileshare')."); + $"Invalid configuration in '{filePath}': 'evidence-source' type '{es.Type}' is not supported (must be 'url' or 'fileshare')."); } if (string.IsNullOrWhiteSpace(es.Location)) @@ -581,7 +596,7 @@ internal static IReadOnlyList Lint(string filePath) else if (seenIds.TryGetValue(r.Id, out var firstIndex)) { errors.Add( - $"reviews[{i}] has duplicate ID '{r.Id}' (first defined at reviews[{firstIndex}])."); + $"Invalid configuration in '{filePath}': reviews[{i}] has duplicate ID '{r.Id}' (first defined at reviews[{firstIndex}])."); } else { diff --git a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs index 692e80d..0a33898 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs @@ -340,14 +340,18 @@ public void Program_Run_WithLintFlag_ValidConfig_ReportsSuccess() """); var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); - // Act - Program.Run(context); + // Act — dispose the context before reading the log to release the file handle on Windows + int exitCode; + using (var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile])) + { + Program.Run(context); + exitCode = context.ExitCode; + } // Assert — exit code is zero and log contains success message var logContent = File.ReadAllText(logFile); - Assert.AreEqual(0, context.ExitCode); + Assert.AreEqual(0, exitCode); Assert.Contains("is valid", logContent); } @@ -361,14 +365,18 @@ public void Program_Run_WithLintFlag_MissingConfig_ReportsError() using var tempDir = new TestDirectory(); var nonExistentFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, "nonexistent.yaml"); var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", nonExistentFile]); - // Act - Program.Run(context); + // Act — dispose the context before reading the log to release the file handle on Windows + int exitCode; + using (var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", nonExistentFile])) + { + Program.Run(context); + exitCode = context.ExitCode; + } // Assert — non-zero exit code and log contains an error mentioning the missing file var logContent = File.ReadAllText(logFile); - Assert.AreEqual(1, context.ExitCode); + Assert.AreEqual(1, exitCode); Assert.Contains("Error:", logContent); Assert.Contains("nonexistent.yaml", logContent); } @@ -403,14 +411,18 @@ public void Program_Run_WithLintFlag_DuplicateIds_ReportsError() """); var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); - // Act - Program.Run(context); + // Act — dispose the context before reading the log to release the file handle on Windows + int exitCode; + using (var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile])) + { + Program.Run(context); + exitCode = context.ExitCode; + } // Assert — non-zero exit code and log contains a clear duplicate-ID error message var logContent = File.ReadAllText(logFile); - Assert.AreEqual(1, context.ExitCode); + Assert.AreEqual(1, exitCode); Assert.Contains("Error:", logContent); Assert.Contains("duplicate ID", logContent); Assert.Contains("Core-Logic", logContent); @@ -439,14 +451,18 @@ public void Program_Run_WithLintFlag_UnknownSourceType_ReportsError() """); var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); - // Act - Program.Run(context); + // Act — dispose the context before reading the log to release the file handle on Windows + int exitCode; + using (var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile])) + { + Program.Run(context); + exitCode = context.ExitCode; + } // Assert — non-zero exit code and log contains a clear unsupported-type error message var logContent = File.ReadAllText(logFile); - Assert.AreEqual(1, context.ExitCode); + Assert.AreEqual(1, exitCode); Assert.Contains("Error:", logContent); Assert.Contains("ftp", logContent); Assert.Contains("not supported", logContent); @@ -466,14 +482,18 @@ public void Program_Run_WithLintFlag_CorruptedYaml_ReportsError() """); var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); - // Act - Program.Run(context); + // Act — dispose the context before reading the log to release the file handle on Windows + int exitCode; + using (var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile])) + { + Program.Run(context); + exitCode = context.ExitCode; + } // Assert — non-zero exit code and log contains an error naming the definition file and a line number var logContent = File.ReadAllText(logFile); - Assert.AreEqual(1, context.ExitCode); + Assert.AreEqual(1, exitCode); Assert.Contains("Error:", logContent); Assert.Contains("definition.yaml", logContent); Assert.Contains("at line", logContent); @@ -499,14 +519,18 @@ public void Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError() """); var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); - // Act - Program.Run(context); + // Act — dispose the context before reading the log to release the file handle on Windows + int exitCode; + using (var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile])) + { + Program.Run(context); + exitCode = context.ExitCode; + } // Assert — non-zero exit code and log names the file and the missing field var logContent = File.ReadAllText(logFile); - Assert.AreEqual(1, context.ExitCode); + Assert.AreEqual(1, exitCode); Assert.Contains("Error:", logContent); Assert.Contains("definition.yaml", logContent); Assert.Contains("evidence-source", logContent); @@ -537,15 +561,19 @@ public void Program_Run_WithLintFlag_MultipleErrors_ReportsAll() """); var logFile = PathHelpers.SafePathCombine(tempDir.DirectoryPath, LintLogFile); - using var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile]); - // Act - Program.Run(context); + // Act — dispose the context before reading the log to release the file handle on Windows + int exitCode; + using (var context = Context.Create(["--silent", "--log", logFile, "--lint", "--definition", definitionFile])) + { + Program.Run(context); + exitCode = context.ExitCode; + } // Assert — non-zero exit code and log contains BOTH the missing evidence-source error // AND the duplicate ID error, proving all errors are accumulated in one pass. var logContent = File.ReadAllText(logFile); - Assert.AreEqual(1, context.ExitCode); + Assert.AreEqual(1, exitCode); Assert.Contains("evidence-source", logContent); Assert.Contains("duplicate ID", logContent); Assert.Contains("Core-Logic", logContent); diff --git a/test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs b/test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs index 926a38d..b94ab9e 100644 --- a/test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs +++ b/test/DemaConsulting.ReviewMark.Tests/TestDirectory.cs @@ -44,9 +44,22 @@ public TestDirectory() /// public void Dispose() { - if (Directory.Exists(DirectoryPath)) + if (!Directory.Exists(DirectoryPath)) + { + return; + } + + try { Directory.Delete(DirectoryPath, recursive: true); } + catch (IOException) + { + // Ignore cleanup failures in tests (e.g., transient file locks on Windows). + } + catch (UnauthorizedAccessException) + { + // Ignore cleanup failures in tests (e.g., transient access issues on Windows). + } } } From 7147118cdac9aed2e6b272cd8c47e5d5b2d8efd7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 15:51:55 +0000 Subject: [PATCH 09/11] docs: add --lint to README and user guide Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/a5fb2a08-3a4d-4a2e-908c-d95bbe84dae0 --- README.md | 14 ++++++-- docs/guide/guide.md | 78 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 86 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index bae4b91..75504e9 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,7 @@ DEMA Consulting tool for automated file-review evidence management in regulated - 📋 **Coverage Reporting** - Review plan shows which files are covered and flags uncovered files - 📊 **Status Reporting** - Review report shows whether each review-set is Current, Stale, Missing, or Failed - 🔍 **Review Elaboration** - `--elaborate` prints the ID, fingerprint, and file list for a review set +- 🔎 **Configuration Linting** - `--lint` validates the definition file and reports all structural and semantic issues - 🚦 **Enforcement** - `--enforce` exits non-zero if any review-set is stale or missing, or any file is uncovered - 🔄 **Re-indexing** - `--index` scans PDF evidence files and writes an up-to-date `index.json` - ✅ **Self-Validation** - Built-in validation tests with TRX and JUnit output @@ -100,6 +101,12 @@ reviewmark --validate # Save validation results reviewmark --validate --results results.trx +# Validate definition file +reviewmark --lint + +# Validate a specific definition file +reviewmark --lint --definition path/to/definition.yaml + # Silent mode with logging reviewmark --silent --log output.log ``` @@ -112,6 +119,7 @@ reviewmark --silent --log output.log | `-?`, `-h`, `--help` | Display help message | | `--silent` | Suppress console output | | `--validate` | Run self-validation | +| `--lint` | Validate the definition file and report issues | | `--results ` | Write validation results to file (TRX or JUnit format) | | `--log ` | Write output to log file | | `--definition ` | Specify the definition YAML file (default: .reviewmark.yaml) | @@ -147,9 +155,10 @@ Running self-validation produces a report containing the following information: ✓ ReviewMark_WorkingDirectoryOverride - Passed ✓ ReviewMark_Enforce - Passed ✓ ReviewMark_Elaborate - Passed +✓ ReviewMark_Lint - Passed -Total Tests: 8 -Passed: 8 +Total Tests: 9 +Passed: 9 Failed: 0 ``` @@ -163,6 +172,7 @@ Each test in the report proves: - **`ReviewMark_WorkingDirectoryOverride`** - `--dir` overrides the working directory for file operations. - **`ReviewMark_Enforce`** - `--enforce` exits with non-zero code when reviews have issues. - **`ReviewMark_Elaborate`** - `--elaborate` prints a Markdown elaboration of a review set. +- **`ReviewMark_Lint`** - `--lint` validates a definition file and reports issues. See the [User Guide][link-guide] for more details on the self-validation tests. diff --git a/docs/guide/guide.md b/docs/guide/guide.md index 161db14..43463ef 100644 --- a/docs/guide/guide.md +++ b/docs/guide/guide.md @@ -103,9 +103,10 @@ Example validation report: ✓ ReviewMark_WorkingDirectoryOverride - Passed ✓ ReviewMark_Enforce - Passed ✓ ReviewMark_Elaborate - Passed +✓ ReviewMark_Lint - Passed -Total Tests: 8 -Passed: 8 +Total Tests: 9 +Passed: 9 Failed: 0 ``` @@ -121,6 +122,53 @@ Each test proves specific functionality works correctly: - **`ReviewMark_WorkingDirectoryOverride`** - `--dir` overrides the working directory for file operations. - **`ReviewMark_Enforce`** - `--enforce` exits with non-zero code when reviews have issues. - **`ReviewMark_Elaborate`** - `--elaborate` prints a Markdown elaboration of a review set. +- **`ReviewMark_Lint`** - `--lint` validates a definition file and reports issues. + +## Lint Definition File + +The `--lint` command validates the definition file (`.reviewmark.yaml`) and reports all +structural and semantic issues in a single pass. Unlike running the full tool, `--lint` never +queries the evidence store — it only checks the definition file itself. + +A successful lint exits with code 0; any issue causes a non-zero exit code. + +### Running Lint + +Lint the default definition file (`.reviewmark.yaml` in the working directory): + +```bash +reviewmark --lint +``` + +Lint a specific definition file: + +```bash +reviewmark --lint --definition path/to/definition.yaml +``` + +### What Lint Checks + +Lint checks the following: + +- **File readability** — the definition file exists and can be read. +- **YAML syntax** — the file is valid YAML; syntax errors include the filename and line number. +- **`evidence-source` block** — the block is present, has a `type` field (`url` or `fileshare`), + and has a `location` field. +- **Review sets** — each set has an `id`, a `title`, and at least one `paths` entry. +- **Duplicate IDs** — no two review sets share the same `id`. + +All detected issues are reported together so you can fix multiple problems in one pass. + +### Lint Error Messages + +Lint errors always include the filename. For YAML syntax errors, the line and column number +are also included: + +```text +Error: Failed to parse 'definition.yaml' at line 3, column 5: (yaml parse details) +Error: Invalid configuration in 'definition.yaml': Configuration is missing required 'evidence-source' block. +Error: Invalid configuration in 'definition.yaml': reviews[1] has duplicate ID 'core-module' (first defined at reviews[0]). +``` ## Silent Mode @@ -163,6 +211,7 @@ The following command-line options are supported: | `-?`, `-h`, `--help` | Display help message | | `--silent` | Suppress console output | | `--validate` | Run self-validation | +| `--lint` | Validate the definition file and report issues | | `--results ` | Write validation results to file (TRX or JUnit format) | | `--log ` | Write output to log file | | `--definition ` | Specify the definition YAML file (default: .reviewmark.yaml) | @@ -561,13 +610,34 @@ reviews: - "!src/Data/Generated/**" # exclude auto-generated entity classes ``` -## Example 4: Self-Validation with Results +## Example 4: Lint a Definition File + +Lint the default definition file (`.reviewmark.yaml`) to catch all configuration errors before +running the full tool: + +```bash +reviewmark --lint +``` + +Lint a specific definition file: + +```bash +reviewmark --lint --definition path/to/.reviewmark.yaml +``` + +With silent mode and logging (useful in CI pipelines): + +```bash +reviewmark --silent --log lint.log --lint +``` + +## Example 5: Self-Validation with Results ```bash reviewmark --validate --results validation-results.trx ``` -## Example 5: Silent Mode with Logging +## Example 6: Silent Mode with Logging ```bash reviewmark --silent --log tool-output.log From 3cce5e80cce1913cd77edf588da729897b062d17 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 16:28:46 +0000 Subject: [PATCH 10/11] fix: correct British spelling 'recognised' to American 'recognized' Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/2712fd02-b8b6-44bc-99bf-2a973437ffbb --- src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs index b886802..13ee2cd 100644 --- a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs @@ -136,7 +136,7 @@ file sealed class ReviewSetYaml file static class ReviewMarkConfigurationHelpers { /// - /// Returns true when is a recognised evidence-source + /// Returns true when is a recognized evidence-source /// type (url or fileshare, case-insensitive). /// /// The type string to test. From 0a40cb638cd7750bf4e357cc88632c752dab71f3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 16:52:27 +0000 Subject: [PATCH 11/11] feat: change lint output to [location]: [severity]: [issue] format Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> Agent-Logs-Url: https://github.com/demaconsulting/ReviewMark/sessions/3576c6ae-5203-472e-b193-49f8d6e46feb --- docs/guide/guide.md | 16 +++++++---- src/DemaConsulting.ReviewMark/Program.cs | 6 ++--- .../ReviewMarkConfiguration.cs | 27 ++++++++++++------- src/DemaConsulting.ReviewMark/Validation.cs | 2 +- .../ProgramTests.cs | 15 +++++------ 5 files changed, 38 insertions(+), 28 deletions(-) diff --git a/docs/guide/guide.md b/docs/guide/guide.md index 43463ef..63dfd2c 100644 --- a/docs/guide/guide.md +++ b/docs/guide/guide.md @@ -161,13 +161,19 @@ All detected issues are reported together so you can fix multiple problems in on ### Lint Error Messages -Lint errors always include the filename. For YAML syntax errors, the line and column number -are also included: +Lint errors follow the standard `[location]: [severity]: [issue]` format. For YAML syntax +errors the location includes the line and column number: ```text -Error: Failed to parse 'definition.yaml' at line 3, column 5: (yaml parse details) -Error: Invalid configuration in 'definition.yaml': Configuration is missing required 'evidence-source' block. -Error: Invalid configuration in 'definition.yaml': reviews[1] has duplicate ID 'core-module' (first defined at reviews[0]). +definition.yaml:3:5: error: (yaml parse details) +definition.yaml: error: Configuration is missing required 'evidence-source' block. +definition.yaml: error: reviews[1] has duplicate ID 'core-module' (first defined at reviews[0]). +``` + +When no issues are found: + +```text +definition.yaml: No issues found ``` ## Silent Mode diff --git a/src/DemaConsulting.ReviewMark/Program.cs b/src/DemaConsulting.ReviewMark/Program.cs index 5276a11..061adf5 100644 --- a/src/DemaConsulting.ReviewMark/Program.cs +++ b/src/DemaConsulting.ReviewMark/Program.cs @@ -172,19 +172,17 @@ private static void RunLintLogic(Context context) var directory = context.WorkingDirectory ?? Directory.GetCurrentDirectory(); var definitionFile = context.DefinitionFile ?? PathHelpers.SafePathCombine(directory, ".reviewmark.yaml"); - context.WriteLine($"Linting '{definitionFile}'..."); - // Lint the file, collecting all detectable errors in one pass. var errors = ReviewMarkConfiguration.Lint(definitionFile); foreach (var error in errors) { - context.WriteError($"Error: {error}"); + context.WriteError(error); } // Report overall result if (errors.Count == 0) { - context.WriteLine($"'{definitionFile}' is valid."); + context.WriteLine($"{definitionFile}: No issues found"); } } diff --git a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs index 13ee2cd..2a6eb6b 100644 --- a/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs +++ b/src/DemaConsulting.ReviewMark/ReviewMarkConfiguration.cs @@ -535,19 +535,26 @@ internal static IReadOnlyList Lint(string filePath) } catch (Exception ex) when (ex is not InvalidOperationException) { - errors.Add($"Failed to read configuration file '{filePath}': {ex.Message}"); + errors.Add($"{filePath}: error: {ex.Message}"); return errors; } // Try to parse the raw YAML model; if this fails we cannot do semantic checks. + // When the inner exception is a YamlException, format the location as "file:line:col" + // to match the standard linting output convention. ReviewMarkYaml raw; try { raw = ReviewMarkConfigurationHelpers.DeserializeRaw(yaml, filePath); } + catch (InvalidOperationException ex) when (ex.InnerException is YamlException yamlEx) + { + errors.Add($"{filePath}:{yamlEx.Start.Line}:{yamlEx.Start.Column}: error: {yamlEx.Message}"); + return errors; + } catch (InvalidOperationException ex) { - errors.Add(ex.Message); + errors.Add($"{filePath}: error: {ex.Message}"); return errors; } @@ -556,25 +563,25 @@ internal static IReadOnlyList Lint(string filePath) if (es == null) { errors.Add( - $"Invalid configuration in '{filePath}': Configuration is missing required 'evidence-source' block."); + $"{filePath}: error: Configuration is missing required 'evidence-source' block."); } else { if (string.IsNullOrWhiteSpace(es.Type)) { errors.Add( - $"Invalid configuration in '{filePath}': 'evidence-source' is missing a required 'type' field."); + $"{filePath}: error: 'evidence-source' is missing a required 'type' field."); } else if (!ReviewMarkConfigurationHelpers.IsSupportedEvidenceSourceType(es.Type)) { errors.Add( - $"Invalid configuration in '{filePath}': 'evidence-source' type '{es.Type}' is not supported (must be 'url' or 'fileshare')."); + $"{filePath}: error: 'evidence-source' type '{es.Type}' is not supported (must be 'url' or 'fileshare')."); } if (string.IsNullOrWhiteSpace(es.Location)) { errors.Add( - $"Invalid configuration in '{filePath}': 'evidence-source' is missing a required 'location' field."); + $"{filePath}: error: 'evidence-source' is missing a required 'location' field."); } } @@ -591,12 +598,12 @@ internal static IReadOnlyList Lint(string filePath) if (string.IsNullOrWhiteSpace(r.Id)) { errors.Add( - $"Invalid configuration in '{filePath}': Review set at index {i} is missing a required 'id' field."); + $"{filePath}: error: Review set at index {i} is missing a required 'id' field."); } else if (seenIds.TryGetValue(r.Id, out var firstIndex)) { errors.Add( - $"Invalid configuration in '{filePath}': reviews[{i}] has duplicate ID '{r.Id}' (first defined at reviews[{firstIndex}])."); + $"{filePath}: error: reviews[{i}] has duplicate ID '{r.Id}' (first defined at reviews[{firstIndex}])."); } else { @@ -606,13 +613,13 @@ internal static IReadOnlyList Lint(string filePath) if (string.IsNullOrWhiteSpace(r.Title)) { errors.Add( - $"Invalid configuration in '{filePath}': Review set at index {i} is missing a required 'title' field."); + $"{filePath}: error: Review set at index {i} is missing a required 'title' field."); } if (r.Paths == null || !r.Paths.Any(p => !string.IsNullOrWhiteSpace(p))) { errors.Add( - $"Invalid configuration in '{filePath}': Review set at index {i} is missing required 'paths' entries."); + $"{filePath}: error: Review set at index {i} is missing required 'paths' entries."); } } diff --git a/src/DemaConsulting.ReviewMark/Validation.cs b/src/DemaConsulting.ReviewMark/Validation.cs index 41a2950..d7e0568 100644 --- a/src/DemaConsulting.ReviewMark/Validation.cs +++ b/src/DemaConsulting.ReviewMark/Validation.cs @@ -407,7 +407,7 @@ private static void RunLintTest(Context context, DemaConsulting.TestResults.Test // Verify the log contains a success message var logContent = File.ReadAllText(logFile); - return logContent.Contains("is valid") ? null : "Lint output does not contain 'is valid'"; + return logContent.Contains("No issues found") ? null : "Lint output does not contain 'No issues found'"; }); } diff --git a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs index 0a33898..ee250bc 100644 --- a/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs +++ b/test/DemaConsulting.ReviewMark.Tests/ProgramTests.cs @@ -352,7 +352,7 @@ public void Program_Run_WithLintFlag_ValidConfig_ReportsSuccess() // Assert — exit code is zero and log contains success message var logContent = File.ReadAllText(logFile); Assert.AreEqual(0, exitCode); - Assert.Contains("is valid", logContent); + Assert.Contains("No issues found", logContent); } /// @@ -377,7 +377,7 @@ public void Program_Run_WithLintFlag_MissingConfig_ReportsError() // Assert — non-zero exit code and log contains an error mentioning the missing file var logContent = File.ReadAllText(logFile); Assert.AreEqual(1, exitCode); - Assert.Contains("Error:", logContent); + Assert.Contains("error:", logContent); Assert.Contains("nonexistent.yaml", logContent); } @@ -423,7 +423,7 @@ public void Program_Run_WithLintFlag_DuplicateIds_ReportsError() // Assert — non-zero exit code and log contains a clear duplicate-ID error message var logContent = File.ReadAllText(logFile); Assert.AreEqual(1, exitCode); - Assert.Contains("Error:", logContent); + Assert.Contains("error:", logContent); Assert.Contains("duplicate ID", logContent); Assert.Contains("Core-Logic", logContent); } @@ -463,7 +463,7 @@ public void Program_Run_WithLintFlag_UnknownSourceType_ReportsError() // Assert — non-zero exit code and log contains a clear unsupported-type error message var logContent = File.ReadAllText(logFile); Assert.AreEqual(1, exitCode); - Assert.Contains("Error:", logContent); + Assert.Contains("error:", logContent); Assert.Contains("ftp", logContent); Assert.Contains("not supported", logContent); } @@ -494,9 +494,8 @@ public void Program_Run_WithLintFlag_CorruptedYaml_ReportsError() // Assert — non-zero exit code and log contains an error naming the definition file and a line number var logContent = File.ReadAllText(logFile); Assert.AreEqual(1, exitCode); - Assert.Contains("Error:", logContent); - Assert.Contains("definition.yaml", logContent); - Assert.Contains("at line", logContent); + Assert.Contains("error:", logContent); + Assert.Contains("definition.yaml:", logContent); } /// @@ -531,7 +530,7 @@ public void Program_Run_WithLintFlag_MissingEvidenceSource_ReportsError() // Assert — non-zero exit code and log names the file and the missing field var logContent = File.ReadAllText(logFile); Assert.AreEqual(1, exitCode); - Assert.Contains("Error:", logContent); + Assert.Contains("error:", logContent); Assert.Contains("definition.yaml", logContent); Assert.Contains("evidence-source", logContent); }