Skip to content

Fix release workflow#18

Merged
mtmk merged 1 commit intomainfrom
fix-release-workflow
Apr 10, 2026
Merged

Fix release workflow#18
mtmk merged 1 commit intomainfrom
fix-release-workflow

Conversation

@mtmk
Copy link
Copy Markdown
Collaborator

@mtmk mtmk commented Apr 10, 2026

The release workflow triggered on push to main, so any merged PR that changed version.txt would push packages to NuGet. Switch to the release: published event so publishing requires a maintainer to create a GitHub release, and validate the tag matches version.txt before packing. Pass NUGET_API_KEY via env to avoid direct shell interpolation.

The workflow triggered on push to main, so any merged PR that
changed version.txt would push packages to NuGet. Switch to the
release:published event so publishing requires a maintainer to
create a GitHub release, and validate the tag matches version.txt
before packing. Pass NUGET_API_KEY via env to avoid direct shell
interpolation.
@synadia-claude-reviewer
Copy link
Copy Markdown

synadia-claude-reviewer bot commented Apr 10, 2026

Claude finished @mtmk's task in 2m 7s —— View job


PR Review: Fix release workflow

  • Read PR diff and workflow file
  • Review for correctness and security
  • Post inline comments if needed
  • Post overall summary

Overall Assessment

The changes are correct and a clear security improvement. No bugs introduced.

What works well:

  • Switching to release: published correctly gates publishing behind an explicit maintainer action. The old push: branches: [main] trigger meant any merged PR touching version.txt could fire a NuGet push.
  • contents: read is the right permission now that the tag-creation step is gone.
  • Moving NUGET_API_KEY to the env: block is strictly better — secrets containing shell-special characters (e.g. !) can silently misbehave when interpolated directly into run: commands via ${{ }}.
  • The validation logic is correct: for release: published, GITHUB_REF_NAME is the tag name (e.g. v1.0.0), ${tag#v} strips the leading v, and the comparison against version.txt via head -n 1 is sound.
  • Removing the conditional if: ${{ fromJSON(steps.tag.outputs.create) }} guards simplifies the workflow without loss of safety — the trigger change ensures this only runs when a release is deliberately published, and --skip-duplicate handles any edge case of re-publishing.

One thing to be aware of (pre-existing, not introduced here):

src/Directory.Build.props sets the NuGet package version using:

<Version>$([System.IO.File]::ReadAllText("$(MSBuildProjectDirectory)/version.txt"))</Version>

ReadAllText does not trim. If version.txt ever gains a trailing newline, the packed version could have embedded whitespace. The shell validation uses head -n 1 which strips it, so the tag comparison would pass while the packed version string differs. This is worth guarding against — e.g. $([System.IO.File]::ReadAllText(...).Trim()) — but it's out of scope for this PR.

No issues with the PR as written. LGTM.

@mtmk mtmk merged commit fb9d183 into main Apr 10, 2026
6 checks passed
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