Fix Prism default language meta ordering

[PrzemyslawKlys]

Fixes build error caused by using meta before declaration when normalizing code block classes.

[claude]

PR Review: Fix Prism default language meta ordering

Summary

This PR fixes a critical build error caused by using the meta variable before it was declared. The fix correctly moves the variable declaration before its first usage.

Code Quality: ✅ Excellent

What was fixed:
The code was attempting to use meta in line 1091 (old line numbers):

htmlContent = NormalizeCodeBlockClasses(htmlContent, ResolvePrismDefaultLanguage(meta, spec));

But meta wasn't declared until line 1092:

var meta = matter?.Meta ?? new Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase);

The fix correctly reorders these lines so meta is declared first (PowerForge.Web/Services/WebSiteBuilder.cs:1091-1092).

Analysis

✅ Correctness

• The fix is correct - it resolves the compilation error by ensuring variables are declared before use
• The logic flow remains intact - meta is properly initialized before being passed to ResolvePrismDefaultLanguage()
• No side effects from the reordering since both operations are independent

✅ Best Practices

• Follows C# variable scoping rules
• Maintains existing code style and patterns
• Minimal, focused change that addresses only the specific issue

✅ Performance

• No performance impact - purely a code ordering fix
• Same operations executed in correct sequence

✅ Security

• No security implications - purely a compilation fix

Testing Considerations

⚠️ Observation: No test files were found for WebSiteBuilder.cs. While this fix resolves a compilation error (which would be caught immediately), consider adding unit tests for:

• NormalizeCodeBlockClasses() behavior with different default languages
• ResolvePrismDefaultLanguage() with various meta configurations
• Integration tests for the markdown rendering pipeline

This would help prevent similar issues and ensure the Prism syntax highlighting features work as expected.

Recommendation

✅ Approved - This is a straightforward bug fix that resolves a compilation error. The change is minimal, correct, and maintains the intended functionality.

Suggestions for future improvements:

1. Consider adding unit tests for the markdown processing pipeline
2. Static analysis tools (like Roslyn analyzers) might have caught this earlier if not already in use

Great catch on fixing this before it reached production! 🎯

[Copilot]

Pull request overview

This PR fixes a build error caused by using the meta variable before it was declared. The code attempted to call NormalizeCodeBlockClasses with meta as a parameter before the meta variable was initialized on the next line.

Changes:

• Moved the NormalizeCodeBlockClasses call to after the meta variable declaration to fix the compile-time error

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Pull Request Review

Summary

This PR fixes a critical compilation error in the C# code and improves the PowerShell build script's robustness. Both changes are well-justified and address real issues.

---

✅ Approved Changes

1. C# Variable Declaration Fix (WebSiteBuilder.cs:1091-1092)

Issue Fixed: Variable meta was being used before declaration on line 1090 (in ResolvePrismDefaultLanguage(meta, spec)), but wasn't declared until line 1091.

The Fix: Correctly reordered the lines so meta is declared before use:

// Before (broken):
htmlContent = NormalizeCodeBlockClasses(htmlContent, ResolvePrismDefaultLanguage(meta, spec));
var meta = matter?.Meta ?? new Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase);

// After (fixed):
var meta = matter?.Meta ?? new Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase);
htmlContent = NormalizeCodeBlockClasses(htmlContent, ResolvePrismDefaultLanguage(meta, spec));

Analysis:

• This is a textbook variable-before-use error that would cause compilation failure
• The fix is straightforward and correct
• No logic changes, just proper ordering
• The meta variable is needed by ResolvePrismDefaultLanguage() to determine the Prism syntax highlighting language

2. PowerShell Build Script Improvements (Build-Module.ps1:39-61)

Improvements Added:

1. Fallback Module Loading (lines 39-46): Adds logic to detect .NET runtime and load the compiled binary module if the initial import fails

   • Detects .NET 10.0 vs 8.0 runtime
   • Attempts to load the DLL directly from the build output

2. Error Handling (lines 48-50): Throws a clear error if Invoke-ModuleBuild is unavailable after all import attempts

3. Command Rename (line 61): Changes from Build-Module to Invoke-ModuleBuild (likely a command rename or refactoring)

Analysis:

• The regex pattern '(?m)^Microsoft\\.NETCore\\.App\\s+10\\.' correctly detects .NET 10.0 runtime
• Proper fallback chain: try PSD1 import → try DLL import → fail with clear error
• The error message is actionable and clear
• Uses -ErrorAction SilentlyContinue appropriately for conditional checks

---

Code Quality Assessment

✅ Strengths

1. Bug Fix: Addresses a real compilation error
2. Clear Intent: PR title and description accurately describe the changes
3. Minimal Scope: Only changes what's necessary
4. Error Handling: PowerShell script now has proper fallback logic
5. Defensive Coding: Checks for command availability before use

No Issues Found

Security: ✅ No security concerns

• No user input handling changes
• No external data processing changes
• Build script runs in trusted environment

Performance: ✅ No performance concerns

• C# fix has zero performance impact (just reordering)
• PowerShell checks are O(1) operations at build time

Testing: ⚠️ Consider adding

• Unit test for ResolvePrismDefaultLanguage with null/populated meta
• Unit test for NormalizeCodeBlockClasses with various code block formats
• Build script test for different .NET runtime scenarios

Potential Bugs: ✅ None identified

• Variable scoping is correct
• Null safety is maintained with ?? operator
• Regex pattern correctly escapes backslashes

---

Recommendations

1. ✅ Merge Ready: This PR fixes a build-breaking bug and should be merged
2. Future Enhancement: Consider adding integration tests that catch variable-ordering issues at compile time
3. Documentation: The build script's new fallback logic is self-documenting but could benefit from inline comments explaining why the fallback is needed

---

Test Coverage Suggestion

Consider adding a test case that verifies the proper ordering of meta declaration before use in code normalization.

---

Verdict: ✅ APPROVED - This is a clean, focused fix that resolves a critical bug. No blocking issues identified.

---

Review generated by Claude Code

[claude]

Pull Request Review

Overview

This PR fixes a build error caused by using the meta variable before its declaration in WebSiteBuilder.cs, and adds fallback logic to ensure Invoke-ModuleBuild is available in the CI build script.

---

Code Quality and Best Practices

✅ Positive Aspects

1. C# Fix (WebSiteBuilder.cs:1088-1092): The variable ordering fix is correct and follows proper declaration-before-use principles.

2. PowerShell Fallback Logic: The added fallback mechanism demonstrates good defensive programming by ensuring the required command is available before use.

3. Runtime Detection: Smart use of dotnet --list-runtimes to detect .NET 10 vs .NET 8 runtime and select the appropriate TFM (lines 40-41).

4. Error Handling: Proper error handling with $LASTEXITCODE checks and meaningful error messages (lines 48-51, 59-60).

⚠️ Areas for Improvement

1. Code Duplication: Lines 24-35 and lines 43-52 contain nearly identical dotnet build logic. This violates DRY principles.

   • Recommendation: Extract the build logic into a helper function or consolidate the two build attempts.

2. Inconsistent Error Handling: Line 32 uses return while line 50 uses throw. This creates inconsistent behavior.

   • Recommendation: Use throw consistently for build failures to ensure proper error propagation in CI environments.

3. Command Name Change: The change from Build-Module to Invoke-ModuleBuild (line 72) suggests a refactoring.

   • Consider adding a comment explaining why the fallback is needed

---

Potential Bugs or Issues

Critical Review

1. Variable Ordering Fix (WebSiteBuilder.cs:1091-1092): The diff shows the meta declaration was moved to BEFORE the NormalizeCodeBlockClasses call. This correctly fixes the use-before-declaration bug. ✅

2. Module Import Order (Build-Module.ps1:37-61): The script imports the PSD1 module (line 37), then conditionally imports the binary DLL (lines 54-56). This could lead to:

   • Two versions of the module loaded simultaneously
   • Cmdlet conflicts if both exports overlap
   • Recommendation: Consider using Import-Module -Force or checking if already loaded properly

Medium Priority

1. Silent Failure Path: If neither the PSD1 import nor the binary import provides Invoke-ModuleBuild, the script only fails at line 59-60. Consider moving this check earlier.

