fix(github-actions): Publish the packages the build actually produces - #44
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
🤖 CodeAnt AI — Review Status
|
Reviewer's GuideThis PR fixes the GitHub Actions NuGet publish steps so they enumerate and publish the actual built packages, fail loudly when none are found, and avoid publishing test/sample outputs, using Flow diagram for safe NuGet package publishingflowchart TD
A[Build workflow produces package files] --> B[find . for .nupkg files]
B --> C{Packages found?}
C -->|No| D[Emit GitHub error and exit 1]
C -->|Yes| E[Exclude tests and samples]
E --> F[dotnet nuget push each package]
F --> G[Find .snupkg files]
G --> H[Best-effort push symbol packages]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Bito Automatic Review Skipped - Branch Excluded |
|
Warning Review limit reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Comment |
codeant-ai
Bot
added
the
size:M
Up to standards ✅🟢 Issues
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/build-dotnet.yml" line_range="238" />
<code_context>
+ # `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
</code_context>
<issue_to_address>
**issue (bug_risk):** The `find` process runs inside process substitution, so its exit status is not propagated to `mapfile`. If `find` encounters an error after producing a partial result, the workflow publishes only the partial package list and reports success instead of failing.
**Triggers:** When filesystem traversal fails after discovering at least one package.
**Suggested fix:** Run `find` into a temporary file or otherwise capture and check its exit status before populating `PACKAGES`.
</issue_to_address>
### Comment 2
<location path=".github/workflows/build-dotnet.yml" line_range="232-237" />
<code_context>
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)
+
</code_context>
<issue_to_address>
**nitpick:** The comment claims that the sample solution produces a demo application package because `Directory.Build.props` packs every non-test project, but `samples/SampleApp/Directory.Build.props` explicitly sets `GeneratePackageOnBuild=false` and `IsPackable=false`. The explanation therefore misstates why `samples/` must be excluded and can mislead future changes to the workflow.
**Suggested fix:** Update the comment to describe the actual sample build/package behavior, or remove the claim that the demo application itself is packaged.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the file selection or exclusion rules are wrong, this can publish unintended NuGet packages to GitHub Packages, where external consumers may download them before the workflow is reverted. Reverting stops future publishes but does not reliably undo packages already published or any downstream consumption.
Blocking findings: .github/workflows/build-dotnet.yml:238
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
While this PR improves package discovery by moving to a recursive search, there are implementation risks that may lead to the accidental publishing of test or sample artifacts. The current approach uses case-sensitive directory patterns (./tests/ and ./samples/) which typically do not match standard .NET PascalCase naming conventions on Linux runners. Additionally, the publishing logic is duplicated across multiple workflow steps and triggers, increasing the maintenance surface. Codacy analysis indicates the changes are otherwise up to standards, but these logic issues should be addressed to ensure the integrity of the published packages.
About this PR
- The CI workflow contains significant duplication of the publishing logic. To prevent future drift and ensure consistent filtering (like the exclusion of test projects), consider moving the package discovery and 'dotnet nuget push' logic into a shared shell script or a local GitHub Action.
Test suggestions
- Recursive discovery of packages in deep directory structures
- Exclusion of packages matching '-path ./tests/' or '-path ./samples/'
- Workflow failure when zero '.nupkg' files are found by the find command
- Optional publishing of '.snupkg' files where failure to push does not stop the workflow
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Recursive discovery of packages in deep directory structures
2. Exclusion of packages matching '-path ./tests/*' or '-path ./samples/*'
3. Workflow failure when zero '.nupkg' files are found by the find command
4. Optional publishing of '.snupkg' files where failure to push does not stop the workflow
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
codeant-ai
Bot
added
size:M
|
Bito Automatic Review Skipped - Branch Excluded |
1 similar comment
|
Bito Automatic Review Skipped - Branch Excluded |
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/<Project>/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 <local feed> --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
…ript 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
… 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
78feea1 to
1f71a62
Compare
codeant-ai
Bot
added
size:M
There was a problem hiding this comment.
Pull request overview
Fixes GitHub Packages publishing so nested build outputs are discovered reliably and empty publishes fail clearly.
Changes:
- Centralizes package discovery and publishing in a strict shell script.
- Excludes test and sample packages.
- Enforces LF line endings for shell scripts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/build-dotnet.yml |
Uses the shared publisher for main and PR builds. |
.github/scripts/publish-nuget-packages.sh |
Discovers, validates, and publishes packages. |
.gitattributes |
Enforces LF endings for shell scripts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Review Agent Run #013a4dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KECFm7givf8zoQi2hJmCiN
…cript 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KECFm7givf8zoQi2hJmCiN
|
Code Review Agent Run #ecda14Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
User description
Summary
Both GitHub Packages publish steps would have reported success while publishing nothing. This has never been seen because the step is gated on
refs/heads/mainand nothing has been pushed there — the first push, which is what merging #11 does, would have been the first occurrence.Found by GitHub Copilot's PR reviewer on #11. Closes #43.
The defect
**does not recurse.globstaris off by default and GitHub runs steps withbash -ewithout enabling it, so the pattern is equivalent to./*/*.nupkg— one directory level. Packages are produced atsrc/Spectre/<Project>/bin/Release/, four levels down.Verified against a fixture mirroring the real layout:
And the failure is silent. An unmatched glob is passed through literally, and
dotnet nuget pushgiven a non-matching literal exits0without publishing:No error, no warning, green step, zero packages — and #7 would have looked satisfied.
Why not just enable globstar
That swaps one defect for another.
Directory.Build.propssetsGeneratePackageOnBuild=truefor every non-test project, and the workflow builds the sample solution in Release at line 175, sosamples/SampleApp/src/SampleApp/bin/Release/*.nupkgexists on the runner. A recursive glob would publish the demo application to the feed.The fix
Both steps enumerate with
find, excludingtests/andsamples/— the same shaperelease.ymlalready uses — and fail when nothing is found:Symbol packages stay best-effort — a missing
.snupkgis not a failed release — but are enumerated the same way rather than globbed.Testing
Workflow YAML parses, and no
dotnet nuget pushcommand containing**remains in either step. Behaviour verified against a fixture with two library packages, one sample package and one test package:No source code is touched, so build and tests are unaffected.
Related
mainSummary by Sourcery
Make GitHub Actions publish the packages actually produced by the build and fail clearly when package discovery yields no publishable artifacts.
Bug Fixes:
Enhancements:
CI:
CodeAnt-AI Description
Publish the packages produced by the build reliably
What Changed
Impact
✅ Fewer empty successful publishes✅ Only intended library packages reach GitHub Packages✅ Clearer package publishing failures💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by Bito
This PR makes GitHub Actions publish the intended NuGet artifacts by centralizing package discovery and restricting regular packages to Release outputs. It also preserves optional symbol publishing behavior and enforces Linux-compatible shell-script line endings for CI.
Detailed Changes