chore: harden pr-check-changelog script #1513
Conversation
|
Hi @Pikolosan, this is **LinkBot** 👋
Linking pull requests to issues helps us significantly with reviewing pull requests and keeping the repository healthy. 🚨 This pull request does not have an issue linked. Please link an issue using the following format: 📖 Guide: If no issue exists yet, please create one: Thanks! |
WalkthroughReworks Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/pr-check-changelog.sh (1)
78-90: Consider validating required environment variable.Per coding guidelines, scripts should validate all required environment variables.
GITHUB_REPOSITORYis documented as required (line 61) but not validated before use. Whileset -uwill now catch unset variables, an explicit check provides a clearer error message.♻️ Suggested validation
CHANGELOG="CHANGELOG.md" +# Validate required environment variable +if [[ -z "${GITHUB_REPOSITORY:-}" ]]; then + echo -e "${RED}❌ GITHUB_REPOSITORY environment variable is not set.${RESET}" + exit 1 +fi + # ANSI color codes RED="\033[31m"
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @Pikolosan
Adding set -euo pipefail could be a good addition, but this changelog script is deliberately generating non zero exit statuses, to deliberately log failures
I think set -euo pipefail will exit if any non zero exit status is found
It might still be a good idea to add it but i'd want to see:
- handling of deliberate non zero codes
and i'd want to see this tested quite a bit on a fork, because this is one of our most used workflows and it will prevent merging for users if it breaks
docs/sdk_developers/training/testing_forks.md
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@Pikolosan This is looking good!!
If you have questions on testing please reach out. We are happy to assist!
|
Hey, yes i am trying to test the script and wanted to ask some related questions. Test Case : Wrong place entries, under release section ( Wrong behaviour of code - As does it does not track if entry placed under release section) ~/Documents/CODE/OpenSouce/FirstCommit/hiero-sdk-python (chore/harden-pr-check-changelog)
$ bash .github/scripts/pr-check-changelog.sh
=== Raw git diff of CHANGELOG.md against upstream/main ===
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ba2fab..2c73fbb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -236,7 +236,7 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Support for message chunking in `TopicSubmitMessageTransaction`.
### Changed
-
+- Hardened PR changelog check script by enabling strict Bash mode. (#1492)
- bot workflows to include new changelog entry
- Removed duplicate import of transaction_pb2 in transaction.py
- Refactor `TokenInfo` into an immutable dataclass, remove all setters, and rewrite `_from_proto` as a pure factory for consistent parsing [#800]
=================================
✅ Changelog check passed.Test Case : Adding a changelog entry under unreleased but not under sub category. ~/Documents/CODE/OpenSouce/FirstCommit/hiero-sdk-python (chore/harden-pr-check-changelog)
$ bash .github/scripts/pr-check-changelog.sh
=== Raw git diff of CHANGELOG.md against upstream/main ===
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ba2fab..81ab70e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,7 @@ This project adheres to [Semantic Versioning](https://semver.org).
This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
''## [Unreleased]''
-
+- Hardened PR changelog check script by enabling strict Bash mode. (#1492)
'' ### Tests"
- Improve unit test coverage for `Hbar`, including edge cases, validation, comparisons, and hashing. (#1483)
@@ -236,7 +236,6 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Support for message chunking in `TopicSubmitMessageTransaction`.
### Changed
-
- bot workflows to include new changelog entry
- Removed duplicate import of transaction_pb2 in transaction.py
- Refactor `TokenInfo` into an immutable dataclass, remove all setters, and rewrite `_from_proto` as a pure factory for consistent parsing [#800]
=================================
✅ Changelog check passed.The logic keeps accepting them instead of failing. |
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@Pikolosan This is great work!
Thanks for sharing the script and test results — after reviewing the implementation, the behavior you’re seeing is explained by the current logic.
Although the header comments state that the script enforces correct placement under [Unreleased] and a category subtitle, the implementation does not fully achieve that:
-
current_subtitle persists until another ### or ## header is encountered, which allows dangling bullets to be incorrectly classified as valid.
-
Added bullets are matched against the full file by exact string equality, without verifying their actual location in the diff hunk, which can allow misplaced entries to pass.
These are existing logic issues, but they become more concerning with the addition of set -euo pipefail. The script intentionally relies on commands that may return non-zero exit codes, and without refactoring or explicit guarding, strict mode risks premature exits in a workflow that blocks merges.
Before merging this change, I think we need:
-
Explicit handling of expected non-zero exit paths (or refactoring to be set -e safe)
-
Validation that subtitle scoping is reset correctly to avoid false positives
-
Testing via forked workflows per docs/sdk_developers/training/testing_forks.md
What do you think?
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
|
Yes, I agree with your assessment. Adding set -euo pipefail makes sense, but the current script logic needs to be hardened first. As you pointed out, current_subtitle scoping and matching added bullets by string equality rather than diff location can lead to false positives, which becomes riskier under strict mode. If you’re willing to harden the logic and test properly, I’m open to this. |
|
@aceppaluni and @exploreriii #!/bin/bash
set -uo pipefail
CHANGELOG="CHANGELOG.md"
# ANSI color codes
RED="\033[31m"
GREEN="\033[32m"
YELLOW="\033[33m"
RESET="\033[0m"
failed=0
# Fetch upstream
git remote get-url upstream &>/dev/null || git remote add upstream https://github.com/${GITHUB_REPOSITORY}.git
git fetch upstream main >/dev/null 2>&1
# Get raw diff (git diff may legitimately return non-zero)
raw_diff=$(git diff upstream/main -- "$CHANGELOG" || true)
# Show raw diff with colors
echo "=== Raw git diff of $CHANGELOG against upstream/main ==="
while IFS= read -r line; do
if [[ $line =~ ^\+ && ! $line =~ ^\+\+\+ ]]; then
echo -e "${GREEN}$line${RESET}"
elif [[ $line =~ ^- && ! $line =~ ^--- ]]; then
echo -e "${RED}$line${RESET}"
else
echo "$line"
fi
done <<< "$raw_diff"
echo "================================="
# Extract added bullet lines
declare -A added_bullets=()
file_line=0
in_hunk=0
while IFS= read -r line; do
if [[ $line =~ ^\@\@ ]]; then
# Extract starting line number of the new file
file_line=$(echo "$line" | sed -E 's/.*\+([0-9]+).*/\1/')
in_hunk=1
continue
fi
# Ignore lines until we are inside a hunk
if [[ $in_hunk -eq 0 ]]; then
continue
fi
if [[ $line =~ ^\+ && ! $line =~ ^\+\+\+ ]]; then
content="${line#+}"
if [[ $content =~ ^[[:space:]]*[-*] ]]; then
added_bullets[$file_line]="$content"
fi
((file_line++))
elif [[ ! $line =~ ^\- ]]; then
((file_line++))
fi
done <<< "$raw_diff"
# Extract deleted bullet lines
deleted_bullets=()
while IFS= read -r line; do
[[ -n "$line" ]] && deleted_bullets+=("$line")
done < <(
echo "$raw_diff" \
| grep '^\-' \
| grep -vE '^(--- |\+\+\+ |@@ )' \
| sed 's/^-//' \
|| true
)
# Warn if no added entries
if [[ ${#added_bullets[@]} -eq 0 ]]; then
echo -e "${RED}❌ No new changelog entries detected in this PR.${RESET}"
echo -e "${YELLOW}⚠️ Please add an entry in [Unreleased] under the appropriate subheading.${RESET}"
failed=1
fi
# Initialize results
correctly_placed=""
orphan_entries=""
wrong_release_entries=""
# Walk through changelog to classify entries
line_no=0
current_release=""
current_subtitle=""
in_unreleased=0
while IFS= read -r line; do
((line_no++))
if [[ $line =~ ^##\ \[Unreleased\] ]]; then
current_release="Unreleased"
in_unreleased=1
current_subtitle=""
continue
elif [[ $line =~ ^##\ \[.*\] ]]; then
current_release="$line"
in_unreleased=0
current_subtitle=""
continue
elif [[ $line =~ ^### ]]; then
current_subtitle="$line"
continue
fi
if [[ -n "${added_bullets[$line_no]:-}" ]]; then
added="${added_bullets[$line_no]}"
if [[ "$in_unreleased" -eq 0 ]]; then
wrong_release_entries+="$added (added under released version $current_release)"$'\n'
failed=1
elif [[ -z "$current_subtitle" ]]; then
orphan_entries+="$added (NOT under a subtitle)"$'\n'
failed=1
else
correctly_placed+="$added (placed under $current_subtitle)"$'\n'
fi
fi
done < "$CHANGELOG"
# Display results
if [[ -n "$orphan_entries" ]]; then
echo -e "${RED}❌ Some CHANGELOG entries are not under a subtitle in [Unreleased]:${RESET}"
echo "$orphan_entries"
failed=1
fi
if [[ -n "$wrong_release_entries" ]]; then
echo -e "${RED}❌ Some changelog entries were added under a released version (should be in [Unreleased]):${RESET}"
echo "$wrong_release_entries"
failed=1
fi
if [[ -n "$correctly_placed" ]]; then
echo -e "${GREEN}✅ Some CHANGELOG entries are correctly placed under [Unreleased]:${RESET}"
echo "$correctly_placed"
fi
# Display deleted entries
if [[ ${#deleted_bullets[@]} -gt 0 ]]; then
echo -e "${RED}❌ Changelog entries removed in this PR:${RESET}"
for deleted in "${deleted_bullets[@]}"; do
echo -e " - ${RED}$deleted${RESET}"
done
echo -e "${YELLOW}⚠️ Please add these entries back under the appropriate sections${RESET}"
failed=1
fi
# Exit
if [[ $failed -eq 1 ]]; then
echo -e "${RED}❌ Changelog check failed.${RESET}"
exit 1
else
echo -e "${GREEN}✅ Changelog check passed.${RESET}"
exit 0
fi
Testing Results:TEST CASE : Adding entry under unreleased but not under any subcategory.Bash Output: ~/Documents/CODE/OpenSouce/FirstCommit/hiero-sdk-python (chore/harden-pr-check-changelog)
$ bash .github/scripts/pr-check-changelog-sample.sh
=== Raw git diff of CHANGELOG.md against upstream/main ===
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ba2fab..2ecc49c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,7 @@ This project adheres to [Semantic Versioning](https://semver.org).
This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
## [Unreleased]
-
+-Hello
### Tests
- Improve unit test coverage for `Hbar`, including edge cases, validation, comparisons, and hashing. (#1483)
@@ -236,7 +236,6 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Support for message chunking in `TopicSubmitMessageTransaction`.
### Changed
-
- bot workflows to include new changelog entry
- Removed duplicate import of transaction_pb2 in transaction.py
- Refactor `TokenInfo` into an immutable dataclass, remove all setters, and rewrite `_from_proto` as a pure factory for consistent parsing [#800]
=================================
❌ Some CHANGELOG entries are not under a subtitle in [Unreleased]:
-Hello (NOT under a subtitle)
❌ Changelog check failed.TEST CASE : Correct EntryBash Output : Test CASE : Entry under release sectionBash Output : ~/Documents/CODE/OpenSouce/FirstCommit/hiero-sdk-python (chore/harden-pr-check-changelog)
$ bash .github/scripts/pr-check-changelog-sample.sh
=== Raw git diff of CHANGELOG.md against upstream/main ===
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ba2fab..3c18593 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -223,7 +223,7 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
## [0.1.10] - 2025-12-03
### Added
-
+- Hello
- Added docs/sdk_developers/training/workflow: a training for developers to learn the workflow to contribute to the python SDK.
- Added Improved NFT allowance deletion flow with receipt-based status checks and strict `SPENDER_DOES_NOT_HAVE_ALLOWANCE` verification.
- Add `max_automatic_token_associations`, `staked_account_id`, `staked_node_id` and `decline_staking_reward` fields to `AccountUpdateTransaction` (#801)
@@ -236,7 +236,6 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Support for message chunking in `TopicSubmitMessageTransaction`.
### Changed
-
- bot workflows to include new changelog entry
- Removed duplicate import of transaction_pb2 in transaction.py
- Refactor `TokenInfo` into an immutable dataclass, remove all setters, and rewrite `_from_proto` as a pure factory for consistent parsing [#800]
=================================
❌ Some changelog entries were added under a released version (should be in [Unreleased]):
) Hello (added under released version ## [0.1.10] - 2025-12-03
❌ Changelog check failed.TEST CASE : Multiple entries , one correct and other under release section. Parth J Chaudhary@Pikolosan-PC MINGW64 ~/Documents/CODE/OpenSouce/FirstCommit/hiero-sdk-python (chore/harden-pr-check-changelog)
$ bash .github/scripts/pr-check-changelog-sample.sh
=== Raw git diff of CHANGELOG.md against upstream/main ===
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ba2fab..22fb41f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -106,7 +106,7 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Added prompt for coderabbit to review `Query` and it's sub-classes.
### Changed
-
+- Hello
- Add return type hint to `AccountId.__repr__` for type consistency. (#1503)
- Added AI usage guidelines to the Good First Issue and Beginner issue templates to guide contributors on responsible AI use. (#1490)
@@ -223,7 +223,7 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
## [0.1.10] - 2025-12-03
### Added
-
+- Hello
- Added docs/sdk_developers/training/workflow: a training for developers to learn the workflow to contribute to the python SDK.
- Added Improved NFT allowance deletion flow with receipt-based status checks and strict `SPENDER_DOES_NOT_HAVE_ALLOWANCE` verification.
- Add `max_automatic_token_associations`, `staked_account_id`, `staked_node_id` and `decline_staking_reward` fields to `AccountUpdateTransaction` (#801)
@@ -236,7 +236,6 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Support for message chunking in `TopicSubmitMessageTransaction`.
### Changed
-
- bot workflows to include new changelog entry
- Removed duplicate import of transaction_pb2 in transaction.py
- Refactor `TokenInfo` into an immutable dataclass, remove all setters, and rewrite `_from_proto` as a pure factory for consistent parsing [#800]
=================================
❌ Some changelog entries were added under a released version (should be in [Unreleased]):
) Hello (added under released version ## [0.1.10] - 2025-12-03
✅ Some CHANGELOG entries are correctly placed under [Unreleased]:
) Hello (placed under ### Changed
❌ Changelog check failed.Test Case : Deleted Entries / no entries addedBash output : ~/Documents/CODE/OpenSouce/FirstCommit/hiero-sdk-python (chore/harden-pr-check-changelog)
$ bash .github/scripts/pr-check-changelog-sample.sh
=== Raw git diff of CHANGELOG.md against upstream/main ===
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ba2fab..504a621 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -106,7 +106,6 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Added prompt for coderabbit to review `Query` and it's sub-classes.
### Changed
-
- Add return type hint to `AccountId.__repr__` for type consistency. (#1503)
- Added AI usage guidelines to the Good First Issue and Beginner issue templates to guide contributors on responsible AI use. (#1490)
@@ -223,7 +222,6 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
## [0.1.10] - 2025-12-03
### Added
-
- Added docs/sdk_developers/training/workflow: a training for developers to learn the workflow to contribute to the python SDK.
- Added Improved NFT allowance deletion flow with receipt-based status checks and strict `SPENDER_DOES_NOT_HAVE_ALLOWANCE` verification.
- Add `max_automatic_token_associations`, `staked_account_id`, `staked_node_id` and `decline_staking_reward` fields to `AccountUpdateTransaction` (#801)
@@ -236,7 +234,6 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
- Support for message chunking in `TopicSubmitMessageTransaction`.
### Changed
-
- bot workflows to include new changelog entry
- Removed duplicate import of transaction_pb2 in transaction.py
- Refactor `TokenInfo` into an immutable dataclass, remove all setters, and rewrite `_from_proto` as a pure factory for consistent parsing [#800]
=================================
❌ No new changelog entries detected in this PR.
⚠️ Please add an entry in [Unreleased] under the appropriate subheading.
❌ Changelog check failed. |
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
|
Hi @Pikolosan, This pull request has had no commit activity for 10 days. Are you still working on the issue? please push a commit to keep the PR active or it will be closed due to inactivity. From the Python SDK Team |
There was a problem hiding this comment.
Thanks for the thorough follow-up work on this — this is a strong improvement 👍
What started as a simple hardening change surfaced some real correctness issues in the existing script, and I appreciate you taking the time to address those rather than forcing strict mode onto brittle logic.
In particular, the rewritten implementation:
-
Uses
set -uo pipefailresponsibly, with explicit handling for commands that are expected to return non-zero -
Correctly scopes [Unreleased] and subtitle sections, avoiding the previous
current_subtitleleakage -
Validates added changelog entries by diff position, not string matching across the whole file
-
Properly fails on orphaned entries, released-section entries, deleted entries, and missing entries
The manual testing you documented across multiple scenarios gives good confidence that this won’t introduce regressions in one of our most critical workflows.
From my side, this now looks safe to merge and is a net improvement in both security and correctness of the PR changelog check.
Thanks again for sticking with this and iterating based on feedback — great work!
Please be sure to address merge conflicts and let us know if you need assistance. We're happy to help!
| @@ -1,77 +1,90 @@ | |||
| #!/bin/bash | |||
| set -uo pipefail | |||
There was a problem hiding this comment.
Missing -e flag violates coding guidelines.
The coding guidelines require set -euo pipefail for shell scripts, but -e is omitted here. While the omission may be intentional to handle non-fatal git command failures, this can be achieved while still using -e by appending || true to specific commands that may legitimately fail (which is already done at lines 105 and 158).
🛡️ Suggested fix
-set -uo pipefail
+set -euo pipefailAs per coding guidelines: .github/scripts/**/*.sh: MUST use: set -euo pipefail.
📝 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.
| set -uo pipefail | |
| set -euo pipefail |
There was a problem hiding this comment.
agree with this, rest look fine
Signed-off-by: Parth J Chaudhary <parthchaudhary.jc@yahoo.com>
Signed-off-by: Parth J Chaudhary <parthchaudhary.jc@yahoo.com>
Pikolosan
force-pushed
the
chore/harden-pr-check-changelog
branch
from
24ce92d to
e935f44
Compare
|
I have updated the code logic and rebased rebased on the latest upstream main and resolved changelog conflicts. |
prajeeta15
left a comment
prajeeta15
left a comment
There was a problem hiding this comment.
LGTM! this is really great work !! thank you so much :D
AntonioCeppellini
left a comment
AntonioCeppellini
left a comment
There was a problem hiding this comment.
LGMT, thank you for your contribution @Pikolosan well done :D
|
Converting back to draft as @manishdait requested changes - thank you! |
exploreriii
added
the
status: needs developer revision
6 participants
Summary
This PR hardens the
.github/scripts/pr-check-changelog.shscript by addingset -euo pipefailat the top.This ensures the script fails on errors, treats unset variables as errors, and returns non-zero on pipeline failures, making it consistent with other scripts in the repository.
Issue
Fixes #1492
Changes
set -euo pipefailto.github/scripts/pr-check-changelog.shChecklist
/assign)[Unreleased] > ### Changedgit commit -S -s)