nav: enforce requiredSurfaces + versioned site-nav.json

[PrzemyslawKlys]

Implements Roadmap M1 tightening for navigation surfaces and stable nav export.

Changes

• Verify: eatureContracts..requiredSurfaces now emits a Theme contract warning when the site does not define Navigation.Surfaces, so CI can fail deterministically.
• Surface name normalization: �pi / �piDocs are treated as aliases of �pidocs for required-surface checks.
• Nav export: generated data/site-nav.json now includes schemaVersion: 1 to support stable non-Scriban consumers.
• Docs: updated starter/theme/content/roadmap docs to reflect canonical surface names + alias behavior + nav export versioning.
• Tests: added coverage for missing-surfaces warning and alias normalization.

Validation

• dotnet test .\PSPublishModule.sln -c Release (382 passed)
• theme schema JSON parse check passed

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Pull Request Review: nav: enforce requiredSurfaces + versioned site-nav.json

Summary

This PR implements Roadmap M1 tightening for navigation surfaces and stable nav export. The changes are well-implemented with good test coverage and clear documentation updates.

---

✅ Code Quality & Best Practices

Strengths:

• Consistent naming conventions: The NormalizeSurfaceName method follows the existing pattern used in NormalizeFeatureName
• Good separation of concerns: Validation logic is properly isolated in WebSiteVerifier.ThemeFeatureContracts.cs:134-184
• Comprehensive null/whitespace checks: Defensive programming throughout (e.g., lines 140-142, 144-157, 166-174)
• Case-insensitive comparisons: Proper use of StringComparer.OrdinalIgnoreCase for navigation surface matching
• Clear error messages: Warning messages are descriptive and actionable (lines 156, 181)

Minor Suggestion:

• The requiredPreview fallback on line 154 ("main,docs,apidocs") is hardcoded. Consider extracting this as a constant for maintainability.

---

✅ Test Coverage

Excellent test coverage with two new tests:

1. Verify_ThemeFeatureContracts_WarnWhenRequiredSurfacesMissingAndSiteDoesNotDefineSurfaces (lines 337-424)

   • Tests the new warning behavior when surfaces are required but not defined
   • Properly validates the warning message contains expected keywords
   • Good use of temporary directories with proper cleanup

2. Verify_ThemeFeatureContracts_RespectsApiSurfaceAliasesWhenSurfacesAreDefined (lines 426-517)

   • Tests the alias normalization feature (api → apidocs)
   • Uses Assert.DoesNotContain to verify no warning is emitted
   • Validates that apiDocs surface definition satisfies api requirement

Observation:

• Both tests use Assert.True(result.Success) which is correct - the validation should succeed with warnings, not fail
• The tests properly clean up temporary files in the finally block

---

✅ Performance Considerations

• Efficient lookups: Uses HashSet<string> for surface name lookups (line 160) - O(1) performance
• Minimal allocations: Only materializes required collections through LINQ
• Early returns: Properly exits early when no validation is needed (lines 141-142, 145-158)
• String operations: Uses StringBuilder in requiredPreview construction (line 147) - efficient for potentially large lists

No performance concerns identified.

---

✅ Security Concerns

No security issues identified.

The code:

• Does not execute external commands
• Does not deserialize untrusted data
• Does not construct file paths from user input without validation
• Only reads configuration data that's already been parsed and validated by the framework

---

🔍 Potential Issues

1. Missing NormalizeSurfaceName Method Reference

The code in WebSiteVerifier.ThemeFeatureContracts.cs calls NormalizeSurfaceName (lines 149, 162, 172), but this method is not defined in this file.

Resolution: I verified this method exists in WebSiteVerifier.NavigationRules.cs:1022-1034, so this is working as intended through the partial class pattern.

2. Schema Version Hardcoded

In WebSiteBuilder.NavigationData.cs:40, the schema version is hardcoded as schemaVersion = 1.

Suggestion: Consider extracting this as a named constant:

private const int SiteNavSchemaVersion = 1;

This makes it easier to increment the version in the future and documents the versioning scheme.

---

📋 Documentation Updates

Well-documented changes across all relevant files:

1. PowerForge.Web.ContentSpec.md - Added schema version explanation and alias behavior
2. PowerForge.Web.Roadmap.md - Updated milestone with canonical surface names
3. PowerForge.Web.Theme.md - Clarified requiredSurfaces enforcement behavior
4. PowerForge.Web.WebsiteStarter.md - Documented canonical names and alias support
5. powerforge.web.themespec.schema.json - Added helpful description for requiredSurfaces

Documentation quality: Clear, consistent, and actionable.

---

🎯 Validation

Per the PR description:

• ✅ All 382 tests passed (dotnet test)
• ✅ Theme schema JSON parse check passed

---

Recommendations

Optional Improvements:

1. Extract magic strings as constants (WebSiteVerifier.ThemeFeatureContracts.cs:154):

   private const string DefaultRequiredSurfaces = "main,docs,apidocs";

2. Extract schema version constant (WebSiteBuilder.NavigationData.cs:40):

   private const int SiteNavSchemaVersion = 1;

3. Consider adding a test for multiple aliases normalizing to the same surface (e.g., both api and apiDocs in requiredSurfaces should deduplicate to apidocs)

Code Example for Suggestion #3:

[Fact]
public void NormalizeSurfaceName_DeduplicatesAliases()
{
    var surfaces = new[] { "api", "apiDocs", "apidocs" };
    var normalized = surfaces
        .Select(s => NormalizeSurfaceName(s))
        .Distinct(StringComparer.OrdinalIgnoreCase)
        .ToArray();
    
    Assert.Single(normalized);
    Assert.Equal("apidocs", normalized[0]);
}

---

✅ Overall Assessment

Status: ✅ Approved - Ready to Merge

This is a well-crafted PR that:

• Implements the feature correctly with proper alias handling
• Includes comprehensive test coverage
• Updates all relevant documentation
• Follows existing code patterns and conventions
• Has no security, performance, or critical issues

The suggestions above are optional enhancements and do not block merging.

Great work! 🎉

---

Review performed by Claude Code