Skip to content

Add unified staged release publishing#310

Merged
PrzemyslawKlys merged 1 commit intomainfrom
codex/unified-release-publish
Apr 7, 2026
Merged

Add unified staged release publishing#310
PrzemyslawKlys merged 1 commit intomainfrom
codex/unified-release-publish

Conversation

@PrzemyslawKlys
Copy link
Copy Markdown
Member

Summary

  • add a first-class unified GitHub section to powerforge release so staged UploadReady assets can publish as one repo release
  • resolve relative Winget output under the active staged release root and include generated Winget manifests in release assets/checksums
  • keep IX-style wrappers thin by moving the repo-release orchestration into PowerForge instead of custom repo scripts

Validation

  • dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter "PowerForgeReleaseServiceTests"
  • dotnet build .\PowerForge.Cli\PowerForge.Cli.csproj -c Release -f net10.0
  • dotnet build .\PSPublishModule\PSPublishModule.csproj -c Release -f net8.0
  • dotnet build .\PSPublishModule\PSPublishModule.csproj -c Release -f net472

@claude
Copy link
Copy Markdown

claude Bot commented Apr 7, 2026

Code Review

Overall this is a clean, well-structured addition. The orchestration logic is properly separated, the tests cover the happy path, and the schema/docs are in sync. A few things worth addressing:


Security

Direct token in release.json (PowerForgeRelease.csPowerForgeReleaseGitHubOptions.Token)
The Token field lets a GitHub PAT live inside a config file that is often committed to VCS. The TokenFilePath / TokenEnvName alternatives are the right answer; consider adding a doc note (or a runtime warning log) that Token is intended only for CI injection, not for checked-in config.

Token file size (ResolveSecret)
File.ReadAllText(resolvedPath) has no size guard. A token file should be ~40–100 chars; reading an accidentally-pointed large file silently is harmless but sloppy. A short max-byte read or at least a size check before trimming would be defensive.


Code Quality

Duplicate template method (ApplyUnifiedGitHubTemplate vs the existing ApplyGitHubTemplate)
The two methods are nearly identical — the only difference is that the unified variant replaces {Target} and {Project} with empty string. Consider either overloading / making target optional in ApplyGitHubTemplate, or calling ApplyGitHubTemplate(template, target: string.Empty, ...) from the new method.

{DateTime} and {Timestamp} collapse to the same value (both ApplyGitHubTemplate and the new ApplyUnifiedGitHubTemplate)
Both tokens map to now.ToString("yyyyMMddHHmmss"). Presumably one should be local and the other UTC, or one should be a Unix epoch. Pre-existing issue but worth fixing while touching this code.

ReuseExistingReleaseOnConflict is hardcoded true (PublishUnifiedGitHubRelease)
Silently reusing an existing release is a safe default for CI retries, but there is no way to opt out. Exposing this as PowerForgeReleaseGitHubOptions.ReuseExisting (defaulting to true) would give callers control without breaking the current behavior.

Redundant null-forgiving operators
Several ! assertions follow explicit IsNullOrWhiteSpace guards (e.g. version!, stageRoot!). These are harmless but add noise — worth removing or replacing with a local variable that carries a non-null type.

Double-write of summary files
PopulateReleaseOutputs writes both release-manifest.json and SHA256SUMS.txt, then RewriteReleaseSummaryFiles writes them again after Winget assets are added. The correctness is fine (second write wins), but it might be worth skipping the first write entirely and always deferring to the RewriteReleaseSummaryFiles call, or at least adding a comment explaining the intentional two-pass approach.


Test Coverage

The two new tests cover the main happy path well. Missing edge cases:

Scenario Covered?
Relative Winget output, no stage root ✅ (existing tests)
Relative Winget output under stage root ✅ (new)
Unified publish suppresses package GitHub ✅ (new)
Version not resolvable (ambiguous multi-version assets)
Missing Owner/Repository
Missing token (all three resolution paths exhausted)
No staged assets available
publishGitHubRelease delegate throws ❌ (the catch block is untested)

The failure-path tests matter here because errors are surfaced as a PowerForgeUnifiedGitHubReleaseResult with Success = false rather than thrown exceptions — verifying those result objects are well-formed is low-cost and would catch regression in error propagation.


Minor / Nits

  • ResolveConfiguredStageRoot is used only inside GenerateWingetOutputs; PopulateReleaseOutputs resolves the stage root via its own path. Consolidating these would remove a latent divergence risk.
  • The doc bullet-list indentation fix (line 10 of the docs diff) changes a nested bullet to a top-level one — verify this is the intended hierarchy change and not an accidental dedent.

Overall the implementation is solid and the feature is well-scoped. The main asks are: test the error paths, consider exposing ReuseExisting, and de-duplicate the template method.

@PrzemyslawKlys PrzemyslawKlys merged commit b44ba56 into main Apr 7, 2026
6 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the codex/unified-release-publish branch April 7, 2026 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant