From 2b7e127327a3e73ad7c8a8803cc629c4f386eeca Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 1 Apr 2026 22:49:53 +0000 Subject: [PATCH 1/3] fix: address formal review findings from all 9 review-sets Agent-Logs-Url: https://github.com/demaconsulting/SpdxTool/sessions/7e5da612-1917-415a-9035-a05226aa5faf Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> --- docs/design/commands/commands.md | 10 ++++----- docs/design/introduction.md | 5 +++-- docs/design/spdxtool-system.md | 8 +++---- docs/design/spdxtool-targets-system.md | 10 ++++----- docs/design/utility.md | 2 +- src/DemaConsulting.SpdxTool/Commands/Query.cs | 8 +++---- .../Commands/RenameId.cs | 8 +------ .../Commands/Validate.cs | 2 +- src/DemaConsulting.SpdxTool/Context.cs | 22 +++++++++++++++++-- .../DotnetRunner.cs | 6 +++-- test/DemaConsulting.SpdxTool.Tests/Runner.cs | 6 +++-- 11 files changed, 52 insertions(+), 35 deletions(-) diff --git a/docs/design/commands/commands.md b/docs/design/commands/commands.md index 1399acb..7d84e6c 100644 --- a/docs/design/commands/commands.md +++ b/docs/design/commands/commands.md @@ -4,14 +4,14 @@ The Commands subsystem provides the implementations for all CLI subcommands exposed by DemaConsulting.SpdxTool. Each subcommand corresponds to a discrete SPDX document -operation or workflow step, registered by name in the `CommandRegistry` and dispatched +operation or workflow step, registered by name in the `CommandsRegistry` and dispatched by `Program`. ## Architecture ### Command Registry Pattern -All commands are registered in `CommandRegistry` as `CommandEntry` instances. When +All commands are registered in `CommandsRegistry` as `CommandEntry` instances. When `Program` parses a command name from the CLI arguments, it looks up the corresponding `Command` implementation in the registry and calls `Execute(Context, string[])`. @@ -51,7 +51,7 @@ Error handling uses two exception types: CLI arguments │ ▼ -CommandRegistry.Lookup(commandName) +CommandsRegistry.Lookup(commandName) │ ▼ Command.Execute(Context, args) @@ -69,7 +69,7 @@ Command.Execute(Context, args) The `RunWorkflow` command reads a YAML workflow file and iterates over its steps. Each step specifies a command name and its arguments. Steps are dispatched back -through `CommandRegistry`, allowing any registered command to be used as a workflow +through `CommandsRegistry`, allowing any registered command to be used as a workflow step. Variable substitution (`${{ variables.name }}`) is performed on step arguments before dispatch, using values from the `Context` variable map. @@ -89,6 +89,6 @@ enabling versioned and distributable workflow definitions. - Commands are stateless; all mutable state is carried by the `Context` parameter. - Commands do not reference each other directly; all cross-command calls go through - `CommandRegistry` to maintain loose coupling. + `CommandsRegistry` to maintain loose coupling. - File paths in command arguments are resolved relative to the current working directory using `PathHelpers`. diff --git a/docs/design/introduction.md b/docs/design/introduction.md index 6b5ad2e..21043ac 100644 --- a/docs/design/introduction.md +++ b/docs/design/introduction.md @@ -63,9 +63,10 @@ DemaConsulting.SpdxTool (System) │ ├── ValidateRunNuGetWorkflow.cs (Unit) │ ├── ValidateToMarkdown.cs (Unit) │ └── ValidateUpdatePackage.cs (Unit) -├── Utility (Subsystem) +├── Spdx (Unit Group) │ ├── RelationshipDirection.cs (Unit) -│ ├── SpdxHelpers.cs (Unit) +│ └── SpdxHelpers.cs (Unit) +├── Utility (Subsystem) │ ├── PathHelpers.cs (Unit) │ └── Wildcard.cs (Unit) ├── Context.cs (Unit) diff --git a/docs/design/spdxtool-system.md b/docs/design/spdxtool-system.md index 6fd7306..7a839a8 100644 --- a/docs/design/spdxtool-system.md +++ b/docs/design/spdxtool-system.md @@ -18,10 +18,10 @@ rather than a regular command. ### Major Components - **Program** — parses the CLI arguments, initializes a `Context`, and dispatches to - `CommandRegistry`. + `CommandsRegistry`. - **Context** — carries the runtime execution state: console/log output streams, the silent flag, and the mutable variables map used by workflow commands. -- **CommandRegistry** — maintains the map of command names to `Command` implementations +- **CommandsRegistry** — maintains the map of command names to `Command` implementations and performs command lookup and dispatch. - **Commands subsystem** — contains one `Command`-derived class per supported CLI subcommand (e.g., `AddPackage`, `RunWorkflow`, `Validate`). @@ -73,7 +73,7 @@ Program.cs ────────────────────── Context.cs (output, log, variables) │ │ │ --validate flag ▼ ▼ -CommandRegistry ──► Command.Execute() SelfTest.Validate.Run() +CommandsRegistry ──► Command.Execute() SelfTest.Validate.Run() │ │ ▼ ▼ SPDX document I/O SelfTest.* @@ -96,7 +96,7 @@ CommandRegistry ──► Command.Execute() SelfTest.Validate.Run() ## Integration Patterns - **Command pattern**: Each CLI subcommand is a self-contained `Command` class with - `Execute(Context, string[])` semantics, registered by name in `CommandRegistry`. + `Execute(Context, string[])` semantics, registered by name in `CommandsRegistry`. - **Variable substitution**: Workflow YAML supports `${{ variables.name }}` tokens that are replaced at runtime from the `Context` variable map. - **Source-filter evidence**: ReqStream source filters (`windows@`, `ubuntu@`, `net8.0@`) diff --git a/docs/design/spdxtool-targets-system.md b/docs/design/spdxtool-targets-system.md index 4b5f44b..80f0b8e 100644 --- a/docs/design/spdxtool-targets-system.md +++ b/docs/design/spdxtool-targets-system.md @@ -24,7 +24,7 @@ in the MSBuild pipeline. The `DecorateNuGetSbom` target conditionally invokes `spdx-tool run-workflow` with a user-supplied workflow file. The workflow file path is specified via the -`SpdxToolWorkflow` MSBuild property. The `spdx-tool` global tool must be installed +`SpdxWorkflowFile` MSBuild property. The `spdx-tool` global tool must be installed and available on the system `PATH`. ### Configuration Properties @@ -33,7 +33,7 @@ and available on the system `PATH`. |----------------------|---------|------------------------------------------------------| | `DecorateSBOM` | `false` | Set to `true` to enable SBOM decoration during pack | | `GenerateSBOM` | `true` | When `false`, skips decoration (no SBOM to decorate) | -| `SpdxToolWorkflow` | — | Path to the workflow YAML file for decoration | +| `SpdxWorkflowFile` | — | Path to the workflow YAML file for decoration | ## Conditional Execution @@ -41,7 +41,7 @@ The `DecorateNuGetSbom` target is skipped when: - `DecorateSBOM` is not set to `true` (opt-in required) - `GenerateSBOM` is `false` (no SBOM generated to decorate) -- `SpdxToolWorkflow` path does not exist (build error reported) +- `SpdxWorkflowFile` path does not exist (build error reported) ## Data Flow @@ -58,9 +58,9 @@ DecorateNuGetSbom target │ ├─► Check GenerateSBOM == true (skip if false) │ - ├─► Check SpdxToolWorkflow exists (error if missing) + ├─► Check SpdxWorkflowFile exists (error if missing) │ - └─► Execute: spdx-tool run-workflow + └─► Execute: spdx-tool run-workflow │ └─► Workflow modifies the SPDX JSON embedded in .nupkg ``` diff --git a/docs/design/utility.md b/docs/design/utility.md index be5ab93..2a16806 100644 --- a/docs/design/utility.md +++ b/docs/design/utility.md @@ -31,7 +31,7 @@ subsystem, including relationship traversal helpers and package attribute manipu ### RelationshipDirection `RelationshipDirection` is an enumeration expressing the traversal direction of an -SPDX relationship query: `Forward`, `Reverse`, or `Both`. Consumed by query and +SPDX relationship query: `Parent`, `Child`, or `Sibling`. Consumed by query and find operations in the Commands subsystem. ## Design Constraints diff --git a/src/DemaConsulting.SpdxTool/Commands/Query.cs b/src/DemaConsulting.SpdxTool/Commands/Query.cs index 5c6a86b..8c86bd0 100644 --- a/src/DemaConsulting.SpdxTool/Commands/Query.cs +++ b/src/DemaConsulting.SpdxTool/Commands/Query.cs @@ -163,10 +163,10 @@ public static string QueryProgramOutput(string pattern, string program, string[] throw new CommandErrorException($"Unable to start program '{program}'"); } - // Save the output - var output = - process.StandardOutput.ReadToEnd().Trim() + - process.StandardError.ReadToEnd().Trim(); + // Save the output (read both streams concurrently to prevent deadlock) + var stdoutTask = process.StandardOutput.ReadToEndAsync(); + var stderrTask = process.StandardError.ReadToEndAsync(); + var output = stdoutTask.Result.Trim() + stderrTask.Result.Trim(); // Wait for the process to exit process.WaitForExit(); diff --git a/src/DemaConsulting.SpdxTool/Commands/RenameId.cs b/src/DemaConsulting.SpdxTool/Commands/RenameId.cs index 492b40c..e3c12c2 100644 --- a/src/DemaConsulting.SpdxTool/Commands/RenameId.cs +++ b/src/DemaConsulting.SpdxTool/Commands/RenameId.cs @@ -98,7 +98,7 @@ public override void Run(Context context, YamlMappingNode step, Dictionary p.Id == newId) || Array.Exists(doc.Files, f => f.Id == newId) || diff --git a/src/DemaConsulting.SpdxTool/Commands/Validate.cs b/src/DemaConsulting.SpdxTool/Commands/Validate.cs index e87bbfb..280611a 100644 --- a/src/DemaConsulting.SpdxTool/Commands/Validate.cs +++ b/src/DemaConsulting.SpdxTool/Commands/Validate.cs @@ -92,7 +92,7 @@ public override void Run(Context context, YamlMappingNode step, Dictionary Date: Wed, 1 Apr 2026 22:53:22 +0000 Subject: [PATCH 2/3] fix: use Task.WaitAll before accessing Result to address code review feedback Agent-Logs-Url: https://github.com/demaconsulting/SpdxTool/sessions/7e5da612-1917-415a-9035-a05226aa5faf Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> --- src/DemaConsulting.SpdxTool/Commands/Query.cs | 1 + test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs | 1 + test/DemaConsulting.SpdxTool.Tests/Runner.cs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/DemaConsulting.SpdxTool/Commands/Query.cs b/src/DemaConsulting.SpdxTool/Commands/Query.cs index 8c86bd0..1b8db5c 100644 --- a/src/DemaConsulting.SpdxTool/Commands/Query.cs +++ b/src/DemaConsulting.SpdxTool/Commands/Query.cs @@ -166,6 +166,7 @@ public static string QueryProgramOutput(string pattern, string program, string[] // Save the output (read both streams concurrently to prevent deadlock) var stdoutTask = process.StandardOutput.ReadToEndAsync(); var stderrTask = process.StandardError.ReadToEndAsync(); + Task.WaitAll(stdoutTask, stderrTask); var output = stdoutTask.Result.Trim() + stderrTask.Result.Trim(); // Wait for the process to exit diff --git a/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs b/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs index 5261824..4249ada 100644 --- a/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs +++ b/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs @@ -65,6 +65,7 @@ public static int Run(out string output, string workingDirectory, params string[ // Save the output (read both streams concurrently to prevent deadlock) var stdoutTask = process.StandardOutput.ReadToEndAsync(); var stderrTask = process.StandardError.ReadToEndAsync(); + Task.WaitAll(stdoutTask, stderrTask); output = stdoutTask.Result + stderrTask.Result; // Wait for the process to exit diff --git a/test/DemaConsulting.SpdxTool.Tests/Runner.cs b/test/DemaConsulting.SpdxTool.Tests/Runner.cs index 58a049b..15e209a 100644 --- a/test/DemaConsulting.SpdxTool.Tests/Runner.cs +++ b/test/DemaConsulting.SpdxTool.Tests/Runner.cs @@ -59,6 +59,7 @@ public static int Run(out string output, string program, params string[] argumen // Save the output (read both streams concurrently to prevent deadlock) var stdoutTask = process.StandardOutput.ReadToEndAsync(); var stderrTask = process.StandardError.ReadToEndAsync(); + Task.WaitAll(stdoutTask, stderrTask); output = stdoutTask.Result + stderrTask.Result; // Wait for the process to exit From 02fd499ee7623a05d097ec65de21dd88627f7eb8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 1 Apr 2026 23:26:04 +0000 Subject: [PATCH 3/3] fix: address PR review feedback on Context.cs exception handling and Process disposal Agent-Logs-Url: https://github.com/demaconsulting/SpdxTool/sessions/d6c7890b-568e-452b-a0be-0390962bb82c Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> --- src/DemaConsulting.SpdxTool/Context.cs | 12 ++++++++++++ .../DotnetRunner.cs | 4 ++-- test/DemaConsulting.SpdxTool.Tests/Runner.cs | 4 ++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/DemaConsulting.SpdxTool/Context.cs b/src/DemaConsulting.SpdxTool/Context.cs index 6a28ce0..dd7807b 100644 --- a/src/DemaConsulting.SpdxTool/Context.cs +++ b/src/DemaConsulting.SpdxTool/Context.cs @@ -236,6 +236,18 @@ public static Context Create(string[] args) { logWriter = new StreamWriter(logFile); } + catch (UnauthorizedAccessException e) + { + throw new InvalidOperationException($"Access denied creating log file '{logFile}': {e.Message}", e); + } + catch (ArgumentException e) + { + throw new InvalidOperationException($"Invalid log file path '{logFile}': {e.Message}", e); + } + catch (NotSupportedException e) + { + throw new InvalidOperationException($"Unsupported log file path '{logFile}': {e.Message}", e); + } catch (IOException e) { throw new InvalidOperationException($"Cannot create log file '{logFile}': {e.Message}", e); diff --git a/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs b/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs index 4249ada..53e9eef 100644 --- a/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs +++ b/test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs @@ -59,8 +59,8 @@ public static int Run(out string output, string workingDirectory, params string[ } // Start the process - var process = Process.Start(startInfo) ?? - throw new InvalidOperationException("Failed to start dotnet process"); + using var process = Process.Start(startInfo) ?? + throw new InvalidOperationException("Failed to start dotnet process"); // Save the output (read both streams concurrently to prevent deadlock) var stdoutTask = process.StandardOutput.ReadToEndAsync(); diff --git a/test/DemaConsulting.SpdxTool.Tests/Runner.cs b/test/DemaConsulting.SpdxTool.Tests/Runner.cs index 15e209a..6b0efd8 100644 --- a/test/DemaConsulting.SpdxTool.Tests/Runner.cs +++ b/test/DemaConsulting.SpdxTool.Tests/Runner.cs @@ -53,8 +53,8 @@ public static int Run(out string output, string program, params string[] argumen } // Start the process - var process = Process.Start(startInfo) ?? - throw new InvalidOperationException("Failed to start process"); + using var process = Process.Start(startInfo) ?? + throw new InvalidOperationException("Failed to start process"); // Save the output (read both streams concurrently to prevent deadlock) var stdoutTask = process.StandardOutput.ReadToEndAsync();