API docs: add default language classes for code examples#53
API docs: add default language classes for code examples#53PrzemyslawKlys merged 2 commits intoweb/apidocs-creffrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b8964be29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (expectedMap.TryGetValue(project.ProjectName, out var overrideVersion) && !string.IsNullOrWhiteSpace(overrideVersion)) | ||
| { | ||
| expectedVersion = overrideVersion; |
There was a problem hiding this comment.
When ExpectedVersionMapUseWildcards is enabled, project filtering uses wildcard matching, but version resolution still uses expectedMap.TryGetValue(project.ProjectName, ...), which only supports exact keys. In configs like { "ExpectedVersionMap": { "My.*": "2.0.*" }, "ExpectedVersionMapUseWildcards": true }, matching projects are included but do not receive the mapped version pattern, so they fall back to global/csproj versions and can be packed/published with unintended versions.
Useful? React with 👍 / 👎.
Code Review: PR #53OverviewTitle: API docs - add default language classes for code examples Critical Issues (Must Fix Before Merge)1. Path Traversal Vulnerability in BuildReleaseZipPathLocation: DotNetRepositoryReleaseService.cs:825-834 Issue: spec.ReleaseZipOutputPath is used directly without validation. Malicious configurations could escape the working directory. Risk: An attacker could set ReleaseZipOutputPath to ../../../../etc/ and write files outside the intended directory. Fix: Validate paths to ensure they remain within the root path before use. 2. Race Condition (TOCTOU) in Zip CreationLocation: DotNetRepositoryReleaseService.cs:859-862 Issue: Time-of-check-time-of-use bug between File.Delete and FileStream creation. Fix: Use FileMode.Create which handles both cases atomically instead of FileMode.CreateNew. High Priority Issues3. Configuration Injection RiskLocation: Lines 828, 844. spec.Configuration is used in path construction without validation. Validate against a whitelist of allowed configurations. 4. Silent Certificate Retrieval FailuresLocation: GetCertificateSha256 (lines 1011-1033). Empty catch blocks suppress all exceptions without logging. 5. Code DuplicationGetCertificateSha256 appears identically in both DotNetRepositoryReleaseService.cs and DotNetReleaseBuildService.cs. Extract to a shared utility class. Strengths
Test Coverage RecommendationsAdd tests for: path traversal attempts, invalid certificate thumbprints, configuration injection attempts, concurrent zip creation scenarios, FailOnMissingCommands behavior, and wildcard matching. Overall AssessmentCode Quality: 8/10 - Generally excellent with modern C# patterns RecommendationREQUEST CHANGES - Fix critical security issues (path traversal and race condition) and high-priority items before merging. Once the path validation and race condition are addressed, this will be a strong addition to the codebase. |
There was a problem hiding this comment.
Pull request overview
This pull request claims to add default language classes (csharp/powershell) for API example code blocks to support Prism highlighting without JavaScript fallback. However, the PR contains extensive additional changes far beyond documentation rendering.
Changes:
- Documentation rendering: Added language class attributes to code blocks in WebApiDocsGenerator.cs
- New project build schema and Invoke-ProjectBuild cmdlet for repository-wide .NET build/release pipelines
- Module dependency resolution with online/offline version resolution capabilities
- NuGet package signing with certificate support
- Numerous schema updates and model additions
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 43 comments.
Show a summary per file
| File | Description |
|---|---|
| PowerForge.Web/Services/WebApiDocsGenerator.cs | Adds language class attributes to code/pre tags for Prism syntax highlighting |
| schemas/project.build.schema.json | New schema for .NET project build configuration (entire new file) |
| PowerForge/Services/ModulePipelineRunner.cs | Major feature additions for module dependency resolution, external modules, online version checking |
| PowerForge/Services/DotNetRepositoryReleaseService.cs | Extensive additions for package signing, release zip creation, duplicate detection, wildcard matching |
| PSPublishModule/Cmdlets/InvokeProjectBuildCommand.cs | Entirely new cmdlet for repository-wide build/release pipeline (815 lines) |
| PowerForge/Scripts//.ps1 | Added GUID extraction for PowerShellGet and PSResourceGet module queries |
| Multiple documentation files | Updated cmdlet documentation reflecting new parameters and functionality |
Comments suppressed due to low confidence (1)
PowerForge/Services/ModulePipelineRunner.cs:251
- Condition is always true because of ... is ....
if (moduleSeg.Kind == ModuleDependencyKind.RequiredModule)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (plan.ExternalModuleDependencies is { Length: > 0 }) | ||
| ManifestEditor.TrySetPsDataStringArray(buildResult.ManifestPath, "ExternalModuleDependencies", plan.ExternalModuleDependencies); |
There was a problem hiding this comment.
Duplicate code: ExternalModuleDependencies is being set twice on consecutive lines with identical logic. Remove one of these duplicate lines.
| if (plan.ExternalModuleDependencies is { Length: > 0 }) | |
| ManifestEditor.TrySetPsDataStringArray(buildResult.ManifestPath, "ExternalModuleDependencies", plan.ExternalModuleDependencies); |
| var codeLanguage = GetDefaultCodeLanguage(options); | ||
| AppendMembers(memberHtml, "Methods", type.Methods, codeLanguage); | ||
| AppendMembers(memberHtml, "Properties", type.Properties, codeLanguage); | ||
| AppendMembers(memberHtml, "Fields", type.Fields, codeLanguage); | ||
| AppendMembers(memberHtml, "Events", type.Events, codeLanguage); |
There was a problem hiding this comment.
The PR title and description state this is about "API docs: add default language classes for code examples" but this pull request contains extensive unrelated changes including: new JSON schemas, PowerShell module resolution features, NuGet package signing, GitHub release functionality, and many other features. These substantial functional changes should be in separate PRs with appropriate descriptions. The documentation rendering change is a small part of this PR (lines in WebApiDocsGenerator.cs).
| foreach (var project in release.Projects) | ||
| { | ||
| if (!project.IsPackable) continue; | ||
| var r = new ProjectBuildGitHubResult { ProjectName = project.ProjectName }; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(project.NewVersion)) | ||
| { | ||
| r.Success = false; | ||
| r.ErrorMessage = "Missing project version for GitHub release."; | ||
| result.GitHub.Add(r); | ||
| if (spec.PublishFailFast) | ||
| { | ||
| result.Success = false; | ||
| result.ErrorMessage = r.ErrorMessage; | ||
| break; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(project.ReleaseZipPath)) | ||
| { | ||
| r.Success = false; | ||
| r.ErrorMessage = "No release zip available for GitHub release."; | ||
| result.GitHub.Add(r); | ||
| if (spec.PublishFailFast) | ||
| { | ||
| result.Success = false; | ||
| result.ErrorMessage = r.ErrorMessage; | ||
| break; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (!File.Exists(project.ReleaseZipPath)) | ||
| { | ||
| r.Success = false; | ||
| r.ErrorMessage = $"Release zip not found: {project.ReleaseZipPath}"; | ||
| result.GitHub.Add(r); | ||
| if (spec.PublishFailFast) | ||
| { | ||
| result.Success = false; | ||
| result.ErrorMessage = r.ErrorMessage; | ||
| break; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| var tag = string.IsNullOrWhiteSpace(config.GitHubTagName) | ||
| ? (config.GitHubIncludeProjectNameInTag == false ? $"v{project.NewVersion}" : $"{project.ProjectName}-v{project.NewVersion}") | ||
| : config.GitHubTagName!; | ||
| if (!string.IsNullOrWhiteSpace(config.GitHubTagTemplate)) | ||
| { | ||
| tag = ApplyTemplate( | ||
| config.GitHubTagTemplate!, | ||
| project.ProjectName, | ||
| project.NewVersion ?? project.OldVersion ?? string.Empty, | ||
| config.GitHubPrimaryProject ?? project.ProjectName, | ||
| project.NewVersion ?? project.OldVersion ?? string.Empty, | ||
| repoName, | ||
| dateToken, | ||
| utcDateToken); | ||
| } | ||
|
|
||
| var releaseName = string.IsNullOrWhiteSpace(config.GitHubReleaseName) | ||
| ? tag | ||
| : ApplyTemplate( | ||
| config.GitHubReleaseName!, | ||
| project.ProjectName, | ||
| project.NewVersion ?? project.OldVersion ?? string.Empty, | ||
| config.GitHubPrimaryProject ?? project.ProjectName, | ||
| project.NewVersion ?? project.OldVersion ?? string.Empty, | ||
| repoName, | ||
| dateToken, | ||
| utcDateToken); | ||
|
|
||
| var output = sb.Invoke(config.GitHubUsername, config.GitHubRepositoryName, gitHubToken, tag, releaseName, new[] { project.ReleaseZipPath }, config.GitHubIsPreRelease, config.GitHubGenerateReleaseNotes); | ||
| var status = output.Count > 0 ? output[0]?.BaseObject : null; | ||
|
|
||
| bool ok = false; | ||
| string? releaseUrl = null; | ||
| string? errorMessage = null; | ||
|
|
||
| if (status is SendGitHubReleaseCommand.GitHubReleaseResult gr) | ||
| { | ||
| ok = gr.Succeeded; | ||
| releaseUrl = gr.ReleaseUrl; | ||
| errorMessage = gr.Succeeded ? null : gr.ErrorMessage; | ||
| } | ||
| else if (status is PSObject pso) | ||
| { | ||
| var succeeded = pso.Properties["Succeeded"]?.Value as bool?; | ||
| ok = succeeded ?? false; | ||
| releaseUrl = pso.Properties["ReleaseUrl"]?.Value?.ToString(); | ||
| errorMessage = pso.Properties["ErrorMessage"]?.Value?.ToString(); | ||
| } | ||
| else | ||
| { | ||
| errorMessage = "Unexpected result from Send-GitHubRelease."; | ||
| } | ||
|
|
||
| r.Success = ok; | ||
| r.TagName = tag; | ||
| r.ReleaseUrl = releaseUrl; | ||
| r.ErrorMessage = errorMessage; | ||
| result.GitHub.Add(r); | ||
|
|
||
| if (!ok) | ||
| { | ||
| result.Success = false; | ||
| result.ErrorMessage = errorMessage ?? "GitHub publish failed."; | ||
| if (spec.PublishFailFast) | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var project in release.Projects) | ||
| { | ||
| if (!project.IsPackable) continue; | ||
| var zip = project.ReleaseZipPath; | ||
| if (string.IsNullOrWhiteSpace(zip)) | ||
| continue; | ||
| if (File.Exists(zip)) | ||
| assets.Add(zip!); |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var project in release.Projects) | |
| { | |
| if (!project.IsPackable) continue; | |
| var zip = project.ReleaseZipPath; | |
| if (string.IsNullOrWhiteSpace(zip)) | |
| continue; | |
| if (File.Exists(zip)) | |
| assets.Add(zip!); | |
| foreach (var project in release.Projects.Where(p => p.IsPackable && !string.IsNullOrWhiteSpace(p.ReleaseZipPath))) | |
| { | |
| var zip = project.ReleaseZipPath!; | |
| if (File.Exists(zip)) | |
| assets.Add(zip); |
| foreach (var line in SplitLines(stdout)) | ||
| { | ||
| if (!line.StartsWith("PFDOCS::ERROR::", StringComparison.Ordinal)) continue; | ||
| var b64 = line.Substring("PFDOCS::ERROR::".Length); | ||
| try { return Encoding.UTF8.GetString(Convert.FromBase64String(b64)); } | ||
| catch { return null; } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| if (!spec.WhatIf) | ||
| { | ||
| if (!TryCreateReleaseZip(project, spec.Configuration, zipPath, out var zipError)) | ||
| { | ||
| project.ErrorMessage = zipError; | ||
| _logger.Warn($"{project.ProjectName}: {zipError}"); | ||
| result.Success = false; | ||
| if (spec.PublishFailFast) | ||
| return result; | ||
| } | ||
| else | ||
| { | ||
| _logger.Success($"{project.ProjectName}: release zip created."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Condition is always false because of access to property WhatIf.
| if (!spec.WhatIf) | |
| { | |
| if (!TryCreateReleaseZip(project, spec.Configuration, zipPath, out var zipError)) | |
| { | |
| project.ErrorMessage = zipError; | |
| _logger.Warn($"{project.ProjectName}: {zipError}"); | |
| result.Success = false; | |
| if (spec.PublishFailFast) | |
| return result; | |
| } | |
| else | |
| { | |
| _logger.Success($"{project.ProjectName}: release zip created."); | |
| } | |
| } | |
| if (!TryCreateReleaseZip(project, spec.Configuration, zipPath, out var zipError)) | |
| { | |
| project.ErrorMessage = zipError; | |
| _logger.Warn($"{project.ProjectName}: {zipError}"); | |
| result.Success = false; | |
| if (spec.PublishFailFast) | |
| return result; | |
| } | |
| else | |
| { | |
| _logger.Success($"{project.ProjectName}: release zip created."); | |
| } |
| var synopsisPercent = Percent(synopsisCount, total); | ||
| var descriptionPercent = Percent(descriptionCount, total); | ||
| var examplesPercent = Percent(exampleCount, total); |
There was a problem hiding this comment.
This assignment to examplesPercent is useless, since its value is never read.
| var examplesPercent = Percent(exampleCount, total); |
| !string.IsNullOrWhiteSpace(root)) | ||
| { | ||
| if (!File.Exists(Path.Combine(moduleRoot, root))) | ||
| issues.Add($"RootModule missing: {root}"); |
There was a problem hiding this comment.
These 'if' statements can be combined.
| !string.IsNullOrWhiteSpace(root)) | |
| { | |
| if (!File.Exists(Path.Combine(moduleRoot, root))) | |
| issues.Add($"RootModule missing: {root}"); | |
| !string.IsNullOrWhiteSpace(root) && | |
| !File.Exists(Path.Combine(moduleRoot, root))) | |
| { | |
| issues.Add($"RootModule missing: {root}"); |
| } | ||
|
|
||
| var tag = string.IsNullOrWhiteSpace(config.GitHubTagName) | ||
| ? (config.GitHubIncludeProjectNameInTag == false ? $"v{project.NewVersion}" : $"{project.ProjectName}-v{project.NewVersion}") |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
| ? (config.GitHubIncludeProjectNameInTag == false ? $"v{project.NewVersion}" : $"{project.ProjectName}-v{project.NewVersion}") | |
| ? (!config.GitHubIncludeProjectNameInTag ? $"v{project.NewVersion}" : $"{project.ProjectName}-v{project.NewVersion}") |
PrzemyslawKlys
force-pushed
the
web/apidocs-code-lang
branch
from
0b8964b to
04b5990
Compare
|
Rebased onto origin/web/apidocs-cref and force-pushed. PR now only touches WebApiDocsGenerator.cs (default language classes). Previous scope/extra-files comments should now be outdated. Please re-review. |
Code Review for PR #53: API docs - add default language classes for code examplesSummaryThis PR adds default language classes (language-csharp or language-powershell) to code example blocks in the API documentation generator based on the documentation type. This enables proper syntax highlighting with Prism.js without requiring JavaScript fallback logic. Positives
Issues FoundCRITICAL: Missing parameter propagation The PR is incomplete. The codeLanguage parameter needs to be passed through additional method signatures but is currently missing:
Current impact: Member-level examples (in methods, properties, etc.) will not have language classes applied, only type-level examples will work correctly. The parameter needs to be added to these three methods and all their call sites updated (lines 2817-2823, 2847-2849, 2887, 2898, 2908). Other Observations
RecommendationRequest Changes - The PR needs the additional parameter threading to be complete before merging. The current implementation will only apply language classes to type-level examples but not member-level examples. |
|
Addressed missing codeLanguage propagation. Threaded codeLanguage through AppendMemberSections/AppendMemberCards/AppendMemberCard so member-level examples receive language classes. Pushed commit 8bee2f0. |
Pull Request Review - APPROVEDSummaryThis PR adds default language classes (csharp or powershell) to code blocks in API documentation examples based on the ApiDocsType configuration. This improves syntax highlighting compatibility with Prism.js and other client-side highlighters. Positives
Code Quality (PowerForge.Web/Services/WebApiDocsGenerator.cs)
Considerations
Security & Performance
Repository Conventions (per CONTRIBUTING.md)
VerdictAPPROVED - Clean, focused change that improves syntax highlighting. Implementation is solid and follows repository conventions. Safe to merge as-is, though adding tests would strengthen confidence in future refactorings. |
Summary
Testing