feat(delivery): ✨ add delivery metadata commands and manifest support

[PrzemyslawKlys]

• Add Public/New-ConfigurationDelivery.ps1 to emit Options.Delivery (Enable, InternalsPath, IncludeRootReadme, IncludeRootChangelog, Schema) for build config.
• Add Public/Install-ModuleDocumentation.ps1 to install bundled Internals content (and optional README/CHANGELOG) using PrivateData.PSData.PSPublishModuleDelivery.
• Export New-ConfigurationDelivery and Install-ModuleDocumentation in PSPublishModule.psd1.
• Update Private/New-PersonalManifest.ps1 to validate and emit PSPublishModuleDelivery into PrivateData.PSData when Options.Delivery is present.

[claude]

Pull Request Review: feat(delivery): ✨ add delivery metadata commands and manifest support

Summary

This PR adds new functionality for embedding and installing bundled module documentation/internals. The implementation includes two new public functions and updates to the manifest generation logic. Overall, the code is well-structured and follows PowerShell best practices.

✅ Strengths

1. Excellent Documentation: Both new functions have comprehensive comment-based help with clear examples
2. Consistent Architecture: Follows the established pattern for configuration functions (Type='Options' structure)
3. PowerShell 5.1 Compatibility: Code is written to work across PS5.1 and PS7+ (e.g., using [System.Collections.Generic.List[string]] instead of array literals in pipeline scenarios)
4. Proper Parameter Sets: Install-ModuleDocumentation correctly uses parameter sets for different input modes
5. Good Error Handling: Includes appropriate error messages with context

🔍 Code Quality Observations

New-ConfigurationDelivery.ps1

Line 61: Uses the correct Type = 'Options' pattern, which integrates with the existing New-PrepareStructure.ps1:246-254 processing logic.

Line 47-57: Schema versioning (Schema = '1.0') is a good forward-thinking addition for future compatibility.

Install-ModuleDocumentation.ps1

Line 52: Good use of generic list for performance:

$resolvedTargets = [System.Collections.Generic.List[string]]::new()

Line 77: Uses Test-ModuleManifest which is appropriate, though it can be slow for large manifests.

Line 83-84: Smart fallback logic for boolean values:

$includeReadme = if ($null -ne $delivery.IncludeRootReadme) { [bool]$delivery.IncludeRootReadme } else { $true }

Private/New-PersonalManifest.ps1

Line 102: Good validation - adds PSPublishModuleDelivery to the approved list of PrivateData.PSData entries.

Line 127-129: Conditional logic correctly checks both existence and Enable flag before emitting metadata.

⚠️ Potential Issues & Improvements

1. Missing Enable Check in New-ConfigurationDelivery

Severity: Low
Location: Public/New-ConfigurationDelivery.ps1:50

The function returns early if -Enable is not set, but this means callers can't create a disabled delivery configuration. Consider:

# Current behavior: returns nothing if Enable is false
# Suggested: Always return configuration, let consuming code check Enable flag
if (-not $Enable) { return }  # Line 50

# Alternative approach:
$delivery = [ordered] @{
    Enable = $Enable.IsPresent  # Always include Enable state
    # ... rest of config
}

2. Potential Path Traversal Risk

Severity: Medium
Location: Public/Install-ModuleDocumentation.ps1:86-87

The InternalsPath from manifest metadata is used without validation:

$internalsRel = if ($delivery -and $delivery.InternalsPath) { [string]$delivery.InternalsPath } else { 'Internals' }
$internalsPath = Join-Path $Module.ModuleBase $internalsRel

Recommendation: Add path traversal validation:

# After line 86, add:
if ($internalsRel -match '\.\.[\\/]') {
    throw "InternalsPath '$internalsRel' contains path traversal characters and is not allowed."
}

3. Error Handling: SilentlyContinue May Hide Issues

Severity: Low
Location: Public/Install-ModuleDocumentation.ps1:104, 109, 116

Using -ErrorAction SilentlyContinue when copying README/CHANGELOG files may hide legitimate errors:

$rootFiles = @(Get-ChildItem -Path $Module.ModuleBase -File -ErrorAction SilentlyContinue)
# ... later ...
Copy-Item -LiteralPath $file.FullName -Destination $dest -Force:$Force.IsPresent -ErrorAction SilentlyContinue

Recommendation: Consider warning the user if files are expected but cannot be copied, or use -ErrorAction Stop with a try/catch.

4. Version Resolution Logic Could Be Clearer

Severity: Low
Location: Public/Install-ModuleDocumentation.ps1:66-72

