Fix : Refactored Git command error handling #471
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes JuliaRegistries/TagBotErrorReports#161 - git push failures were not providing enough diagnostic information. Now both stdout and stderr are logged as ERROR level and included in the Abort exception message to help diagnose why pushes are failing. Changes: - Changed logger.info to logger.error for command failures - Include full error details in Abort exception message - This will help users and maintainers diagnose permission issues, authentication failures, or other git push problems
Fixes JuliaRegistries#470 - The previous change included stdout/stderr in the Abort exception message, which broke error collation in TagBotErrorReports because each error had unique details. Now: - stdout/stderr are still logged at ERROR level for visibility - Exception message remains consistent (no stdout/stderr included) - This allows TagBotErrorReports to properly deduplicate similar errors - Users still get detailed diagnostics in the logs
Fixes JuliaRegistries#470 - stdout/stderr details in exception messages prevent TagBotErrorReports from properly collating similar errors. Changes: - stdout/stderr are logged at ERROR level (visible in logs) - Exception message stays consistent (no stdout/stderr appended) - This allows error deduplication to work correctly - Updated test to verify new behavior The detailed output is still available in logs for debugging, but the exception stacktraces remain identical for the same type of error, allowing proper grouping in TagBotErrorReports.
arnavk23
changed the title
Collaborator
Author
|
@IanButterworth Please take a look at this with the context of #470 |
Member
|
I think it would be a shame to lose the stderr info from those bug reports.. at least before we've tried to improve user actionability of the error reporting/manual intervention issues. I'd hold off fixing #470 until we're happy that's as good as it can be. |
- Git: include stderr in Abort messages and add concise hints for common failures; sanitize tokens; only read stderr on error to keep tests stable. - Datetime: add robust parse_git_datetime and use it in time_of_commit and commit datetime caching; normalize offsets and support multiple git formats. - Release: gracefully skip 422 already_exists; clearer guidance for 403 resource not accessible and 401 bad credentials. - Changelog: avoid annotated-tag date failures via robust parsing path. - Tests: update test_git expectations; add workflow hint coverage and datetime fallback cases; add test_repo for existing release error; fix flake8 long lines in test_changelog. - Alignment with PR JuliaRegistries#445: backfill all versions each run, deprecated lookback has no effect; latest release chosen by commit time; registry PR and commit caches; performance metrics. All tests pass: 165 passed.
- Update _changelog fixture to mock Github instance and get_repo - Mock get_all_tags and _build_tags_cache to prevent real API calls - Fix test_previous_release to mock get_all_tags and get_release - Fix test_previous_release_subdir similarly - Mock is_version_yanked in test_collect_data to prevent registry access - Format test_changelog.py with black - All 166 tests pass
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses error deduplication issues in TagBotErrorReports by restructuring Git command error messages to have a consistent format while still providing detailed diagnostic information through logging.
Key Changes:
- Refactored Git command error handling to log stdout/stderr separately while maintaining a consistent exception message format
- Introduced a new
parse_git_datetimefunction to handle various Git date formats with robust fallback parsing - Added user-friendly hints for common Git errors to improve debugging experience
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tagbot/action/git.py | Added parse_git_datetime function for robust date parsing, restructured command method to separate logging from exception messages, and added _hint_for_failure method for contextual error hints |
| tagbot/action/repo.py | Integrated parse_git_datetime in _build_commit_datetime_cache, enhanced create_release with better error handling for 401/403/422 status codes, and improved handle_error with 401 handling |
| test/action/test_git.py | Updated test assertions to match new error message format and added tests for fallback date formats and hint functionality |
| test/action/test_repo.py | Added test for handling existing release errors during creation |
| test/action/test_changelog.py | Updated mock setup to be more consistent and maintainable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tagbot/action/repo.py
Outdated
Comment on lines
1503
to
1514
| if e.status == 403 and "resource not accessible" in str(e).lower(): | ||
| logger.error( | ||
| "Release creation blocked: token lacks required permissions. " | ||
| "Use a PAT with contents:write (and workflows if tagging " | ||
| "workflow changes)." | ||
| ) | ||
| if e.status == 401: | ||
| logger.error( | ||
| "Release creation failed: bad credentials. Refresh the token or " | ||
| "use a PAT with repo scope." | ||
| ) | ||
| raise |
There was a problem hiding this comment.
The new error handling for 401 and 403 status codes in the create_release method lacks test coverage. While these branches log helpful error messages before re-raising the exception, they should be tested to ensure the error messages are logged correctly and the exception is properly re-raised.
Comment on lines
+133
to
+145
| def _hint_for_failure(self, detail: str) -> Optional[str]: | ||
| """Return a user-facing hint for common git errors.""" | ||
| lowered = detail.casefold() | ||
| if "permission to" in lowered and "denied" in lowered: | ||
| return "use a PAT with contents:write or a deploy key" | ||
| if "workflow" in lowered or "workflows" in lowered: | ||
| if "refusing" in lowered or "permission" in lowered: | ||
| return "provide workflow scope or avoid workflow changes" | ||
| if "publickey" in lowered or "permission denied (publickey)" in lowered: | ||
| return "configure SSH deploy key or switch to https with PAT" | ||
| if "bad credentials" in lowered or "authentication failed" in lowered: | ||
| return "token is invalid or lacks access" | ||
| return None |
There was a problem hiding this comment.
The new _hint_for_failure method has multiple conditional branches for different error scenarios (permission denied, publickey, bad credentials), but the test suite only covers the workflow permission scenario. Consider adding tests for the other hint branches to ensure comprehensive test coverage.
arnavk23
changed the title
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Add test_create_release_handles_403_error: verifies permission error message and exception re-raising when token lacks contents:write scope - Add test_create_release_handles_401_error: verifies authentication error message and exception re-raising when credentials are invalid - Both tests verify correct error messages are logged before re-raising - Tests use @patch decorator to mock logger and verify logging calls - All 168 tests now passing
…ey, bad credentials
…test to construct with data - Add recursion guard and depth cap in parse_git_datetime to prevent infinite recursion when offset normalization has no effect - Add test for invalid yet regex-matching datetime to ensure None and no recursion - Fix test_create_release_handles_existing_release_error to construct GithubException with data instead of setting read-only .data - All tests passing (172)
Collaborator
Author
|
@IanButterworth tried again. |
No functional change - HTTP status codes are mutually exclusive, so elif simply makes the code intent clearer to readers.
IanButterworth
approved these changes
Jan 7, 2026
Member
IanButterworth
left a comment
IanButterworth
left a comment
There was a problem hiding this comment.
Worth giving a go! Thanks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem is that by including stdout/stderr in the exception message, each error has unique details that prevent the error deduplication system from grouping similar errors together.
This PR addresses error deduplication issues in TagBotErrorReports by restructuring Git command error messages to have a consistent format while still providing detailed diagnostic information through logging.
Fixes #470