refactor: standardize Good First Issue label handling#2194
refactor: standardize Good First Issue label handling#2194bhuvan-somisetty wants to merge 12 commits into
Conversation
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
Up to standards ✅🟢 Issues
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughThis PR standardizes capitalization of "Good First Issue" label references across GitHub issue templates, automation scripts, and documentation from lowercase variants to consistent title case, and updates the step-security/harden-runner action version. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 51 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/bot-next-issue-recommendation.js (1)
55-65:⚠️ Potential issue | 🔴 CriticalCritical:
labelSet.has('Good First Issue')will never match — recommendation bot is broken for GFIs.Line 55 lowercases every label name before inserting into
labelSet, so the Set only ever contains entries like'good first issue'.Set.prototype.hasuses SameValueZero (case-sensitive) equality, solabelSet.has('Good First Issue')always returnsfalse. As a result:
difficultyLevels.goodFirstIssueis permanentlyfalse.- The early return at line 74 silently skips GFI-only issues (those without a
beginnerlabel).- At line 97,
completedLabelis mislabeled as'Beginner'even when the issue is a GFI.This regresses the assertion in the PR description — "Preserved existing lowercase comparisons that use
.toLowerCase()to avoid changing logic." The comparison side here was changed and breaks the gating.🐛 Proposed fix — keep the comparison case-insensitive (consistent with `bot-gfi-assign-on-comment.js`)
// Determine issue difficulty level const difficultyLevels = { beginner: labelSet.has('beginner'), - goodFirstIssue: labelSet.has('Good First Issue'), + goodFirstIssue: labelSet.has('good first issue'), intermediate: labelSet.has('intermediate'), advanced: labelSet.has('advanced'), };Alternatively, lift
'Good First Issue'into a top-level constant and lowercase it at the comparison site — that would match the pattern used inbot-gfi-assign-on-comment.js(GOOD_FIRST_ISSUE_LABEL.toLowerCase()) and serve as the single source of truth requested in issue#2178.As per coding guidelines: "Use top-level constants for configuration — avoid hardcoded values scattered through the script."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae1e6a30-ef09-4756-add4-335362ab4d57
📒 Files selected for processing (8)
.github/ISSUE_TEMPLATE/00-good-first-issue-candidate.yml.github/ISSUE_TEMPLATE/01-good-first-issue.yml.github/ISSUE_TEMPLATE/02-beginner-issue.yml.github/scripts/bot-gfi-assign-on-comment.js.github/scripts/bot-next-issue-recommendation.js.github/workflows/bot-gfi-candidate-notification.yamldocs/sdk_developers/code_of_conduct.mddocs/sdk_developers/training/workflow/04_assigning_issues.md
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
|
Hi, I have removed the duplicate "Adapt as needed" text from the issue templates as suggested. Please review again. Thanks! |
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
3b724ed to
48c490e
Compare
|
Removed the duplicate "Adapt as needed" text and fixed the DCO sign-off. Please review again. Thanks! |
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
| dist/*.tar.gz | ||
| verify: true | ||
| verify-cert-identity: https://github.com/${{ github.repository }}/.github/workflows/publish.yml@refs/tags/${{ github.ref_name }} | ||
| verify-cert-identity: https://github.com/${{ github.repository }}/.github/workflows/release-publish.yml@refs/tags/${{ github.ref_name }} |
There was a problem hiding this comment.
this is part of a different PR - change should not be part of this
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
|
Removed the unrelated workflow change from this PR. This PR now only focuses on the Good First Issue label/template updates. |
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
5e28a04 to
d97d972
Compare
|
Removed unrelated CodeRabbit workflow changes from this PR. This PR now strictly focuses on Good First Issue label standardization only. Please review again. Thanks! |
There was a problem hiding this comment.
Thanks for putting this together @bhuvan-somisetty , great work on this one ! Standardizing our label handling is a great initiative.
I have left a few inline comments on specific lines. The two main blockers are:
- A critical logic regression in bot-next-issue-recommendation.js: The Set.has() check will now always fail due to strict case-sensitivity against previously lowercased inputs, breaking the bot automation. I recommend centralizing the label constants in .github/scripts/labels.js to prevent these mismatch issues across the codebase.
- Unrelated workflow downgrades: The publish.yml file still contains version downgrades for the security runner that need to be reverted to keep this PR strictly scoped.
| const difficultyLevels = { | ||
| beginner: labelSet.has('beginner'), | ||
| goodFirstIssue: labelSet.has('good first issue'), | ||
| goodFirstIssue: labelSet.has('Good First Issue'), |
There was a problem hiding this comment.
Set.has() is strictly case-sensitive. Because the labels are normalized to lowercase earlier in this script, evaluating against 'Good First Issue' here will always return false. I ran a mock run to verify, and a valid GFI label gets skipped with the message: "Issue is not a Good First Issue or beginner issue, skipping." We should either compare against a lowercased string here, or better yet, centralize this in .github/scripts/labels.js as the single source of truth and import it.
| steps: | ||
| - name: Harden the runner (Audit all outbound calls) | ||
| uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0 | ||
| uses: step-security/harden-runner@6c3c2f2c1c457b00c10c4848d6f5491db3b629df # v2.18.0 |
There was a problem hiding this comment.
This changes the step-security/harden-runner from v2.19.0 to v2.18.0. Since this PR is strictly scoped to label standardization, this unrelated version downgrade needs to be reverted.
| steps: | ||
| - name: Harden the runner (Audit all outbound calls) | ||
| uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0 | ||
| uses: step-security/harden-runner@6c3c2f2c1c457b00c10c4848d6f5491db3b629df # v2.18.0 |
There was a problem hiding this comment.
This changes the step-security/harden-runner from v2.19.0 to v2.18.0. Since this PR is strictly scoped to label standardization, this unrelated version downgrade needs to be reverted.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/publish.yml (1)
18-18:⚠️ Potential issue | 🟠 MajorRevert the
harden-runnerdowngrade from this PR.Line 18 and Line 65 introduce an unrelated
step-security/harden-runnerversion downgrade (v2.19.0→v2.18.0) in a PR scoped to Good First Issue label standardization. Please remove this change from this PR and handle it separately if needed.As per coding guidelines, "Focus feedback on the PR's stated scope."
Also applies to: 65-65
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a17ce467-95fc-4ae2-862c-c05225944645
📒 Files selected for processing (3)
.github/ISSUE_TEMPLATE/00-good-first-issue-candidate.yml.github/ISSUE_TEMPLATE/01-good-first-issue.yml.github/workflows/publish.yml
Signed-off-by: Bhuvan Somisetty <somisettybhuvan5@gmail.com>
2f07929 to
905e101
Compare
|
Removed unrelated changes as requested. This PR now only includes Good First Issue label/template updates. |
exploreriii
left a comment
There was a problem hiding this comment.
@bhuvan-somisetty i am closing this PR - please reopen it using a fresh copy of the updated main, ensuring only the changes are pushed that solve the issue linked and assigned.
Note there are 15 file changes, you are submitting changes to consensus, query, tokens, code rabbit, as well as GFI
https://github.com/hiero-ledger/hiero-sdk-python/pull/2194/changes
Closing as in its current form it is vastly not meeting quality standards
Description:
Standardize usage of the "Good First Issue" label across scripts, workflows, issue templates, and documentation to ensure consistent behavior in GitHub automation.
.toLowerCase()is used-->
Related issue(s):
Fixes #2178
Notes for reviewer:
Changes are limited to label text standardization across relevant files. No functional or behavioral changes were introduced. All comparisons using
.toLowerCase()were intentionally kept unchanged to preserve logic.Checklist: