-
Notifications
You must be signed in to change notification settings - Fork 144
chore: Chain GFI assignment with mentor assignment (#1369) #1386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Chain GFI assignment with mentor assignment (#1369) #1386
Conversation
Signed-off-by: Mounil <[email protected]>
|
TESTING IN PROGRESS |
📝 WalkthroughWalkthroughChains Good First Issue assignment with mentor assignment by invoking the mentor-assignment script immediately after a successful user assignment; adds invocation, error-tolerant logging, and changelog entry documenting the change. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub
participant Bot as GFI Bot
participant Mentor as Mentor Module
rect rgba(200,200,200,0.12)
Note over User,GitHub: Previous flow (blocked by anti-recursion)
User->>GitHub: Comment "/assign"
GitHub->>Bot: Trigger GFI workflow
Bot->>GitHub: Assign user to issue
GitHub-xBot: ✗ further workflow triggers blocked
Bot-xMentor: (mentor not invoked)
end
rect rgba(100,180,100,0.12)
Note over Bot,Mentor: New flow (direct chaining)
User->>GitHub: Comment "/assign"
GitHub->>Bot: Trigger GFI workflow
Bot->>GitHub: Assign user to issue (success)
Bot->>Mentor: require() & invoke mentor-assignment with assigned username
Mentor->>GitHub: Post mentor comment / assign mentor (within same process)
Mentor-->>Bot: Return (success or error logged)
Bot-->>GitHub: Workflow complete
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/scripts/bot-gfi-assign-on-comment.js.github/workflows/archive/bot-mentor-assignment.ymlCHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.js
⚙️ CodeRabbit configuration file
.github/scripts/**/*.js: Review JavaScript scripts as long-lived automation code.Scripts must remain:
Focused
Readable
Purpose-built
All
context.payloadfields MUST be validatedFree-form text MUST NOT be trusted
Dynamic code execution is prohibited
Avoid
child_process.exec; preferexecFileif neededAll async operations MUST be wrapped in try/catch
Errors MUST include contextual metadata
Duplicate API calls MUST be avoided
Marker-based deduplication is required
Scripts MUST NOT assume write access
Permission failures MUST be handled gracefully
Files:
.github/scripts/bot-gfi-assign-on-comment.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 (2)
CHANGELOG.md (1)
90-90: LGTM! Changelog entry clearly documents the chaining feature.The entry accurately describes the new functionality and correctly references issue #1369.
.github/scripts/bot-gfi-assign-on-comment.js (1)
207-221: Approve chaining approach; require() placement is correct as-is.The error-tolerant chaining successfully achieves the PR objective of bypassing GitHub's anti-recursion protection. The require() statement is intentionally placed inside the function body to ensure the module is only loaded when needed, avoiding the recursion check at initialization—moving it to the top would reintroduce the anti-recursion issue.
Error handling is compliant with guidelines: all async operations are wrapped in try/catch, errors include contextual metadata (message, status, issueNumber, assignee), context.payload fields are properly validated with optional chaining, and the non-blocking approach preserves the user assignment success.
No changes needed.
Signed-off-by: Mounil <[email protected]>
Signed-off-by: Mounil <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-gfi-assign-on-comment.js
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.js
⚙️ CodeRabbit configuration file
.github/scripts/**/*.js: Review JavaScript scripts as long-lived automation code.Scripts must remain:
Focused
Readable
Purpose-built
All
context.payloadfields MUST be validatedFree-form text MUST NOT be trusted
Dynamic code execution is prohibited
Avoid
child_process.exec; preferexecFileif neededAll async operations MUST be wrapped in try/catch
Errors MUST include contextual metadata
Duplicate API calls MUST be avoided
Marker-based deduplication is required
Scripts MUST NOT assume write access
Permission failures MUST be handled gracefully
Files:
.github/scripts/bot-gfi-assign-on-comment.js
🧬 Code graph analysis (1)
.github/scripts/bot-gfi-assign-on-comment.js (1)
.github/scripts/archive/gfi_notify_team.js (2)
context(42-42)context(43-43)
⏰ 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/bot-gfi-assign-on-comment.js.github/scripts/bot-mentor-assignment.js
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.js
⚙️ CodeRabbit configuration file
.github/scripts/**/*.js: Review JavaScript scripts as long-lived automation code.Scripts must remain:
Focused
Readable
Purpose-built
All
context.payloadfields MUST be validatedFree-form text MUST NOT be trusted
Dynamic code execution is prohibited
Avoid
child_process.exec; preferexecFileif neededAll async operations MUST be wrapped in try/catch
Errors MUST include contextual metadata
Duplicate API calls MUST be avoided
Marker-based deduplication is required
Scripts MUST NOT assume write access
Permission failures MUST be handled gracefully
Files:
.github/scripts/bot-mentor-assignment.js.github/scripts/bot-gfi-assign-on-comment.js
🧬 Code graph analysis (2)
.github/scripts/bot-mentor-assignment.js (1)
.github/scripts/bot-gfi-assign-on-comment.js (2)
context(88-88)context(89-89)
.github/scripts/bot-gfi-assign-on-comment.js (2)
.github/scripts/bot-mentor-assignment.js (1)
context(148-148).github/scripts/archive/gfi_notify_team.js (2)
context(42-42)context(43-43)
⏰ 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/scripts/bot-mentor-assignment.js (1)
131-134: LGTM! Parameter override correctly implemented.The function signature update and fallback logic correctly support the chaining use case:
- Line 131: Accepts optional
passedAssigneeparameter via destructuring with rename- Line 134: Falls back to
context.payload.assigneewhen no override is provided- The structure
{ login: string, type: string }matches what the calling code provides and what downstream validation expects (lines 136, 140, 149)This implementation properly resolves the context staleness issue by allowing the caller to explicitly provide the freshly assigned user.
|
Hi @Mounil2005 |
Signed-off-by: Mounil <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/bot-gfi-assign-on-comment.js
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.js
⚙️ CodeRabbit configuration file
.github/scripts/**/*.js: Review JavaScript scripts as long-lived automation code.Scripts must remain:
Focused
Readable
Purpose-built
All
context.payloadfields MUST be validatedFree-form text MUST NOT be trusted
Dynamic code execution is prohibited
Avoid
child_process.exec; preferexecFileif neededAll async operations MUST be wrapped in try/catch
Errors MUST include contextual metadata
Duplicate API calls MUST be avoided
Marker-based deduplication is required
Scripts MUST NOT assume write access
Permission failures MUST be handled gracefully
Files:
.github/scripts/bot-gfi-assign-on-comment.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 (2)
.github/scripts/bot-gfi-assign-on-comment.js (2)
34-36: LGTM: Case-insensitive label check is robust.The implementation correctly handles case variations in label names with appropriate type checking. The
somemethod with type guards prevents errors if label data is malformed.
207-224: The interface is correctly compatible. Thebot-mentor-assignment.jsscript is properly designed to accept theassigneeparameter (destructured aspassedAssigneeon line 131) and falls back tocontext.payload.assigneewhen not provided. The chaining code on lines 207-224 correctly:
- Passes
{ github, context, assignee: { login: requesterUsername, type: 'User' } }matching the expected interface- Wraps the async operation in try/catch per coding guidelines
- Includes contextual metadata in error logs (message, status, issueNumber, assignee)
- Gracefully handles mentor assignment failures without breaking the GFI assignment
Hey, I am sorry about that. |
|
Thank you for solving this ! |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1386 +/- ##
=======================================
Coverage 92.44% 92.44%
=======================================
Files 139 139
Lines 8528 8528
=======================================
Hits 7884 7884
Misses 644 644 🚀 New features to boost your workflow:
|
Description:
Chain Good First Issue assignment with mentor assignment to bypass GitHub's anti-recursion protection. Previously, mentor assignment relied on a separate workflow trigger that was blocked by GitHub's workflow-loop protection when the GFI assignment bot assigned users using GITHUB_TOKEN.
Related issue(s):
Fixes #1369
Notes for reviewer:
The implementation chains both actions within a single workflow execution, bypassing GitHub's anti-recursion protection. User assignment will always succeed, while mentor assignment failures are logged but don't affect the primary assignment flow.
Checklist