2. Path Construction: Lines 42, 44 use relative paths with ..\..\PSPublishModule. This could break if invoked from different working directories.

---

Performance Considerations

Good Points

1. Quiet Build Output: Using --verbosity quiet reduces noise
2. Conditional Build: Only builds if DLL not found
3. Early Exit: ErrorAction SilentlyContinue on command checks avoids overhead

Suggestions

1. Runtime Detection: The dotnet --list-runtimes call happens every time even when not needed. Consider caching.

2. Double Build Risk: The script could potentially build the same project twice (lines 24-35 and 43-52).

---

Security Concerns

No Major Issues Detected

1. LiteralPath Usage: Proper use of -LiteralPath prevents path injection ✅
2. No Dynamic Code Execution: No Invoke-Expression or similar dangerous patterns ✅
3. Error Stream Redirection: 2>$null on line 40 is safe ✅

---

Test Coverage

Concerns

1. No Tests for WebSiteBuilder Change: The C# fix should have a test case that exercises the NormalizeCodeBlockClasses path with metadata.

2. No Tests for Build Script Fallback: The new fallback logic is complex enough to warrant tests:

   • Test when PSD1 import succeeds
   • Test when binary DLL import is needed
   • Test TFM detection logic

Recommendations

1. Add unit test for WebSiteBuilder.BuildContentItems that verifies meta is properly available
2. Add Pester tests for the build script fallback scenarios

---

Summary

Overall Assessment: ✅ Approve with suggestions

The PR fixes legitimate bugs and adds useful fallback logic. The C# fix is correct and necessary. The PowerShell changes add robustness to the build process.

Before Merge:

• Consider consolidating duplicate build logic
• Verify module import order does not cause conflicts
• Add test coverage for both changes

Priority: The variable ordering fix is critical for correctness. The PowerShell changes add defensive robustness but introduce some technical debt with duplication.

Recommended Action: Merge after addressing the code duplication concern or create a follow-up issue to refactor the duplicate build logic.

[claude]

Pull Request Review

Thank you for this fix! I have reviewed the changes and here is my feedback:

---

Critical Bug Fix - C# Variable Scoping Issue

File: PowerForge.Web/Services/WebSiteBuilder.cs:1088-1092

Issue Fixed: The code was using the meta variable before it was declared, which would cause a compilation error. The fix correctly moves the variable declaration before its use.

Before: NormalizeCodeBlockClasses was called with meta before meta was declared
After: meta is declared first, then used in NormalizeCodeBlockClasses

This is a straightforward and correct fix. The variable is now declared before use, maintaining the same logic flow.

---

PowerShell Build Script Improvements

Files: Module/Build/Build-Module.ps1, Module/Build/Build-ModuleSelf.ps1

Changes in Build-Module.ps1 (lines 39-61): Added fallback logic to detect and build PSPublishModule.dll if Invoke-ModuleBuild is not available

• Intelligently detects .NET runtime (net10.0 vs net8.0)
• Builds the binary module if missing
• Includes proper error handling with throw statements
• Changes cmdlet call from Build-Module to Invoke-ModuleBuild (line 72)

Changes in Build-ModuleSelf.ps1 (lines 46-57): Added parallel build for PSPublishModule alongside PowerForge.CLI

• Consistent build pattern with CLI build
• Proper verbosity control and error handling
• Added validation that PSPublishModule.csproj exists (line 24)

Code Quality: Excellent

• Consistent error handling patterns
• Good use of parameter splatting
• Appropriate use of Write-Host with color coding for user feedback
• Proper exit code handling with LASTEXITCODE

---

Security Considerations

No security concerns identified:

• No user input is used in unsafe ways
• All paths use -LiteralPath which prevents wildcard injection
• Build commands use proper parameter arrays, not string concatenation
• No credential exposure or sensitive data handling

---

Performance Considerations

Good performance characteristics:

• Build operations only run when necessary (check for existing binary first)
• Quiet verbosity by default reduces I/O overhead
• Sequential builds are appropriate here (module depends on successful build)

---

