From 97226079e51a8c047f3c4097a4bb24de1d920ffb Mon Sep 17 00:00:00 2001 From: Krzysztof Ploch Date: Mon, 24 Aug 2026 01:53:59 +0200 Subject: [PATCH 1/5] fix(github-actions): Publish the packages the build actually produces Both GitHub Packages publish steps used `./**/*.nupkg`. That does not recurse: globstar is a shell option that is off by default, and GitHub runs steps with `bash -e` without enabling it, so the pattern is equivalent to `./*/*.nupkg` and matches one directory level. Packages are produced at src/Spectre//bin/Release, four levels down. The failure mode is silent rather than loud. An unmatched glob is passed through literally, and `dotnet nuget push` given a non-matching literal exits 0 without publishing: $ bash -c "shopt -u globstar; dotnet nuget push './**/*.snupkg' \ --source --skip-duplicate" exit=0 So the step would have reported a successful publish having published nothing. It has never been observed because the step is gated on refs/heads/main and nothing has been pushed there yet -- the first push, which is what merging #11 does, would have been the first occurrence. Enabling globstar alone would have swapped one defect for another: Directory.Build.props packs every non-test project and the workflow builds the sample solution in Release, so a recursive glob would publish the demo application to the feed. Both steps now enumerate with `find`, excluding tests/ and samples/, matching what release.yml already does, and fail when nothing is found -- a publish step that finds no packages must never report success. Verified against a fixture mirroring the real layout: two library packages selected, sample and test packages excluded, and an empty result exits 1 instead of 0. Found by GitHub Copilot's pull request reviewer on #11. Refs: #43 --- .github/workflows/build-dotnet.yml | 45 +++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build-dotnet.yml b/.github/workflows/build-dotnet.yml index d21c745..124b7c7 100644 --- a/.github/workflows/build-dotnet.yml +++ b/.github/workflows/build-dotnet.yml @@ -259,8 +259,29 @@ jobs: - name: Publish NuGet packages to GitHub Packages if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' run: | - dotnet nuget push ./**/*.nupkg --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" - dotnet nuget push ./**/*.snupkg --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" || true + # `**` does not recurse here: globstar is off by default and GitHub runs steps with + # `bash -e`, so `./**/*.nupkg` matches a single directory level while packages are + # produced four levels down. Worse, an unmatched glob is passed through literally and + # `dotnet nuget push` exits 0 on it, so this step would report success having published + # nothing. tests/ and samples/ are excluded because Directory.Build.props packs every + # non-test project and the sample solution is built in Release above. + mapfile -t PACKAGES < <(find . -type f -name '*.nupkg' -not -path './tests/*' -not -path './samples/*' | sort) + + if [ ${#PACKAGES[@]} -eq 0 ]; then + echo "::error::No .nupkg files were found. Refusing to report a successful publish." + exit 1 + fi + + for pkg in "${PACKAGES[@]}"; do + echo "Publishing $pkg" + dotnet nuget push "$pkg" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" + done + + # Symbol packages are best-effort: a missing .snupkg is not a failed release. + find . -type f -name '*.snupkg' -not -path './tests/*' -not -path './samples/*' | sort | while IFS= read -r sym; do + echo "Publishing $sym" + dotnet nuget push "$sym" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" || true + done # Publish PR packages to GitHub Packages for testing/validation. # PR packages include the PR number and branch name in the prerelease tag @@ -277,5 +298,21 @@ jobs: echo "::warning::GH_PACKAGES_TOKEN not available, skipping PR package publish" exit 0 fi - dotnet nuget push ./**/*.nupkg --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" - dotnet nuget push ./**/*.snupkg --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" || true + # Same enumeration as the main-branch publish above; see the comment there for why a + # `./**/*.nupkg` glob silently publishes nothing. + mapfile -t PACKAGES < <(find . -type f -name '*.nupkg' -not -path './tests/*' -not -path './samples/*' | sort) + + if [ ${#PACKAGES[@]} -eq 0 ]; then + echo "::error::No .nupkg files were found. Refusing to report a successful publish." + exit 1 + fi + + for pkg in "${PACKAGES[@]}"; do + echo "Publishing $pkg" + dotnet nuget push "$pkg" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" + done + + find . -type f -name '*.snupkg' -not -path './tests/*' -not -path './samples/*' | sort | while IFS= read -r sym; do + echo "Publishing $sym" + dotnet nuget push "$sym" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" || true + done From 23a9601360580e263bf8f6b0809b542422d6b807 Mon Sep 17 00:00:00 2001 From: Krzysztof Ploch Date: Wed, 26 Aug 2026 16:33:05 +0200 Subject: [PATCH 2/5] refactor(github-actions): Extract NuGet publishing into one shared script The main-branch and pull-request publish steps each carried a copy of the package discovery and push logic, which is why the non-recursive `**` glob bug was present twice. Both now call .github/scripts/publish-nuget-packages.sh, so the two paths cannot drift. The extracted script also hardens the discovery that was carried over: - `-ipath` replaces `-path` for the tests/ and samples/ exclusions. A fixture run confirmed the case-sensitive form does not match a directory named `Tests/`, so a renamed directory could have leaked a test package to the feed. - `find` output is captured into a variable and its exit status checked, rather than piped into `mapfile` through a process substitution, which discards the status. A traversal error after a partial result would otherwise have published a subset and reported success. - The comment justifying the tests/ and samples/ exclusions was wrong: samples/SampleApp/Directory.Build.props sets GeneratePackageOnBuild and IsPackable to false, so the sample never packs. The exclusion is defensive, not load-bearing. .gitattributes pins *.sh to LF. bash does not tolerate CRLF, so a script checked out with CRLF fails on the Linux runner with `bad interpreter: /usr/bin/env bash^M`. Reviewed by Codex and Gemini (both APPROVE_WITH_NOTES). Unifying release.yml onto the same script is tracked separately as #45, because it also changes whether a failed symbol push fails a release. Refs: #43 --- .gitattributes | 5 ++ .github/scripts/publish-nuget-packages.sh | 68 +++++++++++++++++++++++ .github/workflows/build-dotnet.yml | 44 +-------------- 3 files changed, 75 insertions(+), 42 deletions(-) create mode 100755 .github/scripts/publish-nuget-packages.sh diff --git a/.gitattributes b/.gitattributes index b25e53e..48f2f8b 100644 --- a/.gitattributes +++ b/.gitattributes @@ -3,6 +3,11 @@ ############################################################################### * text=auto +# Shell scripts must keep LF in the working tree on every platform: bash does not +# tolerate CRLF, and a script checked out with CRLF fails on the Linux CI runner with +# `bad interpreter: /usr/bin/env bash^M`. +*.sh text eol=lf + ############################################################################### # Set default behavior for command prompt diff. # diff --git a/.github/scripts/publish-nuget-packages.sh b/.github/scripts/publish-nuget-packages.sh new file mode 100755 index 0000000..21559ad --- /dev/null +++ b/.github/scripts/publish-nuget-packages.sh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash +# +# Publishes every NuGet package the build produced to the given feed. +# +# Usage: publish-nuget-packages.sh +# Reads: GH_PACKAGES_TOKEN - the feed API key. Taken from the environment so that this +# script's own arguments carry nothing secret. That narrows the exposure, it does +# not remove it: `dotnet nuget push -k` still places the key in the dotnet +# process's command line, where it is readable from /proc for the life of that +# process. Eliminating it entirely would need an auth mechanism `nuget push` +# does not currently offer. +# +# Why a script and not two inline `run:` blocks: the main-branch and pull-request publish +# steps previously carried a copy each of this logic, and the `./**/*.nupkg` glob bug that +# published nothing was therefore present twice. One implementation cannot drift from itself. +set -euo pipefail + +FEED_URL="${1:?Usage: publish-nuget-packages.sh }" +: "${GH_PACKAGES_TOKEN:?GH_PACKAGES_TOKEN must be set}" + +# Enumerated with `find`, never a glob: globstar is off by default and GitHub runs steps +# with `bash -e`, so `./**/*.nupkg` matches a single directory level while packages are +# produced four levels down. An unmatched glob is then passed through literally and +# `dotnet nuget push` exits 0 on it - the step reports success having published nothing. +# +# tests/ and samples/ are excluded defensively rather than because they would otherwise +# pack: test projects set IsPackable=false, and samples/SampleApp/Directory.Build.props +# sets GeneratePackageOnBuild=false and IsPackable=false. `-ipath` keeps the exclusion +# honest if either directory is ever renamed with different casing. +find_packages() { + find . -type f -name "$1" -not -ipath './tests/*' -not -ipath './samples/*' | sort +} + +# Captured into a variable rather than piped into `mapfile` through a process substitution: +# `mapfile < <(find ...)` discards find's exit status, so a traversal error that occurred +# after a partial result would publish a subset of the packages and report success. +# `set -o pipefail` makes the failing `find` fail the whole `find | sort` pipeline. +if ! packages_found=$(find_packages '*.nupkg'); then + echo "::error::Package discovery failed while enumerating .nupkg files." + exit 1 +fi + +if [ -z "$packages_found" ]; then + echo "::error::No .nupkg files were found. Refusing to report a successful publish." + exit 1 +fi + +mapfile -t packages <<< "$packages_found" + +for pkg in "${packages[@]}"; do + echo "Publishing $pkg" + dotnet nuget push "$pkg" --source "$FEED_URL" --skip-duplicate -k "$GH_PACKAGES_TOKEN" +done + +# Symbol packages are best-effort: a missing or unpushable .snupkg is not a failed release, +# so a discovery failure here warns instead of exiting. +if ! symbols_found=$(find_packages '*.snupkg'); then + echo "::warning::Symbol package discovery failed; publishing without symbol packages." + symbols_found='' +fi + +if [ -n "$symbols_found" ]; then + mapfile -t symbols <<< "$symbols_found" + for sym in "${symbols[@]}"; do + echo "Publishing $sym" + dotnet nuget push "$sym" --source "$FEED_URL" --skip-duplicate -k "$GH_PACKAGES_TOKEN" || true + done +fi diff --git a/.github/workflows/build-dotnet.yml b/.github/workflows/build-dotnet.yml index 124b7c7..226e9fb 100644 --- a/.github/workflows/build-dotnet.yml +++ b/.github/workflows/build-dotnet.yml @@ -258,30 +258,7 @@ jobs: # available package sources". `nuget push` accepts a URL, so no source is needed. - name: Publish NuGet packages to GitHub Packages if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' - run: | - # `**` does not recurse here: globstar is off by default and GitHub runs steps with - # `bash -e`, so `./**/*.nupkg` matches a single directory level while packages are - # produced four levels down. Worse, an unmatched glob is passed through literally and - # `dotnet nuget push` exits 0 on it, so this step would report success having published - # nothing. tests/ and samples/ are excluded because Directory.Build.props packs every - # non-test project and the sample solution is built in Release above. - mapfile -t PACKAGES < <(find . -type f -name '*.nupkg' -not -path './tests/*' -not -path './samples/*' | sort) - - if [ ${#PACKAGES[@]} -eq 0 ]; then - echo "::error::No .nupkg files were found. Refusing to report a successful publish." - exit 1 - fi - - for pkg in "${PACKAGES[@]}"; do - echo "Publishing $pkg" - dotnet nuget push "$pkg" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" - done - - # Symbol packages are best-effort: a missing .snupkg is not a failed release. - find . -type f -name '*.snupkg' -not -path './tests/*' -not -path './samples/*' | sort | while IFS= read -r sym; do - echo "Publishing $sym" - dotnet nuget push "$sym" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" || true - done + run: bash .github/scripts/publish-nuget-packages.sh https://nuget.pkg.github.com/mrploch/index.json # Publish PR packages to GitHub Packages for testing/validation. # PR packages include the PR number and branch name in the prerelease tag @@ -298,21 +275,4 @@ jobs: echo "::warning::GH_PACKAGES_TOKEN not available, skipping PR package publish" exit 0 fi - # Same enumeration as the main-branch publish above; see the comment there for why a - # `./**/*.nupkg` glob silently publishes nothing. - mapfile -t PACKAGES < <(find . -type f -name '*.nupkg' -not -path './tests/*' -not -path './samples/*' | sort) - - if [ ${#PACKAGES[@]} -eq 0 ]; then - echo "::error::No .nupkg files were found. Refusing to report a successful publish." - exit 1 - fi - - for pkg in "${PACKAGES[@]}"; do - echo "Publishing $pkg" - dotnet nuget push "$pkg" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" - done - - find . -type f -name '*.snupkg' -not -path './tests/*' -not -path './samples/*' | sort | while IFS= read -r sym; do - echo "Publishing $sym" - dotnet nuget push "$sym" --source https://nuget.pkg.github.com/mrploch/index.json --skip-duplicate -k "$GH_PACKAGES_TOKEN" || true - done + bash .github/scripts/publish-nuget-packages.sh https://nuget.pkg.github.com/mrploch/index.json From 1f71a6263c8ab152250118be3d6b40a4cddf2c89 Mon Sep 17 00:00:00 2001 From: Krzysztof Ploch Date: Wed, 26 Aug 2026 17:44:01 +0200 Subject: [PATCH 3/5] docs(github-actions): Correct the secret-handling note in the publish script The comment claimed no auth mechanism exists that keeps the API key off a command line. That is wrong: NuGet.Config already registers packageSourceCredentials for the "github" source with %GH_PACKAGES_TOKEN%, so `dotnet nuget push --source github` would authenticate without `-k` and the key would never reach dotnet's argv. Found by the Copilot reviewer, verified against NuGet.Config. The change itself is deferred to #45 rather than made here. This publish path does not run on a PR targeting a feature branch, so a credential-resolution failure would first surface on a push to main - in the exact step this branch exists to make reliable. Refs: #43 --- .github/scripts/publish-nuget-packages.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/scripts/publish-nuget-packages.sh b/.github/scripts/publish-nuget-packages.sh index 21559ad..e3d3fd2 100755 --- a/.github/scripts/publish-nuget-packages.sh +++ b/.github/scripts/publish-nuget-packages.sh @@ -7,8 +7,13 @@ # script's own arguments carry nothing secret. That narrows the exposure, it does # not remove it: `dotnet nuget push -k` still places the key in the dotnet # process's command line, where it is readable from /proc for the life of that -# process. Eliminating it entirely would need an auth mechanism `nuget push` -# does not currently offer. +# process. NuGet.Config already carries packageSourceCredentials for the "github" +# source using %GH_PACKAGES_TOKEN%, so pushing to the source name instead of the +# URL would drop `-k` and keep the key off every command line. That is deferred to +# #45 rather than changed here: this publish path is not exercised by CI on a PR +# targeting a feature branch, so a credential-resolution failure would first show +# up on a push to main - a bad place to discover it, in the very step this branch +# exists to make reliable. # # Why a script and not two inline `run:` blocks: the main-branch and pull-request publish # steps previously carried a copy each of this logic, and the `./**/*.nupkg` glob bug that From 2795211e37c688fd224899f36d7ec36d65ec3cc6 Mon Sep 17 00:00:00 2001 From: Krzysztof Ploch Date: Fri, 28 Aug 2026 16:08:51 +0200 Subject: [PATCH 4/5] fix(github-actions): Publish the Release packages, not the Debug ones Restricting discovery to bin/Release. Without it this script published the Debug build of every package and reported success, which is the same shape of failure it was written to remove. Directory.Build.props sets GeneratePackageOnBuild=true for every non-test project, so a library packs on every build rather than only the Release one. The workflow builds these projects more than once, so a Debug pack sits beside the Release pack carrying an identical version. `sort` orders 'bin/Debug' before 'bin/Release', so the Debug artefact claimed the version on the feed and --skip-duplicate silently swallowed the Release artefact behind it. Observed, not theorised - run 33169218883 on this branch, whose head sha matches this branch's tip, pushed the Debug build of all four packages: Publishing ./src/Spectre/CommandLine.Spectre/bin/Debug/...nupkg -> Your package was pushed. Publishing ./src/Spectre/CommandLine.Spectre/bin/Release/...nupkg -> warn : Error: Version ... has already been pushed. -> already exists at feed 'https://nuget.pkg.github.com/mrploch' The same pattern repeats for all eight .nupkg and .snupkg pairs. Verified the new predicate against a fixture mirroring the real output layout: the old one returns the Debug artefact first and the Release one second, the new one returns only Release, and the tests/ and samples/ exclusions still hold. Found by an adversarial review of this branch that read the branch's own CI log rather than only its diff. Refs: #43 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KECFm7givf8zoQi2hJmCiN --- .github/scripts/publish-nuget-packages.sh | 26 +++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/.github/scripts/publish-nuget-packages.sh b/.github/scripts/publish-nuget-packages.sh index e3d3fd2..8898229 100755 --- a/.github/scripts/publish-nuget-packages.sh +++ b/.github/scripts/publish-nuget-packages.sh @@ -28,12 +28,26 @@ FEED_URL="${1:?Usage: publish-nuget-packages.sh }" # produced four levels down. An unmatched glob is then passed through literally and # `dotnet nuget push` exits 0 on it - the step reports success having published nothing. # -# tests/ and samples/ are excluded defensively rather than because they would otherwise -# pack: test projects set IsPackable=false, and samples/SampleApp/Directory.Build.props -# sets GeneratePackageOnBuild=false and IsPackable=false. `-ipath` keeps the exclusion -# honest if either directory is ever renamed with different casing. +# Restricted to bin/Release: Directory.Build.props sets GeneratePackageOnBuild=true for every +# non-test project, so a library packs on EVERY build, not only the Release one. The workflow +# builds these projects more than once, and a Debug pack therefore sits beside the Release pack +# carrying the identical version. Unfiltered, `sort` orders 'bin/Debug' before 'bin/Release', so +# the Debug artefact claims the version on the feed and --skip-duplicate silently swallows the +# Release one that follows. That is not hypothetical: run 33169218883 on this very branch pushed +# the Debug build of all four packages and reported success -- +# +# Publishing ./src/Spectre/CommandLine.Spectre/bin/Debug/...nupkg -> Your package was pushed. +# Publishing ./src/Spectre/CommandLine.Spectre/bin/Release/...nupkg -> already exists at feed +# +# which is the same shape of failure this script exists to remove: a green step shipping the +# wrong thing. +# +# tests/ and samples/ are excluded defensively rather than because they would otherwise pack: +# test projects set IsPackable=false, and samples/SampleApp/Directory.Build.props sets +# GeneratePackageOnBuild=false and IsPackable=false. `-ipath` keeps every path predicate honest +# if a directory is ever renamed with different casing. find_packages() { - find . -type f -name "$1" -not -ipath './tests/*' -not -ipath './samples/*' | sort + find . -type f -name "$1" -ipath '*/bin/Release/*' -not -ipath './tests/*' -not -ipath './samples/*' | sort } # Captured into a variable rather than piped into `mapfile` through a process substitution: @@ -46,7 +60,7 @@ if ! packages_found=$(find_packages '*.nupkg'); then fi if [ -z "$packages_found" ]; then - echo "::error::No .nupkg files were found. Refusing to report a successful publish." + echo "::error::No Release .nupkg files were found. Refusing to report a successful publish." exit 1 fi From d8f81214ead9cfa6e5bf34af470eccb25d5e8cc7 Mon Sep 17 00:00:00 2001 From: Krzysztof Ploch Date: Fri, 28 Aug 2026 16:47:46 +0200 Subject: [PATCH 5/5] style(github-actions): Clear the SonarCloud findings on the publish script Five open MAJOR issues, all shell hygiene, all pre-existing in substance - two of them only acquired new keys because the previous commit moved the lines they sit on. - S7679: the positional parameter in find_packages is now named, so the function reads as taking a pattern rather than an anonymous $1. - S7688 x2: [[ instead of [ for the two conditional tests. - S7677 x2: the two ::error:: messages go to stderr. The workflow command still renders - the runner scans the step's whole output stream - and the step fails on the exit code regardless, so a lost annotation could not hide the failure. No behavioural change. Re-checked with bash -n and against the same fixture used for the Release-only discovery fix: Debug output and the tests/ tree are still excluded, Release output is still found. Refs: #43 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KECFm7givf8zoQi2hJmCiN --- .github/scripts/publish-nuget-packages.sh | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/.github/scripts/publish-nuget-packages.sh b/.github/scripts/publish-nuget-packages.sh index 8898229..f366f9d 100755 --- a/.github/scripts/publish-nuget-packages.sh +++ b/.github/scripts/publish-nuget-packages.sh @@ -47,7 +47,9 @@ FEED_URL="${1:?Usage: publish-nuget-packages.sh }" # GeneratePackageOnBuild=false and IsPackable=false. `-ipath` keeps every path predicate honest # if a directory is ever renamed with different casing. find_packages() { - find . -type f -name "$1" -ipath '*/bin/Release/*' -not -ipath './tests/*' -not -ipath './samples/*' | sort + local pattern="$1" + + find . -type f -name "$pattern" -ipath '*/bin/Release/*' -not -ipath './tests/*' -not -ipath './samples/*' | sort } # Captured into a variable rather than piped into `mapfile` through a process substitution: @@ -55,12 +57,12 @@ find_packages() { # after a partial result would publish a subset of the packages and report success. # `set -o pipefail` makes the failing `find` fail the whole `find | sort` pipeline. if ! packages_found=$(find_packages '*.nupkg'); then - echo "::error::Package discovery failed while enumerating .nupkg files." + echo "::error::Package discovery failed while enumerating .nupkg files." >&2 exit 1 fi -if [ -z "$packages_found" ]; then - echo "::error::No Release .nupkg files were found. Refusing to report a successful publish." +if [[ -z "$packages_found" ]]; then + echo "::error::No Release .nupkg files were found. Refusing to report a successful publish." >&2 exit 1 fi @@ -78,7 +80,7 @@ if ! symbols_found=$(find_packages '*.snupkg'); then symbols_found='' fi -if [ -n "$symbols_found" ]; then +if [[ -n "$symbols_found" ]]; then mapfile -t symbols <<< "$symbols_found" for sym in "${symbols[@]}"; do echo "Publishing $sym"