fix(github): handle existing release tag (idempotent publish)

[PrzemyslawKlys]

When GitHub release creation returns 422 (tag already exists), reuse the existing release and continue uploading assets.\n\nAlso skips asset uploads when GitHub returns 422 (asset name already exists), making reruns idempotent.

[claude]

Pull Request Review: Fix GitHub Release Tag Collision (Idempotent Publish)

Overview

This PR adds idempotency support to GitHub release publishing by handling 422 status codes for existing tags and assets. The implementation allows reruns to succeed rather than fail when releases or assets already exist.

Strengths

1. Well-Designed Idempotency Logic

• Correctly identifies "already_exists" validation errors by parsing the GitHub API response
• Handles both release creation and asset upload scenarios
• Gracefully falls back to fetching the existing release when tag already exists

2. Consistent Implementation

• The same pattern is applied to both SendGitHubReleaseCommand.cs and GitHubReleasePublisher.cs
• Code duplication is appropriate here since these appear to be separate modules with different purposes

3. Good Error Context

• Warning messages clearly communicate what happened
• Original error messages are preserved for non-idempotent failures

4. Proper Resource Management

• Uses using statements for HttpClient creation in new methods
• Follows existing patterns in the codebase

Code Quality Observations

Minor: Method Visibility Changes

Location: SendGitHubReleaseCommand.cs:165,233

The methods CreateRelease and UploadAssets were changed from static to instance methods. This is necessary to access WriteWarning(), but creates a slight inconsistency since other helper methods remain static.

Impact: Low - The change is necessary and does not affect functionality.

Minor: Empty Catch Block

Location: Both files in IsAlreadyExistsValidationError method (lines 316-318 and 246-248)

Concern: Silently swallows all exceptions during JSON parsing. While this is acceptable for the "already exists" check (where we default to re-throwing the original error), it could hide deserialization issues.

Recommendation: Consider adding a comment explaining why all exceptions are caught, or logging at debug level.

Security Considerations

No new security concerns identified:

• Token handling remains unchanged and follows existing patterns
• No new command injection or XSS vectors introduced
• URI escaping is properly applied to tag names

Performance Considerations

Additional API Call on Collision: When a release already exists, the code now makes an additional GET request to fetch the existing release.

Impact: Minimal - This only occurs on reruns where the release already exists, and is necessary to obtain the upload URL for asset uploads.

Test Coverage

No automated tests found for either file in the repository.

Recommendations:

1. Add unit tests for IsAlreadyExistsValidationError to verify correct parsing of validation errors
2. Add integration tests for idempotent rerun scenarios

Additional Observations

GetReleaseByTag Implementation

Both implementations are solid:

• Proper URI escaping of tag name
• Consistent error handling
• Validates that upload_url is not empty (critical for asset uploads)

Backwards Compatibility

Changes are fully backwards compatible - existing successful flows are unchanged, only the error handling path is enhanced.

Verdict

LGTM with minor suggestions - This is a well-implemented fix for a real CI/CD pain point. The idempotency logic is sound, the implementation is consistent across both modules, and no security or performance issues were identified.

Suggested Follow-ups (Non-blocking):

1. Add explanatory comment in the empty catch blocks
2. Consider adding unit tests for the validation error parsing logic
3. Document the idempotent behavior in the XML doc comments

Files Reviewed:

• PSPublishModule/Cmdlets/SendGitHubReleaseCommand.cs (+73/-2)
• PowerForge/Services/GitHubReleasePublisher.cs (+81/-0)

Total: +154/-2 lines across 2 files

[claude]

Pull Request Review: PR #93 - fix(github): handle existing release tag (idempotent publish)

Summary

This PR adds idempotency to GitHub release creation and asset uploads by gracefully handling the 422 status code when tags or assets already exist. The implementation allows CI/CD pipelines to safely retry release operations without failing.

✅ Strengths

1. Excellent Problem Solution