Test Coverage

Observation: While there are comprehensive test files in PowerForge.Tests/, I do not see specific tests for:

• The NormalizeCodeBlockClasses + ResolvePrismDefaultLanguage interaction
• The new build script fallback logic

Recommendation: Consider adding:

1. A unit test that verifies ResolvePrismDefaultLanguage works correctly with metadata
2. Integration test for the build script fallback mechanism (though this may be covered by CI)

However, given this is a bug fix for a compilation error, the fact that the code now compiles and CI passes is good validation.

---

Additional Observations

Positive:

• PR description clearly explains the issue
• Changes are focused and minimal
• Code follows existing patterns in the codebase
• Consistent formatting and style

Minor Note:

• The change from Build-Module to Invoke-ModuleBuild (line 72) suggests a cmdlet rename. Ensure this is documented if it is part of a larger refactoring.

---

Recommendation: APPROVE

This PR fixes a critical build error and improves the build script robustness. The changes are well-implemented with proper error handling and follow best practices.

Summary:

• Code quality: Excellent
• Bug fix: Correct
• Security: No concerns
• Performance: Appropriate
• Test coverage: Could be enhanced but acceptable for a bug fix

Great work on catching and fixing this ordering issue!

[claude]

Pull Request Review: Fix Prism default language meta ordering

Summary

This PR addresses a build error in WebSiteBuilder.cs where meta was being used before it was declared. The fix correctly reorders the variable declaration before its usage in NormalizeCodeBlockClasses(). Additionally, the PR includes build infrastructure improvements and a bug fix in the RemoveCommentsCommand.

✅ Positive Findings

1. Critical Bug Fix - Variable Declaration Order (PowerForge.Web/Services/WebSiteBuilder.cs:1091-1092)

The main fix correctly addresses the issue:

• Before: meta was used in NormalizeCodeBlockClasses() before declaration
• After: meta is properly declared before use
• This is a straightforward fix that resolves the build error

2. RemoveCommentsCommand Enhancement (PSPublishModule/Cmdlets/RemoveCommentsCommand.cs:143-152, 212-213)

Good defensive programming:

• Pre-computes firstParamOffset by scanning tokens once
• Adds proper bounds check to prevent removing comments before param block when the flag is disabled
• Logic correctly implements the intent of the -RemoveCommentsBeforeParamBlock parameter

3. Build Infrastructure Improvements

Both Build-Module.ps1 and Build-ModuleSelf.ps1 now include better fallback logic:

• Runtime detection (net10.0 vs net8.0) is sensible
• Explicit validation that Invoke-ModuleBuild is available before proceeding
• Good error messages for debugging

🔍 Issues & Concerns

1. Code Duplication in Build-Module.ps1 (Lines 24-35 and 39-57)

Severity: Medium

The dotnet build logic is duplicated twice in the same file:

• First occurrence: Lines 24-35 (inside if (-not $JsonOnly -and -not $NoDotnetBuild))
• Second occurrence: Lines 43-52 (inside the fallback when Invoke-ModuleBuild is not available)

Issues:

• First build uses return on failure (line 32), which exits the script silently
• Second build uses throw on failure (line 50), which is more appropriate
• The conditions overlap - both could potentially run in sequence

Recommendation: Consolidate this logic or add early returns to prevent redundant builds. The script should ideally build once, not potentially twice.

2. Missing Test Coverage for RemoveCommentsCommand Fix

Severity: Low-Medium

The changes to RemoveCommentsCommand.cs (lines 143-152, 212-213) add new logic for handling comments before param blocks, but:

• Existing tests (Remove-Comments.Tests.ps1) cover the -RemoveCommentsBeforeParamBlock flag generally
• However, tests do not specifically validate the edge case this PR fixes (the firstParamOffset boundary check)
• Consider adding a test that ensures comments at exactly the param boundary are handled correctly

3. Error Handling Inconsistency

Severity: Low

In Build-Module.ps1:

• Line 32: Uses return (silent exit, will not fail in CI)
• Line 50: Uses throw (proper error propagation)

Recommendation: Use throw consistently for build failures to ensure CI/CD pipelines catch errors.

