Fix: Compare mode fails with 404 when target tag not yet created (#322)#324
Conversation
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 docstrings
🧪 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.
| ActionInputs.get_from_tag_name(), | ||
| ActionInputs.get_tag_name(), | ||
| ) | ||
| comparison = self._safe_call(repo.compare)(ActionInputs.get_from_tag_name(), ActionInputs.get_tag_name()) | ||
| from_tag = ActionInputs.get_from_tag_name() | ||
| to_tag = ActionInputs.get_tag_name() |
There was a problem hiding this comment.
You are running the same method twice under each other. Please optimize it.
There was a problem hiding this comment.
Code improved.
| tag = ActionInputs.get_tag_name() | ||
| if not tag.strip(): |
There was a problem hiding this comment.
I love to put := everywhere is possible. Not needed, but suggestion 😄 :
| tag = ActionInputs.get_tag_name() | |
| if not tag.strip(): | |
| if not tag := ActionInputs.get_tag_name().strip(): |
There was a problem hiding this comment.
propose used.
| sys.exit(1) | ||
| from_tag = ActionInputs.get_from_tag_name() | ||
| if not from_tag.strip(): | ||
| logger.error("'from-tag-name' must not be empty when running in compare mode. Ending!") | ||
| sys.exit(1) |
There was a problem hiding this comment.
I am really confused here, because you created extra method, that is is_from_tag_name_provided and when the result of this method is True, then you check it here once again. but if ActionInputs.get_from_tag_name(). For me it is the same logic used twice and this whole method is just checking if tag is provided. But if we are in the compare mode, we miss the info that "Existing tag has to be created before RN Generator is started"
| ## Compare Mode Contract | ||
|
|
||
| When contributing changes that touch compare mode (the `from-tag-name` path): | ||
| - Both `tag-name` and `from-tag-name` must exist as git tags before the compare API is called. |
There was a problem hiding this comment.
This is the first time, there is mentioned this crucial info. Should be in the error message, when the run failes.
|
|
||
| You can integrate this Action with your release process. | ||
|
|
||
| ### Example — Manual Release Dispatch |
There was a problem hiding this comment.
Maybe it would be great if this example would get updated and clearly add, that tag creation step has to be before the generate release notes one.
| | `GITHUB_TOKEN` | GitHub token for authentication | Yes | - | | ||
| | `tag-name` | Target tag for the release | Yes | - | | ||
| | `chapters` | YAML multi-line list mapping titles to labels (supports `label` or `labels`) | No | - | | ||
| | `verbose` | Enable detailed logging | No | false | |
There was a problem hiding this comment.
Missing the from-tag-name input a little bit. Understand, that it is in the configuration-reference file probably, but the input is mentioned right under, so it would be nice to have it in this input table as well.
| # Compare mode: validate both tag names are non-empty strings. | ||
| # Only runs when all prior validations passed to avoid cascading errors. | ||
| if not errors and ActionInputs.is_from_tag_name_provided(): | ||
| ActionInputs.validate_compare_mode_tag_names() |
There was a problem hiding this comment.
This whole block could be added after errors section at line 707, since it is maybe better to write errors first (wrong config) and then look at the missing inputs. This if statement will not have to check if not errors (for me a sign, that it should be under). What do you think?
There was a problem hiding this comment.
Removed usage of this method.
| return value.strip() != "" | ||
|
|
||
| @staticmethod | ||
| def is_from_tag_name_provided() -> bool: |
There was a problem hiding this comment.
There already exists a method called: is_from_tag_name_defined. Can you please look at it?
… 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
| mocker.patch( | ||
| "release_notes_generator.action_inputs.ActionInputs.is_from_tag_name_defined", | ||
| return_value=True, | ||
| ) | ||
| try: | ||
| mock_error = mocker.patch("release_notes_generator.action_inputs.logger.error") | ||
| mock_exit = mocker.patch("sys.exit") | ||
| ActionInputs.validate_inputs() | ||
| assert any("from-tag-name" in str(c) for c in mock_error.call_args_list) | ||
| mock_exit.assert_called_once_with(1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the raw-mode call contract.
This test still passes if validate_inputs() regresses to calling is_from_tag_name_defined() without use_raw=True, because the patch returns True for any invocation. Please assert the patched helper was called with use_raw=True so the new compare-mode behavior is actually locked in.
Suggested fix
- mocker.patch(
+ mock_is_from_tag_name_defined = mocker.patch(
"release_notes_generator.action_inputs.ActionInputs.is_from_tag_name_defined",
return_value=True,
)
@@
mock_error = mocker.patch("release_notes_generator.action_inputs.logger.error")
mock_exit = mocker.patch("sys.exit")
ActionInputs.validate_inputs()
+ mock_is_from_tag_name_defined.assert_called_once_with(use_raw=True)
assert any("from-tag-name" in str(c) for c in mock_error.call_args_list)
mock_exit.assert_called_once_with(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mocker.patch( | |
| "release_notes_generator.action_inputs.ActionInputs.is_from_tag_name_defined", | |
| return_value=True, | |
| ) | |
| try: | |
| mock_error = mocker.patch("release_notes_generator.action_inputs.logger.error") | |
| mock_exit = mocker.patch("sys.exit") | |
| ActionInputs.validate_inputs() | |
| assert any("from-tag-name" in str(c) for c in mock_error.call_args_list) | |
| mock_exit.assert_called_once_with(1) | |
| mock_is_from_tag_name_defined = mocker.patch( | |
| "release_notes_generator.action_inputs.ActionInputs.is_from_tag_name_defined", | |
| return_value=True, | |
| ) | |
| try: | |
| mock_error = mocker.patch("release_notes_generator.action_inputs.logger.error") | |
| mock_exit = mocker.patch("sys.exit") | |
| ActionInputs.validate_inputs() | |
| mock_is_from_tag_name_defined.assert_called_once_with(use_raw=True) | |
| assert any("from-tag-name" in str(c) for c in mock_error.call_args_list) | |
| mock_exit.assert_called_once_with(1) |
🤖 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/test_action_inputs.py` around lines 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.
tmikula-dev
left a comment
tmikula-dev
left a comment
There was a problem hiding this comment.
Please see comments.
|
|
||
| ## Compare Mode Contract | ||
|
|
||
| When contributing changes that touch compare mode (the `from-tag-name` path): |
There was a problem hiding this comment.
| When contributing changes that touch compare mode (the `from-tag-name` path): | |
| When contributing changes that touch compare mode (once `from-tag-name` input is provided): |
| - Input-level check: `ActionInputs.validate_compare_mode_tag_names()` (called from `validate_inputs()`) exits with code 1 if either input string is empty. | ||
| - Tag-existence check: `DataMiner._handle_compare_mode()` calls `repo.get_git_ref("tags/<tag>")` for both tags and exits with code 1 naming the absent tag if either lookup fails. | ||
| - Any change to this validation logic must update `docs/features/compare_mode.md` and the tests in `tests/unit/release_notes_generator/data/test_miner.py`. |
There was a problem hiding this comment.
Why is please in the CONTRIBUTING file described, how the tags are checked? Other topics in this file are named as Branch Naming or Proposing New Features. This described section looks here odd to me.
| return value.strip() != "" | ||
| if use_raw: | ||
| return get_action_input(FROM_TAG_NAME, default="") != "" | ||
| return ActionInputs.get_from_tag_name().strip() != "" |
There was a problem hiding this comment.
Can you please explain to me the reason of having this use_raw switch? If this whole workaround is for user not to be able to add a whitespace only as the from-tag input, I feel like we do to much for a bad config.
From input is set as a whitespace -> comparison happens, the tag called whitespace is not exisiting -> error. This is too confusing for not almost existing issue.
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.