Use NuGet packages for HtmlTinkerX and OfficeIMO.Markdown#51
Use NuGet packages for HtmlTinkerX and OfficeIMO.Markdown#51PrzemyslawKlys merged 1 commit 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: dbbabad59e
ℹ️ 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".
| var expectedVersion = expectedGlobal; | ||
| var expectedSource = "global"; | ||
| if (expectedMap.TryGetValue(project.ProjectName, out var overrideVersion) && !string.IsNullOrWhiteSpace(overrideVersion)) | ||
| { |
There was a problem hiding this comment.
When ExpectedVersionMapUseWildcards is enabled, the docs say map keys can include */?, but the version override logic still uses expectedMap.TryGetValue(project.ProjectName, ...), which only matches exact keys. That means wildcard entries never influence the resolved version for matching projects; they only affect the include-only filtering earlier. For configurations that rely on wildcard version overrides (e.g., a single pattern for multiple projects), those projects will silently fall back to the global/CSProj version instead of the intended map value.
Useful? React with 👍 / 👎.
Code Review - PR #51Thank you for this contribution! I've conducted a comprehensive review of the changes. While there's significant functionality being added, I've identified several critical concerns that should be addressed. Critical Issues1. Scope Creep - PR Does Not Match DescriptionSeverity: CRITICAL The PR title states 'Use NuGet packages for HtmlTinkerX and OfficeIMO.Markdown' (suggesting a simple dependency update), but this PR actually includes:
Recommendation: Split this into separate PRs for safer review, testing, and rollback. 2. API Key Exposure via Process ArgumentsSeverity: CRITICAL Location: PowerForge/Services/DotNetRepositoryReleaseService.cs:636-637 API keys passed as command-line arguments are visible in process listings, audit logs, and memory dumps. Should use environment variables instead. 3. GitHub Token Exposure in PowerShellSeverity: HIGH Location: PSPublishModule/Cmdlets/InvokeProjectBuildCommand.cs:252-255, 346 GitHub token passed as parameter could be exposed in PowerShell transcript logs and debug output. Use secure credential objects or environment variables. 4. Missing Test CoverageSeverity: CRITICAL The new InvokeProjectBuildCommand.cs (815 lines) has zero test coverage for critical functionality including secret resolution, GitHub release modes, template token replacement, and error handling. High Priority Issues5. Bare Catch Blocks Swallowing ExceptionsMultiple instances in InvokeProjectBuildCommand.cs (lines 534, 544, 74) and DotNetRepositoryReleaseService.cs that silently hide errors, making debugging nearly impossible. 6. PowerShell Cmdlet Availability Not ValidatedCode assumes Send-GitHubRelease cmdlet exists without validation. Should check availability before invocation. 7. Invalid Certificate Store Names Silently DefaultParseCertificateStore silently falls back to CurrentUser for typos in LocalMachine. Should throw on invalid values. Medium Priority Issues8. Race Condition in Staging Path PreparationBetween Directory.Delete and Directory.CreateDirectory, another process could interfere. 9. Template Token Validation MissingNo validation that template tokens are valid. Invalid tokens remain in output. 10. Path Traversal Risk in Zip CreationShould validate relative paths do not contain .. sequences. Summary
Recommendations
Let me know if you would like me to elaborate on any of these issues! |
There was a problem hiding this comment.
Pull request overview
This pull request replaces local project references with NuGet packages for HtmlTinkerX and OfficeIMO.Markdown in PowerForge.Web. However, the PR contains significantly more changes than described, including extensive new functionality for:
- Module GUID tracking in PSResourceGet/PowerShellGet
- Online module version resolution
- ExternalModuleDependencies support
- Extensive .NET repository build/release pipeline features
- Package signing capabilities
- GitHub release automation
Changes:
- Replace local project references with NuGet packages (HtmlTinkerX 2.0.6, OfficeIMO.Markdown 0.5.2)
- Add GUID tracking to PSResourceInfo and module discovery scripts
- Implement online module version resolution without installation
- Add comprehensive .NET project build/release pipeline with signing and GitHub integration
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 41 comments.
Show a summary per file
| File | Description |
|---|---|
| PowerForge.Web/PowerForge.Web.csproj | Replaces local project references with NuGet package references |
| schemas/project.build.schema.json | New JSON schema for project build configuration (257 lines) |
| schemas/powerforge.segments.schema.json | Adds ResolveMissingModulesOnline, WarnIfRequiredModulesOutdated, FailOnMissingCommands |
| PowerForge/Services/*.cs | Multiple services enhanced with GUID tracking, online resolution, signing, and release features |
| PowerForge/Scripts/*.ps1 | PowerShell scripts updated to extract module GUIDs |
| PowerForge/Models/*.cs | Models extended with ExternalModuleDependencies, signing config, GitHub release options |
| PSPublishModule/Cmdlets/*.cs | Cmdlets updated for new features; InvokeProjectBuildCommand added (815 lines) |
| PowerForge.Web.Cli/Program.cs | Web pipeline runner enhanced with progress tracking and duration reporting |
| Module/Docs/*.md | Documentation updated for new parameters and cmdlets |
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.
| 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; |
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; | |
| foreach (var project in release.Projects.Where(p => p.IsPackable && !string.IsNullOrWhiteSpace(p.ReleaseZipPath))) | |
| { | |
| var zip = project.ReleaseZipPath; |
| 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(...)'.
| foreach (var item in items) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(item)) continue; | ||
| set.Add(item.Trim()); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var kvp in expectedMap) | ||
| { | ||
| if (MatchesPattern(projectName, kvp.Key, allowWildcards)) | ||
| return true; | ||
| } |
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); |
| var repos = ParseRepositoryList(repository); | ||
| var runner = new PowerShellRunner(); | ||
|
|
||
| IReadOnlyList<PSResourceInfo> items = Array.Empty<PSResourceInfo>(); |
| !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
powerforge-nuget-deps-clean
branch
from
dbbabad to
df84758
Compare
|
Rebased onto origin/web/apidocs-cref and force-pushed. PR now only changes PowerForge.Web.csproj (2 insertions/2 deletions). Previous review comments about extra files should now be outdated. Please re-review. |
Pull Request Review: Use NuGet packages for HtmlTinkerX and OfficeIMO.MarkdownSummaryThis PR replaces local project references with NuGet package references for two dependencies:
Code Quality ✅
Dependency Impact Analysis 📦The change affects three key services in PowerForge.Web:
Potential Issues
|
Summary
Testing