feat: add CRM people sync cache and human audit event ingestion#34
Merged
Conversation
📝 WalkthroughWalkthroughIntroduces audit logging for Discord CRM commands, a CRM→people sync pipeline with scheduler and jobs, DB migrations for people and audit_events, worker API endpoints for sync/audit/webhook, shared audit/people persistence utilities, settings updates, namespace-package removals, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant DiscordBot as Discord Bot
participant CRMCog as CRM Cog
participant AuditLogger as DiscordAuditLogger
participant WorkerAPI as Worker API
participant Database as Postgres
DiscordBot->>CRMCog: /search-members (interaction)
CRMCog->>CRMCog: execute command logic
CRMCog->>AuditLogger: log_command(action, result, metadata)
AuditLogger->>AuditLogger: build payload & spawn background task
AuditLogger-->>DiscordBot: return immediately (best-effort)
par Async background
AuditLogger->>WorkerAPI: POST /audit/events (payload)
WorkerAPI->>Database: insert_audit_event(payload)
Database-->>WorkerAPI: created event (with person_id)
WorkerAPI-->>AuditLogger: 201 Created
end
sequenceDiagram
participant API as Worker API
participant Scheduler as CRM Sync Scheduler
participant Processor as PeopleSyncProcessor
participant Client as EspoPeopleSyncClient
participant EspoCRM as EspoCRM API
participant Database as Postgres
API->>Scheduler: start (if enabled)
Scheduler->>Scheduler: periodic trigger
Scheduler->>API: enqueue sync_people_from_crm_job
API->>Processor: sync_all_contacts()
loop per page
Processor->>Client: list_contact_page(offset, page_size)
Client->>EspoCRM: GET /api/v1/Contact?offset=X&limit=Y
EspoCRM-->>Client: contact page
Client-->>Processor: page
end
loop per contact
Processor->>Processor: _to_person_record(raw)
Processor->>Database: upsert_person(person_record)
Database-->>Processor: person_id
end
Processor-->>API: sync result (synced/failed counts)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 11
🧹 Nitpick comments (21)
tests/unit/test_shared_queue.py (1)
24-34: Consider splitting into two focused tests.The function currently asserts two distinct behaviours: (1) a known status maps correctly (
"queued"→QUEUED) and (2) an unknown status falls back toFAILEDand emits a warning. Mixing them under…_unknown_valuesmakes it harder to identify which path broke on failure.♻️ Proposed split
-def test_parse_status_handles_unknown_values() -> None: - """Unknown DB status should fallback to FAILED and emit a warning.""" - assert _parse_status("queued") == JobStatus.QUEUED - - with patch("five08.queue.logger.warning") as mock_warning: - result = _parse_status("unexpected-status") - - assert result == JobStatus.FAILED - mock_warning.assert_called_once_with( - "Unknown job status from DB: %s", "unexpected-status" - ) +def test_parse_status_known_value() -> None: + """Known DB status strings should map to the corresponding JobStatus.""" + assert _parse_status("queued") == JobStatus.QUEUED + + +def test_parse_status_unknown_value_falls_back_to_failed_with_warning() -> None: + """Unknown DB status should fall back to FAILED and emit a warning.""" + with patch("five08.queue.logger.warning") as mock_warning: + result = _parse_status("unexpected-status") + + assert result == JobStatus.FAILED + mock_warning.assert_called_once_with( + "Unknown job status from DB: %s", "unexpected-status" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_shared_queue.py` around lines 24 - 34, Split the current test_parse_status_handles_unknown_values into two focused tests: one that verifies _parse_status("queued") returns JobStatus.QUEUED, and a second that verifies an unknown status (e.g. "unexpected-status") causes _parse_status to return JobStatus.FAILED and emits a warning via five08.queue.logger.warning; in the second test use patch("five08.queue.logger.warning") as mock_warning, call result = _parse_status("unexpected-status"), assert result == JobStatus.FAILED, and assert mock_warning.assert_called_once_with("Unknown job status from DB: %s", "unexpected-status").apps/worker/Dockerfile (1)
15-16: Consolidategroupadd/useradd/chowninto oneRUNto reduce image layers.Two separate
RUNinstructions means an extra writable layer in the image, which also inflates the image size since thechown -Rcopies the inodes for everything installed byuv sync.♻️ Proposed consolidation
-RUN groupadd -r appgroup && useradd -r -g appgroup -d /app -s /usr/sbin/nologin appuser -RUN chown -R appuser:appgroup /app +RUN groupadd -r appgroup && useradd -r -g appgroup -d /app -s /usr/sbin/nologin appuser \ + && chown -R appuser:appgroup /app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/Dockerfile` around lines 15 - 16, Consolidate the two Dockerfile RUN steps into a single RUN so groupadd, useradd and chown happen in one layer: perform groupadd -r appgroup, useradd -r -g appgroup -d /app -s /usr/sbin/nologin appuser, and then chown -R appuser:appgroup /app all within the same RUN to avoid creating an extra writable layer; update the Dockerfile lines that reference groupadd, useradd, chown, appuser, appgroup and /app accordingly.apps/worker/src/five08/worker/models.py (1)
3-5: Optional: consolidate the twofrom typing importstatements.
LiteralandAnyare both fromtyping; a single import is marginally cleaner.♻️ Proposed cleanup
-from typing import Literal from typing import Any +from typing import Any, Literal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/models.py` around lines 3 - 5, Consolidate the two typing imports in apps/worker/src/five08/worker/models.py by replacing the separate "from typing import Literal" and "from typing import Any" with a single import that includes both symbols (e.g., import Literal and Any together from typing); update the import line that currently references Literal and Any so there's only one from typing import statement.tests/unit/test_skills_extractor.py (1)
6-12: Optional: exercise the public interface instead of the private method.
SkillsExtractor()withoutOPENAI_API_KEYleavesself.client = None, soextract_skills(...)delegates straight to_extract_skills_heuristic. Calling the public entry point would keep the test decoupled from the internal routing detail and still cover the regex fix.♻️ Proposed refactor
- result = extractor._extract_skills_heuristic("Built services in Go and Python") + result = extractor.extract_skills("Built services in Go and Python")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_skills_extractor.py` around lines 6 - 12, Change the test to exercise the public API by calling SkillsExtractor.extract_skills(...) instead of the private _extract_skills_heuristic; instantiate SkillsExtractor() (which may set self.client=None when OPENAI_API_KEY is absent) and call extractor.extract_skills("Built services in Go and Python"), then assert that "go" and "python" are present in the returned object's .skills so the test stays decoupled from internal routing and still verifies the two-letter token handling.apps/worker/src/five08/worker/migrations/versions/20260221_0200_create_people_and_audit_events.py (2)
58-58: Redundant index:idx_people_discord_user_idduplicates the implicit index fromuq_people_discord_user_id.The unique constraint on
discord_user_id(line 58) already creates an implicit B-tree index in Postgres. The explicitidx_people_discord_user_idindex (line 63) is unnecessary and doubles write overhead for that column.♻️ Remove the redundant index
op.create_index("idx_people_email", "people", ["email"]) op.create_index("idx_people_email_508", "people", ["email_508"]) - op.create_index("idx_people_discord_user_id", "people", ["discord_user_id"])And in
downgrade():- op.drop_index("idx_people_discord_user_id", table_name="people") op.drop_index("idx_people_email_508", table_name="people") op.drop_index("idx_people_email", table_name="people")Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/migrations/versions/20260221_0200_create_people_and_audit_events.py` at line 58, Remove the redundant explicit index on discord_user_id: delete the sa.Index("idx_people_discord_user_id", "discord_user_id") declaration so the UniqueConstraint("discord_user_id", name="uq_people_discord_user_id") provides the implicit index, and also remove the corresponding op.drop_index("idx_people_discord_user_id") call from downgrade() to avoid attempting to drop a non-existent/duplicate index; ensure only the UniqueConstraint remains for discord_user_id across upgrade() and downgrade().
132-170: Consider a reusable trigger function instead of per-table copies.Each table gets its own
<table>_set_updated_at_fn()that has identical logic (NEW.updated_at = NOW()). The existingjobsmigration does the same. A single shared function (e.g.,set_updated_at_fn()) referenced by all triggers would reduce duplication as new tables are added. Not blocking, but worth considering as the schema grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/migrations/versions/20260221_0200_create_people_and_audit_events.py` around lines 132 - 170, Replace the per-table identical trigger functions with a single shared function and update triggers to call it: remove people_set_updated_at_fn and audit_events_set_updated_at_fn and instead create one set_updated_at_fn() that sets NEW.updated_at = NOW() and returns NEW, then update people_set_updated_at_tr and audit_events_set_updated_at_tr (and any similar triggers like the jobs trigger) to EXECUTE FUNCTION set_updated_at_fn(); this deduplicates the logic while preserving the existing trigger names and behavior.apps/discord_bot/src/five08/discord_bot/utils/audit.py (2)
57-58: Fire-and-forget task reference may be garbage-collected before completion.
asyncio.create_task()returns a task, butlog_commanddoesn't hold a strong reference to it beyond the local variable. If no other reference exists, the task can be garbage-collected (and cancelled) before it completes. CPython's event loop holds a weak reference to tasks, so in practice this usually works, but it's documented as unreliable.Consider storing tasks in a set on the instance:
♻️ Keep strong references to background tasks
Add a
_pending_tasksset to the constructor:def __init__( self, *, base_url: str | None, shared_secret: str | None, timeout_seconds: float, ) -> None: self.base_url = (base_url or "").strip().rstrip("/") self.shared_secret = (shared_secret or "").strip() self.timeout_seconds = timeout_seconds + self._pending_tasks: set[asyncio.Task[None]] = set()Then in
log_command:task = asyncio.create_task(self._post_event(event_payload)) - task.add_done_callback(self._on_task_done) + self._pending_tasks.add(task) + task.add_done_callback(self._pending_tasks.discard) + task.add_done_callback(self._on_task_done)🤖 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 57 - 58, The created background Task in log_command is only held in a local variable and can be garbage-collected; to fix, add a strong-reference set (e.g., self._pending_tasks) in the class constructor, add each created task (asyncio.create_task(self._post_event(...))) to self._pending_tasks, and ensure _on_task_done removes the completed task from self._pending_tasks (and also handles exceptions from task.result()). This guarantees the background task for _post_event is retained until completion and cleaned up in the _on_task_done callback.
118-129: Caller-suppliedmetadatacan silently overwrite base fields likecommandandguild_id.
base_metadata.update(metadata)at line 129 means any key in the caller'smetadatadict (e.g.,"command","guild_id") will overwrite the auto-populated base fields. If this is intentional for flexibility, a brief docstring note would help. If not, consider namespacing or merging in the opposite direction.🤖 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 118 - 129, base_metadata currently gets blindly updated with the caller-supplied metadata via base_metadata.update(metadata), allowing callers to overwrite important fields like "command" and "guild_id"; change the merge so base fields win (e.g., merge into metadata instead: merged = {**metadata, **base_metadata} or update base_metadata only for keys not present), or namespace caller data under an "extra" or "caller_metadata" key before merging; update the code paths that use base_metadata (and any docstring/comments for the enclosing function) to reflect the chosen behavior and ensure keys like command, guild_id, channel_id, interaction_id cannot be silently overwritten.apps/discord_bot/src/five08/discord_bot/cogs/crm.py (1)
106-116:"cog" in locals()is fragile — consider restructuring the exception handler.If the variable
cogis renamed or the code is refactored, this check will silently skip auditing. Sincecogis assigned on line 68 before the role check on line 76, the only way it can be missing in theexceptblock is ifget_cogitself raises, which is unlikely. A simpler approach:♻️ Use a default variable instead of locals() introspection
async def callback(self, interaction: discord.Interaction) -> None: """Handle resume download button click.""" + cog = None try: # Get the CRM cog to access the API cog = interaction.client.get_cog("CRMCog") # type: ignore[attr-defined] ... except Exception as e: logger.error(f"Unexpected error in resume button callback: {e}") - if "cog" in locals() and cog: + if cog: cog._audit_command(🤖 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 106 - 116, The except block uses a fragile "cog" in locals() check; instead ensure a stable cog variable by declaring and initializing cog = None in the outer scope (before the try) or assign cog to a known default so the except block can reliably reference it; update the exception handler to check "if cog:" (or "if cog is not None:") and call cog._audit_command(interaction=interaction, action="crm.resume_download_button", ...) so auditing always runs when cog was set, without relying on locals() introspection.tests/unit/test_discord_audit.py (1)
22-57: Tests cover the key best-effort behaviors — consider adding a happy-path test in a follow-up.The two tests cover the critical cases: disabled logger is a no-op, and exceptions are caught as warnings. Missing coverage for a successful POST (asserting the correct URL, headers, and payload are sent) would help catch regressions in
_send_event_syncand_build_payload.Would you like me to draft additional test cases for the happy-path POST and HTTP error-status scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_discord_audit.py` around lines 22 - 57, Add a happy-path test and an HTTP-error-status test for DiscordAuditLogger to verify _send_event_sync actually POSTS the expected URL, headers, timeout and JSON payload and treats non-2xx responses as warnings; specifically, create tests that instantiate DiscordAuditLogger with base_url/shared_secret, call _build_payload to produce the payload, patch requests.post (patching "five08.discord_bot.utils.audit.requests.post") to return a mock response with status_code=200 and assert mock_post.called_once and that it was called with the constructed events URL (base_url + "/events"), the JSON payload, a timeout, and headers (verify the presence of the shared_secret in headers), and in a separate test patch requests.post to return status_code=500 (or raise) and patch logger.warning to assert a warning was emitted; use the existing helper _mock_interaction() and reference DiscordAuditLogger._build_payload and DiscordAuditLogger._send_event_sync to locate the code under test.tests/unit/test_worker_people_sync.py (1)
6-46: Test coverage is a good start; consider adding edge-case scenarios.Current tests cover the happy path for Discord fields and email fallback. Additional test cases that would strengthen coverage:
- Missing
idfield → returnsNonecDiscordRolesas a list or dict (other shapes handled in_discord_roles)- Whitespace-only or empty string fields
discord_user_idextracted from explicit field vs. parsed from username🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_worker_people_sync.py` around lines 6 - 46, Add unit tests for edge cases around PeopleSyncProcessor._to_person_record and its helpers (e.g., _discord_roles): ensure when input lacks "id" the function returns None; verify cDiscordRoles passed as a list and as a dict are handled (call _to_person_record with cDiscordRoles as ["Member","Admin"] and as {"roles": ["Member"]}); add tests for whitespace-only or empty string fields (cDiscordUsername, cDiscordRoles, cGithubUsername) to ensure they normalize to None/empty; and add tests where discord_user_id is provided explicitly (discord_user_id field present) versus only embedded in cDiscordUsername to confirm both extraction paths work.apps/worker/src/five08/worker/crm/people_sync.py (2)
211-231: Discord user ID fallback re-reads raw username — correct but worth a comment.
_discord_usernamestrips the(ID: ...)suffix, so thediscord_usernameparameter passed here is already cleaned. The fallback at line 224 intentionally re-reads the rawcDiscordUsernameto recover the ID. The logic is correct, but a brief inline comment explaining why it re-reads the raw field would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/crm/people_sync.py` around lines 211 - 231, In _discord_user_id, add a brief inline comment before the fallback that re-reads raw_contact["cDiscordUsername"] explaining that discord_username is already cleaned by _discord_username (it strips the "(ID: ...)" suffix) so we must re-read the raw cDiscordUsername and run _DISCORD_ID_RE on it to recover the original numeric ID; reference the function name _discord_user_id, the parameter discord_username, helper _text_or_none, and the regex _DISCORD_ID_RE in the comment so future readers understand why the raw field is used instead of the cleaned discord_username.
18-23: Duplicate Espo API URL construction withEspoCRMClientinprocessor.py.Lines 22–23 duplicate the URL-building pattern found in
apps/worker/src/five08/worker/crm/processor.py(lines 18–20). Consider extracting a small factory or shared helper (e.g.,get_espo_api() -> EspoAPI) to keep the base-URL construction in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/crm/people_sync.py` around lines 18 - 23, Espo API URL construction is duplicated between EspoPeopleSyncClient.__init__ and EspoCRMClient in processor.py; create a shared factory function (e.g., get_espo_api()) that builds api_url from settings.espo_base_url and returns an EspoAPI instance using settings.espo_api_key, then replace the direct construction in EspoPeopleSyncClient.__init__ (and in EspoCRMClient) to call get_espo_api() so the base-URL trimming and "/api/v1" suffix logic is centralized.packages/shared/src/five08/audit.py (2)
199-257:normalize_actor_subjectis called twice; consider refactoringresolve_person_idto accept a pre-normalized subject.In
insert_audit_event,resolve_person_id(line 209) internally callsnormalize_actor_subject, and then it's called again at line 214. This is a minor redundancy. More importantly, the person lookup and the event insert happen in two separate connections/transactions — if the person row is deleted between the two calls,person_idwill be stale. Given the "best-effort" nature of audit linking this is probably acceptable, but worth noting.A small refactor could let
resolve_person_idaccept an already-normalized subject to avoid the double work, or both operations could share a single connection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/five08/audit.py` around lines 199 - 257, The insert_audit_event function currently calls normalize_actor_subject twice (once inside resolve_person_id and once directly) and performs person lookup and insert in separate DB transactions; refactor resolve_person_id to accept an optional pre-normalized subject (add parameter normalized_subject: Optional[str]) and update its callers so insert_audit_event computes normalized_subject once (via normalize_actor_subject(payload.actor_provider, payload.actor_subject)) and passes that into resolve_person_id, and/or open a single connection with get_postgres_connection and reuse it for both the resolve_person_id lookup and the INSERT to ensure consistent person_id resolution; update function signature and all call sites of resolve_person_id accordingly (symbols: insert_audit_event, resolve_person_id, normalize_actor_subject, get_postgres_connection).
108-160: Each call toupsert_person/resolve_person_id/insert_audit_eventopens and closes a new DB connection.
get_postgres_connectioncreates a fresh connection every time. For the current audit-event write path (insert_audit_event), that means two connections are opened (one insideresolve_person_id, one for the INSERT). Under load — e.g., batch imports triggering many audit events — this can exhaust connections or add latency. Consider accepting an optional connection parameter or introducing a lightweight connection pool (e.g.,psycopg_pool.ConnectionPool).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/five08/audit.py` around lines 108 - 160, The code opens a fresh DB connection on every call to upsert_person / resolve_person_id / insert_audit_event which causes excessive connections; modify these functions (upsert_person, resolve_person_id, insert_audit_event) to accept an optional connection/transaction parameter (e.g., conn) and, if provided, reuse it instead of calling get_postgres_connection(); when conn is None fall back to creating a new connection with get_postgres_connection() and ensure you only close connections you created (do not close a passed-in conn). Alternatively, introduce a shared psycopg_pool.ConnectionPool instance and have these functions acquire/release connections from that pool instead of calling get_postgres_connection() directly; update call sites to pass the same conn (or use a pooled client) for the whole audit-event write path to avoid creating multiple connections per request.apps/worker/src/five08/worker/api.py (2)
337-380: Consider extracting shared webhook validation logic.
espocrm_people_sync_webhook_handlerandespocrm_webhook_handler(Line 236) share identical auth → JSON parse → list check →EspoCRMWebhookPayload.from_list→ dedup logic. Extracting this into a shared helper would reduce duplication and ensure both paths stay in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/api.py` around lines 337 - 380, Extract the repeated auth/JSON/list/validation/dedup logic from espocrm_people_sync_webhook_handler and espocrm_webhook_handler into a single helper (e.g. _parse_and_validate_espocrm_webhook(request)) that: calls _is_authorized, attempts await request.json(), checks the payload is a list, calls EspoCRMWebhookPayload.from_list(payload_data) and returns the deduplicated event id list (or raises/returns appropriate web responses/errors the handlers can forward). Replace the duplicated blocks in espocrm_people_sync_webhook_handler and espocrm_webhook_handler to call this helper and then continue with their existing enqueueing logic (such as creating the bucket and invoking _enqueue_espocrm_people_sync_batch), preserving the same response shapes and error status codes.
383-433:audit_event_handlerperforms a synchronous DB write in the request path, deviating from the ingest-fast guideline.The coding guideline for this file states: "Keep ingest endpoints fast by validating input, persisting jobs, enqueueing, and returning 202 status." This handler instead blocks on
insert_audit_eventviaasyncio.to_threadand returns 201. If the DB is slow, individual requests will stall.If immediate
event_id/person_idfeedback is not required by callers, consider enqueueing the audit event as a job (matching the pattern of the other endpoints) and returning 202. If the synchronous write is intentional, documenting the rationale would help. As per coding guidelines, "Keep ingest endpoints fast by validating input, persisting jobs, enqueueing, and returning 202 status".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/api.py` around lines 383 - 433, audit_event_handler currently blocks on a synchronous DB write via asyncio.to_thread(insert_audit_event, ... ) and returns 201; change it to follow the ingest-fast pattern: after validating and model-validating payload (AuditEventPayload) build the AuditEventInput but do NOT call insert_audit_event directly — instead persist/enqueue a job (e.g., create or enqueue an audit ingest job using your existing job-queue helper used by other endpoints) using settings and the AuditEventInput, then return a 202 response with the job id/acknowledgement; if a synchronous write is truly required, add a clear comment explaining why and keep the existing insert_audit_event call and 201 response.tests/unit/test_worker_api.py (1)
240-247: Consider assertinginsert_audit_eventwas called with the expectedAuditEventInput.The test verifies the response shape but doesn't assert that
insert_audit_eventreceived the correctly mappedAuditEventInput. This leaves the payload → domain-model mapping untested — a bug in the field mapping (e.g., swappedactor_subject/actor_display_name) would pass silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_worker_api.py` around lines 240 - 247, Add an assertion that insert_audit_event was invoked with the correctly mapped AuditEventInput to validate payload→domain mapping: after calling audit_event_handler(request) build the expected AuditEventInput instance (populating fields like actor_subject, actor_display_name, person_id, event_type, metadata from the test request) and assert mock_insert was called once with that AuditEventInput (use mock_insert.assert_called_once_with(...) or equivalent); reference the insert_audit_event mock and the AuditEventInput type to locate where to add this check in the test that calls audit_event_handler.tests/integration/test_email_monitor.py (3)
48-60: No explicit assertions — test only verifies the coroutine doesn't raise.The test doesn't assert anything about observable side-effects (e.g. that
channel.sendwas or was not called, or that the email was skipped gracefully). With the mock data(None, b"malformed email data")the tuple checkisinstance(response_part, tuple)isTrue, so the parse path is reached andemail.message_from_stringsilently succeeds. Adding at least a negative assertion (e.g. that no channel messages were sent for the malformed message) would make the intent verifiable.♻️ Proposed addition
with patch("imaplib.IMAP4_SSL", return_value=mock_imap_server): await EmailMonitor.task_poll_inbox.coro(email_monitor_real_poll) + + # Malformed data should not trigger any channel messages + mock_discord_channel.send.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_email_monitor.py` around lines 48 - 60, The test test_poll_inbox_handles_email_parsing_errors currently only ensures the coroutine doesn't raise but has no observable assertions; update it to assert the expected side-effect for malformed data by verifying that the mocked Discord channel did not receive any sends. After calling await EmailMonitor.task_poll_inbox.coro(email_monitor_real_poll) (with IMAP4_SSL patched and mock_imap_server.fetch returning the malformed tuple), add an assertion like mock_discord_channel.send.assert_not_called() (or the appropriate mock on email_monitor_real_poll.bot.get_channel().send) so the test explicitly verifies that no message was posted for the malformed email.
29-32: Fixture is fragile against__init__re-enabling task auto-start.
email_monitor_real_pollrelies ontask_poll_inbox.start()remaining commented out inEmailMonitor.__init__. If that line is ever uncommented, this fixture would kick off a realasyncio.Taskagainst a mock bot and could cause test pollution or hangs.♻️ Proposed guard
`@pytest.fixture` def email_monitor_real_poll(self, mock_bot): """Create an EmailMonitor instance with the real poll coroutine.""" - return EmailMonitor(mock_bot) + with patch.object(EmailMonitor, "__init__", lambda self, bot: setattr(self, "bot", bot)): + return EmailMonitor(mock_bot)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_email_monitor.py` around lines 29 - 32, The fixture email_monitor_real_poll is fragile because it assumes EmailMonitor.__init__ won't call task_poll_inbox.start(); to fix, prevent any auto-start when creating EmailMonitor by stubbing or monkeypatching the task starter before instantiation: e.g., replace EmailMonitor.task_poll_inbox or its start method with a no-op/dummy object (or set a flag on EmailMonitor to disable auto-start) so that constructing EmailMonitor(mock_bot) cannot launch a real asyncio.Task; update the fixture to perform this guard (referencing EmailMonitor and task_poll_inbox.start) before calling EmailMonitor(mock_bot).
44-46: Use the documentedLoop.__call__API instead of the internal.coroattribute.
.corois not part of discord.py's documented public API fordiscord.ext.tasks.Loopand could break silently on version updates. The documented alternative is to call the bound loop directly via__call__:await email_monitor_real_poll.task_poll_inbox()This uses the official API that has been stable since discord.py v1.6.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_email_monitor.py` around lines 44 - 46, Replace the use of the internal .coro attribute on the Loop with the documented Loop.__call__ API: instead of calling EmailMonitor.task_poll_inbox.coro(email_monitor_real_poll), invoke the bound loop directly via email_monitor_real_poll.task_poll_inbox() (i.e., await the loop call), keeping the same exception expectation; reference symbols: EmailMonitor.task_poll_inbox, email_monitor_real_poll and Loop.__call__.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 65-66: AUDIT_API_BASE_URL is empty and lacks the guidance comments
used for other required envs; update the .env.example to set a sensible local
default (e.g., AUDIT_API_BASE_URL=http://worker:8090) and add a short comment
like "Required: URL of the Audit API (local dev default shown)" similar to the
existing comments for WEBHOOK_SHARED_SECRET and OPENAI_API_KEY so developers
know what to set and local-compose works out of the box; leave
AUDIT_API_TIMEOUT_SECONDS unchanged unless you want to document its units in a
one-line comment.
In `@apps/discord_bot/Dockerfile`:
- Around line 1-2: Update the Dockerfile comment to state the exact uv image
version that corresponds to the pinned digest: replace the current tag reference
"ghcr.io/astral-sh/uv:python3.12-bookworm-slim" with the fully qualified tag
including the uv release (e.g.,
"ghcr.io/astral-sh/uv:0.10.2-python3.12-bookworm-slim") so the comment next to
the FROM line accurately documents which uv version the sha256 digest
represents.
In `@apps/worker/Dockerfile`:
- Around line 1-2: Update the base image comment and FROM line so the Dockerfile
no longer references the removed bookworm-slim tag: replace the comment on the
first line and the existing FROM ghcr.io/astral-sh/uv@sha256:e5b65587... with a
pinned python3.12-trixie image digest (e.g., FROM
ghcr.io/astral-sh/uv@sha256:<new-digest>), obtaining the correct SHA256 by
querying GHCR for the python3.12-trixie manifest; ensure the comment reflects
the uv 0.10.0 removal date and that the new digest is pinned for reproducible
builds.
In `@apps/worker/src/five08/worker/api.py`:
- Around line 403-424: The try/except around the insert_audit_event call only
catches ValueError, leaving DB errors unhandled; update the error handling in
the block that calls insert_audit_event so that after the existing ValueError
except you also catch database-related exceptions (preferably the project's DB
exception class such as SQLAlchemyError/DatabaseError or, if uncertain, a
generic Exception as a fallback) and return a sanitized 500 JSON response while
logging the exception; keep the AuditEventInput construction and ValueError
handling intact and ensure you reference insert_audit_event and AuditEventInput
when locating the code to modify.
- Around line 104-108: In _log_background_task_result replace the
logger.exception call (which runs outside an except block) with a logger.error
call that passes exc_info=exc so the task's actual traceback is logged; locate
the exception retrieval via task.exception() and change the logging invocation
to use logger.error("Background task failed name=%s error=%s", name, exc,
exc_info=exc) so the traceback is preserved.
- Around line 258-264: The created background task for _enqueue_espocrm_batch is
only weakly referenced and may be garbage-collected; instead register tasks in
the app-level background set used elsewhere (e.g., CRM_SYNC_TASK_KEY) so they
are strongly referenced: obtain or create the set on request.app, add the
created task to that set, attach a done callback that both
_log_background_task_result and removes the task from the set when complete;
apply the same change to espocrm_people_sync_webhook_handler and ensure the
app-level set is initialized on startup (as other code does) so tasks persist
until finished.
In
`@apps/worker/src/five08/worker/migrations/versions/20260221_0200_create_people_and_audit_events.py`:
- Around line 17-21: The migration creates UUID ids for people and audit_events
without server-side defaults while jobs uses
server_default=sa.text("gen_random_uuid()"), which is intentional because the
application (packages/shared/src/five08/audit.py) generates UUIDs via uuid4()
for person_id and event_id; update the people/audit_events migration (look for
op.create_table with "people" and the audit_events op.create_table) to include a
short explanatory comment above the table/column definitions stating that UUIDs
are generated by the application (uuid4) and therefore no server_default is set,
or alternatively standardize by adding
server_default=sa.text("gen_random_uuid()") to people.id and audit_events.id if
you prefer DB-side generation—choose one approach and document it in the
migration so the inconsistency with the jobs table is explicit.
In `@apps/worker/src/five08/worker/models.py`:
- Line 73: The occurred_at field currently allows naive datetimes; change its
type to pydantic.AwareDatetime to enforce timezone-aware values, update the
import to pull AwareDatetime from pydantic, and keep the default None if you
still want optionality (e.g., occurred_at: AwareDatetime | None = None) so
validation will reject tz-naive datetimes at assignment/parse time; locate the
occurred_at definition in the model class and replace the datetime type with
AwareDatetime and add the import.
In `@docker-compose.yml`:
- Around line 83-86: The compose file exposes POSTGRES_URL without a fallback
which causes runtime errors where the app expects it (see code that raises
RuntimeError in migrations and settings validation requiring POSTGRES_URL);
update the docker-compose environment entry for POSTGRES_URL to provide a
sensible default fallback (e.g., a local Postgres connection URI that matches
POSTGRES_USER/POSTGRES_PASSWORD/POSTGRES_DB) so that ${POSTGRES_URL} becomes
${POSTGRES_URL:-<local-postgres-connection-string>} and migrations/non-local
deployments won't fail on fresh setups.
In `@packages/shared/src/five08/settings.py`:
- Around line 44-57: The validator validate_required_secrets currently calls
os.getenv(...) which bypasses pydantic-settings’ .env loading and can
false-positive; change it to check the resolved model attributes
(self.postgres_url and self.minio_root_password) instead of os.getenv, keeping
the runtime_env logic and raising the same ValueError messages if those self
fields are empty/None; if you want to enforce “explicit” (non-default) values
compare against known defaults rather than os.getenv, and remove the unused os
import if no longer referenced.
In `@tests/integration/test_email_monitor.py`:
- Around line 35-38: The test docstring for test_poll_inbox_handles_imap_errors
is incorrect: it says IMAP errors are "handled gracefully" but the test asserts
the error propagates using pytest.raises; update the docstring in
tests/integration/test_email_monitor.py for the async
test_poll_inbox_handles_imap_errors to state that IMAP errors are
propagated/bubbled up (e.g., "Test that IMAP errors are propagated") so it
accurately reflects the assertion and the inline comment about bubble-up
behavior.
---
Duplicate comments:
In `@docker-compose.yml`:
- Around line 118-121: The POSTGRES_URL environment variable in
docker-compose.yml has no default fallback while
POSTGRES_USER/POSTGRES_PASSWORD/POSTGRES_DB do; update the POSTGRES_URL entry to
provide a sensible default fallback (e.g., matching the other defaults and
service host/port) using the same shell-style default expansion
(${POSTGRES_URL:-...}) so the stack can run without an explicit env set, and
ensure the default URL references the same DB name/user/password used by
POSTGRES_USER, POSTGRES_PASSWORD and POSTGRES_DB.
---
Nitpick comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 106-116: The except block uses a fragile "cog" in locals() check;
instead ensure a stable cog variable by declaring and initializing cog = None in
the outer scope (before the try) or assign cog to a known default so the except
block can reliably reference it; update the exception handler to check "if cog:"
(or "if cog is not None:") and call cog._audit_command(interaction=interaction,
action="crm.resume_download_button", ...) so auditing always runs when cog was
set, without relying on locals() introspection.
In `@apps/discord_bot/src/five08/discord_bot/utils/audit.py`:
- Around line 57-58: The created background Task in log_command is only held in
a local variable and can be garbage-collected; to fix, add a strong-reference
set (e.g., self._pending_tasks) in the class constructor, add each created task
(asyncio.create_task(self._post_event(...))) to self._pending_tasks, and ensure
_on_task_done removes the completed task from self._pending_tasks (and also
handles exceptions from task.result()). This guarantees the background task for
_post_event is retained until completion and cleaned up in the _on_task_done
callback.
- Around line 118-129: base_metadata currently gets blindly updated with the
caller-supplied metadata via base_metadata.update(metadata), allowing callers to
overwrite important fields like "command" and "guild_id"; change the merge so
base fields win (e.g., merge into metadata instead: merged = {**metadata,
**base_metadata} or update base_metadata only for keys not present), or
namespace caller data under an "extra" or "caller_metadata" key before merging;
update the code paths that use base_metadata (and any docstring/comments for the
enclosing function) to reflect the chosen behavior and ensure keys like command,
guild_id, channel_id, interaction_id cannot be silently overwritten.
In `@apps/worker/Dockerfile`:
- Around line 15-16: Consolidate the two Dockerfile RUN steps into a single RUN
so groupadd, useradd and chown happen in one layer: perform groupadd -r
appgroup, useradd -r -g appgroup -d /app -s /usr/sbin/nologin appuser, and then
chown -R appuser:appgroup /app all within the same RUN to avoid creating an
extra writable layer; update the Dockerfile lines that reference groupadd,
useradd, chown, appuser, appgroup and /app accordingly.
In `@apps/worker/src/five08/worker/api.py`:
- Around line 337-380: Extract the repeated auth/JSON/list/validation/dedup
logic from espocrm_people_sync_webhook_handler and espocrm_webhook_handler into
a single helper (e.g. _parse_and_validate_espocrm_webhook(request)) that: calls
_is_authorized, attempts await request.json(), checks the payload is a list,
calls EspoCRMWebhookPayload.from_list(payload_data) and returns the deduplicated
event id list (or raises/returns appropriate web responses/errors the handlers
can forward). Replace the duplicated blocks in
espocrm_people_sync_webhook_handler and espocrm_webhook_handler to call this
helper and then continue with their existing enqueueing logic (such as creating
the bucket and invoking _enqueue_espocrm_people_sync_batch), preserving the same
response shapes and error status codes.
- Around line 383-433: audit_event_handler currently blocks on a synchronous DB
write via asyncio.to_thread(insert_audit_event, ... ) and returns 201; change it
to follow the ingest-fast pattern: after validating and model-validating payload
(AuditEventPayload) build the AuditEventInput but do NOT call insert_audit_event
directly — instead persist/enqueue a job (e.g., create or enqueue an audit
ingest job using your existing job-queue helper used by other endpoints) using
settings and the AuditEventInput, then return a 202 response with the job
id/acknowledgement; if a synchronous write is truly required, add a clear
comment explaining why and keep the existing insert_audit_event call and 201
response.
In `@apps/worker/src/five08/worker/crm/people_sync.py`:
- Around line 211-231: In _discord_user_id, add a brief inline comment before
the fallback that re-reads raw_contact["cDiscordUsername"] explaining that
discord_username is already cleaned by _discord_username (it strips the "(ID:
...)" suffix) so we must re-read the raw cDiscordUsername and run _DISCORD_ID_RE
on it to recover the original numeric ID; reference the function name
_discord_user_id, the parameter discord_username, helper _text_or_none, and the
regex _DISCORD_ID_RE in the comment so future readers understand why the raw
field is used instead of the cleaned discord_username.
- Around line 18-23: Espo API URL construction is duplicated between
EspoPeopleSyncClient.__init__ and EspoCRMClient in processor.py; create a shared
factory function (e.g., get_espo_api()) that builds api_url from
settings.espo_base_url and returns an EspoAPI instance using
settings.espo_api_key, then replace the direct construction in
EspoPeopleSyncClient.__init__ (and in EspoCRMClient) to call get_espo_api() so
the base-URL trimming and "/api/v1" suffix logic is centralized.
In
`@apps/worker/src/five08/worker/migrations/versions/20260221_0200_create_people_and_audit_events.py`:
- Line 58: Remove the redundant explicit index on discord_user_id: delete the
sa.Index("idx_people_discord_user_id", "discord_user_id") declaration so the
UniqueConstraint("discord_user_id", name="uq_people_discord_user_id") provides
the implicit index, and also remove the corresponding
op.drop_index("idx_people_discord_user_id") call from downgrade() to avoid
attempting to drop a non-existent/duplicate index; ensure only the
UniqueConstraint remains for discord_user_id across upgrade() and downgrade().
- Around line 132-170: Replace the per-table identical trigger functions with a
single shared function and update triggers to call it: remove
people_set_updated_at_fn and audit_events_set_updated_at_fn and instead create
one set_updated_at_fn() that sets NEW.updated_at = NOW() and returns NEW, then
update people_set_updated_at_tr and audit_events_set_updated_at_tr (and any
similar triggers like the jobs trigger) to EXECUTE FUNCTION set_updated_at_fn();
this deduplicates the logic while preserving the existing trigger names and
behavior.
In `@apps/worker/src/five08/worker/models.py`:
- Around line 3-5: Consolidate the two typing imports in
apps/worker/src/five08/worker/models.py by replacing the separate "from typing
import Literal" and "from typing import Any" with a single import that includes
both symbols (e.g., import Literal and Any together from typing); update the
import line that currently references Literal and Any so there's only one from
typing import statement.
In `@packages/shared/src/five08/audit.py`:
- Around line 199-257: The insert_audit_event function currently calls
normalize_actor_subject twice (once inside resolve_person_id and once directly)
and performs person lookup and insert in separate DB transactions; refactor
resolve_person_id to accept an optional pre-normalized subject (add parameter
normalized_subject: Optional[str]) and update its callers so insert_audit_event
computes normalized_subject once (via
normalize_actor_subject(payload.actor_provider, payload.actor_subject)) and
passes that into resolve_person_id, and/or open a single connection with
get_postgres_connection and reuse it for both the resolve_person_id lookup and
the INSERT to ensure consistent person_id resolution; update function signature
and all call sites of resolve_person_id accordingly (symbols:
insert_audit_event, resolve_person_id, normalize_actor_subject,
get_postgres_connection).
- Around line 108-160: The code opens a fresh DB connection on every call to
upsert_person / resolve_person_id / insert_audit_event which causes excessive
connections; modify these functions (upsert_person, resolve_person_id,
insert_audit_event) to accept an optional connection/transaction parameter
(e.g., conn) and, if provided, reuse it instead of calling
get_postgres_connection(); when conn is None fall back to creating a new
connection with get_postgres_connection() and ensure you only close connections
you created (do not close a passed-in conn). Alternatively, introduce a shared
psycopg_pool.ConnectionPool instance and have these functions acquire/release
connections from that pool instead of calling get_postgres_connection()
directly; update call sites to pass the same conn (or use a pooled client) for
the whole audit-event write path to avoid creating multiple connections per
request.
In `@tests/integration/test_email_monitor.py`:
- Around line 48-60: The test test_poll_inbox_handles_email_parsing_errors
currently only ensures the coroutine doesn't raise but has no observable
assertions; update it to assert the expected side-effect for malformed data by
verifying that the mocked Discord channel did not receive any sends. After
calling await EmailMonitor.task_poll_inbox.coro(email_monitor_real_poll) (with
IMAP4_SSL patched and mock_imap_server.fetch returning the malformed tuple), add
an assertion like mock_discord_channel.send.assert_not_called() (or the
appropriate mock on email_monitor_real_poll.bot.get_channel().send) so the test
explicitly verifies that no message was posted for the malformed email.
- Around line 29-32: The fixture email_monitor_real_poll is fragile because it
assumes EmailMonitor.__init__ won't call task_poll_inbox.start(); to fix,
prevent any auto-start when creating EmailMonitor by stubbing or monkeypatching
the task starter before instantiation: e.g., replace
EmailMonitor.task_poll_inbox or its start method with a no-op/dummy object (or
set a flag on EmailMonitor to disable auto-start) so that constructing
EmailMonitor(mock_bot) cannot launch a real asyncio.Task; update the fixture to
perform this guard (referencing EmailMonitor and task_poll_inbox.start) before
calling EmailMonitor(mock_bot).
- Around line 44-46: Replace the use of the internal .coro attribute on the Loop
with the documented Loop.__call__ API: instead of calling
EmailMonitor.task_poll_inbox.coro(email_monitor_real_poll), invoke the bound
loop directly via email_monitor_real_poll.task_poll_inbox() (i.e., await the
loop call), keeping the same exception expectation; reference symbols:
EmailMonitor.task_poll_inbox, email_monitor_real_poll and Loop.__call__.
In `@tests/unit/test_discord_audit.py`:
- Around line 22-57: Add a happy-path test and an HTTP-error-status test for
DiscordAuditLogger to verify _send_event_sync actually POSTS the expected URL,
headers, timeout and JSON payload and treats non-2xx responses as warnings;
specifically, create tests that instantiate DiscordAuditLogger with
base_url/shared_secret, call _build_payload to produce the payload, patch
requests.post (patching "five08.discord_bot.utils.audit.requests.post") to
return a mock response with status_code=200 and assert mock_post.called_once and
that it was called with the constructed events URL (base_url + "/events"), the
JSON payload, a timeout, and headers (verify the presence of the shared_secret
in headers), and in a separate test patch requests.post to return
status_code=500 (or raise) and patch logger.warning to assert a warning was
emitted; use the existing helper _mock_interaction() and reference
DiscordAuditLogger._build_payload and DiscordAuditLogger._send_event_sync to
locate the code under test.
In `@tests/unit/test_shared_queue.py`:
- Around line 24-34: Split the current test_parse_status_handles_unknown_values
into two focused tests: one that verifies _parse_status("queued") returns
JobStatus.QUEUED, and a second that verifies an unknown status (e.g.
"unexpected-status") causes _parse_status to return JobStatus.FAILED and emits a
warning via five08.queue.logger.warning; in the second test use
patch("five08.queue.logger.warning") as mock_warning, call result =
_parse_status("unexpected-status"), assert result == JobStatus.FAILED, and
assert mock_warning.assert_called_once_with("Unknown job status from DB: %s",
"unexpected-status").
In `@tests/unit/test_skills_extractor.py`:
- Around line 6-12: Change the test to exercise the public API by calling
SkillsExtractor.extract_skills(...) instead of the private
_extract_skills_heuristic; instantiate SkillsExtractor() (which may set
self.client=None when OPENAI_API_KEY is absent) and call
extractor.extract_skills("Built services in Go and Python"), then assert that
"go" and "python" are present in the returned object's .skills so the test stays
decoupled from internal routing and still verifies the two-letter token
handling.
In `@tests/unit/test_worker_api.py`:
- Around line 240-247: Add an assertion that insert_audit_event was invoked with
the correctly mapped AuditEventInput to validate payload→domain mapping: after
calling audit_event_handler(request) build the expected AuditEventInput instance
(populating fields like actor_subject, actor_display_name, person_id,
event_type, metadata from the test request) and assert mock_insert was called
once with that AuditEventInput (use mock_insert.assert_called_once_with(...) or
equivalent); reference the insert_audit_event mock and the AuditEventInput type
to locate where to add this check in the test that calls audit_event_handler.
In `@tests/unit/test_worker_people_sync.py`:
- Around line 6-46: Add unit tests for edge cases around
PeopleSyncProcessor._to_person_record and its helpers (e.g., _discord_roles):
ensure when input lacks "id" the function returns None; verify cDiscordRoles
passed as a list and as a dict are handled (call _to_person_record with
cDiscordRoles as ["Member","Admin"] and as {"roles": ["Member"]}); add tests for
whitespace-only or empty string fields (cDiscordUsername, cDiscordRoles,
cGithubUsername) to ensure they normalize to None/empty; and add tests where
discord_user_id is provided explicitly (discord_user_id field present) versus
only embedded in cDiscordUsername to confirm both extraction paths work.
Comment on lines
+65
to
+66
| AUDIT_API_BASE_URL= | ||
| AUDIT_API_TIMEOUT_SECONDS=2.0 |
There was a problem hiding this comment.
AUDIT_API_BASE_URL is blank with no guidance comment.
Other empty required values (e.g., WEBHOOK_SHARED_SECRET, OPENAI_API_KEY) have comments like "Required: …" or descriptive placeholders. Consider adding a comment and a sensible local default (e.g., http://worker:8090) so developers know what to set and local-compose works out of the box.
Proposed fix
-AUDIT_API_BASE_URL=
-AUDIT_API_TIMEOUT_SECONDS=2.0
+# Required for Discord bot audit logging; points to the worker ingest API
+AUDIT_API_BASE_URL=http://worker:8090
+AUDIT_API_TIMEOUT_SECONDS=2.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AUDIT_API_BASE_URL= | |
| AUDIT_API_TIMEOUT_SECONDS=2.0 | |
| # Required for Discord bot audit logging; points to the worker ingest API | |
| AUDIT_API_BASE_URL=http://worker:8090 | |
| AUDIT_API_TIMEOUT_SECONDS=2.0 |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 65-65: [UnorderedKey] The AUDIT_API_BASE_URL key should go before the CHECK_EMAIL_WAIT key
(UnorderedKey)
[warning] 66-66: [UnorderedKey] The AUDIT_API_TIMEOUT_SECONDS key should go before the CHECK_EMAIL_WAIT key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 65 - 66, AUDIT_API_BASE_URL is empty and lacks the
guidance comments used for other required envs; update the .env.example to set a
sensible local default (e.g., AUDIT_API_BASE_URL=http://worker:8090) and add a
short comment like "Required: URL of the Audit API (local dev default shown)"
similar to the existing comments for WEBHOOK_SHARED_SECRET and OPENAI_API_KEY so
developers know what to set and local-compose works out of the box; leave
AUDIT_API_TIMEOUT_SECONDS unchanged unless you want to document its units in a
one-line comment.
Comment on lines
+1
to
2
| # Pinned from ghcr.io/astral-sh/uv:python3.12-bookworm-slim (verified 2026-02-21). | ||
| FROM ghcr.io/astral-sh/uv@sha256:e5b65587bce7de595f299855d7385fe7fca39b8a74baa261ba1b7147afa78e58 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the digest matches the claimed tag and check for current tags
IMAGE="ghcr.io/astral-sh/uv"
DIGEST="sha256:e5b65587bce7de595f299855d7385fe7fca39b8a74baa261ba1b7147afa78e58"
echo "=== Fetching manifest for the pinned digest ==="
curl -fsSL \
-H "Accept: application/vnd.oci.image.manifest.v1+json" \
"https://ghcr.io/v2/astral-sh/uv/manifests/${DIGEST}" \
2>/dev/null | jq '{mediaType, config: .config.digest}' || echo "Manifest fetch failed or digest not found"
echo ""
echo "=== Checking available tags for python3.12-bookworm variant ==="
# Query the Docker registry API to list tags
curl -fsSL \
"https://ghcr.io/v2/astral-sh/uv/tags/list" \
2>/dev/null | jq '.tags[] | select(contains("python3.12") and contains("bookworm"))' | head -20Repository: 508-dev/508-workflows
Length of output: 177
🌐 Web query:
astral-sh uv Docker image python3.12-bookworm-slim ghcr.io releases
💡 Result:
-
Image / registry:
ghcr.io/astral-sh/uv(Astral’s official GHCR package). The canonical place to browse all tags (“releases”) and digests is the GitHub Container package page (and its “Versions” list). [2] -
Tag scheme (how releases are published):
- Distroless / base tags (e.g.
latest,0.10.4,0.10) and - Derived “Python base” images are tagged as:
ghcr.io/astral-sh/uv:{major}.{minor}.{patch}-{base}andghcr.io/astral-sh/uv:{major}.{minor}-{base}. [1]
- Distroless / base tags (e.g.
-
For the specific base you asked about (
python3.12-bookworm-slim):- You should expect version-pinned tags like:
ghcr.io/astral-sh/uv:<UV_VERSION>-python3.12-bookworm-slim(example known tag:0.5.26-python3.12-bookworm-slim). [4] - There was also discussion that the published tag name is
python3.12-bookworm-slim(notpython3.12-slim-bookworm). [3]
- You should expect version-pinned tags like:
🌐 Web query:
uv package manager Docker image latest tags 2026
💡 Result:
As of Feb 21, 2026, the official uv Docker images are published as ghcr.io/astral-sh/uv and also mirrored on Docker Hub as docker.io/astral/uv. [1][2]
“Latest” uv version (what :latest points to)
- The latest released uv version is 0.10.2 (released 2026-02-10), so
ghcr.io/astral-sh/uv:latest/astral/uv:latestshould correspond to that release. [2][3]
Tag schemes you can use (2026)
Distroless (just the uv binaries):
:latest:{major}.{minor}.{patch}(example shown in docs::0.10.2):{major}.{minor}(tracks latest patch in that minor line, e.g.:0.8) [2]
Derived OS / Python base images (uv preinstalled):
- Alpine bases:
:alpine,:alpine3.23,:alpine3.22, etc. [2] - Debian bases (current in docs):
:debian,:debian-slim,:trixie,:trixie-slim[2] - Python bases (examples):
:python3.14-alpine,:python3.13-alpine,:python3.14-trixie,:python3.14-trixie-slim, etc. [2]
Version-pinned derived images (recommended for reproducibility):
:{major}.{minor}.{patch}-{base}(example::0.10.2-alpine):{major}.{minor}-{base}(example::0.10-alpine) [2]
Clarify the uv version in the pinned image comment.
The digest pinning is correct for security. However, the referenced tag ghcr.io/astral-sh/uv:python3.12-bookworm-slim should include the uv version number for reproducibility and clarity. According to the uv Docker image tagging scheme, this should be formatted as ghcr.io/astral-sh/uv:{version}-python3.12-bookworm-slim (example: 0.10.2-python3.12-bookworm-slim for the latest release as of Feb 21, 2026). Update the comment to specify the exact version this digest corresponds to.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/discord_bot/Dockerfile` around lines 1 - 2, Update the Dockerfile
comment to state the exact uv image version that corresponds to the pinned
digest: replace the current tag reference
"ghcr.io/astral-sh/uv:python3.12-bookworm-slim" with the fully qualified tag
including the uv release (e.g.,
"ghcr.io/astral-sh/uv:0.10.2-python3.12-bookworm-slim") so the comment next to
the FROM line accurately documents which uv version the sha256 digest
represents.
Comment on lines
+1
to
2
| # Pinned from ghcr.io/astral-sh/uv:python3.12-bookworm-slim (verified 2026-02-21). | ||
| FROM ghcr.io/astral-sh/uv@sha256:e5b65587bce7de595f299855d7385fe7fca39b8a74baa261ba1b7147afa78e58 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
uv 0.9 release notes Docker images bookworm removed trixie
💡 Result:
- In uv 0.9.0 (released 2025-10-07), the
uv:debiananduv:debian-slimDocker images switched their base from Debian 12 (Bookworm) to Debian 13 (Trixie). The old base was still available viauv:bookwormanduv:bookworm-slimat that time. [1] - The Bookworm-tagged images were removed later, in uv 0.10.0 (released 2026-02-05):
uv:bookwormanduv:bookworm-slimare “no longer published”, and you’re told to useuv:debianoruv:trixieinstead. [2]
Sources
[1] astral-sh/uv v0.9.0 release notes (GitHub releases mirror) (phillipring.com)
[2] astral-sh/uv v0.10.0 release notes (GitHub releases) (github.com)
Citations:
- 1: https://phillipring.com/astral-sh/uv/releases?utm_source=openai
- 2: https://github.com/astral-sh/uv/releases
🏁 Script executed:
#!/bin/bash
# Check if the bookworm SHA is still resolvable and get the python3.12-trixie digest
echo "=== Checking old bookworm-slim SHA digest ==="
curl -s -I \
"https://ghcr.io/v2/astral-sh/uv/manifests/sha256:e5b65587bce7de595f299855d7385fe7fca39b8a74baa261ba1b7147afa78e58" \
-H "Accept: application/vnd.oci.image.manifest.v2+json,application/vnd.oci.image.index.v1+json" \
2>&1 | head -20
echo -e "\n=== Checking python3.12-trixie tag manifest ==="
curl -s -I \
"https://ghcr.io/v2/astral-sh/uv/manifests/python3.12-trixie" \
-H "Accept: application/vnd.oci.image.manifest.v2+json,application/vnd.oci.image.index.v1+json" \
2>&1 | head -20Repository: 508-dev/508-workflows
Length of output: 702
Correct the uv version and release date: bookworm-slim was removed in uv 0.10.0 (2026-02-05), not 0.9 (2026-01-29).
The Debian Bookworm Docker images were removed in uv 0.10.0 (released 2026-02-05), not uv 0.9. At uv 0.9.0 (released 2025-10-07), the bookworm-slim tag was still available. The comment on line 1 now documents an image tag that is no longer published. While the SHA digest pin may still resolve against cached GHCR layers, the underlying image is no longer maintained and won't receive security or OS-level patches.
Migrate to python3.12-trixie instead. Pin the new SHA256 digest after obtaining it—pinning specific digests is best practice for reproducible builds, as tags can be moved across different commit SHAs.
Proposed migration to python3.12-trixie
-# Pinned from ghcr.io/astral-sh/uv:python3.12-bookworm-slim (verified 2026-02-21).
-FROM ghcr.io/astral-sh/uv@sha256:e5b65587bce7de595f299855d7385fe7fca39b8a74baa261ba1b7147afa78e58
+# Pinned from ghcr.io/astral-sh/uv:python3.12-trixie (verified <DATE>).
+FROM ghcr.io/astral-sh/uv@sha256:<NEW_DIGEST>Obtain the new digest by querying GHCR for the current python3.12-trixie manifest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/worker/Dockerfile` around lines 1 - 2, Update the base image comment and
FROM line so the Dockerfile no longer references the removed bookworm-slim tag:
replace the comment on the first line and the existing FROM
ghcr.io/astral-sh/uv@sha256:e5b65587... with a pinned python3.12-trixie image
digest (e.g., FROM ghcr.io/astral-sh/uv@sha256:<new-digest>), obtaining the
correct SHA256 by querying GHCR for the python3.12-trixie manifest; ensure the
comment reflects the uv 0.10.0 removal date and that the new digest is pinned
for reproducible builds.
| actor_display_name: str | None = None | ||
| correlation_id: str | None = None | ||
| metadata: dict[str, Any] = Field(default_factory=dict) | ||
| occurred_at: datetime | None = None |
There was a problem hiding this comment.
Consider enforcing timezone-aware datetimes for occurred_at.
Using plain datetime allows naive (tz-unaware) values to be submitted. When persisted to a TIMESTAMP WITH TIME ZONE column in PostgreSQL, the session timezone is applied to naive datetimes, which can produce incorrect audit timestamps. Pydantic v2 exposes AwareDatetime precisely for this:
🛡️ Proposed fix
-from datetime import datetime
+from pydantic import AwareDatetime, BaseModel, Field
...
- occurred_at: datetime | None = None
+ occurred_at: AwareDatetime | None = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/worker/src/five08/worker/models.py` at line 73, The occurred_at field
currently allows naive datetimes; change its type to pydantic.AwareDatetime to
enforce timezone-aware values, update the import to pull AwareDatetime from
pydantic, and keep the default None if you still want optionality (e.g.,
occurred_at: AwareDatetime | None = None) so validation will reject tz-naive
datetimes at assignment/parse time; locate the occurred_at definition in the
model class and replace the datetime type with AwareDatetime and add the import.
Comment on lines
+83
to
+86
| POSTGRES_USER: ${POSTGRES_USER:-postgres} | ||
| POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres} | ||
| POSTGRES_DB: ${POSTGRES_DB:-workflows} | ||
| POSTGRES_URL: ${POSTGRES_URL} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking for POSTGRES_URL usage in Python files ==="
rg -n -C3 'POSTGRES_URL' --type=py
echo ""
echo "=== Searching for database connection patterns ==="
rg -n -C5 'create_engine|create_async_engine|AsyncSession|sessionmaker|get_db|database_url|db_url' --type=py | head -100Repository: 508-dev/508-workflows
Length of output: 3350
POSTGRES_URL is required but has no default — migrations and non-local deployments will fail.
Lines 86 and 121 pass ${POSTGRES_URL} without a fallback. The application code explicitly requires this variable:
apps/worker/src/five08/worker/migrations/env.pyline 22–23 raisesRuntimeError("POSTGRES_URL is required for worker job migrations.")if the variable is missing.packages/shared/src/five08/settings.pyline 51–52 validates thatPOSTGRES_URLmust be set for non-local environments.
Without a default in the compose file, migrations and non-local deployments will fail on fresh setups. Add a default:
Suggested fix
- POSTGRES_URL: ${POSTGRES_URL}
+ POSTGRES_URL: ${POSTGRES_URL:-postgresql://postgres:postgres@postgres:5432/workflows}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| POSTGRES_USER: ${POSTGRES_USER:-postgres} | |
| POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres} | |
| POSTGRES_DB: ${POSTGRES_DB:-workflows} | |
| POSTGRES_URL: ${POSTGRES_URL} | |
| POSTGRES_USER: ${POSTGRES_USER:-postgres} | |
| POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres} | |
| POSTGRES_DB: ${POSTGRES_DB:-workflows} | |
| POSTGRES_URL: ${POSTGRES_URL:-postgresql://postgres:postgres@postgres:5432/workflows} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 83 - 86, The compose file exposes
POSTGRES_URL without a fallback which causes runtime errors where the app
expects it (see code that raises RuntimeError in migrations and settings
validation requiring POSTGRES_URL); update the docker-compose environment entry
for POSTGRES_URL to provide a sensible default fallback (e.g., a local Postgres
connection URI that matches POSTGRES_USER/POSTGRES_PASSWORD/POSTGRES_DB) so that
${POSTGRES_URL} becomes ${POSTGRES_URL:-<local-postgres-connection-string>} and
migrations/non-local deployments won't fail on fresh setups.
Comment on lines
35
to
38
| async def test_poll_inbox_handles_imap_errors( | ||
| self, email_monitor, mock_discord_channel, capfd | ||
| self, email_monitor_real_poll, mock_discord_channel | ||
| ): | ||
| """Test that IMAP errors are handled gracefully.""" |
There was a problem hiding this comment.
Docstring contradicts the test's actual assertion.
The docstring says "IMAP errors are handled gracefully," but the test asserts the error propagates with pytest.raises. The inline comment on line 44 already acknowledges the bubble-up behavior; the docstring should match.
🐛 Proposed fix
- """Test that IMAP errors are handled gracefully."""
+ """Test that IMAP transport errors propagate out of the poll coroutine."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def test_poll_inbox_handles_imap_errors( | |
| self, email_monitor, mock_discord_channel, capfd | |
| self, email_monitor_real_poll, mock_discord_channel | |
| ): | |
| """Test that IMAP errors are handled gracefully.""" | |
| async def test_poll_inbox_handles_imap_errors( | |
| self, email_monitor_real_poll, mock_discord_channel | |
| ): | |
| """Test that IMAP transport errors propagate out of the poll coroutine.""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_email_monitor.py` around lines 35 - 38, The test
docstring for test_poll_inbox_handles_imap_errors is incorrect: it says IMAP
errors are "handled gracefully" but the test asserts the error propagates using
pytest.raises; update the docstring in tests/integration/test_email_monitor.py
for the async test_poll_inbox_handles_imap_errors to state that IMAP errors are
propagated/bubbled up (e.g., "Test that IMAP errors are propagated") so it
accurately reflects the assertion and the inline comment about bubble-up
behavior.
…schema # Conflicts: # apps/worker/pyproject.toml # uv.lock
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/worker/src/five08/worker/migrations/versions/20260221_0100_create_jobs_table.py (1)
19-24: Consider adding a DB-levelserver_defaultfor UUID generation on theidcolumn.The
idcolumn currently has noserver_default, while other columns in the same table (attempts,max_attempts,created_at) do. Addingserver_default=sa.text("gen_random_uuid()")provides a defensive safeguard against any future code path that might omit the UUID.🛡️ Proposed addition of server_default
sa.Column( "id", postgresql.UUID(as_uuid=True), nullable=False, primary_key=True, + server_default=sa.text("gen_random_uuid()"), ),Note:
gen_random_uuid()is built-in from PostgreSQL 13+; for older versions useuuid_generate_v4()(requiresCREATE EXTENSION IF NOT EXISTS "uuid-ossp").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/migrations/versions/20260221_0100_create_jobs_table.py` around lines 19 - 24, The id column definition in the migration (sa.Column("id", postgresql.UUID(as_uuid=True), nullable=False, primary_key=True)) should include a DB-level default to defensively generate UUIDs; update that sa.Column call to add server_default=sa.text("gen_random_uuid()") (or use sa.text("uuid_generate_v4()") and ensure the uuid-ossp extension is created if supporting older Postgres versions) so the database will emit a UUID even if the application omits it.packages/shared/src/five08/settings.py (1)
42-55: Validator correctly uses resolved model fields — previousos.getenv()issue addressed.The
self.postgres_url.strip()check is sound, but note it only catches an explicitPOSTGRES_URL=""assignment. A production deployment that accidentally inherits the non-empty default (postgresql://postgres@postgres:5432/workflows) will pass validation silently. Onlyminio_root_password(default"") provides a meaningful guard.If the intent is to detect "not explicitly configured for production," comparing against the known default is more reliable:
♻️ Optional: strengthen the postgres_url check
- if not self.postgres_url.strip(): + _local_default = "postgresql://postgres@postgres:5432/workflows" + if not self.postgres_url.strip() or self.postgres_url == _local_default: raise ValueError("POSTGRES_URL must be set when RUNTIME_ENV is non-local.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/five08/settings.py` around lines 42 - 55, The validator validate_required_secrets currently only checks postgres_url for emptiness which misses cases where the field equals the non-production default string; update validate_required_secrets to also treat the known default "postgresql://postgres@postgres:5432/workflows" as invalid for non-local runtimes by comparing self.postgres_url.strip() against that default constant (or a module-level DEFAULT_POSTGRES_URL) and raising ValueError if it matches, keeping the existing minio_root_password check intact.apps/worker/src/five08/worker/api.py (1)
317-332:sync_people_handleris missing error handling consistent with the other enqueue handlers.
espocrm_webhook_handlerandespocrm_people_sync_webhook_handlerboth wrap their enqueue calls intry/except → 503. A queue/DB failure in_enqueue_full_crm_sync_jobhere will propagate as an unhandled 500 instead.♻️ Proposed fix
queue = request.app[QUEUE_KEY] + try: job = await _enqueue_full_crm_sync_job(queue, reason="manual") + except Exception: + logger.exception( + "Failed enqueueing CRM full-sync job queue=%s", settings.redis_queue_name + ) + return web.json_response({"error": "enqueue_failed"}, status=503) return web.json_response(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/five08/worker/api.py` around lines 317 - 332, sync_people_handler lacks the try/except used by other enqueue handlers (espocrm_webhook_handler, espocrm_people_sync_webhook_handler) so failures in _enqueue_full_crm_sync_job will surface as a 500; wrap the call to _enqueue_full_crm_sync_job in a try/except that catches exceptions, logs or records the error, and returns a web.json_response with status 503 and an appropriate error payload (matching the pattern used by the other handlers) to handle queue/DB failures gracefully.tests/unit/test_worker_api.py (1)
168-186: ImplicitAsyncMockauto-detection is inconsistent with adjacent tests.Python 3.12 auto-selects
AsyncMockfor coroutine patches, so this works. However, the sibling testtest_espocrm_people_sync_webhook_handler_enqueues_contact_jobs(line 204) usesnew_callable=AsyncMockexplicitly. Being consistent avoids relying on the auto-detection and makes the intent clearer.♻️ Suggested change
- with patch("five08.worker.api._enqueue_full_crm_sync_job") as mock_enqueue: + with patch( + "five08.worker.api._enqueue_full_crm_sync_job", new_callable=AsyncMock + ) as mock_enqueue:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_worker_api.py` around lines 168 - 186, The test uses patch on five08.worker.api._enqueue_full_crm_sync_job without explicitly specifying AsyncMock, relying on Py3.12 auto-detection; make it explicit for consistency with nearby tests by using patch(..., new_callable=AsyncMock) when mocking _enqueue_full_crm_sync_job in test_sync_people_handler_enqueues_full_sync and then set mock_enqueue.return_value to the Mock(id="job-sync", created=True) as before so the async call returns the expected job object.tests/unit/test_shared_settings.py (1)
9-26: Tests are correct; consider adding coverage for thepostgres_urlvalidation branch and env bypass.The two tests exercise the happy path and the
minio_root_passwordwhitespace guard well. Two gaps worth filling optionally:
- No test for an explicit
postgres_url=""in a non-local env (the otherValueErrorbranch).- No test that
runtime_env="dev"orruntime_env="test"skips validation entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_shared_settings.py` around lines 9 - 26, Add two unit tests to cover the missing validation branches: (1) a test function that creates SharedSettings with runtime_env="production" and postgres_url="" (or whitespace) and asserts ValidationError (or the specific ValueError message tied to postgres_url) is raised, referencing the SharedSettings constructor used in test_non_local_settings_require_non_empty_secrets; (2) a test function that constructs SharedSettings with runtime_env="dev" (or "test") and empty/whitespace postgres_url and minio_root_password and asserts no ValidationError is raised (i.e., validation is skipped in non-production environments), referencing the same SharedSettings class and its runtime_env behavior to ensure env bypass coverage.apps/worker/pyproject.toml (1)
9-11: Comment placement is slightly ambiguous —alembicis also pinned above it.The comment on line 10 clearly applies to
dramatiq[redis]==2.0.1, butalembic==1.18.4on line 9 uses the same==pin style without explanation. Consider moving the comment directly abovedramatiqor expanding it to cover both pins.✏️ Suggested clarification
"alembic==1.18.4", - # Pin intentionally for reproducible broker behavior across worker/api images. "dramatiq[redis]==2.0.1", + # Pin intentionally: dramatiq major version must match across worker/api images for protocol compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/pyproject.toml` around lines 9 - 11, The comment about intentional pinning is ambiguous because it sits between "alembic==1.18.4" and "dramatiq[redis]==2.0.1"; move the comment so it sits directly above "dramatiq[redis]==2.0.1" or reword it to explicitly state which packages are intentionally pinned (e.g., mention both "alembic" and "dramatiq[redis]") so readers know whether the reproduction requirement applies to one or both pins.
🤖 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/worker/src/five08/worker/api.py`:
- Around line 404-425: The try/except only catches ValueError from the
insert_audit_event call so DB errors (psycopg/asyncpg, constraint/connection
failures) bubble up as 500s; update the exception handling around the
asyncio.to_thread(insert_audit_event, ...) block to also catch database-related
exceptions (e.g., psycopg.Error or asyncpg.PostgresError and/or a broad
DatabaseError base) and convert them into a clear JSON error response (e.g.,
{"error":"database_error","detail":...}) with an appropriate status (502 or 500)
while logging the exception; also add a unit/integration test that mocks
insert_audit_event to raise a DB exception and asserts the handler returns the
expected JSON error response and status.
---
Nitpick comments:
In `@apps/worker/pyproject.toml`:
- Around line 9-11: The comment about intentional pinning is ambiguous because
it sits between "alembic==1.18.4" and "dramatiq[redis]==2.0.1"; move the comment
so it sits directly above "dramatiq[redis]==2.0.1" or reword it to explicitly
state which packages are intentionally pinned (e.g., mention both "alembic" and
"dramatiq[redis]") so readers know whether the reproduction requirement applies
to one or both pins.
In `@apps/worker/src/five08/worker/api.py`:
- Around line 317-332: sync_people_handler lacks the try/except used by other
enqueue handlers (espocrm_webhook_handler, espocrm_people_sync_webhook_handler)
so failures in _enqueue_full_crm_sync_job will surface as a 500; wrap the call
to _enqueue_full_crm_sync_job in a try/except that catches exceptions, logs or
records the error, and returns a web.json_response with status 503 and an
appropriate error payload (matching the pattern used by the other handlers) to
handle queue/DB failures gracefully.
In
`@apps/worker/src/five08/worker/migrations/versions/20260221_0100_create_jobs_table.py`:
- Around line 19-24: The id column definition in the migration (sa.Column("id",
postgresql.UUID(as_uuid=True), nullable=False, primary_key=True)) should include
a DB-level default to defensively generate UUIDs; update that sa.Column call to
add server_default=sa.text("gen_random_uuid()") (or use
sa.text("uuid_generate_v4()") and ensure the uuid-ossp extension is created if
supporting older Postgres versions) so the database will emit a UUID even if the
application omits it.
In `@packages/shared/src/five08/settings.py`:
- Around line 42-55: The validator validate_required_secrets currently only
checks postgres_url for emptiness which misses cases where the field equals the
non-production default string; update validate_required_secrets to also treat
the known default "postgresql://postgres@postgres:5432/workflows" as invalid for
non-local runtimes by comparing self.postgres_url.strip() against that default
constant (or a module-level DEFAULT_POSTGRES_URL) and raising ValueError if it
matches, keeping the existing minio_root_password check intact.
In `@tests/unit/test_shared_settings.py`:
- Around line 9-26: Add two unit tests to cover the missing validation branches:
(1) a test function that creates SharedSettings with runtime_env="production"
and postgres_url="" (or whitespace) and asserts ValidationError (or the specific
ValueError message tied to postgres_url) is raised, referencing the
SharedSettings constructor used in
test_non_local_settings_require_non_empty_secrets; (2) a test function that
constructs SharedSettings with runtime_env="dev" (or "test") and
empty/whitespace postgres_url and minio_root_password and asserts no
ValidationError is raised (i.e., validation is skipped in non-production
environments), referencing the same SharedSettings class and its runtime_env
behavior to ensure env bypass coverage.
In `@tests/unit/test_worker_api.py`:
- Around line 168-186: The test uses patch on
five08.worker.api._enqueue_full_crm_sync_job without explicitly specifying
AsyncMock, relying on Py3.12 auto-detection; make it explicit for consistency
with nearby tests by using patch(..., new_callable=AsyncMock) when mocking
_enqueue_full_crm_sync_job in test_sync_people_handler_enqueues_full_sync and
then set mock_enqueue.return_value to the Mock(id="job-sync", created=True) as
before so the async call returns the expected job object.
This was referenced Feb 21, 2026
This was referenced Mar 3, 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
Adds best-effort Discord CRM audit logging via a new bot audit helper and worker
/audit/eventsingest path backed by shared audit persistence utilities and newpeople/audit_eventsmigrations, introduces CRM people sync flows (scheduled full sync, manual/sync/people, and webhook/webhooks/espocrm/people-sync) with new worker jobs/processors, and updates related settings, compose/env/docs, namespace packaging, and focused tests across bot/worker/shared modules.Related Issue
N/A.
How Has This Been Tested?
./scripts/test.sh(203 passed).Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores