Extract job matching into a dedicated cog#175
Conversation
📝 WalkthroughWalkthroughRefactors Discord audit logging into a mixin, moves job-matching and resume UI/commands from CRMCog into a new JobsCog with role sync and resume extraction, adjusts candidate_search ranking/fallbacks, and updates tests to exercise JobsCog behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Discord User
participant Bot as Discord Bot
participant Jobs as JobsCog
participant Search as Candidate Search
participant CRM as CRM Backend
participant Extract as Resume Extractor
User->>Bot: /match-candidates or thread_create
Bot->>Jobs: deliver interaction / event
activate Jobs
Jobs->>Jobs: parse posting (content, tags, attachments)
Jobs->>Search: search_candidates(posting)
activate Search
Search-->>Jobs: ranked candidates
deactivate Search
Jobs->>Bot: present paginated results + MatchResumeSelect
Bot->>Jobs: resume selection callback
Jobs->>Extract: extract_text(from attachment/URL)
activate Extract
Extract-->>Jobs: extracted text
deactivate Extract
Jobs->>CRM: request resume download / audit via _audit_command
activate CRM
CRM-->>Jobs: ack / resource
deactivate CRM
Jobs->>Bot: deliver resume / final messages
deactivate Jobs
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Pull request overview
This PR extracts the job posting + matching workflow out of CRMCog into a dedicated JobsCog, keeping CRM-specific responsibilities (like resume download) in the CRM cog while consolidating the jobs pipeline in one place.
Changes:
- Added a new
JobsCogimplementing auto-match,/match-candidates, job-channel registration, and Discord role sync. - Removed the jobs workflow code from
CRMCogto reduce responsibility overlap. - Updated comments/docs in shared candidate search to clarify ranking and location hinting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/shared/src/five08/candidate_search.py | Updates ranking/location-related comments and adds resilience notes around SQL/Python scoring. |
| apps/discord_bot/src/five08/discord_bot/cogs/jobs.py | New cog containing job-channel registration, auto-match listeners, manual matching, link/attachment ingestion, and role sync. |
| apps/discord_bot/src/five08/discord_bot/cogs/crm.py | Removes job/matching-related functionality now owned by JobsCog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 7. Required skill strength score (sum of skill_attrs values). | ||
| 8. Preferred skill count matched. | ||
| 9. Seniority score (applied in Python after the query). | ||
|
|
There was a problem hiding this comment.
The ranking-priority docstring no longer mentions the US-only hard filter, but the SQL query still applies a hard filter when requirements.location_type == "us_only" (AND (NOT %s OR LOWER(...) = ANY(%s))). Please update the docstring to reflect that us_only is still enforced as an exclusionary filter (or adjust the query if the intent was to remove the hard filter).
| When requirements.location_type == "us_only", a hard US-only filter is applied in the | |
| underlying SQL query so that only candidates whose normalized address_country is | |
| interpreted as being in the US are returned; non-US candidates are excluded rather | |
| than merely being down-ranked by the location signal. |
| async def on_thread_create(self, thread: discord.Thread) -> None: | ||
| """Auto-run matching for new threads in registered channels.""" | ||
| guild = thread.guild | ||
| parent = thread.parent | ||
| if guild is None or parent is None: | ||
| return | ||
| if not await self._refresh_jobs_channel_cache_if_missing(guild.id): | ||
| return | ||
|
|
||
| if not self._is_jobs_channel_registered(guild.id, parent.id): | ||
| return | ||
|
|
||
| owner = guild.get_member(thread.owner_id) if thread.owner_id else None | ||
| if owner is None or owner.bot: | ||
| return | ||
| if not check_user_roles_with_hierarchy(owner.roles, ["Member"]): | ||
| return | ||
|
|
||
| await self._run_auto_match_candidates_for_thread( | ||
| thread=thread, | ||
| trigger="thread_create", | ||
| ) |
There was a problem hiding this comment.
Auto-match on on_thread_create will run in any registered parent channel, but unlike the previous implementation it no longer checks whether the parent channel is publicly visible to @everyone. That can leak candidate details in public forum/text channels. Add the same visibility guard used in on_message / the prior CRM implementation (skip auto-match publish when parent.permissions_for(guild.default_role).view_channel is true, and unmark the thread so it can be retried if needed).
| "Audit write failed in match resume select: %s", audit_exc | ||
| ) | ||
| await interaction.followup.send( | ||
| "❌ An unexpected error occurred while downloading the resume." |
There was a problem hiding this comment.
MatchResumeSelect.callback defers with ephemeral=True, but in the exception handler the follow-up message is sent without ephemeral=True. In discord.py follow-ups are not guaranteed to inherit ephemerality from the defer, so this error could be posted publicly in the thread/channel. Pass ephemeral=True to interaction.followup.send(...) here (and anywhere else in the callback where the intent is an ephemeral response).
| "❌ An unexpected error occurred while downloading the resume." | |
| "❌ An unexpected error occurred while downloading the resume.", | |
| ephemeral=True, |
| async def _fetch_match_candidates_link_text(self, url: str) -> str | None: | ||
| timeout = aiohttp.ClientTimeout(total=12) | ||
| headers = {"User-Agent": "508-job-match/1.0"} | ||
| try: | ||
| async with aiohttp.ClientSession( | ||
| timeout=timeout, headers=headers | ||
| ) as session: | ||
| async with session.get(url, allow_redirects=True) as response: | ||
| if response.status >= 400: | ||
| return None | ||
| content_type = ( | ||
| response.headers.get("Content-Type", "") | ||
| .split(";")[0] | ||
| .strip() | ||
| .lower() | ||
| ) | ||
| final_url = str(response.url) | ||
| raw = await response.read() | ||
| except Exception as exc: | ||
| logger.info("Failed fetching JD link url=%s error=%s", url, exc) | ||
| return None | ||
|
|
There was a problem hiding this comment.
_fetch_match_candidates_link_text fetches URLs derived from user-supplied content (message text/attachments/embeds) with no SSRF protections beyond scheme/netloc checks. This allows requests to internal/private IPs (e.g., 169.254.169.254, localhost, RFC1918 ranges) and can be abused via redirects. Add host validation before fetching (block localhost/private/reserved IPs, consider allowlisting known JD hosts, and/or cap redirects / re-validate the final URL).
…ured-post # Conflicts: # apps/discord_bot/src/five08/discord_bot/cogs/crm.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from five08.discord_bot.config import settings | ||
| from five08.discord_webhook import DiscordWebhookLogger |
There was a problem hiding this comment.
Importing settings at module import time makes five08.discord_bot.utils.audit eagerly instantiate Settings(), which can break unit tests and any context that imports the audit helpers without Discord-bot env vars already set. Consider removing the top-level from five08.discord_bot.config import settings and instead importing settings lazily inside create_discord_audit_logger() (or _init_audit_logger) so config validation only happens when a cog actually initializes the logger.
| Ranking priority: | ||
| 1. Members before prospects. | ||
| 2. US-only location restriction when enabled (hard filter; non-US candidates excluded). | ||
| 3. Location signal when posting has location constraints (explicit mismatch demoted). | ||
| 4. Match score (weighted score from skills + CRM/location signals). | ||
| 5. Timezone match (soft signal; 1 when candidate timezone is in preferred_timezones). | ||
| 6. Required skill count matched. | ||
| 7. Discord role type matched (1 if any discord role matches the required role types). | ||
| 8. Required skill strength score (sum of skill_attrs values). | ||
| 9. Preferred skill count matched. | ||
| 10. Seniority score (applied in Python after the query). | ||
| 2. Location signal when posting has location constraints (explicit mismatch demoted). | ||
| 3. Match score (weighted score from skills + CRM/location signals). | ||
| 4. Timezone match (soft signal; 1 when candidate timezone is in preferred_timezones). | ||
| 5. Required skill count matched. | ||
| 6. Discord role type matched (1 if any discord role matches the required role types). | ||
| 7. Required skill strength score (sum of skill_attrs values). | ||
| 8. Preferred skill count matched. | ||
| 9. Seniority score (applied in Python after the query). |
There was a problem hiding this comment.
The ranking docstring no longer mentions the us_only hard filter, but the SQL still applies it (-- Hard location filter when us_only ... LOWER(address_country) = ANY(us_values)). Please update the ranking/behavior docs to reflect that us_only excludes non‑US candidates rather than being only a soft ranking signal.
|
|
||
| try: | ||
| updated, skipped, failed = await self._bulk_sync_guild_roles(guild) | ||
| logger.info( | ||
| "Startup discord role sync: guild=%s updated=%d skipped=%d failed=%d", | ||
| guild.name, | ||
| updated, | ||
| skipped, | ||
| failed, | ||
| ) | ||
| except Exception as exc: | ||
| logger.warning( |
There was a problem hiding this comment.
on_thread_create can trigger auto-matching for threads created in registered parent channels (e.g., forum channels), but unlike the on_message path it doesn’t check whether the parent channel is publicly visible. This risks posting candidate match output (names/links) into a public channel. Add a permissions check similar to on_message (e.g., parent.permissions_for(guild.default_role).view_channel) and skip auto-match publishing when the parent is visible to @everyone.
| async def _fetch_match_candidates_link_text(self, url: str) -> str | None: | ||
| timeout = aiohttp.ClientTimeout(total=12) | ||
| headers = {"User-Agent": "508-job-match/1.0"} | ||
| try: | ||
| async with aiohttp.ClientSession( | ||
| timeout=timeout, headers=headers | ||
| ) as session: | ||
| async with session.get(url, allow_redirects=True) as response: | ||
| if response.status >= 400: | ||
| return None | ||
| content_type = ( | ||
| response.headers.get("Content-Type", "") | ||
| .split(";")[0] | ||
| .strip() | ||
| .lower() | ||
| ) | ||
| final_url = str(response.url) | ||
| raw = await response.read() | ||
| except Exception as exc: | ||
| logger.info("Failed fetching JD link url=%s error=%s", url, exc) | ||
| return None | ||
|
|
There was a problem hiding this comment.
_fetch_match_candidates_link_text will fetch any URL discovered in the job post (after a lightweight “likely JD” substring check). This can be abused for SSRF (e.g., hitting link-local/private IPs or internal services) and may also follow redirects to such hosts. Consider adding URL safety checks (require https, block localhost/private IP ranges and IP-literals, optionally enforce an allowlist of known job-board domains) before issuing the request.
![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 (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py (1)
2006-2022:⚠️ Potential issue | 🔴 CriticalThis extraction still breaks the repo’s existing
CRMCogjobs surface.CI is already failing with missing
CRMCogmembers/module symbols such asregister_jobs_channel,on_message,_resolve_jobs_channel_target,_get_role_id_cache, andDISCORD_ROLES_EXCLUDE_FROM_SYNC. Please migrate the remaining references toJobsCogin this PR, or keep temporary delegating shims/re-exports so the refactor does not land half-migrated.🤖 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 2006 - 2022, CRMCog lost several members during the refactor causing CI failures; restore compatibility by either migrating all callers to use JobsCog directly or adding delegating shims and re-exports: obtain the JobsCog instance with self.bot.get_cog("JobsCog") (store as self.jobs_cog) and implement wrapper methods on CRMCog named register_jobs_channel, on_message, _resolve_jobs_channel_target, and _get_role_id_cache that simply call through to the corresponding JobsCog methods; also re-export or attach DISCORD_ROLES_EXCLUDE_FROM_SYNC on the module/CRMCog by importing it from the JobsCog module and exposing it under the expected name so existing references continue to work.
🧹 Nitpick comments (3)
packages/shared/src/five08/candidate_search.py (1)
178-191: Minor docstring inconsistency:has_crm_linkordering not documented.The SQL ORDER BY (line 381) and Python sort (line 489) both rank by
has_crm_link DESCimmediately afteris_member, but the docstring only mentions "Members before prospects" and omits this intermediate ranking criterion. This could mislead readers about actual tie-breaking behavior.📝 Suggested docstring update
Ranking priority: 1. Members before prospects. - 2. Location signal when posting has location constraints (explicit mismatch demoted). - 3. Match score (weighted score from skills + CRM/location signals). - 4. Timezone match (soft signal; 1 when candidate timezone is in preferred_timezones). - 5. Required skill count matched. - 6. Discord role type matched (1 if any discord role matches the required role types). - 7. Required skill strength score (sum of skill_attrs values). - 8. Preferred skill count matched. - 9. Seniority score (applied in Python after the query). + 2. CRM-linked candidates before Discord-only entries. + 3. Location signal when posting has location constraints (explicit mismatch demoted). + 4. Match score (weighted score from skills + CRM/location signals). + 5. Timezone match (soft signal; 1 when candidate timezone is in preferred_timezones). + 6. Required skill count matched. + 7. Discord role type matched (1 if any discord role matches the required role types). + 8. Required skill strength score (sum of skill_attrs values). + 9. Preferred skill count matched. + 10. Seniority score (applied in Python after the query).🤖 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 178 - 191, Docstring omission: the ranking logic described for candidate ordering omits the intermediate tie-breaker "has_crm_link" used in the SQL ORDER BY and Python sort. Update the docstring in packages/shared/src/five08/candidate_search.py to list the full ranking priority including "has_crm_link DESC" immediately after "is_member" (i.e., "Members before prospects" -> "is_member, has_crm_link"), and ensure the written ordering matches the actual ORDER BY clause and the Python sort that reference has_crm_link so readers see the true tie-breaking behavior.apps/discord_bot/src/five08/discord_bot/utils/audit.py (1)
417-423: Consider documenting the initialization requirement.The mixin declares
audit_loggeras a type annotation without a default value. If a cog forgets to call_init_audit_logger()in its__init__, subsequent calls to_audit_command()will raiseAttributeError. A brief docstring note would help prevent misuse.📝 Suggested docstring clarification
class DiscordAuditCogMixin: - """Shared audit helpers for Discord bot cogs.""" + """Shared audit helpers for Discord bot cogs. + + Cogs using this mixin must call ``_init_audit_logger()`` in their + ``__init__`` before invoking any audit methods. + """ audit_logger: DiscordAuditLogger🤖 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/utils/audit.py` around lines 417 - 423, Add a short note to the DiscordAuditCogMixin class docstring stating that the audit_logger attribute must be initialized by calling _init_audit_logger (typically from the cog's __init__) before any calls to _audit_command; mention that failing to do so will raise AttributeError. Also consider adding a guard in _audit_command to raise a clear RuntimeError if audit_logger is missing to make the failure explicit at runtime.apps/discord_bot/src/five08/discord_bot/cogs/jobs.py (1)
268-282: Consider logging the exception when fetching starter message fails.The bare
exceptat line 275 silently swallows all exceptions. While the fallback behavior is correct, logging the failure would aid debugging whenfetch_messageconsistently fails.♻️ Proposed fix
if starter is None: try: starter = await thread.fetch_message(thread.id) - except Exception: + except Exception as exc: + logger.debug("Failed to fetch starter message for thread %s: %s", thread.id, exc) starter = None🤖 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/jobs.py` around lines 268 - 282, In _read_thread_post, the bare except around await thread.fetch_message(thread.id) swallows errors; change it to except Exception as e and log the exception (e.g., logger.exception or process_logger.error with the exception) before setting starter = None so failures to fetch the starter message are recorded; update the except block that follows thread.fetch_message to capture the exception variable and emit a clear log message including thread.id and the exception details.
🤖 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/jobs.py`:
- Around line 1337-1342: Several calls to _audit_command inside the
crm.match_candidates flow (and other spots) are not best-effort wrapped and can
raise, aborting the command; wrap each offending call to _audit_command
(references: the calls at lines reported in this review such as the call
currently passing interaction=interaction, action="crm.match_candidates",
result="error", metadata={"stage":"not_thread"} and the other similar calls) in
a try/except that catches Exception and handles failures non-fatally (e.g.,
logger.exception or logger.error the exception and continue)—ensure every
_audit_command invocation in this command and the other listed locations is
enclosed in such a try/except so audit failures do not interrupt the Discord
command flow.
- Around line 1573-1583: The call to _audit_command in the
crm.sync_discord_roles flow must be made best-effort so audit failures don't
abort the command; wrap the existing self._audit_command(...) invocation in a
try/except that catches broad exceptions (Exception), logs the exception using
the cog's logger (or process logger) with context (e.g., action name and
metadata), and does not re-raise so the subsequent success message to the
interaction still runs; ensure you reference the same metadata variables
(updated, skipped, failed) when logging the error for debugging.
---
Outside diff comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 2006-2022: CRMCog lost several members during the refactor causing
CI failures; restore compatibility by either migrating all callers to use
JobsCog directly or adding delegating shims and re-exports: obtain the JobsCog
instance with self.bot.get_cog("JobsCog") (store as self.jobs_cog) and implement
wrapper methods on CRMCog named register_jobs_channel, on_message,
_resolve_jobs_channel_target, and _get_role_id_cache that simply call through to
the corresponding JobsCog methods; also re-export or attach
DISCORD_ROLES_EXCLUDE_FROM_SYNC on the module/CRMCog by importing it from the
JobsCog module and exposing it under the expected name so existing references
continue to work.
---
Nitpick comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/jobs.py`:
- Around line 268-282: In _read_thread_post, the bare except around await
thread.fetch_message(thread.id) swallows errors; change it to except Exception
as e and log the exception (e.g., logger.exception or process_logger.error with
the exception) before setting starter = None so failures to fetch the starter
message are recorded; update the except block that follows thread.fetch_message
to capture the exception variable and emit a clear log message including
thread.id and the exception details.
In `@apps/discord_bot/src/five08/discord_bot/utils/audit.py`:
- Around line 417-423: Add a short note to the DiscordAuditCogMixin class
docstring stating that the audit_logger attribute must be initialized by calling
_init_audit_logger (typically from the cog's __init__) before any calls to
_audit_command; mention that failing to do so will raise AttributeError. Also
consider adding a guard in _audit_command to raise a clear RuntimeError if
audit_logger is missing to make the failure explicit at runtime.
In `@packages/shared/src/five08/candidate_search.py`:
- Around line 178-191: Docstring omission: the ranking logic described for
candidate ordering omits the intermediate tie-breaker "has_crm_link" used in the
SQL ORDER BY and Python sort. Update the docstring in
packages/shared/src/five08/candidate_search.py to list the full ranking priority
including "has_crm_link DESC" immediately after "is_member" (i.e., "Members
before prospects" -> "is_member, has_crm_link"), and ensure the written ordering
matches the actual ORDER BY clause and the Python sort that reference
has_crm_link so readers see the true tie-breaking behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5d244db-bbf4-48c3-bbe2-8ec893826865
📒 Files selected for processing (6)
apps/discord_bot/src/five08/discord_bot/cogs/admin_login.pyapps/discord_bot/src/five08/discord_bot/cogs/crm.pyapps/discord_bot/src/five08/discord_bot/cogs/jobs.pyapps/discord_bot/src/five08/discord_bot/cogs/migadu.pyapps/discord_bot/src/five08/discord_bot/utils/audit.pypackages/shared/src/five08/candidate_search.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| if not check_user_roles_with_hierarchy(owner.roles, ["Member"]): | ||
| return | ||
|
|
There was a problem hiding this comment.
Auto-matching now publishes match results for any registered forum channel without checking whether the forum is publicly viewable. Previously the auto-match path explicitly skipped publishing when the parent channel was visible to the guild’s default role, to avoid leaking candidate/resume metadata. Consider restoring an equivalent permission check (e.g., parent.permissions_for(guild.default_role).view_channel) and bailing out (and/or only allowing auto-match in private member-only forums).
| # Only run auto-match in forums that are not publicly visible to the default role. | |
| default_role = guild.default_role | |
| if parent.permissions_for(default_role).view_channel: | |
| logger.info( | |
| "Skipping auto-match for publicly visible forum channel %s (%s) in guild %s", | |
| parent.id, | |
| parent.name, | |
| guild.name, | |
| ) | |
| return |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
apps/discord_bot/src/five08/discord_bot/cogs/jobs.py (2)
1298-1303:⚠️ Potential issue | 🟡 MinorMake these audit writes best-effort too.
These
_audit_commandcalls are still bare. If audit ingest raises in any of these branches, the user-facing/match-candidatesflow fails even though the command logic already knows how to continue.As per coding guidelines: "Audit logging must be best effort: command flows in Discord cogs should never fail if audit writes fail"
Also applies to: 1329-1334, 1344-1352, 1376-1381, 1389-1394, 1413-1418, 1432-1444
🤖 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/jobs.py` around lines 1298 - 1303, The _audit_command calls inside the jobs cog are currently unguarded and can raise, which must not break the /match-candidates flow; wrap each invocation of _audit_command (the calls that pass interaction=interaction and action="crm.match_candidates") in a best-effort try/except: call _audit_command as before, but catch Exception, swallow it (do not re-raise) and optionally emit a non-fatal logger.warning or logger.exception noting the audit write failed and include the exception and relevant metadata (interaction id/command/action) so failures are recorded without affecting user flow; apply this pattern to all occurrences referenced (the blocks around lines with the existing _audit_command calls: the current one and the duplicates noted).
1468-1478:⚠️ Potential issue | 🟡 MinorKeep the role-sync audit write best-effort.
This success-path audit call is still unwrapped, so an audit outage can abort the command after the expensive sync has already completed.
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/jobs.py` around lines 1468 - 1478, The audit write using self._audit_command(...) in the success path of the crm.sync_discord_roles flow must be made best-effort so an audit backend outage cannot abort the completed sync; wrap the call to self._audit_command(interaction=interaction, action="crm.sync_discord_roles", ...) in a try/except that catches broad exceptions (or specific audit-client exceptions) and logs the failure (e.g., self.logger.warning or process logger) without re-raising, ensuring the command returns success even if the audit write fails.
🤖 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/jobs.py`:
- Around line 682-720: _fetch_match_candidates_link_text fetches arbitrary URLs
with redirects allowed and reads the entire response into memory; to fix it,
validate and block private/internal/loopback IPs and disallowed hostnames after
resolving the URL (and after following redirects) before downloading, disable or
limit redirects (set allow_redirects=False or cap follow depth), stream the
response instead of await response.read() and enforce a strict maximum byte
limit when reading (stop and close connection if exceeded), and skip body
download for disallowed content-types by checking
response.headers["Content-Type"] first; reference symbols:
_fetch_match_candidates_link_text, session.get(..., allow_redirects=...),
response.headers / response.url / response.read(), and
MATCH_CANDIDATES_MAX_LINK_TEXT_CHARS.
- Around line 1366-1386: The try/except around extract_job_requirements only
catches RuntimeError so other exceptions escape; change the handler to catch all
extraction failures (e.g., use except Exception as exc, optionally excluding
asyncio.CancelledError if you need cancellation propagation) and keep the
existing behavior: call self._audit_command with stage "extract_requirements"
and the error string, then send the same interaction.followup.send user-facing
message and return; update the except clause in the jobs.py block containing
extract_job_requirements to reflect this broader catch.
- Around line 1492-1523: The incremental member-update handler is writing bot
accounts back into the DB; add a guard to skip bot members (same behavior as
_bulk_sync_guild_roles) by returning early when the Member is a bot (check
after.bot) before computing roles_changed/name_changed and before calling
upsert_discord_member or update_person_discord_roles (refer to the
on_member_update handler and the calls to upsert_discord_member and
update_person_discord_roles).
- Around line 1342-1364: Move the interaction acknowledgement before starting
I/O: call interaction.response.defer(ephemeral=False) before invoking
_build_match_candidates_posting(starter) to avoid the 3s timeout, then update
any calls that currently use interaction.response.send_message after the defer
(e.g., the error send when posting.strip() is empty) to use
interaction.followup.send instead; keep the existing _audit_command calls and
metadata logic intact, only reorder the defer and switch subsequent
response.send_message usages to followup.send.
- Around line 1061-1092: on_ready currently runs a full refresh on every
reconnect; add an idempotent guard (e.g., self._startup_synced boolean)
initialized False in the Cog __init__ and set it True after the first successful
run so subsequent on_ready calls return immediately. Update the on_ready
listener to check self._startup_synced at the top and skip refreshing
role/channel caches and calling _bulk_sync_guild_roles if True; ensure the flag
is only set after the sync completes successfully (or set/cleared appropriately
on failure) to avoid repeated full DB syncs on reconnects.
---
Duplicate comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/jobs.py`:
- Around line 1298-1303: The _audit_command calls inside the jobs cog are
currently unguarded and can raise, which must not break the /match-candidates
flow; wrap each invocation of _audit_command (the calls that pass
interaction=interaction and action="crm.match_candidates") in a best-effort
try/except: call _audit_command as before, but catch Exception, swallow it (do
not re-raise) and optionally emit a non-fatal logger.warning or logger.exception
noting the audit write failed and include the exception and relevant metadata
(interaction id/command/action) so failures are recorded without affecting user
flow; apply this pattern to all occurrences referenced (the blocks around lines
with the existing _audit_command calls: the current one and the duplicates
noted).
- Around line 1468-1478: The audit write using self._audit_command(...) in the
success path of the crm.sync_discord_roles flow must be made best-effort so an
audit backend outage cannot abort the completed sync; wrap the call to
self._audit_command(interaction=interaction, action="crm.sync_discord_roles",
...) in a try/except that catches broad exceptions (or specific audit-client
exceptions) and logs the failure (e.g., self.logger.warning or process logger)
without re-raising, ensuring the command returns success even if the audit write
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 399770a7-ec2f-44b5-bc09-d552e4a2804c
📒 Files selected for processing (2)
apps/discord_bot/src/five08/discord_bot/cogs/jobs.pytests/unit/test_crm.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_crm.py (1)
863-874: Consider extracting repeatedDummyForumChannelandDummyThreadinto shared fixtures.The
DummyForumChannelclass (andDummyThread) is defined identically in 15+ test methods. Consolidating these into module-level fixtures or helper classes would reduce duplication and make future maintenance easier.♻️ Example consolidation
# At module level or in a conftest.py class DummyForumChannel: def __init__(self, channel_id: int, name: str = "jobs") -> None: self.id = channel_id self.name = name def permissions_for(self, _role): return Mock(view_channel=False) class DummyThread: def __init__(self, parent: DummyForumChannel) -> None: self.parent = parent `@pytest.fixture` def dummy_forum_channel(): return DummyForumChannel(456, "jobs") `@pytest.fixture` def dummy_thread(dummy_forum_channel): return DummyThread(dummy_forum_channel)Then tests can inject these fixtures as needed rather than redefining them locally.
Also applies to: 1007-1017, 1030-1036, 1052-1064, 1090-1102, 1129-1137, 1157-1165, 1200-1210, 1230-1240, 1264-1274, 1303-1315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_crm.py` around lines 863 - 874, Extract the repeated DummyForumChannel and DummyThread definitions into shared fixtures to remove duplication: define module-level classes DummyForumChannel and DummyThread once, add pytest fixtures (e.g., dummy_forum_channel and dummy_thread) that construct these objects, and update tests that currently define local DummyForumChannel/DummyThread to accept and use the fixtures instead; ensure the fixture constructors match the tests' expectations (attributes like id, name, parent, applied_tags, and any permissions_for behavior) so no test logic needs further changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test_crm.py`:
- Around line 863-874: Extract the repeated DummyForumChannel and DummyThread
definitions into shared fixtures to remove duplication: define module-level
classes DummyForumChannel and DummyThread once, add pytest fixtures (e.g.,
dummy_forum_channel and dummy_thread) that construct these objects, and update
tests that currently define local DummyForumChannel/DummyThread to accept and
use the fixtures instead; ensure the fixture constructors match the tests'
expectations (attributes like id, name, parent, applied_tags, and any
permissions_for behavior) so no test logic needs further changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae9cd703-41dd-4710-86d1-b5f5c1e2ded7
📒 Files selected for processing (4)
apps/discord_bot/src/five08/discord_bot/cogs/jobs.pyapps/discord_bot/src/five08/discord_bot/utils/audit.pypackages/shared/src/five08/candidate_search.pytests/unit/test_crm.py
Description
Extracted the auto-match,
/match-candidates, job-channel registration, and Discord role sync flow into a newJobsCogsoCRMCogno longer owns the jobs workflow.Added targeted comments around posting extraction, locality-role inference, auto-match dedupe, and candidate ranking to make the end-to-end flow easier to follow.
Kept resume downloads delegated through the CRM cog and preserved the existing candidate search behavior while clarifying the ranking docs/comments.
Related Issue
N/A
How Has This Been Tested?
python3 -m py_compile apps/discord_bot/src/five08/discord_bot/cogs/crm.py apps/discord_bot/src/five08/discord_bot/cogs/jobs.py packages/shared/src/five08/candidate_search.pyruff check apps/discord_bot/src/five08/discord_bot/cogs/crm.py apps/discord_bot/src/five08/discord_bot/cogs/jobs.py packages/shared/src/five08/candidate_search.pyPre-commit hooks during
git commit(ruff,ruff format,mypy).Summary by CodeRabbit
New Features
Refactor
Removed
Bug Fixes