The version resolution when using -Module parameter with -RequiredVersion is complex and might surprise users:

if ($RequiredVersion -and $Module.Version -ne $RequiredVersion) {
    $resolved = Get-Module -ListAvailable -Name $Module.Name | Where-Object { $_.Version -eq $RequiredVersion } ...

This changes the piped module object, which might be unexpected. Consider adding a note to the documentation about this behavior.

5. Missing Validation in New-PersonalManifest.ps1

Severity: Low
Location: Private/New-PersonalManifest.ps1:127

The code checks $Configuration.Options.Delivery.Enable but doesn't validate the structure of the Delivery object. Consider adding validation for required fields (InternalsPath, Schema, etc.).

🧪 Test Coverage

Missing: No test files were added for the new functionality.

Recommendation: Add tests for:

• New-ConfigurationDelivery.ps1: Test output structure, enable/disable behavior
• Install-ModuleDocumentation.ps1: Test parameter sets, file copying, version resolution
• New-PersonalManifest.ps1: Test that delivery metadata is correctly embedded in the manifest

You can follow the pattern in Tests/Build-Module.Tests.ps1 for guidance.

🔒 Security Considerations

1. Path Traversal: See issue Customized Release Path #2 above - should validate InternalsPath from manifest
2. File Overwrite: The -Force parameter allows overwriting files, which is expected but should be used cautiously
3. Manifest Trust: Install-ModuleDocumentation trusts metadata from the module manifest (PSPublishModuleDelivery). This is generally safe since module manifests are part of the module package, but consider documenting this trust boundary.

🎯 Performance Considerations

• Test-ModuleManifest (line 77) can be slow but is necessary for proper metadata parsing
• File copying operations are synchronous; for large Internals folders, this could take time
• Multiple Get-ChildItem calls for README/CHANGELOG could be optimized to a single call with filtering

📋 Documentation

Strengths:

• Excellent inline documentation
• Clear parameter descriptions
• Good examples in comment-based help

Suggestions:

• Add a note in Install-ModuleDocumentation about requiring New-ConfigurationDelivery -Enable during module build
• Document the relationship between the build-time New-ConfigurationDelivery and runtime Install-ModuleDocumentation
• Consider adding a "NOTES" section explaining the workflow

🎉 Conclusion

This is a solid PR that adds useful functionality while maintaining consistency with the existing codebase. The code quality is high, follows PowerShell conventions, and includes good documentation.

Recommendation: ✅ Approve with minor suggestions

The identified issues are mostly low severity and can be addressed in follow-up PRs. The path traversal validation (issue #2) should be considered before merge, but the risk is relatively low since the InternalsPath comes from the module manifest itself (which is part of the signed/trusted module package).

Priority fixes before merge:

1. Add path traversal validation for InternalsPath (Medium severity)
2. Consider whether the early return on -Enable:$false in New-ConfigurationDelivery is the desired behavior

Nice to have (can be follow-up PRs):

• Add test coverage
• Improve error handling for README/CHANGELOG copying
• Add more documentation about the build-to-install workflow

---

Review generated with Claude Code 🤖

[claude]

Code Review: PR #41 - Delivery Metadata Commands and Manifest Support

Summary

This PR adds a delivery system for bundling and installing module documentation/examples with clear metadata support. Two new public functions are introduced with manifest generation logic extended.

Strengths

• Well-documented with excellent comment-based help
• Follows established module conventions consistently
• Proper pipeline support and ShouldProcess implementation
• Thoughtful PS5/PS7 cross-version compatibility
• Clean separation of concerns between configuration and installation
• Schema versioning for future extensibility

Critical Issues

1. Path Traversal Security Risk (Install-ModuleDocumentation.ps1:83,87)
A malicious manifest could specify InternalsPath='../../../etc' to access files outside module base. Add validation:

$resolvedBase = [System.IO.Path]::GetFullPath($Module.ModuleBase)
$resolvedInternals = [System.IO.Path]::GetFullPath($internalsPath)
if (-not $resolvedInternals.StartsWith($resolvedBase)) {
    throw "InternalsPath attempts to access paths outside module base"
}

2. Test-ModuleManifest Warnings (Install-ModuleDocumentation.ps1:80)
Add -WarningAction SilentlyContinue to prevent warnings interfering with variable assignment.

High Priority

3. Missing Structure Validation (New-PersonalManifest.ps1:128-130)
Validate Configuration.Options.Delivery is a hashtable before embedding in manifest.

4. Inconsistent Error Handling (Install-ModuleDocumentation.ps1:102,110,117)
Uses -ErrorAction Stop for Internals but SilentlyContinue for optional files, which could hide legitimate errors.

5. Parameter Validation (New-ConfigurationDelivery.ps1:45)
Add [ValidateNotNullOrEmpty()] to InternalsPath parameter.

Medium Priority

6. Test Coverage
No tests for new functions. Consider adding Pester tests covering module not found, missing Internals, path traversal, version subfolder behavior, and pipeline scenarios.

7. Documentation
Add .LINK sections pointing to related functions like Install-ModuleDocumentation and New-ConfigurationInformation.

Performance & Security

• Efficient file operations with proper Generic.List usage
• Low security risk overall, but path traversal must be addressed
• Consider that PSPublishModuleDelivery metadata comes from potentially untrusted manifests

Conclusion

Well-crafted PR following PowerShell conventions with clean, documented code. Main concerns are input validation and path security that should be addressed before merge.

Recommendation: Approve with changes requested for security validation

[claude]

Pull Request Review: feat(delivery) - Add delivery metadata commands and manifest support

Summary

This PR introduces a new delivery system for bundling and installing internal documentation/examples alongside PowerShell modules. The implementation includes two new public cmdlets (New-ConfigurationDelivery and Install-ModuleDocumentation), manifest metadata emission, and build-time staging of Internals content.

Code Quality & Best Practices

✅ Strengths

1. Well-documented functions: Both public functions have comprehensive comment-based help with synopsis, description, parameters, and examples.

2. Parameter validation: Good use of [ValidateSet] for ReadmeDestination and ChangelogDestination in New-ConfigurationDelivery.ps1:54-57.

3. Pipeline support: Install-ModuleDocumentation properly supports pipeline input with two parameter sets (ByName and ByModule).

4. ShouldProcess support: Install-ModuleDocumentation correctly implements -WhatIf and -Confirm support.

5. Cross-platform compatibility: Code handles path separators correctly for Windows/Unix systems.

6. Version compatibility: Code is designed to work with both PS5.1 and PS7+ (e.g., Install-ModuleDocumentation.ps1:53,55-59).

⚠️ Issues & Concerns

Critical Issues

1. Missing Error Handling in Copy Operations (Start-ModuleMerging.ps1:205-219)

   • The file copy operations use -ErrorAction SilentlyContinue, which silently swallows errors
   • Users won't know if README/CHANGELOG files failed to copy
   • Recommendation: Use -ErrorAction Stop and wrap in try-catch, or at minimum use -ErrorAction Continue with warning messages

2. Potential Path Traversal Risk (Install-ModuleDocumentation.ps1:83)

   • The InternalsPath from manifest metadata is used directly without validation
   • Malicious manifests could specify paths like ../../ to escape module base
   • Recommendation: Add path validation to ensure InternalsPath doesn't escape module boundaries:

   $internalsPath = Join-Path $Module.ModuleBase $internalsRel
   $resolvedPath = [System.IO.Path]::GetFullPath($internalsPath)
   if (-not $resolvedPath.StartsWith([System.IO.Path]::GetFullPath($Module.ModuleBase))) {
       throw "InternalsPath attempts to escape module base directory"
   }

3. Missing Validation in Manifest Update (New-PersonalManifest.ps1:102)

   • Added PSPublishModuleDelivery to $ValidateEntriesPrivateData but no schema validation
   • Invalid delivery data could corrupt manifests
   • Recommendation: Validate the delivery object structure before adding to manifest

Moderate Issues

4. Inconsistent Default Behavior (Install-ModuleDocumentation.ps1:84-85)

   • Defaults IncludeRootReadme and IncludeRootChangelog to $true when delivery metadata is absent
   • But New-ConfigurationDelivery.ps1:52-53 defaults these switches to $false
   • Impact: Unexpected behavior when delivery metadata is missing
   • Recommendation: Align defaults or document the intentional difference

5. Regex Pattern Vulnerability (Start-ModuleMerging.ps1:33-34)

   • Uses -notlike with wildcards to filter directories: 'Private*', 'Public*', etc.
   • Could inadvertently exclude directories like PrivateData or PublicDocs
   • Recommendation: Use exact matching or more specific patterns:

   $_ -ne 'Private' -and $_ -ne 'Public' -and $_ -ne 'Classes' -and $_ -ne 'Enums'

6. Missing Schema Validation (New-ConfigurationDelivery.ps1:69)

   • Hard-coded schema version '1.1' with no validation mechanism
   • Future schema changes could break older installations
   • Recommendation: Document schema evolution strategy and add version checking in Install-ModuleDocumentation

7. Potential Performance Issue (Start-ModuleMerging.ps1:55-59)

   • Recursive Get-ChildItem on every Internals* directory during build
   • Could be slow with large directory structures
   • Recommendation: Consider caching or filtering at the glob level

Minor Issues

8. Inconsistent Null Checks (Start-ModuleMerging.ps1:189-192)

   • Uses $null -eq $IsWindows check pattern inconsistently
   • Some locations check, others don't (e.g., line 62)
   • Recommendation: Establish consistent pattern throughout

9. Missing Output Object (Install-ModuleDocumentation.ps1:126)

   • Returns destination paths but no structured object with metadata
   • Makes it harder for downstream automation to understand what was installed
   • Recommendation: Return [PSCustomObject] with properties like ModuleName, Version, DestinationPath, FilesInstalled

10. Typo in Comment (Private/New-PersonalManifest.ps1:23)

    • Line 23: # if module is hybrid (binary + script we need to ddo this) - "ddo" should be "do"
    • Pre-existing but worth fixing

11. BOM in File (Start-ModuleMerging.ps1:1)

    • File starts with UTF-8 BOM character ()
    • May cause issues with some tools
    • Recommendation: Save without BOM (as done elsewhere in codebase)

Security Concerns

1. Path Traversal Risk: As mentioned in issue Customized Release Path #2, untrusted manifest data could specify malicious paths
2. File Overwrite Risk: Install-ModuleDocumentation overwrites files when -Force is used without additional confirmation beyond -WhatIf
3. No Integrity Checking: No validation that copied files match expected content (consider checksums)

Performance Considerations

1. Recursive Directory Scanning: Start-ModuleMerging.ps1:55-59 recursively scans directories during build, potentially slow
2. Test-ModuleManifest Call: Install-ModuleDocumentation.ps1:80 calls Test-ModuleManifest which can be slow on large modules
3. Multiple File Iterations: Start-ModuleMerging.ps1:202-223 iterates root files twice (once for README, once for CHANGELOG) - could be combined

Test Coverage

⚠️ No test files are included in this PR. Consider adding:

1. Unit tests for New-ConfigurationDelivery:

   • Verify correct structure emission
   • Test with/without optional parameters
   • Validate schema version

2. Integration tests for Install-ModuleDocumentation:

   • Test with valid/invalid module names
   • Test pipeline input (by name and by object)
   • Test path traversal prevention
   • Test missing Internals directory
   • Test with/without delivery metadata
   • Test -WhatIf and -Force behavior

3. Build tests for Start-ModuleMerging:

   • Verify README/CHANGELOG copied to correct destinations
   • Test all destination modes (Internals/Root/Both/None)
   • Test with missing files

Additional Recommendations

1. Documentation: Add examples to README showing the complete workflow
2. Consistency: The feature adds Microsoft.PowerShell.PSResourceGet as a required module but Install-ModuleDocumentation doesn't appear to use it - clarify if this is for future use
3. Validation Function: Consider extracting path validation logic into a private helper function for reuse
4. Logging: Add verbose output to help users understand what's being copied during build/install

Summary

This is a useful feature that enables better distribution of module documentation. The code is generally well-structured and follows PowerShell best practices. However, there are some security concerns (path traversal) and error handling issues that should be addressed before merging.

Recommendation: Address critical issues (#1, #2, #3) before merging, and consider the moderate issues for improved robustness.

---

🤖 Generated with Claude Code

[claude]

Pull Request Review: Delivery Metadata Commands and Manifest Support

Thank you for this comprehensive feature addition! This PR adds valuable functionality for bundling and deploying internal documentation.

Strengths

1. Well-structured design with clear separation between configuration and execution
2. Excellent documentation with comprehensive help and examples
3. Good backward compatibility handling
4. Robust parameter validation using ValidateSet
5. Proper ShouldProcess support

Potential Bugs and Issues - High Priority

1. Install-ModuleDocumentation.ps1:146, 153 - Empty catch blocks hide failures silently. Recommendation: Add Write-Warning for failed copy operations
2. Copy-PSPDirectoryTree.ps1:21 - Selective error suppression only re-throws if target doesn't exist, swallows permission/lock errors
3. Register-Certificate.ps1:65-67 - Duplicate thumbprint checking logic (lines 53-58 and 65-70). The second check is unreachable

Medium Priority Issues

4. Install-ModuleDocumentation.ps1:97 - Test-ModuleManifest is slow; consider Import-PowerShellDataFile instead
5. Start-ModuleMerging.ps1:33-35 - Pattern matching won't catch subdirectories like Foo/Private
6. OnImportAndRemove.cs:46 - StartsWith could match wrong assemblies (Foo matches FooBar). Use AssemblyName for precise matching

Security Concerns

1. Copy-PSPDirectoryTree.ps1:14 - No explicit validation against path traversal attacks
2. Ensure PFX passwords are handled as SecureString and not logged

Performance

1. Install-ModuleDocumentation.ps1:141 - Inefficient file filtering. Use -Filter parameter with Get-ChildItem
2. Copy-PSPDirectoryTree.ps1 - Recursive function could cause stack issues on deep trees

Test Coverage - MISSING

• No tests found for New-ConfigurationDelivery
• No tests found for Install-ModuleDocumentation
• No tests found for Copy-PSPDirectoryTree

Recommendations: Add Pester tests for all new public functions covering parameter combinations, pipeline input, error handling, and edge cases.

Recommendations Summary

Must fix before merge:

1. Address silent error suppression (Install-ModuleDocumentation lines 146, 153)
2. Remove duplicate logic (Register-Certificate.ps1 lines 65-77)
3. Add basic test coverage for new public functions

Should fix:
4. Improve error handling in Copy-PSPDirectoryTree.ps1
5. Add path traversal validation
6. Optimize Test-ModuleManifest usage
7. Fix assembly resolution matching

Overall Assessment

This is a well-designed and valuable feature with sound architecture and good API design. Main concerns are error handling robustness and test coverage. Once addressed, this will be a solid addition.

Recommendation: Request changes to address high-priority issues and add test coverage before merging.

[claude]

Pull Request Review: feat(delivery) - Add delivery metadata commands and manifest support

Summary

This PR adds a comprehensive delivery/documentation system to PSPublishModule, enabling modules to bundle and install internal documentation (Internals folder) with metadata support. The implementation includes three new public cmdlets, signing enhancements, and build configuration improvements.

Overall Assessment

✅ Recommended for merge with minor suggestions for improvement.

The PR demonstrates excellent code quality with well-structured functionality, comprehensive documentation, and thoughtful error handling. The feature is well-designed and follows PowerShell best practices.

---

Code Quality & Best Practices ⭐

Strengths

1. Excellent Documentation: All new cmdlets have comprehensive comment-based help with multiple examples
2. Parameter Validation: Good use of ValidateSet, proper parameter sets (ByName, ByModule, ByPath)
3. Pipeline Support: Proper pipeline support with ValueFromPipeline and ValueFromPipelineByPropertyName
4. Cross-Platform: Code handles Windows/Linux/macOS differences appropriately (e.g., path separators, IsWindows checks)
5. ShouldProcess Support: Install-ModuleDocumentation properly implements -WhatIf and -Confirm
6. Backward Compatibility: Legacy parameter support (CreateVersionSubfolder) with graceful deprecation

Suggestions for Improvement

1. Register-Certificate.ps1:108-112 - Duplicate Internals exclusion logic

The exclusion logic appears to hardcode the Internals path exclusion even when it's already in the ExcludePath array:

$items = $items | Where-Object {
    $full = $_.FullName
    ($full -notlike '*\Internals\*' -and $full -notlike '*/Internals/*') -and (
        ($ExcludePath | ForEach-Object { $full -like ("*" + $_ + "*") }) -notcontains $true)
}

Issue: The Internals check is hardcoded separately from the ExcludePath logic, which could cause confusion if someone passes "Internals" in ExcludePath.

Suggestion: Simplify to rely on ExcludePath being populated correctly by Start-ModuleSigning:

if ($ExcludePath) {
    $items = $items | Where-Object {
        $full = $_.FullName
        -not ($ExcludePath | Where-Object { $full -like "*$_*" })
    }
}

This same pattern appears at lines 108-112 and 126-130 (Windows and Linux paths).

---

Potential Bugs & Issues 🐛

1. Install-ModuleDocumentation.ps1:181-182 - Redundant Start-Process call

if ($IsWindows) { Start-Process -FilePath $readme.FullName | Out-Null }
else { Start-Process -FilePath $readme.FullName | Out-Null }

Issue: Both branches are identical, making the if/else unnecessary.

Fix: Simplify to:

Start-Process -FilePath $readme.FullName | Out-Null

2. Copy-PSPDirectoryTree.ps1:21 - Silent failure on copy error

} catch {
    if (-not (Test-Path -LiteralPath $target)) { throw }
    # If not overwriting and file exists, skip silently
}

Issue: The catch block silently suppresses ALL errors when the target exists, not just "file exists" errors. Other errors (permissions, disk full, etc.) would be hidden.

Suggestion: Be more specific about which errors to suppress:

} catch {
    if (-not (Test-Path -LiteralPath $target)) { 
        throw  # File doesn't exist, so this is a real error
    }
    # Target exists and we're not overwriting - skip silently
    # Consider: Write-Verbose "Skipping existing file: $target"
}

3. Show-ModuleDocumentation.ps1:159 - Missing error handling on Start-Process

if ($Open) {
    Start-Process -FilePath $target | Out-Null
    return
}

Issue: Start-Process can fail (file association missing, permission denied) but errors are not handled.

Suggestion: Wrap in try/catch like Install-ModuleDocumentation does at line 177-189.

4. OnImportAndRemove.cs:79-83 - Incomplete .NET version detection

private bool IsNet5OrHigher() {
    return System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription.StartsWith(".NET 5", StringComparison.OrdinalIgnoreCase) ||
           // ... only checks .NET 5-8
}

Issue: Hardcoded version checks will fail for .NET 9+. The method exists but isn't used anywhere in the code.

Suggestion: Either remove unused methods (IsNetCore, IsNet5OrHigher) or implement a more robust version check:

private bool IsNet5OrHigher() {
    var description = System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription;
    if (description.StartsWith(".NET ", StringComparison.OrdinalIgnoreCase)) {
        var versionString = description.Substring(5).Split(' ')[0];
        if (int.TryParse(versionString[0].ToString(), out int majorVersion)) {
            return majorVersion >= 5;
        }
    }
    return false;
}

---

Performance Considerations ⚡

1. Install-ModuleDocumentation.ps1:157-172 - Multiple file system iterations

$rootFiles = @(Get-ChildItem -Path $Module.ModuleBase -File -ErrorAction SilentlyContinue)

if ($includeReadme -and $rootFiles.Count -gt 0) {
    foreach ($file in $rootFiles) {
        if ($file.Name -like 'README*') { ... }
    }
}
if ($includeChlog -and $rootFiles.Count -gt 0) {
    foreach ($file in $rootFiles) {
        if ($file.Name -like 'CHANGELOG*') { ... }
    }
}

Impact: Minor - iterates through all root files twice when both README and CHANGELOG are requested.

Optimization: Single pass with filter:

$rootFiles = @(Get-ChildItem -Path $Module.ModuleBase -File -ErrorAction SilentlyContinue)
foreach ($file in $rootFiles) {
    if ($includeReadme -and $file.Name -like 'README*') {
        try { Copy-Item -LiteralPath $file.FullName -Destination $dest -Force:$Force.IsPresent -ErrorAction Stop } catch { }
    }
    if ($includeChlog -and $file.Name -like 'CHANGELOG*') {
        try { Copy-Item -LiteralPath $file.FullName -Destination $dest -Force:$Force.IsPresent -ErrorAction Stop } catch { }
    }
}

2. Register-Certificate.ps1:106 & 124 - Recursive Get-ChildItem with Include

Using -Recurse with -Include can be slow on large directories. Current implementation is acceptable, but for future optimization consider:

• Pre-filtering directories before recursion
• Using -Filter when possible (though it doesn't support multiple extensions)

---

Security Concerns 🔒

1. Certificate Handling - Well implemented ✅

• Proper use of PFX passwords
• Thumbprint-based selection from cert store
• No hardcoded credentials
• Good separation of certificate concerns

2. Path Traversal Protection - Good ✅

• Consistent use of -LiteralPath throughout prevents injection
• Join-Path used properly for path construction
• No string concatenation for paths

3. Code Signing Logic - Secure ✅

• Default behavior excludes Internals (opt-in required)
• Scripts-only by default (binaries/exe require opt-in)
• Proper timestamp server usage
• Good defaults for security (SHA256, timestamp verification)

Minor Concern: Copy-PSPDirectoryTree.ps1 - No validation

The function doesn't validate that Source is not a parent of Destination, which could cause infinite recursion if misconfigured. Consider adding a check:

if ($Destination.StartsWith($Source)) {
    throw "Destination cannot be a subdirectory of Source"
}

---

Test Coverage 🧪

Missing: No test files are included in this PR.

Recommendations:

1. Add Pester tests for new cmdlets:

   • Install-ModuleDocumentation (ByName, ByModule, Layout options, OnExists behaviors)
   • Show-ModuleDocumentation (Readme, Changelog, File parameter)
   • New-ConfigurationDelivery (output structure validation)

2. Add integration tests for signing configuration:

   • Verify IncludeInternals/ExcludePath logic
   • Validate signing include patterns

3. Add edge case tests:

   • Missing Internals folder
   • Module without PSPublishModuleDelivery metadata
   • Invalid Layout/OnExists values

---

Documentation 📚

Strengths

• Comprehensive comment-based help for all new cmdlets
• Multiple examples per cmdlet
• Clear parameter descriptions
• Auto-generated markdown documentation updated

Suggestions

1. Consider adding a README or example in the Internals folder demonstrating the feature
2. The Build-Module.ps1 change adds Microsoft.PowerShell.PSResourceGet but doesn't document why (is this for Install-ModuleDocumentation?)

---

Architecture & Design 🏗️

Excellent Design Decisions

1. Schema versioning: PSPublishModuleDelivery includes Schema: '1.1' for future compatibility
2. Layout abstraction: Direct/Module/ModuleAndVersion provides flexibility
3. OnExists behavior: Merge/Overwrite/Skip/Stop gives users control
4. Metadata in manifest: Leveraging PrivateData.PSData is the right approach
5. Separation of concerns: Delivery config, installation, and viewing are separate cmdlets

Minor Suggestions

1. Consider making Copy-PSPDirectoryTree a public cmdlet (or documenting why it's private)
2. The signing configuration is comprehensive but complex - consider a helper cmdlet like New-ConfigurationSigning to match New-ConfigurationDelivery

---

Additional Notes

.editorconfig Addition

The new .editorconfig file is a great addition for C# consistency. The settings look reasonable and follow Microsoft conventions.

Version Bump

The PR bumps ModuleVersion to 2.0.26, which is appropriate for a feature addition.

---

Conclusion

This is a high-quality PR that adds valuable functionality to PSPublishModule. The code is well-structured, follows PowerShell best practices, and includes comprehensive documentation. The issues identified are minor and don't block merging, but addressing them would further improve code quality and maintainability.

Recommendation: ✅ Approve and merge after considering the suggestions above (optional improvements).

Great work! 🎉

[claude]

Pull Request Review - PR #41

Summary

This PR adds delivery metadata commands and manifest support, introducing two new public functions (New-ConfigurationDelivery, Install-ModuleDocumentation, Show-ModuleDocumentation) along with supporting infrastructure to bundle and install module documentation/examples (Internals folder).

Code Quality & Best Practices

✅ Strengths

1. Comprehensive Documentation: All new functions have detailed comment-based help with multiple examples
2. Parameter Validation: Good use of ValidateSet for constrained parameters (e.g., Layout, OnExists)
3. Pipeline Support: Proper use of parameter sets and pipeline binding by property name/value
4. ShouldProcess Support: Install-ModuleDocumentation correctly implements SupportsShouldProcess
5. Backward Compatibility: The CreateVersionSubfolder parameter includes back-compat logic
6. Consistent Coding Style: Code follows PowerShell best practices with proper indentation and naming

⚠️ Areas for Improvement

1. Duplicate Code Block (Bug) - Install-ModuleDocumentation.ps1:218-224

Lines 218-224 duplicate the LICENSE copying logic from lines 195-201. This appears to be a copy-paste error.

# Lines 195-201 (first occurrence - correct placement)
if ($includeLicense -and $rootFiles.Count -gt 0) {
    foreach ($file in $rootFiles) {
        if ($file.Name -like 'LICENSE*') {
            try { Copy-Item -LiteralPath $file.FullName -Destination (Join-Path $dest 'license.txt') -Force:$Force.IsPresent -ErrorAction Stop } catch { }
        }
    }
}

# Lines 218-224 (duplicate - should be removed)
if ($includeLicense -and $rootFiles.Count -gt 0) {
    foreach ($file in $rootFiles) {
        if ($file.Name -like 'LICENSE*') {
            try { Copy-Item -LiteralPath $file.FullName -Destination $dest -Force:$Force.IsPresent -ErrorAction Stop } catch { }
        }
    }
}

Recommendation: Remove lines 218-224. The first occurrence (195-201) is positioned correctly after root files are copied.

2. Error Handling Could Be More Informative

Multiple locations use empty catch blocks or suppress errors silently:

• Install-ModuleDocumentation.ps1:184, 191, 198, 208, 214, 221: Silent try/catch blocks
• Show-ModuleDocumentation.ps1:139: Silent catch without logging

Recommendation: Consider adding -ErrorAction SilentlyContinue to the cmdlet call itself or log warnings when operations fail, especially for file copies that users might want to know about.

3. Manifest Parsing Risk - Show-ModuleDocumentation.ps1:139

try { $manifest = Test-ModuleManifest -Path $manifestPath; $delivery = $manifest.PrivateData.PSData.PSPublishModuleDelivery } catch {}

Recommendation: Log at least a verbose message if manifest parsing fails, as this could indicate a corrupted manifest.

4. C# Runtime Detection - OnImportAndRemove.cs:60-84

The IsNet5OrHigher() method uses string comparison which will fail for .NET 9+:

private bool IsNet5OrHigher() {
    return System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription.StartsWith(".NET 5", StringComparison.OrdinalIgnoreCase) ||
           System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription.StartsWith(".NET 6", StringComparison.OrdinalIgnoreCase) ||
           // ... will need updating for every new version

Recommendation: Use Environment.Version.Major >= 5 or check if not .NET Framework/Core as a more future-proof approach.

Potential Bugs

🔴 Critical

1. Duplicate LICENSE copy (mentioned above) - Install-ModuleDocumentation.ps1:218-224

🟡 Medium

2. Incorrect Variable Usage - New-PersonalManifest.ps1:27

$Data.CmdletsToExport = @("*")  # Should be $Manifest.CmdletsToExport

This references $Data before it's defined (line 95). Should use $Manifest instead.

Performance Considerations

✅ Good Practices

1. Generic List Usage: Install-ModuleDocumentation.ps1:100 uses [System.Collections.Generic.List[string]]::new() for better performance
2. Recursive Copy Helper: Copy-PSPDirectoryTree.ps1 is well-implemented with proper recursion
3. Efficient File Operations: Uses -LiteralPath consistently to avoid wildcard expansion overhead

💡 Suggestions

1. File Enumeration: Multiple calls to Get-ChildItem with filters could be combined in some places
2. Manifest Testing: Test-ModuleManifest is called multiple times; consider caching results if used repeatedly

Security Concerns

✅ Security Positives

1. Consistent Use of -LiteralPath: Prevents command injection via path wildcards
2. Signing Enhancements: New signing options (SignIncludeInternals, SignIncludeBinaries, SignIncludeExe) provide fine-grained control
3. Path Validation: Good validation before file operations
4. No Arbitrary Code Execution: File operations are well-controlled

⚠️ Minor Concerns

1. Path Traversal: While unlikely to be exploited, consider validating that resolved paths stay within expected boundaries
2. Start-Process Usage: Show-ModuleDocumentation.ps1:211 and Install-ModuleDocumentation.ps1:273,276 use Start-Process with user-controlled paths. Consider adding validation.

Test Coverage

❌ Missing Tests

No test coverage found for the new functions:

• Install-ModuleDocumentation
• New-ConfigurationDelivery
• Show-ModuleDocumentation
• Copy-PSPDirectoryTree
• Resolve-DocFile

Recommendation: Add Pester tests covering:

• Module resolution (by name, by pipeline, version selection)
• Layout options (Direct, Module, ModuleAndVersion)
• OnExists behaviors (Merge, Overwrite, Skip, Stop)
• File copying with/without Force
• Delivery metadata reading/writing
• Error conditions (module not found, missing Internals, etc.)

Additional Notes

Configuration Changes

1. New Module Dependency: Added Microsoft.PowerShell.PSResourceGet to required modules - ensure this is documented
2. EditorConfig Addition: Comprehensive .editorconfig for C# - good addition for consistency
3. Manifest Schema: New PSPublishModuleDelivery schema version "1.2" in PrivateData.PSData

Documentation

• Help documentation is comprehensive and well-structured
• Examples are practical and cover common use cases
• Parameter descriptions could be slightly more detailed in some cases

Recommendations Summary

Must Fix (Blocking Issues)

1. ❌ Remove duplicate LICENSE copy code block (Install-ModuleDocumentation.ps1:218-224)
2. ❌ Fix incorrect variable reference (New-PersonalManifest.ps1:27 - use $Manifest instead of $Data)

Should Fix (Before Merge)

3. ⚠️ Add test coverage for new public functions
4. ⚠️ Improve error handling with informative messages instead of empty catch blocks
5. ⚠️ Validate Start-Process file paths before opening

Nice to Have (Future Improvements)

6. 💡 Make C# runtime detection more future-proof
7. 💡 Consider caching manifest parsing results
8. 💡 Add verbose/warning output for failed file operations

Overall Assessment

This is a well-structured feature addition with good documentation and thoughtful design. The main concerns are:

• Two bugs that should be fixed before merge (duplicate code, wrong variable)
• Missing test coverage for new functionality
• Some minor error handling improvements

Once the critical bugs are addressed and tests are added, this PR will be in excellent shape! 🎉

---

Review generated by Claude Code