Fix: Compare mode fails with 404 when target tag not yet created (#322)#324
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds compare-mode tag validation before ChangesCompare-mode tag validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds clearer, fail-fast handling for compare mode when required tags are missing, preventing a confusing AttributeError after a swallowed 404 from repo.compare().
Changes:
- Add compare-mode tag existence checks (via
repo.get_git_ref("tags/<name>")) before callingrepo.compare(). - Centralize the user-facing missing-tag error string and update docs/action metadata to state both tags must exist.
- Add unit tests covering compare-mode missing-tag exits and the new ActionInputs guard.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/release_notes_generator/test_action_inputs.py | Adds unit tests for validate_compare_mode_tag_names(). |
| tests/unit/release_notes_generator/data/test_miner.py | Adds end-to-end compare-mode exit tests and wires get_git_ref mocks. |
| release_notes_generator/utils/constants.py | Introduces COMPARE_MODE_TAG_MISSING_ERROR message constant. |
| release_notes_generator/data/miner.py | Adds compare-mode tag validation prior to repo.compare(). |
| release_notes_generator/action_inputs.py | Adds validate_compare_mode_tag_names() and calls it from validate_inputs(). |
| README.md | Documents compare-mode prerequisite that both tags exist. |
| docs/motivation.md | Updates “Fail Safe” description to include compare-mode tag prerequisite. |
| docs/features/compare_mode.md | Documents prerequisite and updates flow diagram accordingly. |
| CONTRIBUTING.md | Adds “Compare Mode Contract” section for contributors. |
| action.yml | Clarifies from-tag-name description and compare-mode activation/prerequisite. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@release_notes_generator/action_inputs.py`:
- Around line 690-693: The compare-mode validation in ActionInputs is
incorrectly gated on the normalized from_tag_name value, which skips the new
fail-fast check for blank or whitespace-only input. Update the conditional
around validate_compare_mode_tag_names() to depend on the raw from-tag-name
being present/provided rather than from_tag_name.strip(), so explicitly invalid
compare-mode inputs still trigger the clear input error while keeping the
existing prior-validation guard.
In `@tests/unit/release_notes_generator/data/test_miner.py`:
- Line 593: The inline comment inside the `_make_compare_miner` helper in
`test_miner.py` violates the test-file comment rule; remove it or fold the
intent into the surrounding code so no non-section comments remain outside test
methods. Use the surrounding helper logic in `_make_compare_miner` and the
`tag_exists` simulation to make the behavior self-explanatory without comments,
keeping only `# --- section ---` headers in `test_*.py` files.
In `@tests/unit/release_notes_generator/test_action_inputs.py`:
- Around line 191-199: The success-path test for
ActionInputs.validate_compare_mode_tag_names only verifies that sys.exit is not
called, but it should also assert that no error is logged. Update the test to
patch release_notes_generator.action_inputs.logger.error alongside sys.exit,
then assert the logger.error mock is not called when both tag names are set.
This keeps the test aligned with the “no exit, no error” contract and covers the
logging behavior of ActionInputs.validate_compare_mode_tag_names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f025c7dd-3428-4c27-830c-b324334e5126
📒 Files selected for processing (10)
CONTRIBUTING.mdREADME.mdaction.ymldocs/features/compare_mode.mddocs/motivation.mdrelease_notes_generator/action_inputs.pyrelease_notes_generator/data/miner.pyrelease_notes_generator/utils/constants.pytests/unit/release_notes_generator/data/test_miner.pytests/unit/release_notes_generator/test_action_inputs.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@release_notes_generator/action_inputs.py`:
- Around line 178-183: The docstring in the input-checking helper should match
the actual whitespace-only handling: it currently says “non-blank value,” but
the implementation intentionally treats whitespace-only input as explicitly
provided. Update the summary in the helper used by validate_inputs() and the
from-tag-name input logic to describe “explicitly provided” raw input without
implying it is already valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a13fc02d-9b9e-451a-a454-f0349cb5fbe7
📒 Files selected for processing (4)
release_notes_generator/action_inputs.pyrelease_notes_generator/utils/constants.pytests/unit/release_notes_generator/data/test_miner.pytests/unit/release_notes_generator/test_action_inputs.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/release_notes_generator/data/test_miner.py
…d improve tag validation logging
There was a problem hiding this comment.
🧹 Nitpick comments (2)
release_notes_generator/data/miner.py (1)
166-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAvoid introducing another
sys.exit()path inDataMiner.Lines 184 and 192 make this leaf module terminate the process directly. Raising a typed error here would keep compare-mode failures composable and preserve exit-code translation at the entry point instead of burying everything behind
SystemExit. As per coding guidelines: "Prefer raising exceptions in leaf modules and translating failures to action failure output / exit codes at the entry point".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@release_notes_generator/data/miner.py` around lines 166 - 193, The _validate_tag_exists method in DataMiner is terminating the process directly with sys.exit(), which should be avoided in this leaf module. Replace both exit paths in _validate_tag_exists with a typed exception that carries the validation context, and let the entry point handle translating that failure into the appropriate exit code. Keep the existing logging in place, but make sure compare-mode callers can catch and compose the error rather than having Repository tag validation end the process inside this method.Source: Coding guidelines
tests/unit/release_notes_generator/data/test_miner.py (1)
829-843: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing second-tag error-path cases.
These tests only exercise non-404 and unexpected exceptions on the first
get_git_ref()call. Because validation is sequential, a regression in the second-tag branch would still pass this suite. Please add theto_tagequivalents for those two scenarios. As per coding guidelines: "Cover all distinct combinations in tests; each test must state its scenario in the docstring".Also applies to: 860-873
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/release_notes_generator/data/test_miner.py` around lines 829 - 843, The compare-mode from-tag error coverage is incomplete because it only tests the first get_git_ref() validation path; add matching to_tag scenarios for the non-404 GitHubException case and the unexpected exception case so a regression in the second-tag branch is caught. Update the relevant tests in test_miner.py around test_mine_data_compare_mode_from_tag_api_error_exits and the neighboring exception-case test, using _make_compare_miner, mock_repo.get_git_ref, and logger.error, and make each new test’s docstring clearly state the specific scenario it covers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@release_notes_generator/data/miner.py`:
- Around line 166-193: The _validate_tag_exists method in DataMiner is
terminating the process directly with sys.exit(), which should be avoided in
this leaf module. Replace both exit paths in _validate_tag_exists with a typed
exception that carries the validation context, and let the entry point handle
translating that failure into the appropriate exit code. Keep the existing
logging in place, but make sure compare-mode callers can catch and compose the
error rather than having Repository tag validation end the process inside this
method.
In `@tests/unit/release_notes_generator/data/test_miner.py`:
- Around line 829-843: The compare-mode from-tag error coverage is incomplete
because it only tests the first get_git_ref() validation path; add matching
to_tag scenarios for the non-404 GitHubException case and the unexpected
exception case so a regression in the second-tag branch is caught. Update the
relevant tests in test_miner.py around
test_mine_data_compare_mode_from_tag_api_error_exits and the neighboring
exception-case test, using _make_compare_miner, mock_repo.get_git_ref, and
logger.error, and make each new test’s docstring clearly state the specific
scenario it covers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2468c00b-14a2-4b58-a90b-b8372840483a
📒 Files selected for processing (8)
CONTRIBUTING.mdREADME.mddocs/features/compare_mode.mddocs/motivation.mdrelease_notes_generator/action_inputs.pyrelease_notes_generator/data/SPEC.mdrelease_notes_generator/data/miner.pytests/unit/release_notes_generator/data/test_miner.py
✅ Files skipped from review due to trivial changes (3)
- docs/motivation.md
- README.md
- docs/features/compare_mode.md
🚧 Files skipped from review as they are similar to previous changes (2)
- CONTRIBUTING.md
- release_notes_generator/action_inputs.py
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
Please see comments.
… and improving validation logic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/release_notes_generator/test_action_inputs.py`:
- Around line 220-229: The test for ActionInputs.validate_inputs is too
permissive because it only patches ActionInputs.is_from_tag_name_defined to
return True, so it won’t catch a regression where the helper is called without
use_raw=True. Update the test to assert the patched
ActionInputs.is_from_tag_name_defined helper was called with use_raw=True during
validate_inputs(), alongside the existing logger.error and sys.exit checks, so
the raw-mode contract is explicitly enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17835c68-b0f3-4ad3-854a-a65f12c6fb22
📒 Files selected for processing (4)
README.mdrelease_notes_generator/action_inputs.pyrelease_notes_generator/data/miner.pytests/unit/release_notes_generator/test_action_inputs.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- release_notes_generator/data/miner.py
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
Please see comments.
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
In following three comments I suggest way simpler way how to do the same check (without creating extra mode for one validation line). I find yours just little to much confusing. It is up to you, if you want to implement it, else I will close my comments and will provide an approve. Thanks
miroslavpojer
deleted the
bugfix/322-Compare-Mode-Fails-with-404-When-Target-Tag-Not-Yet-Created-2
branch
Problem
When
from-tag-nameis provided and either tag does not exist in the repository,repo.compare()returns a GitHub 404 which is swallowed by_safe_call, leavingcomparisonasNone. The subsequentlist(comparison.commits)call then raisesan
AttributeError, producing a confusing crash instead of a clear action failure.Solution
Add fail-fast tag-existence validation before the compare API is called:
DataMiner._validate_tag_exists(repo, tag_name)— callsrepo.get_git_ref("tags/<name>")via the existing
_safe_callwrapper; returnsFalseif the ref is absent or the API errors.DataMiner._validate_compare_mode_tags(repo)— iterates bothfrom-tag-nameandtag-name;logs a clear error and exits with code 1 on the first missing tag.
ActionInputs.validate_compare_mode_tag_names()— input-level guard (called fromvalidate_inputs()) that exits early if either tag input is an empty string, before anyAPI call is made.
COMPARE_MODE_TAG_MISSING_ERRORconstant centralises the user-facing error string.if comparison is Noneguard afterrepo.compare()is retained to handlenon-404 API failures (network errors, rate-limit exhaustion, etc.) that can occur
even after both tags are confirmed to exist.
Tests
test_mine_data_compare_mode_exits_when_target_tag_missing— end-to-end:mine_dataexits and
repo.compareis never called whentag-nameref is absent.test_mine_data_compare_mode_exits_when_from_tag_missing— same forfrom-tag-name.test_validate_compare_mode_tag_names_*— three unit tests for theActionInputsinput-level guard (both set, empty
tag-name, emptyfrom-tag-name).Docs / contract
action.yml— updatedfrom-tag-namedescription to state both tags must exist.README.md,docs/features/compare_mode.md,docs/motivation.md— added prerequisitenote; removed an unimplemented "list of recent tags" promise.
CONTRIBUTING.md— added compare mode contract section for future contributors.Release Notes
tag-nameandfrom-tag-nameare validated before any API call is madeCloses #322
Update 2026-06-29
Correction to Solution section (as-implemented, ref f5e6830):
The Solution description above does not match the code that was merged. Actual behaviour:
DataMiner._validate_tag_exists(repo, tag_name)— callsrepo.get_git_ref(f"tags/{tag}")directly in atry/except GithubExceptionblock (not via_safe_call). On a 404 or any other error it logs the specific failure and callssys.exit(1). It does not return a boolean and does not use the_safe_callwrapper.DataMiner._validate_compare_mode_tags(repo)— this helper was not added. Instead,_handle_compare_modecallsself._validate_tag_exists(repo, from_tag)andself._validate_tag_exists(repo, to_tag)directly, in sequence.COMPARE_MODE_TAG_MISSING_ERRORconstant — was not added; error strings are inlined in_validate_tag_exists.ActionInputs.validate_compare_mode_tag_names()— implemented as described; validates non-empty strings and exits before any API call.Summary by CodeRabbit
Documentation
Bug Fixes
tag-nameandfrom-tag-nameexist before calling the compare API, and exits early with a clear, tag-specific error if either is absent.from-tag-namein compare mode.