4. PowerForge.Web Variable Shadowing (WebSiteBuilder.cs:1091)

Severity: Very Low (style/clarity)

Line 1091 declares meta which shadows the meta variable used earlier in the same scope. While the code now works correctly, the variable name reuse could be confusing.

Recommendation: Consider using a more descriptive variable name like contentMeta or simply use matter?.Meta directly in both places for clarity.

🔒 Security Assessment

No security concerns identified.

• Changes are primarily build/infrastructure related
• The RemoveCommentsCommand changes are defensive (adding boundary checks)
• No user input flows through the modified code paths without validation

⚡ Performance Considerations

Minor Performance Note: The new firstParamOffset loop (RemoveCommentsCommand.cs:145-152) adds an O(n) pass through tokens before the main processing loop. This is acceptable given:

• Token arrays are typically small (less than 10k tokens)
• This prevents incorrect comment removal, which would be a correctness issue
• Alternative would be more complex state tracking during the main loop

📊 Test Coverage

Existing Coverage: Good

• Remove-Comments.Tests.ps1 covers the main scenarios
• WebApiDocsGeneratorCrefTests.cs validates the cref token handling

Gap: Missing edge case test for the firstParamOffset boundary condition.

✨ Suggestions

1. Consolidate duplicate build logic in Build-Module.ps1 (High Priority)
2. Change return to throw on line 32 of Build-Module.ps1 (Medium Priority)
3. Add boundary test case for RemoveCommentsCommand (Low Priority)
4. Consider variable naming in WebSiteBuilder.cs for clarity (Very Low Priority)

📝 Overall Assessment

Status: ✅ Approve with minor recommendations

The PR successfully fixes the build error and improves build infrastructure. The core bug fix is correct and the code changes follow the repository patterns (keeping logic in cmdlets/services, typed models, etc. per CONTRIBUTING.md).

The main concern is the code duplication in Build-Module.ps1, which should ideally be addressed to prevent confusion and potential double-builds. However, this does not block the PR as it is an improvement over the previous state.

Recommended Action: Merge after addressing the critical duplication issue, or merge as-is if this is a quick fix with plans to refactor the build scripts later.

---

Great work fixing the variable ordering issue!

[claude]

Pull Request Review

I've reviewed PR #56 and here's my detailed feedback:

Summary

This PR addresses multiple build-related issues and includes 6 commits fixing:

1. A variable ordering bug in WebSiteBuilder.cs
2. Build pipeline improvements to ensure Invoke-ModuleBuild is available
3. Comment preservation logic for param blocks in RemoveCommentsCommand.cs

---

Code Quality & Best Practices

✅ WebSiteBuilder.cs - Variable Declaration Ordering Fix

File: PowerForge.Web/Services/WebSiteBuilder.cs:1088-1092

Issue Fixed: Variable meta was being used in line 1088 before being declared in line 1091.

// BEFORE (broken):
htmlContent = NormalizeCodeBlockClasses(htmlContent, ResolvePrismDefaultLanguage(meta, spec));
var meta = matter?.Meta ?? new Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase);

// AFTER (correct):
var meta = matter?.Meta ?? new Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase);
htmlContent = NormalizeCodeBlockClasses(htmlContent, ResolvePrismDefaultLanguage(meta, spec));

Assessment: ✅ Excellent fix. This resolves a compilation error and the ordering is now logically correct.

---

✅ RemoveCommentsCommand.cs - Improved Comment Preservation

File: PSPublishModule/Cmdlets/RemoveCommentsCommand.cs:143,203-204

Changes:

1. Line 135: Changed from unused _ = ast to var scriptAst = ast as ScriptBlockAst
2. Line 143: Added var scriptParamOffset = scriptAst?.ParamBlock?.Extent.StartOffset ?? -1;
3. Lines 203-204: Added check to preserve comments before param block when flag is not set

Assessment: ✅ This is a well-designed improvement. The logic now correctly:

• Uses the AST to determine the actual param block location
• Preserves comments before param blocks by default (aligns with the parameter documentation)
• Only removes them when RemoveCommentsBeforeParamBlock is explicitly set

