feat: add mentor assignment workflow #1201
Conversation
Mounil2005
force-pushed
the
feature/mentor-assignment-workflow
branch
from
b702cfe to
8cdee86
Compare
📝 WalkthroughWalkthroughAdds an automated mentor-assignment feature: a mentor roster JSON, a GitHub Actions workflow triggered on issue assignment, and a Node script that validates "Good First Issue" assignments, detects new contributors, avoids duplicate assignments, deterministically selects a mentor, and posts a templated comment. Changes
Sequence DiagramsequenceDiagram
actor GitHub as "GitHub (issues.assigned)"
participant Workflow as "Actions Workflow"
participant Runner as "Actions Runner"
participant Script as "Mentor Assignment Script"
participant GHAPI as "GitHub API"
GitHub->>Workflow: issues.assigned event
Workflow->>Runner: start job (checkout, harden, permissions)
Runner->>Script: run local script via actions/github-script (context, token)
rect rgb(230,245,255)
Note right of Script: Load roster (.github/mentor_roster.json or MENTOR_ROSTER_PATH)
Script->>GHAPI: Fetch issue details (labels, assignee)
GHAPI-->>Script: issue data
Script->>GHAPI: Query assignee's merged PRs (determine new contributor)
GHAPI-->>Script: PR results
end
rect rgb(255,250,220)
Note right of Script: Duplicate checks & decision
Script->>GHAPI: List/search issue comments for mentor-assignment marker
GHAPI-->>Script: comments
alt Conditions met (Good First Issue, new contributor, no duplicate)
Script->>Script: select mentor (UTC day index mod roster length)
Script->>GHAPI: Post mentor-assignment comment (with retry/concurrency checks)
GHAPI-->>Script: comment created
else
Script->>Script: exit without posting
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1).github/scripts/bot-mentor-assignment.js (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/mentor_roster.json.github/scripts/mentor_assignment.js.github/workflows/mentor-assignment.ymlCHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/mentor_assignment.js (1)
.github/scripts/pr_inactivity_reminder.js (2)
owner(77-77)repo(78-78)
🪛 GitHub Check: Codacy Static Code Analysis
.github/scripts/mentor_assignment.js
[warning] 44-44: .github/scripts/mentor_assignment.js#L44
The numeric literal '86400000' will have at different value at runtime.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
CHANGELOG.md (1)
12-12: LGTM!The changelog entry is concise, clear, and follows the established format. It accurately describes the new mentor assignment automation.
.github/workflows/mentor-assignment.yml (1)
1-32: Excellent workflow configuration!The workflow is well-structured with:
- Appropriate trigger (
issues.assigned)- Minimal required permissions (
issues: write, contents: read)- Security hardening via
step-security/harden-runner- Actions pinned to commit SHAs for supply chain security
.github/scripts/mentor_assignment.js (5)
1-7: LGTM!Good use of environment variables with sensible defaults. The configurable constants make the script flexible and testable.
9-37: LGTM!The
loadMentorRosterfunction has robust error handling:
- Validates file existence and readability
- Safely parses JSON with descriptive error messages
- Filters blank entries and validates non-empty roster
- Throws clear errors that will help debug configuration issues
50-55: LGTM!The label check is robust:
- Handles both string and object label formats
- Case-insensitive comparison prevents false negatives
- Safe navigation with optional chaining
57-70: LGTM!The
isNewContributorfunction correctly:
- Uses GitHub search API to find merged PRs by the assignee
- Returns
trueonly if no merged contributions exist- Includes error handling with logging
The re-throw on line 68 will propagate errors to the caller (the main function), which will cause the workflow to fail visibly if the API call fails.
72-94: LGTM!The welcome message is well-crafted:
- Includes the comment marker for deduplication
- Provides clear next steps for the mentee
- Professional and encouraging tone
- Links to star the repository (nice touch for community building)
The template matches the requirements from issue #1063.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/mentor_roster.json (1)
5-5: Invalid username already flagged.This comment duplicates a previous review: The username
accepalunidoes not exist on GitHub (HTTP 404). This must be corrected before the roster can be used.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/mentor_roster.json.github/scripts/mentor_assignment.js.github/workflows/mentor-assignment.ymlCHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/mentor_assignment.js (1)
.github/scripts/pr_inactivity_reminder.js (2)
owner(77-77)repo(78-78)
🪛 GitHub Check: Codacy Static Code Analysis
.github/scripts/mentor_assignment.js
[warning] 44-44: .github/scripts/mentor_assignment.js#L44
The numeric literal '86400000' will have at different value at runtime.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
.github/workflows/mentor-assignment.yml (1)
1-32: Well-configured workflow with proper security practices.The workflow correctly:
- Triggers on the appropriate event (issues assigned)
- Sets minimal required permissions (issues: write, contents: read)
- Uses harden-runner for security
- Pins action versions with SHA hashes
- Integrates cleanly with the mentor assignment script
Mounil2005
force-pushed
the
feature/mentor-assignment-workflow
branch
2 times, most recently
from
9ce61c1 to
d0847bc
Compare
|
@exploreriii Pls review! |
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @Mounil2005 i think this looks pretty nice! but currently writing some tests for it
Normally i'd test on a fork but i think that is too complicated to do in this case, so creating a test suite checking most of it
Were you able to test parts?
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1201 +/- ##
=======================================
Coverage 91.79% 91.79%
=======================================
Files 139 139
Lines 8466 8466
=======================================
Hits 7771 7771
Misses 695 695 🚀 New features to boost your workflow:
|
|
🚨 @hiero-ledger/hiero-sdk-python-triage 🚨
It assigns a mentor to each new starter when ASSIGNED to a good first issue, which is labelled as a good first issue. Each DAY a new mentor will be chosen, and this mentor can have x number of new starters to mentor (one mentor to many starters). The goal is to help this new starter merge their first PR. on the next DAY, all new good first issue assignees will be assigned to a second mentor. We currently have 7 mentors, so this means mentors will only be assigned once per week. How this differs from initial discussions: Once this gets merged, triage members will be allocated a mentee for one day in a given week. Please double check: i request your review @hiero-ledger/hiero-sdk-python-triage, approvals or rejections or discussions! Here are the tests on the fokr to demonstrate: |
exploreriii
added
the
discussion
tech0priyanshu
left a comment
tech0priyanshu
left a comment
There was a problem hiding this comment.
Looks great thanks for your contribution!
|
@Mounil2005 Please resolve your conflicts |
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! From the Hiero Python SDK Team |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/scripts/bot-mentor-assignment.js (2)
119-128: Race condition may still allow duplicate comments in concurrent runs.The existing-comment check (lines 119-128) and comment creation (lines 142-147) are not atomic. If two workflow runs execute simultaneously (e.g., rapid re-assignment events), both could pass the check and post duplicate comments.
This is low probability given the specific trigger conditions, but if it becomes an issue, consider:
- Using GitHub's issue lock API during the operation
- Adding a secondary check after a brief delay before posting
- Accepting the rare duplicate as tolerable (current implicit choice)
135-140: Mentor self-assignment is not prevented.As noted in a previous review by tech0priyanshu, if the assignee happens to be in the mentor roster and is selected as today's mentor, they would be assigned as their own mentor. While unlikely (requires the person to be both a new contributor and in the roster), adding a guard improves the user experience.
🔎 Proposed fix
const roster = loadMentorRoster(); - const mentor = selectMentor(roster); + let mentor = selectMentor(roster); + + // Prevent self-assignment by selecting next mentor in rotation + if (mentor.toLowerCase() === mentee.toLowerCase() && roster.length > 1) { + const currentIndex = roster.findIndex(m => m.toLowerCase() === mentor.toLowerCase()); + mentor = roster[(currentIndex + 1) % roster.length]; + console.log(`Avoided self-assignment; reassigning to @${mentor}.`); + } console.log(`Assigning mentor @${mentor} to mentee @${mentee} for issue #${issue.number}.`);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/bot-mentor-assignment.js.github/workflows/bot-mentor-assignment.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
.github/workflows/bot-mentor-assignment.yml (1)
9-11: Minimal permissions follow least privilege — LGTM.The workflow requests only the permissions it needs:
issues: writefor posting comments andcontents: readfor accessing the roster file. Good security practice..github/scripts/bot-mentor-assignment.js (7)
9-37: Robust roster loading with good error handling — LGTM.The function properly:
- Uses a configurable path with sensible default
- Separates file read and JSON parse errors for clearer debugging
- Filters empty entries before validation
- Throws descriptive errors if roster is empty or malformed
39-49: Clean deterministic mentor selection — LGTM.The magic number concern from the previous review has been addressed with the named constant
MILLISECONDS_PER_DAY. The UTC day-based rotation ensures consistent mentor assignment across time zones.
51-56: Defensive label check handles both payload formats — LGTM.Correctly handles labels as both strings and objects with a
nameproperty, with case-insensitive matching.
58-72: Error handling gracefully skips assignment on API failures — LGTM.The previous review concern has been addressed: the function now returns
falseon API errors instead of throwing, preventing workflow failures from transient issues.Note: As Akshat8510 observed, if a contributor picks up multiple GFIs before their first PR merges, each issue will get a mentor comment. This is acceptable behavior given the duplicate-check is per-issue.
74-96: Well-structured welcome comment template — LGTM.The comment includes all required elements: marker for duplicate detection, mentor/mentee mentions, team aliases, and actionable getting-started guidance.
1-7: Configurable constants with sensible defaults — LGTM.Environment variable overrides allow testing and customization without code changes. Default values are appropriate for production use.
148-152: Error handling logs and re-throws for visibility — LGTM.The try-catch wrapper (suggested in a previous review) ensures errors are logged with context before failing the workflow visibly.
Mounil2005
force-pushed
the
feature/mentor-assignment-workflow
branch
from
bc1b9fd to
a621040
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/bot-mentor-assignment.js.github/workflows/bot-mentor-assignment.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
.github/workflows/bot-mentor-assignment.yml (1)
18-24: LGTM: Security hardening and checkout steps follow best practices.The workflow properly hardens the runner with egress auditing and uses pinned action SHAs for supply chain security.
.github/scripts/bot-mentor-assignment.js (8)
1-8: LGTM: Well-organized constants with environment overrides.The configuration constants are appropriately externalized with environment variable overrides, making the script testable and configurable without code changes.
9-37: LGTM: Robust roster loading with comprehensive error handling.The function properly handles file read failures, JSON parse errors, and validates that the roster is non-empty after filtering blank entries. Error messages include context for debugging.
39-58: LGTM: Deterministic mentor selection with self-assignment prevention.The function correctly implements:
- UTC-based daily rotation ensuring consistency across multiple assignments on the same day
- Self-assignment prevention by iterating through roster with offset
- Graceful null return when no eligible mentor exists (handled by caller at lines 186-188)
60-65: LGTM: Case-insensitive label check handles multiple label formats.The function correctly handles both string and object label representations and performs a case-insensitive comparison.
67-100: LGTM: Duplicate assignment prevention with safe error handling.The function correctly prevents duplicate mentor assignments across multiple issues by:
- Paginating through all open issues assigned to the mentee
- Checking each issue's comments for the assignment marker
- Returning a safe default (false) on API errors to avoid workflow failures
The O(n×m) complexity is acceptable given that new contributors typically have few open issues.
102-116: LGTM: New contributor detection with safe error handling.The function correctly identifies first-time contributors by checking for merged PRs and returns a safe default (false) on API failures to prevent workflow failures.
The design accepts that a motivated contributor picking up multiple GFIs before their first PR merges may receive multiple mentor assignments—a reasonable tradeoff vs complex state tracking. (Per Akshat8510's earlier observation.)
118-140: LGTM: Welcoming and informative mentor assignment template.The comment template effectively:
- Welcomes the new contributor warmly
- Introduces the assigned mentor and support resources
- Provides clear next steps
- Includes the marker for duplicate detection
- Encourages repository engagement
142-222: LGTM: Well-orchestrated mentor assignment flow with excellent race condition handling.The main function implements a thoughtful validation and assignment sequence:
- Fast validations first: Payload, bot check, label check (lines 147-157)
- Duplicate prevention: Checks current issue and other open issues (lines 163-172, 179-181)
- Eligibility check: New contributor validation (lines 174-177)
- Assignment: Deterministic mentor selection and posting (lines 183-200)
- Race condition safeguard: Re-checks for concurrent comment creation on failure (lines 201-216)
The outer try-catch ensures visibility of any unexpected failures (lines 217-221). A small race window exists between the initial comment check (line 170) and comment creation (line 195), but the retry logic effectively mitigates duplicate comments in practice.
|
Hello everyone, i have done the requested changes, pls review! Thank you for your suggestions! |
This comment was marked as outdated.
This comment was marked as outdated.
|
What will be the experience for other new contributors? Will they be assigned a mentor or not? |
|
only GFI |
I know..I mean, if there are multiple new contributors in a single day, will each of them be assigned a mentor? |
for each UTC day we pick one mentor from the roster and every new Good First Issue assignee on that day gets that same mentor. There’s no limit of one mentee per day |
|
As per my counts we have 7 people supporting this PR, and one additionally offline, which covers most of the triage team. CC @manishdait and @nadineloepfe, would request at least one of their opinions too. I have to double check the new adjustments made today. |
Mounil2005
force-pushed
the
feature/mentor-assignment-workflow
branch
from
d9e283e to
91d694f
Compare
Added the extra logging you suggested (query string, token presence, merged PR count, and new-starter flag). It ships as a small change to keep existing behavior intact. The new output should make it easier to trace how the bot evaluates mentors. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/mentor_roster.json.github/scripts/bot-mentor-assignment.js.github/workflows/bot-mentor-assignment.ymlCHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
CHANGELOG.md (1)
17-17: LGTM! Changelog entry is clear and well-placed.The entry accurately describes the new mentor assignment feature and follows the project's changelog conventions.
.github/mentor_roster.json (1)
1-12: LGTM! Roster structure is clean and valid.The JSON structure is valid, contains no duplicates, and follows a simple, maintainable format. The username correction noted in past reviews ("aceppaluni" with one 'c') is properly reflected.
.github/scripts/bot-mentor-assignment.js (7)
9-37: LGTM! Roster loading logic is robust.The function correctly:
- Supports path override via environment variable
- Handles file read errors with clear messages
- Parses and validates JSON structure
- Filters blank entries
- Ensures the roster is non-empty before returning
39-49: LGTM! Mentor selection logic is correct and deterministic.The day-based rotation using UTC day index modulo roster length ensures:
- All assignments on the same day get the same mentor
- Rotation advances daily
- Cycles through the roster predictably
The magic number was properly extracted to a named constant.
51-56: LGTM! Label check is defensive and case-insensitive.The function correctly:
- Handles both string and object label formats
- Uses case-insensitive comparison to avoid casing issues
- Safely navigates potentially undefined values
58-80: LGTM! Contributor validation with appropriate error handling.The function correctly:
- Searches for merged PRs only (not just any activity)
- Includes comprehensive logging (query, token presence, PR count, result) for debugging
- Returns
nullon API errors to skip assignment without failing the workflow- Clearly distinguishes between "has contributions" (false) and "cannot verify" (null)
The enhanced logging addresses previous observability concerns.
82-104: LGTM! Comment template is clear and welcoming.The template provides:
- A friendly welcome message
- Clear mentor assignment
- Actionable next steps
- Multiple support channels (mentor + support team)
- Repository star call-to-action
The COMMENT_MARKER enables duplicate detection. Well-crafted mentor assignment message.
106-183: LGTM! Main orchestration is well-structured and resilient.The function implements a robust mentor assignment flow:
Validation & Guards (lines 108-121):
- Validates payload presence
- Skips bot assignees
- Verifies Good First Issue label
Duplicate Prevention (lines 127-136):
- Paginates all existing comments
- Checks for marker to avoid duplicates
Contributor Verification (lines 138-146):
- Distinguishes between API errors (
null) and existing contributors (false)- Provides clear logging for each case
Assignment & Concurrency Handling (lines 148-177):
- Loads roster and selects mentor deterministically
- Posts comment with retry logic
- Re-checks for duplicates after failures to handle race conditions
- Only propagates errors if genuinely failed (not duplicate)
Error Handling (lines 178-182):
- Comprehensive try-catch wrapper
- Clear error logging before re-throwing
The implementation addresses all concerns raised in past reviews.
6-6: The suggested correction is also incorrect. Both team names return 404 errors from the GitHub API—the current name@hiero-ledger/hiero-sdk-python-good-first-issue-supportand the suggested correction@hiero-ledger/hiero-sdk-good-first-issue-support(without "python") are both invalid. The correct team alias should be determined from the actual teams configured in the hiero-ledger organization and set via theSUPPORT_TEAM_ALIASenvironment variable.Likely an incorrect or invalid review comment.
Mounil2005
force-pushed
the
feature/mentor-assignment-workflow
branch
from
0080378 to
93463ba
Compare
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Mounil2005
force-pushed
the
feature/mentor-assignment-workflow
branch
from
93463ba to
b89dc1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-mentor-assignment.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
.github/scripts/bot-mentor-assignment.js (7)
1-8: Well-structured configuration with sensible defaults.The use of environment variables for all configurable values (comment marker, team aliases, roster path) provides excellent flexibility for testing and future adjustments without code changes.
39-49: Deterministic mentor selection logic is correct.The UTC-based day rotation ensures consistent mentor assignment across all issues on the same calendar day. The named constant
MILLISECONDS_PER_DAYaddresses the previous code quality feedback.
51-56: Label matching logic is defensive and correct.The case-insensitive comparison and dual handling of string vs. object labels ensure compatibility with different payload formats.
58-94: Fork detection and new contributor verification are well-implemented.The function correctly:
- Detects forks and searches the parent repository for merged PRs (lines 62-68), addressing the edge case where contributors work from forks
- Provides comprehensive diagnostic logging (lines 76-77, 85, 87) for troubleshooting
- Returns
nullon API errors (line 92) to distinguish "verification failed" from "has contributions", enabling appropriate handling by the callerThe defensive fallback to the original repo when fork detection fails (line 70) ensures the workflow remains functional even during API issues.
96-118: Comment template is well-structured and professional.The welcome message provides clear guidance for new contributors and properly leverages GitHub mentions for mentor and team notifications. The marker placement enables reliable duplicate detection.
120-197: Excellent orchestration with comprehensive error handling and concurrency safeguards.The main function implements a robust workflow:
- Early validation and filtering (lines 122-135): Efficiently skips inappropriate cases (missing payload, bots, non-GFI issues) before expensive operations
- Thorough duplicate prevention (lines 141-150): Uses pagination to ensure no existing assignment, preventing redundant mentor notifications
- Graceful degradation (lines 154-156): Distinguishes API errors from contributor status, avoiding workflow failure when GitHub Search API is unavailable
- Race condition handling (lines 176-191): Re-checks for duplicates after comment posting fails, correctly handling concurrent workflow runs
The nested try-catch structure (outer at line 121, inner at line 169) ensures errors are logged before propagation, providing excellent observability.
9-37: The mentor roster file is properly configured and contains 8 valid mentor entries with the requiredorderarray structure. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-mentor-assignment.js
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/bot-mentor-assignment.js (1)
.github/scripts/pr_inactivity_reminder.js (2)
owner(77-77)repo(78-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
.github/scripts/bot-mentor-assignment.js (8)
1-8: LGTM! Clean setup with sensible defaults.The imports and environment variable configuration are appropriate. The team alias at line 6 correctly matches the updated name from previous feedback.
9-37: Robust roster loading with comprehensive validation.The function correctly handles file I/O errors, JSON parsing errors, and filters malformed entries. The validation ensures a non-empty roster before proceeding.
39-49: LGTM! Deterministic day-based rotation.The mentor selection logic is sound and the extraction of
MILLISECONDS_PER_DAY(line 44) addresses previous feedback about the magic number. All issues assigned on the same UTC day will share the same mentor.
51-56: LGTM! Flexible label matching.The function correctly handles both string and object label formats and performs case-insensitive matching, which is appropriate for label validation.
80-82: Token presence logging may not reflect actual API authentication.The log checks environment variables (
process.env.GITHUB_TOKEN || process.env.GH_TOKEN), but the actual GitHub API calls authenticate via thegithubparameter, which is already configured by the Actions workflow. This log won't accurately indicate whether the API calls will succeed.Since this was added for debugging purposes per previous feedback and doesn't affect functionality, this is acceptable. However, be aware that a "false" log here doesn't necessarily mean the API calls will fail.
64-76: Fork detection logic is sound for the typical use case.The function correctly detects when the repository is a fork and searches the parent repository for merged PRs. This is appropriate for testing the workflow on personal forks, where contributions would be merged to the parent.
Note: If a fork becomes a standalone development repository (no longer syncing with parent), the logic will still check the parent. Based on the PR objectives, this appears to be the intended behavior.
101-123: LGTM! Clear and welcoming template.The comment template is well-structured, provides actionable guidance, and correctly uses the configured team aliases and marker.
125-202: Well-structured orchestration with comprehensive safeguards.The main function correctly implements the mentor assignment flow:
- Validates payload and skips inappropriate cases (bots, non-GFI issues)
- Uses pagination to check for existing comments
- Handles API errors gracefully (returns null from
isNewContributorto skip assignment)- Provides clear, detailed logging for observability (addressing previous feedback)
- Top-level error handling ensures workflow fails visibly when unexpected errors occur
The flow aligns with the PR objectives: assign mentors only to new contributors on Good First Issues, with daily rotation and duplicate prevention.
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Mounil2005
force-pushed
the
feature/mentor-assignment-workflow
branch
from
17e54ab to
6c2a6b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/scripts/bot-mentor-assignment.js (1)
192-193: Consider generalizing the log message.The log message assumes the error was due to a concurrent workflow run, but it could also result from network issues, rate limiting, or permissions errors. While this doesn't affect correctness, a more general message would be more accurate.
🔎 Optional improvement
- return console.log(`Mentor assignment comment already exists on issue #${issue.number} after concurrent run. Skipping. (${message})`); + return console.log(`Mentor assignment comment already exists on issue #${issue.number}. Skipping duplicate. (${message})`);Based on past review comments, this was previously flagged as a nitpick.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-mentor-assignment.js
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/bot-mentor-assignment.js (1)
.github/scripts/pr_inactivity_reminder.js (2)
owner(77-77)repo(78-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
.github/scripts/bot-mentor-assignment.js (6)
1-8: LGTM! Clean configuration setup.The imports and constants are well-structured. The environment variable overrides provide flexibility for testing and different deployment scenarios.
9-37: LGTM! Robust roster loading.The function properly handles file I/O errors, JSON parsing errors, and filters malformed entries. The validation ensures the roster is non-empty before returning.
39-49: LGTM! Deterministic mentor selection.The daily rotation logic is sound and the magic number has been properly extracted into a named constant. All assignments on the same UTC day will consistently select the same mentor.
51-56: LGTM! Flexible label checking.The function correctly handles both string and object label formats and performs case-insensitive matching.
102-124: LGTM! Welcoming and informative comment template.The message provides clear guidance for new contributors and includes all relevant contacts and resources.
126-203: LGTM! Well-structured main orchestration logic.The function correctly implements the mentor assignment flow:
- Validates payload and skips inappropriate cases (bots, non-GFI issues)
- Prevents duplicate comments with pagination
- Verifies the assignee is a new contributor, correctly handling API failures (returns null and skips)
- Includes concurrency safeguards that re-check for duplicates on error before re-throwing
The error handling ensures failures are logged and visible in workflow runs.
Signed-off-by: Mounil <mounilkankhara@gmail.com>
|
Well done Mouni! Thanks ever so much for this much needed innovation! |
Signed-off-by: Mounil <mounilkankhara@gmail.com>
7 participants
Description:
Summary
issues.assignedRelated issue(s):
Fixes #1063
Notes for reviewer:
How it works:
Checklist