Add match scoring and resume download dropdown#151
Merged
Conversation
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a resume-download UI (select + view) into the Discord CRM cog and wires it into match results; augments candidate scoring with a float Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Interaction as Discord Interaction
participant CRMCog as CRM Cog
participant DB as Database
participant Audit as Audit Log
User->>Interaction: Clicks "Resume download" dropdown
Interaction->>CRMCog: MatchResumeSelect.callback(selected resume_id)
CRMCog->>CRMCog: Resolve cog instance & lookup contact_name
CRMCog->>Interaction: Defer interaction
CRMCog->>DB: Fetch resume blob for resume_id
DB-->>CRMCog: Resume content
CRMCog->>User: Send resume (DM or attachment)
CRMCog->>Audit: Record download event
Audit-->>CRMCog: Acknowledgement
CRMCog-->>Interaction: Send confirmation / follow-up message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/test_candidate_search.py (1)
309-341:⚠️ Potential issue | 🟠 MajorThis test does not exercise the new score-first ranking path.
The test data omits
match_score/has_crm_link, so it isn’t validating the new ordering logic you introduced. Set explicit row scores (and link flags) so the assertion reflects the intended behavior.✅ Suggested test adjustment
def test_search_candidates_secondary_sort_skill_score_over_discord_role() -> None: """Candidate with stronger skill match should rank above one with only role match.""" role_match_row = _make_row( crm_contact_id="role", is_member=False, discord_roles=["Full Stack"], discord_role_matched=1, skills=["python"], required_matched=1, required_skill_score=1, + has_crm_link=True, + match_score=14.0, ) skill_score_row = _make_row( crm_contact_id="skill", is_member=False, discord_roles=[], discord_role_matched=0, skills=["python", "django"], required_matched=2, required_skill_score=8, + has_crm_link=True, + match_score=36.0, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_candidate_search.py` around lines 309 - 341, The test test_search_candidates_secondary_sort_skill_score_over_discord_role currently omits the new score-first ranking fields; update the two test rows created via _make_row to include explicit match_score and has_crm_link values so the new ordering path in search_candidates is exercised (e.g. assign a higher match_score to the "skill" row and appropriate has_crm_link booleans), keep using _patch_db and the same reqs so the assertions (results[0].crm_contact_id == "skill", results[1].crm_contact_id == "role") validate the score-first logic.packages/shared/src/five08/candidate_search.py (1)
228-228:⚠️ Potential issue | 🟠 MajorApply final score filtering before truncating results.
LIMITis enforced in SQL before the Python-side final score (match_score + seniority) andmin_match_scorefilter run. That can drop candidates that should qualify and distort the top-N output for the same query.Also applies to: 266-267, 293-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/five08/candidate_search.py` at line 228, SQL LIMIT is currently applied before the Python-side final scoring (match_score + seniority) and min_match_score filtering, which can drop qualifying candidates; remove or avoid using LIMIT in the SQL in the search function(s) that build this query (the query block containing "LIMIT %s") so that you fetch an adequate result set, then in the corresponding function (where final_score is computed as match_score + seniority and min_match_score is applied) compute final_score for each row, filter out rows with final_score < min_match_score, sort by final_score descending, and only then truncate to the requested N results; apply this change consistently for the other two query sites mentioned (the occurrences at the other "LIMIT %s" blocks referenced in the comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 219-240: The audit writes inside the callback (calls to
cog._audit_command when reporting success and inside the except block) must be
made best-effort so they never change the user-facing flow; wrap each call to
cog._audit_command in its own local try/except that catches Exception and logs
the audit failure (e.g., logger.exception or logger.error) but does not
re-raise, leaving the existing interaction responses (interaction.followup.send
and the success path that uses download_ok/resume_id/self.values) unchanged.
- Around line 202-218: The callback in the SelectMenu handler (callback)
currently proceeds to call CRMCog._download_and_send_resume without checking the
invoking member's roles; add an authorization check at the start of callback
(before deferring or downloading) that inspects interaction.user or
interaction.user.roles / interaction.user.guild_permissions and verifies the
required role(s) or permission(s) (e.g., a "recruiter" role or a specific
permission) and if the user is not authorized send an ephemeral denial message
and return; ensure the check uses the same role/permission logic used elsewhere
in CRMCog for consistency and keep the call to _download_and_send_resume only
after authorization succeeds.
---
Outside diff comments:
In `@packages/shared/src/five08/candidate_search.py`:
- Line 228: SQL LIMIT is currently applied before the Python-side final scoring
(match_score + seniority) and min_match_score filtering, which can drop
qualifying candidates; remove or avoid using LIMIT in the SQL in the search
function(s) that build this query (the query block containing "LIMIT %s") so
that you fetch an adequate result set, then in the corresponding function (where
final_score is computed as match_score + seniority and min_match_score is
applied) compute final_score for each row, filter out rows with final_score <
min_match_score, sort by final_score descending, and only then truncate to the
requested N results; apply this change consistently for the other two query
sites mentioned (the occurrences at the other "LIMIT %s" blocks referenced in
the comment).
In `@tests/unit/test_candidate_search.py`:
- Around line 309-341: The test
test_search_candidates_secondary_sort_skill_score_over_discord_role currently
omits the new score-first ranking fields; update the two test rows created via
_make_row to include explicit match_score and has_crm_link values so the new
ordering path in search_candidates is exercised (e.g. assign a higher
match_score to the "skill" row and appropriate has_crm_link booleans), keep
using _patch_db and the same reqs so the assertions (results[0].crm_contact_id
== "skill", results[1].crm_contact_id == "role") validate the score-first logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46c090e2-8e80-44d0-a627-7a9ebf1fde24
📒 Files selected for processing (3)
apps/discord_bot/src/five08/discord_bot/cogs/crm.pypackages/shared/src/five08/candidate_search.pytests/unit/test_candidate_search.py
Comment on lines
+202
to
+218
| async def callback(self, interaction: discord.Interaction) -> None: | ||
| try: | ||
| cog = interaction.client.get_cog("CRMCog") # type: ignore[attr-defined] | ||
| if not cog: | ||
| await interaction.response.send_message( | ||
| "❌ CRM functionality not available.", ephemeral=True | ||
| ) | ||
| return | ||
|
|
||
| resume_id = self.values[0] | ||
| contact_name = self._resume_lookup.get(resume_id, "Unknown") | ||
|
|
||
| await interaction.response.defer(ephemeral=True) | ||
|
|
||
| download_ok = await cog._download_and_send_resume( | ||
| interaction, contact_name, resume_id | ||
| ) |
There was a problem hiding this comment.
Enforce role authorization in the resume select callback.
This callback currently allows resume download without a member-role check. Since results are posted in-thread, this opens an unauthorized resume access path.
🔐 Suggested fix
async def callback(self, interaction: discord.Interaction) -> None:
try:
cog = interaction.client.get_cog("CRMCog") # type: ignore[attr-defined]
if not cog:
await interaction.response.send_message(
"❌ CRM functionality not available.", ephemeral=True
)
return
+
+ if not cog._check_member_role(interaction):
+ cog._audit_command(
+ interaction=interaction,
+ action="crm.match_candidates_resume_select",
+ result="denied",
+ metadata={"reason": "missing_member_role"},
+ resource_type="discord_ui_action",
+ resource_id=self.values[0] if self.values else None,
+ )
+ await interaction.response.send_message(
+ "❌ You must have the Member role to download resumes.",
+ ephemeral=True,
+ )
+ return
resume_id = self.values[0]
contact_name = self._resume_lookup.get(resume_id, "Unknown")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py` around lines 202 - 218,
The callback in the SelectMenu handler (callback) currently proceeds to call
CRMCog._download_and_send_resume without checking the invoking member's roles;
add an authorization check at the start of callback (before deferring or
downloading) that inspects interaction.user or interaction.user.roles /
interaction.user.guild_permissions and verifies the required role(s) or
permission(s) (e.g., a "recruiter" role or a specific permission) and if the
user is not authorized send an ephemeral denial message and return; ensure the
check uses the same role/permission logic used elsewhere in CRMCog for
consistency and keep the call to _download_and_send_resume only after
authorization succeeds.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py (2)
202-218:⚠️ Potential issue | 🔴 CriticalEnforce role authorization before resume download in select callback.
At Line 211 onward,
MatchResumeSelect.callbackallows resume download withoutMemberrole validation, which creates an unauthorized access path from the public thread UI. This should mirrorResumeDownloadButton.callback’scog._check_member_role(interaction)gate before defer/download.🔐 Suggested fix
async def callback(self, interaction: discord.Interaction) -> None: try: cog = interaction.client.get_cog("CRMCog") # type: ignore[attr-defined] if not cog: await interaction.response.send_message( "❌ CRM functionality not available.", ephemeral=True ) return + + if not cog._check_member_role(interaction): + try: + cog._audit_command( + interaction=interaction, + action="crm.match_candidates_resume_select", + result="denied", + metadata={"reason": "missing_member_role"}, + resource_type="discord_ui_action", + resource_id=self.values[0] if self.values else None, + ) + except Exception as audit_exc: + logger.warning("Audit write failed in resume select deny path: %s", audit_exc) + await interaction.response.send_message( + "❌ You must have the Member role to download resumes.", + ephemeral=True, + ) + return resume_id = self.values[0] contact_name = self._resume_lookup.get(resume_id, "Unknown")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py` around lines 202 - 218, MatchResumeSelect.callback currently allows downloading resumes without verifying the caller's Member role; add the same authorization gate used in ResumeDownloadButton.callback by calling cog._check_member_role(interaction) (or the appropriate method on CRMCog) immediately after obtaining cog and before calling interaction.response.defer and cog._download_and_send_resume, send an ephemeral "not authorized" response and return when the check fails so unauthorized users cannot trigger the download path.
219-240:⚠️ Potential issue | 🟠 MajorMake audit writes best-effort in the select callback.
At Line 219 and Line 230, audit calls are in the main control path. If
_audit_commandraises, the user-facing success path can be turned into an error path. Wrap each audit write with localtry/exceptand continue the interaction flow.🛡️ Suggested fix
download_ok = await cog._download_and_send_resume( interaction, contact_name, resume_id ) - cog._audit_command( - interaction=interaction, - action="crm.match_candidates_resume_select", - result="success" if download_ok else "error", - metadata={"contact_name": contact_name}, - resource_type="crm_contact", - resource_id=resume_id, - ) + try: + cog._audit_command( + interaction=interaction, + action="crm.match_candidates_resume_select", + result="success" if download_ok else "error", + metadata={"contact_name": contact_name}, + resource_type="crm_contact", + resource_id=resume_id, + ) + except Exception as audit_exc: + logger.warning("Audit write failed in resume select success path: %s", audit_exc) except Exception as exc: logger.error("Unexpected error in match resume select: %s", exc) if "cog" in locals() and cog: - cog._audit_command( - interaction=interaction, - action="crm.match_candidates_resume_select", - result="error", - metadata={"error": str(exc)}, - resource_type="discord_ui_action", - resource_id=self.values[0] if self.values else None, - ) + try: + cog._audit_command( + interaction=interaction, + action="crm.match_candidates_resume_select", + result="error", + metadata={"error": str(exc)}, + resource_type="discord_ui_action", + resource_id=self.values[0] if self.values else None, + ) + except Exception as audit_exc: + logger.warning("Audit write failed in resume select error path: %s", audit_exc) await interaction.followup.send( "❌ An unexpected error occurred while downloading the resume." )As per coding guidelines, "Audit logging must be best effort: command flows in Discord cogs should never fail if audit writes fail".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py` around lines 219 - 240, The audit writes in the select callback (calls to cog._audit_command) must be made best-effort so failures don’t change the user flow; wrap each call to cog._audit_command (both the success-path call and the error-path call) in its own try/except that catches Exception, logs the audit failure (e.g., logger.error or logger.exception with context like action and resource_id), and does not re-raise so the interaction followup/send continues normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 202-218: MatchResumeSelect.callback currently allows downloading
resumes without verifying the caller's Member role; add the same authorization
gate used in ResumeDownloadButton.callback by calling
cog._check_member_role(interaction) (or the appropriate method on CRMCog)
immediately after obtaining cog and before calling interaction.response.defer
and cog._download_and_send_resume, send an ephemeral "not authorized" response
and return when the check fails so unauthorized users cannot trigger the
download path.
- Around line 219-240: The audit writes in the select callback (calls to
cog._audit_command) must be made best-effort so failures don’t change the user
flow; wrap each call to cog._audit_command (both the success-path call and the
error-path call) in its own try/except that catches Exception, logs the audit
failure (e.g., logger.error or logger.exception with context like action and
resource_id), and does not re-raise so the interaction followup/send continues
normally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4927b33e-6c24-4e79-a17a-aa73303a8b7b
📒 Files selected for processing (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/five08/candidate_search.py (1)
241-323:⚠️ Potential issue | 🟠 MajorApply final limiting after final score/filter, not before.
Line 241 limits rows before Line 280 seniority adjustment and Lines 306-307
min_match_scorefiltering. This can hide valid higher final-score candidates and return an incomplete top-N list for display.Proposed mitigation (over-fetch + final cap)
@@ def search_candidates( @@ ) -> list[CandidateMatch]: @@ - with get_postgres_connection(settings) as conn: + # Over-fetch because final score/min_match_score are applied in Python. + sql_limit = max(limit * 5, limit) + with get_postgres_connection(settings) as conn: with conn.cursor(row_factory=dict_row) as cursor: cursor.execute( query, ( @@ - limit, + sql_limit, ), ) rows = cursor.fetchall() @@ - return results + return results[:limit] if limit > 0 else results🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/five08/candidate_search.py` around lines 241 - 323, The current SQL LIMIT (passed as the variable limit into cursor.execute for query) truncates rows before we compute seniority-adjusted match_score and apply min_match_score and final sorting; change the approach to over-fetch and enforce the final cap in Python: update the DB call to request more rows (e.g., multiply limit by a safe factor or replace the SQL LIMIT with a larger parameter), then after building results, applying min_match_score and sorting (the existing results.sort lambda) slice the final list to the requested limit (e.g., results = results[:limit]) so the top-N returned respects post-processing adjustments; touch the code around query/cursor.execute, the variable limit, the results construction, the min_match_score filter, and the final slice.
🧹 Nitpick comments (2)
packages/shared/src/five08/candidate_search.py (2)
265-268: Move invariant set construction outside the row loop.Lines 266-268 rebuild the same sets for every row. Build them once before the loop to reduce per-row overhead.
Suggested refactor
- results: list[CandidateMatch] = [] - for row in rows: - candidate_skills: list[str] = row.get("skills") or [] - required_set = set(required_skills) - preferred_set = set(preferred_skills) - role_types_set = set(role_types) + results: list[CandidateMatch] = [] + required_set = set(required_skills) + preferred_set = set(preferred_skills) + role_types_set = set(role_types) + for row in rows: + candidate_skills: list[str] = row.get("skills") or []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/five08/candidate_search.py` around lines 265 - 268, The code is recreating the same sets (required_set, preferred_set, role_types_set) inside the per-row processing; move construction of required_set = set(required_skills), preferred_set = set(preferred_skills), and role_types_set = set(role_types) out of the row loop and compute them once before iterating rows (so only candidate_skills = row.get("skills") or [] remains per-row), updating any references in the loop to use these precomputed sets to eliminate repeated allocation and reduce per-row overhead.
309-320: Clarify the secondary-sort comment.Line 309 says the sort only breaks ties, but Lines 311-320 re-rank on the updated
match_scoreand other fields. Please update the comment to match behavior (or narrow the key if tie-break-only is intended).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/five08/candidate_search.py` around lines 309 - 320, The comment above the results.sort call is misleading: it says this is only a tie-breaker but the key lambda (used in results.sort) includes c.match_score and several other ranking fields (e.g., c.match_score, c.matched_required_skills, c.matched_preferred_skills, c.seniority_score), so update the comment to state that this sort fully re-ranks results based on those fields (preserving or explicitly changing primary SQL ranking as appropriate); alternatively, if you intended tie-break-only behavior, narrow the sort key to exclude c.match_score and any fields that would change the primary order and keep only true tie-breakers (e.g., c.is_member, c.has_crm_link, c.seniority_score) and then update the comment to reflect that tie-break-only intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/shared/src/five08/candidate_search.py`:
- Around line 241-323: The current SQL LIMIT (passed as the variable limit into
cursor.execute for query) truncates rows before we compute seniority-adjusted
match_score and apply min_match_score and final sorting; change the approach to
over-fetch and enforce the final cap in Python: update the DB call to request
more rows (e.g., multiply limit by a safe factor or replace the SQL LIMIT with a
larger parameter), then after building results, applying min_match_score and
sorting (the existing results.sort lambda) slice the final list to the requested
limit (e.g., results = results[:limit]) so the top-N returned respects
post-processing adjustments; touch the code around query/cursor.execute, the
variable limit, the results construction, the min_match_score filter, and the
final slice.
---
Nitpick comments:
In `@packages/shared/src/five08/candidate_search.py`:
- Around line 265-268: The code is recreating the same sets (required_set,
preferred_set, role_types_set) inside the per-row processing; move construction
of required_set = set(required_skills), preferred_set = set(preferred_skills),
and role_types_set = set(role_types) out of the row loop and compute them once
before iterating rows (so only candidate_skills = row.get("skills") or []
remains per-row), updating any references in the loop to use these precomputed
sets to eliminate repeated allocation and reduce per-row overhead.
- Around line 309-320: The comment above the results.sort call is misleading: it
says this is only a tie-breaker but the key lambda (used in results.sort)
includes c.match_score and several other ranking fields (e.g., c.match_score,
c.matched_required_skills, c.matched_preferred_skills, c.seniority_score), so
update the comment to state that this sort fully re-ranks results based on those
fields (preserving or explicitly changing primary SQL ranking as appropriate);
alternatively, if you intended tie-break-only behavior, narrow the sort key to
exclude c.match_score and any fields that would change the primary order and
keep only true tie-breakers (e.g., c.is_member, c.has_crm_link,
c.seniority_score) and then update the comment to reflect that tie-break-only
intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 267b80ce-e45f-446e-8402-c369551b6582
📒 Files selected for processing (1)
packages/shared/src/five08/candidate_search.py
This was referenced Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issue
How Has This Been Tested?
Summary by CodeRabbit
New Features
Enhancements
Tests