• The idempotency approach is well-suited for CI/CD retries and makes the release process more robust
• Clear business value: prevents build failures on reruns

2. Code Quality

• Consistent implementation across both files (SendGitHubReleaseCommand.cs and GitHubReleasePublisher.cs)
• Good error messages that explain what's happening
• Appropriate use of warnings instead of errors for idempotent cases
• Well-structured validation error parsing with defensive error handling

3. Error Handling

• The IsAlreadyExistsValidationError method has good defensive programming with try-catch
• Graceful fallback when JSON parsing fails
• Proper checking of both code and field to ensure accurate detection

🔍 Observations & Minor Suggestions

1. Method Visibility Changes (SendGitHubReleaseCommand.cs:165, 219, 233)

The methods CreateRelease, GetReleaseByTag, and UploadAssets were changed from static to instance methods. This is necessary to call WriteWarning, but it's worth noting:

• Impact: Minor - these are private methods
• Suggestion: Consider if this is intentional or if there's value in keeping them static and passing a warning delegate

2. Duplicate Code Between Files

The IsAlreadyExistsValidationError method and error response classes are duplicated between SendGitHubReleaseCommand.cs and GitHubReleasePublisher.cs.

• Impact: Medium - maintenance burden if GitHub changes error format
• Suggestion: Consider extracting to a shared utility class in a future refactor (not blocking for this PR)

3. Missing Validation Error Response Properties

The GitHubValidationErrorResponse class captures message and errors, but GitHub's validation error response may include additional fields like documentation_url.

• Impact: Low - current implementation is sufficient
• Consideration: The current approach is fine for this use case

4. Asset Upload Continuation Logic (SendGitHubReleaseCommand.cs:252-256, GitHubReleasePublisher.cs:166-170)

When an asset already exists, the code uses continue to skip to the next asset. This is correct but:

• Consideration: If a partial upload occurred previously, this will complete the remaining assets
• Behavior: Users should be aware that reruns don't re-upload existing assets (which is actually desirable)

🧪 Test Coverage

Concern: No unit tests found for the new functionality.

The changes introduce new code paths that should be tested:

1. Handling 422 response for existing release tag
2. Handling 422 response for existing asset name
3. Correct parsing of GitHub validation error responses
4. Behavior when error response format is invalid/unexpected

Recommendation: Consider adding integration or unit tests for:

• IsAlreadyExistsValidationError with various error response formats
• The idempotent retry behavior (could use mocked HttpClient responses)
• Edge cases: malformed JSON, missing fields, unexpected status codes

🔒 Security Considerations

No security concerns identified.

• Token handling remains unchanged and secure
• No new injection vectors introduced
• Error messages properly trim long responses (4000 char limit)
• URI escaping is properly used for tag names

🚀 Performance Considerations

No performance concerns identified.

• The additional API call to GetReleaseByTag only occurs on the 422 retry path
• Minimal overhead from error response parsing
• Shared HttpClient usage is appropriate (PowerForge)

📝 Additional Notes

1. Consistency in Comments

Both files use identical comments: "Idempotency: reruns frequently hit..." - this is good for maintainability.

2. GetReleaseByTag Implementation (SendGitHubReleaseCommand.cs:219-231, GitHubReleasePublisher.cs:125-143)

The new method correctly:

• Escapes the tag name in the URL
• Validates the response
• Extracts both HtmlUrl and UploadUrl
• Throws appropriate errors if the upload URL is missing

3. Backward Compatibility

This change is backward compatible - no breaking changes to public APIs.

✅ Recommendation

APPROVE with suggestions for follow-up

This PR is well-implemented and ready to merge. The suggestions above are minor improvements that could be addressed in future PRs:

1. Optional: Add unit tests for the new error handling paths
2. Optional: Consider extracting shared GitHub error handling logic to a common utility (future refactor)

The idempotency improvements are valuable and the implementation is solid. The code is production-ready.

---

Reviewed by Claude Code