Code quality: The null-safe navigation and fallback to -1 is appropriate. The condition token.Extent.EndOffset <= scriptParamOffset correctly identifies comments that end before the param block starts.

---

⚠️ Build-Module.ps1 - Self-Bootstrapping Logic

File: Module/Build/Build-Module.ps1:39-61

Changes: Added fallback logic to build and import PSPublishModule binary if Invoke-ModuleBuild command isn't available after module import.

Assessment: This is a reasonable self-bootstrapping approach for the build system, but there are some concerns:

Issues:

1. Code Duplication: Lines 24-35 already build the project, then lines 39-61 potentially rebuild it again. This suggests the initial build or import isn't working as expected.

2. Redundant Build Logic: The new code essentially duplicates the build logic from lines 24-35. Consider refactoring to a shared function.

3. Error Handling: Line 50 uses throw while line 31 uses return. Be consistent with error handling strategy.

Suggested Improvements:

# Consider extracting build logic to a function:
function Build-PSPublishModuleIfNeeded {
    param([string]$Configuration, [string]$CsprojPath)
    # Shared build logic here
}

Recommendation: While functional, consider refactoring to reduce duplication in a future PR.

---

✅ Build-ModuleSelf.ps1 - Consistent Build Pipeline

File: Module/Build/Build-ModuleSelf.ps1:21,24,45-56

Changes: Added PSPublishModule project build step to ensure binary module is available for the CLI pipeline.

Assessment: ✅ Good addition. This ensures both PowerForge.Cli and PSPublishModule are built together, maintaining consistency. The error handling matches the CLI build pattern.

---

Potential Bugs or Issues

🟡 Minor Concern: Race Condition Window

Location: Build-Module.ps1:37-39

Between importing the module (line 37) and checking for Invoke-ModuleBuild (line 39), there's a tiny window where the import could succeed but cmdlet registration could fail. The fallback logic handles this, but it adds complexity.

Mitigation: The current code handles this defensively, so it's acceptable.

---

Performance Considerations

✅ No significant performance concerns. The changes are primarily correctness fixes:

• Variable reordering has zero performance impact
• The scriptParamOffset calculation happens once during parsing (negligible overhead)
• Build scripts run infrequently and the added checks are fast

---

Security Concerns

✅ No security issues identified.

The changes don't introduce:

• Command injection vulnerabilities
• Path traversal issues
• Unsafe deserialization
• Credential handling problems

The certificate thumbprint is hardcoded (line 14 in both build scripts) which is appropriate for this repository's build process.

---

Test Coverage

✅ Existing Tests Cover the Changes

RemoveCommentsCommand.cs:

• Module/Tests/Remove-Comments.Tests.ps1 includes test cases for comment preservation:
  • Line 58-73: Tests that comments before param blocks are preserved by default ✅
  • Line 75-90: Tests that -RemoveCommentsBeforeParamBlock removes them ✅

Recommendation: The existing tests should validate the fix, but consider adding a test case for script-level param blocks (not just function param blocks) to ensure scriptParamOffset works correctly in both contexts.

🟡 Build Script Changes

The build script changes don't have direct unit tests (as is common for build scripts), but they will be validated by CI/CD pipeline execution.

---

Summary & Recommendations

What's Working Well:

✅ Fixes a critical compilation error in WebSiteBuilder
✅ Properly implements comment preservation for param blocks
✅ Ensures build scripts are self-contained and robust
✅ Good error handling and user feedback (emoji indicators)
✅ Follows repository conventions from CONTRIBUTING.md

Suggested Improvements:

1. Refactor Build-Module.ps1: Extract duplicate build logic to reduce redundancy
2. Add Test Case: Consider adding a test for script-level param blocks in Remove-Comments tests
3. Documentation: Add inline comments explaining why the double-build-check is necessary in Build-Module.ps1

Final Verdict:

✅ APPROVED with minor suggestions for future improvement.

The PR successfully fixes the reported issues without introducing new bugs or security concerns. The code quality is good, and the changes align with the repository's architecture. The suggested refactoring can be addressed in a future PR.

---

Great work on the fixes! 🎉