feat: implement issue recommendation system for contributor progression#1290
feat: implement issue recommendation system for contributor progression#1290parvninama wants to merge 13 commits intohiero-ledger:mainfrom
Conversation
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: parvninama <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
|
Hey @parvninama - are you still working on this, or are you ready for a review? If you're ready, go ahead and mark it as ready for review and I'll take a look! |
|
I’m still working on it — just a few minor changes and tests left. I’ll mark it ready for review once those are done. |
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
Signed-off-by: Parv <ninamaparv@gmail.com>
|
All changes and tests are complete — marking this as ready for review now. Thanks! |
|
Hey @parvninama 👋 thanks for the PR! This comment updates automatically as you push changes -- think of it as your PR's live scoreboard! PR Checks✅ DCO Sign-off -- All commits have valid sign-offs. Nice work! ✅ GPG Signature -- All commits have verified GPG signatures. Locked and loaded! ✅ Merge Conflicts -- No merge conflicts detected. Smooth sailing! ✅ Issue Link -- Linked to #1261 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
rwalworth
left a comment
There was a problem hiding this comment.
Thanks for putting this together @parvninama! The structure looks great - the new workflow, entry point, and command module all follow the existing patterns nicely, and the skill-level progression logic (next → same → fallback, skipping GFI fallback for beginners) is implemented correctly. There are a couple blocking issues.
issue (blocking, test): The PR introduces commands/recommend-issues.js (with skill-level selection, recommendation algorithm, and fallback logic) and resolveLinkedIssue in helpers/api.js, but no test files are included.
Looking at the existing pattern in this repo - test-assign-bot.js covers the assign command, test-on-pr-open-bot.js and test-on-pr-update-bot.js cover their entry points, and test-api.js covers the API helpers - new command modules and API additions are expected to have matching tests.
To match that pattern, please add:
- A test file for the recommendation logic (e.g.,
tests/test-recommend-bot.js) covering at minimum:getIssueSkillLevel- correct label detectiongetNextLevel/getFallbackLevel- boundary and normal cases (especially the BEGINNER → GFI skip)getRecommendedIssues- that the correct priority order is used and thenullfallback triggers an error commenthandleRecommendIssues- that missing sender/issue/skillLevel all short-circuit gracefully
- Tests for
resolveLinkedIssueintests/test-api.js(or the new file) - A new step in
.github/workflows/zxc-test-bot-scripts.yamlto run the new test file
Let me know if you'd like to see examples of how the existing tests are structured - happy to help!
| /** | ||
| * Skill hierarchy used to determine progression for recommendations. | ||
| */ | ||
| const SKILL_HIERARCHY = Object.freeze([ | ||
| LABELS.GOOD_FIRST_ISSUE, | ||
| LABELS.BEGINNER, | ||
| LABELS.INTERMEDIATE, | ||
| LABELS.ADVANCED, | ||
| ]); |
There was a problem hiding this comment.
issue (blocking): The PR adds SKILL_HIERARCHY to helpers/constants.js, which is the right place for it - but commands/assign-comments.js still defines its own local copy of the same array (line ~56 of that file). For the extraction to be complete, assign-comments.js should import SKILL_HIERARCHY from helpers instead of defining it locally:
// commands/assign-comments.js
-const SKILL_HIERARCHY = [
- LABELS.GOOD_FIRST_ISSUE,
- LABELS.BEGINNER,
- LABELS.INTERMEDIATE,
- LABELS.ADVANCED,
-];
// (SKILL_HIERARCHY is now imported from helpers via the existing destructure at the top)
const { MAINTAINER_TEAM, LABELS, ISSUE_STATE, SKILL_HIERARCHY } = require('../helpers');Without this change there are two separate sources of truth - if someone updates the hierarchy in constants.js later, assign-comments.js would silently keep the old order.
Note: the local definition in assign-comments.js is not frozen while the new one in constants.js is - a minor inconsistency, but another reason to consolidate.
| /** | ||
| * Returns the highest difficulty level of an issue based on its labels. | ||
| * | ||
| * Checks labels against SKILL_HIERARCHY in descending order and returns the first match. | ||
| * | ||
| * @param {{ labels: Array<string|{ name: string }> }} issue | ||
| * @returns {string|null} Matching level or null if none found. | ||
| */ | ||
| function getIssueSkillLevel(issue) { | ||
| for (const level of [...SKILL_HIERARCHY].reverse()) { | ||
| if (hasLabel(issue, level)) return level; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
thought: commands/assign.js already defines a getIssueSkillLevel function - but the two implementations iterate in opposite directions:
// assign.js — iterates forward (returns LOWEST matching level)
for (const level of SKILL_HIERARCHY) { ... }
// recommend-issues.js — iterates reversed (returns HIGHEST matching level)
for (const level of [...SKILL_HIERARCHY].reverse()) { ... }For a typical single-label issue they return the same result, so there's no bug today. But the name collision is a long-term trap - someone reading either file would expect them to behave identically.
This is out of scope for this PR, but worth a follow-up issue to either:
- Rename the recommend-issues version to
getHighestIssueSkillLevelto make the intent explicit, or - Extract a shared helper to
helperswith ahighestoption
The assign.js version might also benefit from the same highest behavior for correctness, but that's a separate discussion.
| const logger = { | ||
| log: (...args) => getLogger().log(...args), | ||
| error: (...args) => getLogger().error(...args), | ||
| }; |
There was a problem hiding this comment.
thought: The logger delegation pattern is now repeated in three command files:
const logger = {
log: (...args) => getLogger().log(...args),
error: (...args) => getLogger().error(...args),
};Minor, but worth a follow-up to add a getDelegatingLogger() export to helpers/logger.js so command files can just do const logger = getDelegatingLogger(). No action needed on this PR.
| group: on-pr-close-${{ github.event.pull_request.number }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
nitpick: The concurrency block in on-pr-close.yaml indents its children with 8 spaces, while on-pr.yaml uses 6 spaces (consistent with 2-space YAML indentation under a 4-space job block). The YAML is valid either way, but aligning with the existing style would be cleaner:
concurrency:
group: on-pr-close-${{ github.event.pull_request.number }}
cancel-in-progress: true
Changes:
Issue recommendation workflow on PR merge
Linked issue resolution
resolveLinkedIssuehelper usingclosingIssuesReferences(GraphQL)Difficulty-based progression system
SKILL_HIERARCHYin constants (frozen):Issue filtering and discovery
ready for devissues in a single API call (per_page: 50)ready for devlabel requiredError handling and resilience
[]→ no issues foundnull→ API failureRecommendation comment system
Code structure improvements
SKILL_HIERARCHYtohelpers/constants.jsfor shared use acrossapi.jsandrecommend-issues.jsresolveLinkedIssue)Testing:
Case 1 : Next level recommendation
Case 2: Same level recommendation
Case 3: Fallback level recommendation
Case 4: Multiple linked issues
Case 5: No linked issues
Case 6: No READY_FOR_DEV issues
Linked Issue: