Fix formal review findings across all 9 review-sets

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`.

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)

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@`)

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,15 +33,15 @@ 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
 
 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 <SpdxToolWorkflow>
+      └─► Execute: spdx-tool run-workflow <SpdxWorkflowFile>
                         │
                         └─► Workflow modifies the SPDX JSON embedded in .nupkg
 ```

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

src/DemaConsulting.SpdxTool/Commands/Query.cs

@@ -163,10 +163,11 @@ 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();
+        Task.WaitAll(stdoutTask, stderrTask);
+        var output = stdoutTask.Result.Trim() + stderrTask.Result.Trim();
 
         // Wait for the process to exit
         process.WaitForExit();

src/DemaConsulting.SpdxTool/Commands/RenameId.cs

@@ -98,7 +98,7 @@ public override void Run(Context context, YamlMappingNode step, Dictionary<strin
 
         // Get the 'old' input
         var oldId = GetMapString(inputs, "old", variables) ??
-                    throw new YamlException(step.Start, step.End, "'rename-id' command missing 'spdx' input");
+                    throw new YamlException(step.Start, step.End, "'rename-id' command missing 'old' input");
 
         // Rename the ID
         Rename(spdxFile, oldId, newId);
@@ -150,12 +150,6 @@ public static void Rename(SpdxDocument doc, string oldId, string newId)
             throw new CommandUsageException("Invalid new ID");
         }
 
-        // Verify the IDs are different
-        if (oldId == newId)
-        {
-            throw new CommandUsageException("Old and new IDs are the same");
-        }
-
         // Verify ID is not in use
         if (Array.Exists(doc.Packages, p => p.Id == newId) ||
             Array.Exists(doc.Files, f => f.Id == newId) ||

src/DemaConsulting.SpdxTool/Commands/Validate.cs

@@ -92,7 +92,7 @@ public override void Run(Context context, YamlMappingNode step, Dictionary<strin
 
         // Get the 'spdx' input
         var spdxFile = GetMapString(inputs, "spdx", variables) ??
-                       throw new YamlException(step.Start, step.End, "'to-markdown' command missing 'spdx' input");
+                       throw new YamlException(step.Start, step.End, "'validate' command missing 'spdx' input");
 
         // Get the 'ntia' input
         var ntiaValue = GetMapString(inputs, "ntia", variables);

src/DemaConsulting.SpdxTool/Context.cs

@@ -155,7 +155,7 @@
     /// <param name="args">Program arguments</param>
     /// <returns>Program context</returns>
     /// <exception cref="InvalidOperationException">Thrown on invalid arguments</exception>
     public static Context Create(string[] args)
     {
         // Process arguments
         var version = false;
@@ -203,7 +203,12 @@
 
                 case "--depth":
                     // Handle depth argument
-                    depth = int.Parse(ParseArgument(arg, "Missing depth argument"));
+                    var depthStr = ParseArgument(arg, "Missing depth argument");
+                    if (!int.TryParse(depthStr, out depth))
+                    {
+                        throw new InvalidOperationException($"Invalid depth value '{depthStr}': must be an integer");
+                    }
+
                     break;
 
                 case "-l":
@@ -224,7 +229,32 @@
         }
 
         // Return the new context
-        return new Context(logFile != null ? new StreamWriter(logFile) : null, extra.AsReadOnly())
+        StreamWriter? logWriter = null;
+        if (logFile != null)
+        {
+            try
+            {
+                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);
+            }
+        }
+
+        return new Context(logWriter, extra.AsReadOnly())
         {
             Version = version,
             Help = help,

test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs

@@ -59,11 +59,14 @@ 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
-        output = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd();
+        // 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
         process.WaitForExit();

test/DemaConsulting.SpdxTool.Tests/Runner.cs

@@ -53,11 +53,14 @@ 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
-        output = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd();
+        // 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
         process.WaitForExit();