Add discussion label check and test to bot inactivity script#2023
Add discussion label check and test to bot inactivity script#2023oGranny wants to merge 1 commit intohiero-ledger:mainfrom
Conversation
Signed-off-by: oGranny <ogranny.github.io@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the GitHub inactivity bot so it won’t automatically close/unassign pull requests that are intentionally parked for discussion (via the discussion label), addressing #1583.
Changes:
- Add a
discussionlabel check to.github/scripts/bot-inactivity-unassign.shto skip close/unassign actions for labeled PRs. - Add a bash-based test harness intended to validate the discussion-label behavior.
- Add a changelog entry under [Unreleased] documenting the automation change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
CHANGELOG.md |
Adds an Unreleased entry documenting the bot behavior change. |
.github/scripts/bot-inactivity-unassign.sh |
Skips processing for PRs labeled discussion to prevent automated closure/unassignment. |
.github/scripts/test-bot-inactivity-unassign.sh |
Introduces a mock-based bash test suite for the inactivity bot behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - chore: update spam list #1988 | ||
| - chore: Update `bot-advanced-check.yml`, `bot-gfi-assign-on-comment.yml`, `bot-intermediate-assignment.yml`, `bot-linked-issue-enforcer.yml`, `unassign-on-comment.yml`, `working-on-comment.yml` workflow runner configuration | ||
|
|
||
| - chore: Added check for the discussion label in the inactivity bot (`#1583`) |
| # Create a minimal test scenario | ||
| # We'll test the relevant part of the script logic | ||
|
|
||
| # Simulate the check | ||
| local pr_num="100" | ||
| local HAS_DISCUSSION_LABEL | ||
| HAS_DISCUSSION_LABEL=$(gh pr view "$pr_num" --repo "$TEST_REPO" --json labels --jq '.labels[].name | select(. == "discussion")' 2>/dev/null || echo "") | ||
|
|
||
| if [[ -n "$HAS_DISCUSSION_LABEL" ]]; then | ||
| # Should skip closing | ||
| print_result "PR with discussion label detected" "PASS" | ||
|
|
||
| # Verify PR was not closed | ||
| if [[ ! -f "$GH_MOCK_DIR/actions.log" ]] || ! grep -q "CLOSED_PR_$pr_num" "$GH_MOCK_DIR/actions.log" 2>/dev/null; then | ||
| print_result "PR with discussion label was NOT closed" "PASS" | ||
| else | ||
| print_result "PR with discussion label was NOT closed" "FAIL" "PR was incorrectly closed" | ||
| fi |
There was a problem hiding this comment.
This test suite doesn’t execute .github/scripts/bot-inactivity-unassign.sh; it re-implements small pieces of logic (e.g., only querying labels) and then asserts that no close happened. As written, these tests will still pass even if the bot script regresses (since gh pr close is never invoked). Consider running the real script against the mocked gh and asserting on the recorded actions/logs.
| for i in "${!@}"; do | ||
| if [[ "${!i}" == "--remove-assignee" ]]; then | ||
| next=$((i + 1)) | ||
| user="${!next}" | ||
| issue_num="" | ||
| for arg in "$@"; do | ||
| if [[ "$arg" =~ ^[0-9]+$ ]]; then | ||
| issue_num="$arg" | ||
| break | ||
| fi | ||
| done | ||
| echo "UNASSIGNED_${user}_FROM_${issue_num}" >> "$GH_MOCK_DIR/actions.log" | ||
| break | ||
| fi | ||
| done |
There was a problem hiding this comment.
The mock gh issue edit --remove-assignee parsing uses for i in "${!@}"; do and then ${!i} to try to find the flag. ${!@} expands to the positional parameter values (not numeric indices), so this branch won’t reliably detect --remove-assignee and won’t record unassignments in actions.log. Consider converting "$@" to an array and iterating indices (or using a simple while (( $# )); do ...; shift; done parser).
| for i in "${!@}"; do | |
| if [[ "${!i}" == "--remove-assignee" ]]; then | |
| next=$((i + 1)) | |
| user="${!next}" | |
| issue_num="" | |
| for arg in "$@"; do | |
| if [[ "$arg" =~ ^[0-9]+$ ]]; then | |
| issue_num="$arg" | |
| break | |
| fi | |
| done | |
| echo "UNASSIGNED_${user}_FROM_${issue_num}" >> "$GH_MOCK_DIR/actions.log" | |
| break | |
| fi | |
| done | |
| # Copy arguments into an array for indexed access | |
| local -a args=("$@") | |
| # Determine the issue number (first purely numeric argument) | |
| local issue_num="" | |
| for arg in "${args[@]}"; do | |
| if [[ "$arg" =~ ^[0-9]+$ ]]; then | |
| issue_num="$arg" | |
| break | |
| fi | |
| done | |
| # Find the --remove-assignee flag and its following username | |
| local user="" | |
| for i in "${!args[@]}"; do | |
| if [[ "${args[i]}" == "--remove-assignee" ]]; then | |
| local next=$((i + 1)) | |
| if (( next < ${#args[@]} )); then | |
| user="${args[next]}" | |
| echo "UNASSIGNED_${user}_FROM_${issue_num}" >> "$GH_MOCK_DIR/actions.log" | |
| fi | |
| break | |
| fi | |
| done |
| echo "Test 8: Label matching is case-sensitive" | ||
| echo "==========================================" | ||
|
|
||
| # Create PR with "Discussion" (capital D) | ||
| echo '{"state":"OPEN"}' > "$GH_MOCK_DIR/pr_800_state.json" | ||
| echo '{"labels":[{"name":"Discussion"}]}' > "$GH_MOCK_DIR/pr_800_labels.json" | ||
|
|
||
| local pr_num="800" | ||
| local HAS_DISCUSSION_LABEL | ||
| HAS_DISCUSSION_LABEL=$(gh pr view "$pr_num" --repo "$TEST_REPO" --json labels --jq '.labels[].name | select(. == "discussion")' 2>/dev/null || echo "") | ||
|
|
||
| if [[ -z "$HAS_DISCUSSION_LABEL" ]]; then | ||
| print_result "Case-sensitive matching (Discussion != discussion)" "PASS" | ||
| else | ||
| print_result "Case-sensitive matching (Discussion != discussion)" "FAIL" "Should not match different case" |
There was a problem hiding this comment.
test_case_sensitivity asserts that label matching is case-sensitive, but the production bot code uses grep -qi '^discussion$' (case-insensitive). Either update the test to reflect the real behavior, or change the bot to be case-sensitive—right now the test expectation and the implementation disagree.
| echo "Test 8: Label matching is case-sensitive" | |
| echo "==========================================" | |
| # Create PR with "Discussion" (capital D) | |
| echo '{"state":"OPEN"}' > "$GH_MOCK_DIR/pr_800_state.json" | |
| echo '{"labels":[{"name":"Discussion"}]}' > "$GH_MOCK_DIR/pr_800_labels.json" | |
| local pr_num="800" | |
| local HAS_DISCUSSION_LABEL | |
| HAS_DISCUSSION_LABEL=$(gh pr view "$pr_num" --repo "$TEST_REPO" --json labels --jq '.labels[].name | select(. == "discussion")' 2>/dev/null || echo "") | |
| if [[ -z "$HAS_DISCUSSION_LABEL" ]]; then | |
| print_result "Case-sensitive matching (Discussion != discussion)" "PASS" | |
| else | |
| print_result "Case-sensitive matching (Discussion != discussion)" "FAIL" "Should not match different case" | |
| echo "Test 8: Label matching is case-insensitive" | |
| echo "==========================================" | |
| # Create PR with "Discussion" (capital D) and ensure it matches "discussion" | |
| echo '{"state":"OPEN"}' > "$GH_MOCK_DIR/pr_800_state.json" | |
| echo '{"labels":[{"name":"Discussion"}]}' > "$GH_MOCK_DIR/pr_800_labels.json" | |
| local pr_num="800" | |
| local HAS_DISCUSSION_LABEL | |
| HAS_DISCUSSION_LABEL=$(gh pr view "$pr_num" --repo "$TEST_REPO" --json labels --jq '.labels[].name | ascii_downcase | select(. == "discussion")' 2>/dev/null || echo "") | |
| if [[ -n "$HAS_DISCUSSION_LABEL" ]]; then | |
| print_result "Case-insensitive matching (Discussion == discussion)" "PASS" | |
| else | |
| print_result "Case-insensitive matching (Discussion == discussion)" "FAIL" "Should match regardless of case" |
| PR_LABELS=$(gh pr view "$PR_NUM" --repo "$REPO" --json labels --jq '.labels[].name' 2>/dev/null || true) | ||
|
|
||
| if echo "$PR_LABELS" | grep -qi '^discussion$'; then | ||
| echo " [SKIP] PR #$PR_NUM has 'discussion' label — skipping close & unassign" | ||
| continue |
There was a problem hiding this comment.
Label retrieval errors are currently ignored (... || true), which means a transient gh pr view failure could cause PR_LABELS to be empty and the bot to proceed with stale-close/unassign even though the PR might have a discussion label. For safety (and to match the earlier PR state fetch handling), consider detecting gh pr view failure for labels and skipping the PR (or at least logging and continuing) when labels can’t be fetched.
WalkthroughThe PR adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d50fcdd-8abe-447b-9071-8f12791fa6d1
📒 Files selected for processing (3)
.github/scripts/bot-inactivity-unassign.sh.github/scripts/test-bot-inactivity-unassign.shCHANGELOG.md
| PR_LABELS=$(gh pr view "$PR_NUM" --repo "$REPO" --json labels --jq '.labels[].name' 2>/dev/null || true) | ||
|
|
||
| if echo "$PR_LABELS" | grep -qi '^discussion$'; then | ||
| echo " [SKIP] PR #$PR_NUM has 'discussion' label — skipping close & unassign" | ||
| continue | ||
| fi |
There was a problem hiding this comment.
Use exact label matching for discussion (not case-insensitive).
Line 217 currently matches case-insensitively. The requested behavior is to skip only when the label name is exactly discussion.
🔧 Proposed fix
- if echo "$PR_LABELS" | grep -qi '^discussion$'; then
+ if echo "$PR_LABELS" | grep -qx 'discussion'; then📝 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.
| PR_LABELS=$(gh pr view "$PR_NUM" --repo "$REPO" --json labels --jq '.labels[].name' 2>/dev/null || true) | |
| if echo "$PR_LABELS" | grep -qi '^discussion$'; then | |
| echo " [SKIP] PR #$PR_NUM has 'discussion' label — skipping close & unassign" | |
| continue | |
| fi | |
| PR_LABELS=$(gh pr view "$PR_NUM" --repo "$REPO" --json labels --jq '.labels[].name' 2>/dev/null || true) | |
| if echo "$PR_LABELS" | grep -qx 'discussion'; then | |
| echo " [SKIP] PR #$PR_NUM has 'discussion' label — skipping close & unassign" | |
| continue | |
| fi |
| test_pr_with_discussion_label_not_closed | ||
| test_stale_pr_without_discussion_closed | ||
| test_jq_filter_correctness | ||
| test_closed_pr_skipped | ||
| test_active_pr_not_closed | ||
| test_log_output | ||
| test_multiple_labels_with_discussion | ||
| test_case_sensitivity |
There was a problem hiding this comment.
Tests don’t execute the real inactivity bot script.
The suite only exercises mocked gh calls and inline logic; it never runs .github/scripts/bot-inactivity-unassign.sh. This can mask regressions in the actual close/unassign flow and skip behavior. Add at least one test that executes the real script with mocks and asserts resulting actions/logs.
As per coding guidelines, review shell scripts as production-grade CI automation.
exploreriii
added
status: needs triage review
tech0priyanshu
left a comment
tech0priyanshu
left a comment
There was a problem hiding this comment.
Hii @oGranny thanks for contribution .SCRIPT_DIR is never used or exported. Please ensure workflow pass. Right now, we failed / Codacy Static Code Analysis.Once all pass happy to review again.
tech0priyanshu
added
status: needs developer revision
|
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, |
Description:
Adds a check to the inactivity bot script to prevent it from closing or unassigning pull requests that have the 'discussion' label. This ensures that active discussions are not interrupted by automated processes.
Related issue(s):
Fixes #1583
Checklist