feat(web): nav-export command + pipeline step

[PrzemyslawKlys]

Adds a first-class nav export workflow so sites can generate/update static//site-nav.json deterministically without building HTML.

Changes:

• Engine: WebSiteBuilder.ExportSiteNavJson (safe write under site root; overwrite only when generated:true unless forced)
• CLI: powerforge-web nav-export
• Pipeline: task: nav-export
• Schema/docs: pipeline schema + Pipeline.md updated
• Tests: CLI + pipeline coverage

Also: improves WebStaticServer port fallback detection for sharing-violation binding errors (fixes flaky test).

[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-export Command + Pipeline Step

Summary

This PR adds a well-structured nav-export command and pipeline step that generates static navigation JSON files without building HTML. The implementation is solid, with good test coverage and appropriate safety mechanisms. Below are my findings.

---

✅ Strengths

Code Quality

• Clean architecture: Proper separation between CLI handlers, pipeline tasks, and core builder logic
• Good abstraction: The BuildSiteNavJsonForExport method effectively reuses existing navigation building infrastructure
• Consistent patterns: Follows established codebase conventions (partial classes, naming, error handling)
• Documentation: Clear XML documentation on public APIs
• Test coverage: Both CLI and pipeline runner tests cover the happy path

Security

• Path traversal protection: Uses IsPathWithinRoot to prevent writing outside the site root (WebSiteBuilder.NavExport.cs:36)
• Safe overwrite logic: Only overwrites generated files by default, requires explicit flag for user-managed files (WebSiteBuilder.NavExport.cs:51-59)
• Input sanitization: Output paths are trimmed and quotes are removed (WebSiteBuilder.NavExport.cs:34)

Design Decisions

• Smart defaults: Auto-generates output path based on dataRoot configuration
• Flexible CLI: Multiple option aliases (--out, --output, --out-path, --output-path) for user convenience
• Deterministic behavior: The generated: true marker enables safe automation

---

⚠️ Issues & Recommendations

1. Code Duplication - GetDefaultNavExportOutputPath (Medium Priority)

Location:

• WebCliCommandHandlers.NavExportCommands.cs:61-71
• WebPipelineRunner.Tasks.NavExport.cs:34-44

Issue: Identical logic duplicated across two files.

Recommendation: Extract to a shared helper or add to WebCliHelpers. This ensures consistency if the logic changes.

// In WebCliHelpers.cs
internal static string GetDefaultNavExportOutputPath(SiteSpec spec, string rootPath)
{
    var dataRoot = string.IsNullOrWhiteSpace(spec.DataRoot) ? "data" : spec.DataRoot;
    var relativeRoot = Path.IsPathRooted(dataRoot)
        ? "data"
        : dataRoot.TrimStart('/', '\\');
    if (string.IsNullOrWhiteSpace(relativeRoot))
        relativeRoot = "data";

    return Path.Combine(rootPath, "static", relativeRoot, "site-nav.json");
}

---

2. Inconsistent Error Handling (Medium Priority)

Location:

• WebCliCommandHandlers.NavExportCommands.cs:30 - Returns WebNavExportResult with Success=false
• WebPipelineRunner.Tasks.NavExport.cs:27-28 - Throws InvalidOperationException

Issue: CLI handler gracefully handles errors, but pipeline runner throws exceptions. This inconsistency could cause unexpected behavior.

Question: Is this intentional? If so, consider documenting why the pipeline runner uses exceptions while the CLI handler doesn't.

---

3. Missing Edge Case Handling (Low Priority)

Location: WebSiteBuilder.NavExport.cs:114-117

Issue: ToDictionary calls could throw on duplicate slugs.

Recommendation: Add duplicate handling or validation:

var projectMap = projectSpecs
    .Where(p => !string.IsNullOrWhiteSpace(p.Slug))
    .GroupBy(p => p.Slug, StringComparer.OrdinalIgnoreCase)
    .ToDictionary(g => g.Key, g => g.First(), StringComparer.OrdinalIgnoreCase);

Alternatively, log a warning or throw a more descriptive exception if duplicates exist.

---

4. Test Coverage Gaps (Low-Medium Priority)

Missing test scenarios:

• ✅ Happy path: Writing new file
• ❌ Overwrite behavior when generated: false
• ❌ Overwrite behavior when generated: true
• ❌ Force overwrite with --overwrite flag
• ❌ Writing outside site root (security test)
• ❌ Invalid/corrupted existing JSON files
• ❌ Directory creation when parent doesn't exist
• ❌ Absolute vs relative output paths
• ❌ Duplicate project slugs scenario

Recommendation: Add tests for the overwrite safety mechanism since it's a key feature.

---

5. Performance Consideration (Low Priority)

Location: WebSiteBuilder.NavExport.cs:74-94

Issue: IsGeneratedNavExport reads and parses the entire JSON file just to check a single boolean property.

Recommendation: For large files, consider a streaming JSON reader or at minimum catch JsonException specifically:

catch (JsonException)
{
    // invalid JSON, treat as user-managed
    return false;
}
catch (IOException)
{
    // file access issues
    return false;
}

Currently, the blanket catch could hide unexpected errors.

---

6. WebStaticServer Enhancement (Positive Note)

Location: WebStaticServer.cs:154-172

Observation: The addition of error code 32 (sharing violation) and the improved message detection is a good fix for flaky tests. The detailed comments explaining each error code are helpful.

Suggestion: Consider extracting the error codes to named constants for maintainability:

private const int ERROR_ACCESS_DENIED = 5;
private const int ERROR_SHARING_VIOLATION = 32;
private const int ERROR_ALREADY_EXISTS = 183;
private const int WSAEADDRINUSE = 10048;
private const int EADDRINUSE = 98;

---

7. Documentation (Minor)

Location: Docs/PowerForge.Web.Pipeline.md:82-94

Observation: Documentation is clear and well-written.

Suggestion: Consider adding an example showing the force overwrite scenario:

{ "task": "nav-export", "config": "./site.json", "out": "./custom-nav.json", "overwrite": true }

---

🔒 Security Assessment

Overall: ✅ No major security concerns

• Path traversal prevention is properly implemented
• Safe overwrite mechanism prevents accidental data loss
• Input sanitization is adequate
• No injection vulnerabilities detected

Recommendation: Consider logging when path traversal attempts are blocked for security auditing:

if (!IsPathWithinRoot(normalizedRoot, fullOut))
{
    // Log security event here
    return new WebNavExportResult { ... };
}

---

🎯 Performance Considerations

Overall: ✅ No significant performance issues

• Reuses existing build infrastructure efficiently
• No obvious N+1 queries or unnecessary iterations
• File I/O is appropriately scoped

---

📋 Checklist for Merge

• ✅ Code follows established patterns
• ✅ Basic test coverage exists
• ✅ Documentation updated
• ✅ Schema updated for pipeline spec
• ⚠️ Consider addressing code duplication
• ⚠️ Consider adding overwrite safety tests
• ⚠️ Document error handling strategy difference

---

Final Verdict

Recommendation: ✅ Approve with minor suggestions

This is a well-implemented feature that adds valuable functionality. The code quality is high, security is properly considered, and the design is solid. The suggested improvements are mostly minor refinements that could be addressed in follow-up PRs if needed.

The most important items to consider:

1. Deduplicate GetDefaultNavExportOutputPath
2. Add tests for the overwrite safety mechanism
3. Clarify/document the CLI vs pipeline error handling difference

Great work on this feature! 🚀

[claude]

PR Review: feat(web): nav-export command + pipeline step

Summary

This PR adds a first-class nav export workflow to generate site-nav.json deterministically without building HTML. The implementation is well-structured with comprehensive test coverage and good documentation.

✅ Strengths

1. Code Quality & Architecture

• Clean separation of concerns with partial class organization (NavExport.cs)
• Consistent with existing codebase patterns (e.g., BuildSiteNavJson refactoring)
• Well-documented XML comments explaining behavior and parameters
• Good reuse of existing infrastructure (WebSiteBuilder methods)

2. Security & Safety

• Path traversal protection via IsPathWithinRoot check (WebSiteBuilder.NavExport.cs:37)
• Safe overwrite behavior with generated:true marker detection (lines 52-61)
• Comprehensive exception handling in IsGeneratedNavExport (lines 89-100)
• Proper input sanitization with Trim().Trim() (line 35)

3. Test Coverage

• Excellent test coverage with 5 CLI tests + 2 pipeline tests
• Tests cover default output, overwrite safety, force overwrite, path traversal prevention, and pipeline integration
• Good use of temporary directories with cleanup

4. Documentation

• Updated Pipeline.md with clear examples and notes
• JSON schema updated for pipeline spec
• Helpful error messages for users

5. Bug Fix Bonus

• Port fallback detection improvement for WebStaticServer (error code 32 for sharing violations)

🔍 Minor Observations

1. Duplicate Detection Logic (WebSiteBuilder.NavExport.cs:122-136)
The duplicate project slug detection uses warnings but only takes the first occurrence. Consider if these should be surfaced to the result model for CLI/pipeline feedback.

2. Exception Handling Coverage (WebSiteBuilder.NavExport.cs:89-100)
Consider also catching NotSupportedException which can occur with certain JSON structures. However, treating unknown states as user-managed files is a safe default.

3. Method Location (WebCliCommandHandlers.NavExportCommands.cs:61-67)
The ResolveOutputPath helper may be duplicated logic. Consider extracting to WebCliHelpers if used elsewhere.

4. Test Assertion Detail (WebCliNavExportTests.cs:129)
Consider asserting the menu content structure more explicitly to catch regressions in the nav export format.

🎯 Performance Considerations

The nav export reuses the full content discovery pipeline. For large sites, this could be expensive. Worth documenting that nav-export still processes all content even though it does not render HTML.

🔒 Security Assessment

No security concerns identified. The implementation properly validates paths, handles errors gracefully, prevents accidental overwrites, and sanitizes input.

📊 Test Coverage Assessment

Coverage is excellent with 7 tests covering all major paths including happy path, overwrite protection, force overwrite, path traversal prevention, and pipeline integration.

💡 Recommendations

1. Consider adding integration test verifying the nav export JSON schema matches what consumers expect
2. Optional: Extract ResolveOutputPath to WebCliHelpers if duplicated in other commands
3. Future: Consider exposing duplicate slug warnings in WebNavExportResult for better observability

✨ Overall Assessment

Approval: ✅ Recommended for merge

This is high-quality work that follows established patterns, includes comprehensive tests, and addresses a real need for deterministic nav export. The security posture is solid, and the implementation is production-ready.

The code demonstrates strong understanding of the existing codebase, attention to security and safety, commitment to testing, and clear documentation.

Great work! 🚀

---

Reviewed using Claude Code - Generated on 2026-02-15