fix: make pre-release deletion reachable when publishing a full release #30

Closed

heiko wants to merge 0 commits from fix-prerelease-cleanup-dead-code into master

heiko commented

2026-05-04 09:18:11 +02:00

Owner

Summary

cleanupOldReleases filtered with pre-release=false when publishing a full release, so the list never contained pre-releases — the !ri.PreRelease && release.PreRelease condition was permanently unreachable (dead code)
Fix: make a second ListReleases call with pre-release=true after the same-type asset-strip loop, and delete any pre-releases with a semver lower than the new full release
fakeAPI.ListReleases in tests now honours the pre-release filter value, so both calls return the correct subsets and the test genuinely exercises the real code path

Test plan

TestCleanupOldReleasesEmptyList — no panic on empty list (regression for #22)
TestCleanupOldReleasesKeepsNewest — assets stripped from older full releases, newest kept
TestCleanupOldReleasesPreReleasePromotion — pre-release deleted when full release is published (now exercises the live code path)
go test ./internal/cmd/release/... passes

Additional tests added during review

TestCleanupOldReleasesPublishingPreRelease — publishing a pre-release strips assets from older pre-releases but does not touch full releases
TestCleanupOldReleasesPreReleaseBoundary — pre-releases with version >= the new full release tag are kept; only strictly-older ones are deleted
TestCleanupOldReleasesNoAssets — older releases without assets are silently skipped
TestListReleases_PreReleaseFilter — verifies the pre-release query parameter actually reaches the Forgejo HTTP endpoint
golangci-lint run ./internal/... clean

🤖 Generated with Claude Code

## Summary - `cleanupOldReleases` filtered with `pre-release=false` when publishing a full release, so the list never contained pre-releases — the `!ri.PreRelease && release.PreRelease` condition was permanently unreachable (dead code) - Fix: make a **second `ListReleases` call** with `pre-release=true` after the same-type asset-strip loop, and delete any pre-releases with a semver lower than the new full release - `fakeAPI.ListReleases` in tests now honours the `pre-release` filter value, so both calls return the correct subsets and the test genuinely exercises the real code path ## Test plan - [x] `TestCleanupOldReleasesEmptyList` — no panic on empty list (regression for #22) - [x] `TestCleanupOldReleasesKeepsNewest` — assets stripped from older full releases, newest kept - [x] `TestCleanupOldReleasesPreReleasePromotion` — pre-release deleted when full release is published (now exercises the live code path) - [x] `go test ./internal/cmd/release/...` passes ## Additional tests added during review - [x] `TestCleanupOldReleasesPublishingPreRelease` — publishing a pre-release strips assets from older pre-releases but does not touch full releases - [x] `TestCleanupOldReleasesPreReleaseBoundary` — pre-releases with version >= the new full release tag are kept; only strictly-older ones are deleted - [x] `TestCleanupOldReleasesNoAssets` — older releases without assets are silently skipped - [x] `TestListReleases_PreReleaseFilter` — verifies the `pre-release` query parameter actually reaches the Forgejo HTTP endpoint - [x] `golangci-lint run ./internal/...` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code)

heiko added 1 commit

2026-05-04 09:18:11 +02:00

fix: make pre-release deletion reachable when publishing a full release 3d7fcde8b4

The ListReleases filter pre-release=false meant the pre-release deletion
branch (!ri.PreRelease && release.PreRelease) could never fire. Fix by
making a second ListReleases call with pre-release=true explicitly when
publishing a full release, and update fakeAPI in tests to honour the
filter so the two calls return the correct subsets.

Use releases[min(1, len(releases)):] to guard against an empty slice
without losing the path to the pre-release cleanup block.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

heiko added 2 commits

2026-05-04 09:35:15 +02:00

test: improve fakeAPI to distinguish DeleteRelease from DeleteAttachments and fix lint 5ee2a42251

Split fakeAPI.deleted into deletedReleases and strippedAssets so tests can
verify which API method was called. This catches regressions where an entire
release is deleted instead of just stripping assets, or vice versa.

Fix wsl_v5 lint violations in ListReleases with proper blank line spacing.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

docs: clarify min() usage and add (ai) markers per project convention e3b42f6670

Add a comment explaining how min(1, len(releases)):] safely handles both
empty and non-empty slices. Add (ai) markers to the modified godoc and
new inline comment to document AI involvement per CLAUDE.md convention.

heiko commented

2026-05-04 09:35:28 +02:00

Author

Owner

Code Review Summary

Ran a detailed review via the code-review-expert agent. Key findings:

Fixed

✓ wsl_v5 lint violations — blank line spacing in fakeAPI.ListReleases
✓ Test fidelity — split fakeAPI.deleted into deletedReleases and strippedAssets so tests can distinguish DeleteRelease from DeleteAttachments calls. This would catch the very regression the PR fixes.
✓ Code clarity — added comment explaining min(1, len(releases)): bounds-safe slice skipping
✓ Documentation — added (ai) markers per project convention

Status

All tests pass ✓
Lint clean ✓
3 commits: fix + test improvements + docs/clarity

Not addressed in this PR (noted for future)

Integration test for Forgejo API filter parameter (pre-release query param actually reaching endpoint)
Additional edge cases (pre-release publication, boundary comparison, etc.)

The core fix is solid and regressions are now covered by improved test fidelity.

## Code Review Summary Ran a detailed review via the code-review-expert agent. Key findings: ### Fixed - ✓ **wsl_v5 lint violations** — blank line spacing in `fakeAPI.ListReleases` - ✓ **Test fidelity** — split `fakeAPI.deleted` into `deletedReleases` and `strippedAssets` so tests can distinguish `DeleteRelease` from `DeleteAttachments` calls. This would catch the very regression the PR fixes. - ✓ **Code clarity** — added comment explaining `min(1, len(releases)):` bounds-safe slice skipping - ✓ **Documentation** — added (ai) markers per project convention ### Status - All tests pass ✓ - Lint clean ✓ - 3 commits: fix + test improvements + docs/clarity ### Not addressed in this PR (noted for future) - Integration test for Forgejo API filter parameter (`pre-release` query param actually reaching endpoint) - Additional edge cases (pre-release publication, boundary comparison, etc.) The core fix is solid and regressions are now covered by improved test fidelity.

heiko added 1 commit

2026-05-04 09:39:26 +02:00

test: add edge cases for pre-release cleanup and forgejo filter integration 30822d268e

Add three unit tests covering previously-untested branches in cleanupOldReleases:
- Publishing a pre-release: strips assets from older pre-releases but
  must not touch any full release (early-return path).
- Boundary check: pre-releases with version >= the new full release tag
  are kept; only strictly-older ones are deleted.
- Older releases without assets are silently skipped.

Add an integration test in the forgejo client that verifies the
pre-release filter actually reaches the HTTP endpoint as the
"pre-release" query parameter (not "prerelease" or similar). Extend
the test server's GET releases handler to honor the filter so the
client can be exercised end-to-end.

heiko closed this pull request

2026-05-04 11:17:57 +02:00