Draft/ai suggestions#168
Merged
Merged
Conversation
kaseywright
requested changes
May 27, 2026
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces an AI suggestions domain with HTTP-based orchestration between the API and an external AI service. It adds new database tables and columns for AI suggestions/usage tracking, chapter assignment AI toggling with policy/access controls, a pg-boss trigger worker, internal service-to-service endpoints, and supporting environment configuration and architecture documentation. ChangesAI Suggestions and Chapter AI Toggle Rollout
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant aiSuggestionsRoute
participant aiSuggestionsService
participant aiSuggestionsRepository
participant pgBoss
Client->>aiSuggestionsRoute: POST /ai-suggestions/queue-next
aiSuggestionsRoute->>aiSuggestionsService: queueNextVerses(...)
aiSuggestionsService->>aiSuggestionsRepository: hasReachedAiActivationThreshold
aiSuggestionsService->>aiSuggestionsRepository: getChapterAssignmentAiStatus
aiSuggestionsService->>aiSuggestionsRepository: findNextUntranslatedVerses
aiSuggestionsService->>pgBoss: send jobs to AI_SUGGESTION_TRIGGER queue
pgBoss-->>aiSuggestionsService: enqueued
aiSuggestionsService-->>aiSuggestionsRoute: QueueNextVersesResponse
aiSuggestionsRoute-->>Client: 200 {queued, thresholdMet}
sequenceDiagram
participant fluentAI as fluent-ai service
participant requireServiceAuth
participant aiInternalRoute
participant aiSuggestionsService
participant aiSuggestionsRepository
fluentAI->>requireServiceAuth: POST /ai-suggestions/internal/context (Bearer token)
requireServiceAuth->>aiInternalRoute: authorized
aiInternalRoute->>aiSuggestionsService: getSuggestionContext(params)
aiSuggestionsService->>aiSuggestionsRepository: getSuggestionContextData
aiSuggestionsRepository-->>aiSuggestionsService: contextVerses + sourceVerses
aiSuggestionsService-->>aiInternalRoute: SuggestionContextResponse
aiInternalRoute-->>fluentAI: 200 JSON
fluentAI->>requireServiceAuth: POST /ai-suggestions/internal/results (Bearer token)
requireServiceAuth->>aiInternalRoute: authorized
aiInternalRoute->>aiSuggestionsService: saveAiSuggestions(items)
aiSuggestionsService->>aiSuggestionsRepository: upsertAiSuggestions
aiInternalRoute-->>fluentAI: 200 {success: true}
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
Contributor
|
I've added markdown document to guide the transition from the cross-schema read pattern to the new http-based pattern. It defines the basics but I expect there will be a couple challenges where discussion will be needed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts (1)
34-56: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract a shared chapter-assignment progress mapper to avoid field drift.
These two map blocks are effectively identical, and this PR had to update both for
isAiEnabled. Centralizing the mapper will keep response shape changes consistent.Also applies to: 171-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts` around lines 34 - 56, Extract the duplicated map logic that transforms the raw chapter assignment data into the mapped response object into a separate utility function. This mapper function should accept the raw chapter assignment info object and return the transformed object with all fields (assignmentId, projectUnitId, status, bookNameEng, chapterNumber, bibleId, bookId, bookCode, sourceLangCode, assignedUser, peerChecker, totalVerses, completedVerses, createdAt, updatedAt, submittedTime, isAiEnabled). Then replace both instances of the inline map transformation (the one shown in the diff and the identical one around line 171-195) with calls to this shared mapper function to ensure consistency across the service and prevent field drift when the response shape changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/http-decoupling-transition.md`:
- Around line 93-97: The documentation currently states that
handleChapterAssigned is awaited from chapter-assignments.service.ts at three
call sites, but the actual implementation triggers it fire-and-forget rather
than awaiting it. Update the wording in the "current behavior" section to
accurately reflect that handleChapterAssigned is invoked asynchronously without
awaiting, so the transactional analysis and explanation of why the outbox
pattern is needed remains logically sound and matches the real implementation
behavior.
- Around line 29-39: The fenced code block containing the diagram is missing a
language identifier on the opening fence, which violates markdownlint rule
MD040. Modify the opening fence (the triple backticks before the diagram) to
include a language specifier by adding "text" after the backticks, changing it
from ``` to ```text to satisfy the documentation linting requirements.
In `@src/db/external/ai-schema.ts`:
- Around line 162-164: The patchAiSuggestionJobsSchema is derived from
insertAiSuggestionJobsSchema using partial(), but the insert schema omits
critical fields needed for job state transitions: status, retryCount, and
errorMessage. Define a separate patch schema for patchAiSuggestionJobsSchema
that explicitly includes these fields, allowing patch operations to validate
updates to job lifecycle states. You can achieve this by creating a new schema
definition that combines the necessary fields from the insert schema with the
additional fields required for updates, rather than relying solely on partial()
of the insert schema.
- Around line 109-110: The unique constraint on the ai_suggestion_usage_log
table is incomplete and causes usage data loss when the same user views the same
bible text across multiple project units. Update the unique index definition
named uq_ai_usage_user_text in the ai-schema.ts file to include projectUnitId
alongside userId and bibleTextId. Then locate the corresponding
onConflictDoUpdate target in the ai-suggestions.repository.ts file (around the
insert/conflict handling logic) and update it to also reference projectUnitId in
the conflict resolution criteria to ensure the unique constraint matches
properly.
In `@src/domains/ai-suggestions/ai-suggestions.repository.ts`:
- Around line 180-183: The upsert conflict target in the onConflictDoUpdate call
for the ai_suggestion_usage_log table is incomplete. The target array currently
only includes userId and bibleTextId, but since the table also stores
projectUnitId, you need to add ai_suggestion_usage_log.projectUnitId to the
target array in the onConflictDoUpdate function. This ensures that the upsert
key becomes the composite key of userId, bibleTextId, and projectUnitId, so
separate usage records are preserved for the same user and verse across
different project units instead of overwriting the previous record.
In `@src/domains/ai-suggestions/ai-suggestions.route.ts`:
- Around line 36-54: The responses schema in the route handler is missing a
declaration for the 404 NOT_FOUND response that can be returned by the
requireProjectUnitAccess middleware when a project unit is not found, breaking
the API contract accuracy. Add a [HttpStatusCodes.NOT_FOUND] entry to the
responses object using jsonContent with createMessageObjectSchema and an
appropriate error message describing that the project unit was not found. Apply
this fix to all three routes that use requireProjectUnitAccess as indicated in
the comment.
In `@src/domains/ai-suggestions/ai-suggestions.service.ts`:
- Around line 83-92: The function queueNextVersesForAssignment returns a Result
that indicates success or failure, but the current code ignores this return
value and always returns ok({ queued: true, thresholdMet: true }). Capture the
Result returned by the queueNextVersesForAssignment function call, check if it
represents an error using the Result type's error-checking method, and if so,
propagate that error by returning it instead of the success response. Apply this
fix to all occurrences of this pattern in the file (both at lines 83-92 and at
lines 149-157).
In `@src/domains/chapter-assignments/chapter-assignments.route.ts`:
- Around line 321-339: The responses object for the PATCH ai-status endpoint is
missing a BAD_REQUEST (400) response declaration, which is needed since the
route requires JSON body schema validation. Add a new entry to the responses
object with HttpStatusCodes.BAD_REQUEST as the key, using jsonContent with
createMessageObjectSchema to describe validation failures, following the same
pattern as the other error responses like UNAUTHORIZED and FORBIDDEN. The
description should indicate that the issue is related to invalid request body
formatting.
- Around line 309-313: The requireChapterAssignmentAccess middleware is missing
a case statement to handle CHAPTER_ASSIGNMENT_ACTIONS.TOGGLE_AI in its switch
statement, causing all TOGGLE_AI requests to fail with a 404. Add a new case for
CHAPTER_ASSIGNMENT_ACTIONS.TOGGLE_AI in the switch statement within
requireChapterAssignmentAccess that sets the allowed variable by calling
ChapterAssignmentPolicy.toggleAi with the policyUser and policyAssignment
parameters, then adds a break statement to complete the case.
In `@src/domains/chapter-assignments/chapter-assignments.service.ts`:
- Around line 83-89: The handleChapterAssigned() call is being invoked
synchronously within the transaction scope without checking if AI is enabled for
the assignment, which can cause jobs to be enqueued even if the transaction
rolls back or if the assignment has AI disabled. Refactor this by capturing the
trigger intent (the assignment parameters and AI enabled state) within the
transaction, then defer the actual handleChapterAssigned() call to execute after
successful commit using a post-commit hook or callback. Apply this post-commit
pattern to both occurrences of the handleChapterAssigned() invocation (in the
initial assignment and reassignment paths) and ensure the trigger is only queued
when the assignment's AI state indicates it should be enabled.
---
Outside diff comments:
In
`@src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts`:
- Around line 34-56: Extract the duplicated map logic that transforms the raw
chapter assignment data into the mapped response object into a separate utility
function. This mapper function should accept the raw chapter assignment info
object and return the transformed object with all fields (assignmentId,
projectUnitId, status, bookNameEng, chapterNumber, bibleId, bookId, bookCode,
sourceLangCode, assignedUser, peerChecker, totalVerses, completedVerses,
createdAt, updatedAt, submittedTime, isAiEnabled). Then replace both instances
of the inline map transformation (the one shown in the diff and the identical
one around line 171-195) with calls to this shared mapper function to ensure
consistency across the service and prevent field drift when the response shape
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 673ca2b2-9302-4405-a10c-3253f0e23726
📒 Files selected for processing (26)
docs/cross-schema-types.mddocs/http-decoupling-transition.mdsrc/app.tssrc/db/external/ai-schema.tssrc/db/migrations/0011_add_is_ai_enabled_to_chapter_assignments.sqlsrc/db/migrations/meta/0011_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/domains/ai-suggestions/ai-suggestions.auth.middleware.tssrc/domains/ai-suggestions/ai-suggestions.policy.tssrc/domains/ai-suggestions/ai-suggestions.repository.tssrc/domains/ai-suggestions/ai-suggestions.route.tssrc/domains/ai-suggestions/ai-suggestions.service.tssrc/domains/ai-suggestions/ai-suggestions.types.tssrc/domains/chapter-assignments/chapter-assignments.policy.tssrc/domains/chapter-assignments/chapter-assignments.repository.tssrc/domains/chapter-assignments/chapter-assignments.route.tssrc/domains/chapter-assignments/chapter-assignments.service.tssrc/domains/chapter-assignments/chapter-assignments.types.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.repository.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.service.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.types.tssrc/domains/users/chapter-assignments/users-chapter-assignments.service.tssrc/domains/users/chapter-assignments/users-chapter-assignments.types.tssrc/env.tssrc/scripts/clean-ai-jobs.ts
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ai-suggestions-workflow.md`:
- Around line 26-41: Several fenced blocks in the ai-suggestions-workflow
document are missing language tags, causing markdownlint MD040 warnings. Update
the opening fences for the diagram, pseudocode, system instruction, and endpoint
examples to include an appropriate language specifier such as text, pseudocode,
or http. Focus on the unlabeled fences around the workflow diagrams and the
named sections like Trigger A, Trigger B, pg-boss worker, Step 1–3, system
instruction, GET endpoint, and POST usage endpoint.
In `@src/db/migrations/0012_add_ai_tables.sql`:
- Line 26: The uniqueness key for ai_suggestion_usage_log is missing
projectUnitId, so rows from different project units can collide. Update the
unique index in the migration to scope uniqueness to userId, bibleTextId, and
projectUnitId, and make sure the upsert in ai-suggestions.repository.ts uses the
same three-column conflict target so the inserted projectUnitId always matches
the correct row.
In `@src/db/schema.ts`:
- Around line 533-536: The uniqueness constraint for the AI usage log is too
narrow because `uniqueIndex('uq_ai_usage_user_text')` only covers `userId` and
`bibleTextId`, so it must be expanded to include `projectUnitId` in `schema.ts`.
Update the matching migration so the database constraint matches the schema, and
adjust any upsert conflict target that currently references the old `(userId,
bibleTextId)` key to use the new `projectUnitId`-scoped key. Use the `ai usage`
table definition and any insert/upsert code touching that table to locate the
changes.
In `@src/domains/ai-internal/ai-internal.repository.ts`:
- Around line 172-188: The verse range queries in ai-internal.repository return
arrays without an explicit order, so results from sourceVersesQuery and
targetVersesQuery can come back scrambled. Update both query builders to sort by
verse number in ascending order using an ORDER BY on bible_texts.verseNumber (or
the selected verseNumber field) so multi-verse passages are always returned
deterministically and in sequence.
In `@src/domains/ai-internal/ai-internal.route.ts`:
- Around line 13-26: Update the `/internal/suggestion-context` handler in
`aiInternalService` routing so malformed request JSON from `c.req.json()` is
treated as a client error instead of falling into the generic 500 path. Add
explicit handling in the `server.post` try/catch for JSON parse failures,
alongside the existing `ZodError` branch, and return a 400 with a clear
validation-style message; apply the same fix to `/internal/ai-suggestions` so
both endpoints behave consistently.
In `@src/domains/ai-internal/ai-internal.types.ts`:
- Around line 14-25: The internal suggestion item schema is too strict because
`aiSuggestionItemSchema` requires `modelInfo`, while `upsertAiSuggestions()`
allows it to be missing. Relax `modelInfo` in `aiSuggestionItemSchema` so the
request contract matches the write path, and ensure
`upsertAiSuggestionsRequestSchema` still validates each item through that
updated schema without rejecting valid payloads lacking model metadata.
- Around line 3-10: The validation in suggestionContextRequestSchema is too
strict for canonical book codes, so it rejects valid numbered books handled by
ai-internal.constants. Update the bookCode field in
suggestionContextRequestSchema to accept the canonical 4-character formats used
by codes like 1JN, 2CO, and 1TI, while still supporting the existing valid
codes, and keep the rest of the schema unchanged.
In `@src/domains/ai-suggestions/ai-suggestions.service.ts`:
- Around line 127-129: The enqueue logic in ai-suggestions.service.ts is sending
the full jobs array as one payload, but triggerAiSuggestions expects each queue
item to be a single AiSuggestionTriggerJob object. Update the code around
getQueue() and boss.send(...) so each entry in jobs is enqueued individually,
one send call per job, instead of passing the array wholesale. Keep the payload
shape aligned with what the worker reads from job.data in triggerAiSuggestions.
In `@src/lib/ai-client.ts`:
- Around line 17-24: The AI service request in the fetch call can hang
indefinitely on a half-open connection, blocking the pg-boss worker. Update the
request in the AI client logic to use an AbortController with a bounded timeout,
and ensure the timeout is cleared after the request finishes. Make sure the
timeout is applied around the existing fetch(url, { method: 'POST', headers,
body }) call so failures surface as retryable errors instead of stalling the
handler.
In `@src/workers/ai-trigger.worker.ts`:
- Around line 28-55: The ai-trigger worker currently swallows rejected
triggerAiSuggestions() calls by using Promise.allSettled() and returning error
objects, which lets pg-boss.work() treat failed jobs as successful. Update the
logic in ai-trigger.worker.ts around the jobs.map/Promise.allSettled flow so
rejected jobs are surfaced as actual failures to the worker (or failed
individually) instead of being converted into return values, while keeping the
existing per-job logging in the results loop for the AI trigger job handling.
In `@src/workers/standalone-worker.ts`:
- Around line 68-69: The AI worker is being registered without the shared job
metrics hooks, so its in-flight jobs are not counted during shutdown. Update the
standalone worker registration to pass `metricsHooks` into
`registerAiTriggerWorker`, and update `ai-trigger.worker.ts` so each AI job is
wrapped with the same before/after hooks used by the USFM worker, ensuring
`workerMetrics.activeJobs` and shutdown tracking include both queues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb3e7e96-55c4-44e8-92e8-8bd700e47754
📒 Files selected for processing (20)
docs/ai-suggestions-workflow.mddocs/http-decoupling-transition.mdsrc/app.tssrc/db/migrations/0012_add_ai_tables.sqlsrc/db/migrations/meta/0012_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/domains/ai-internal/ai-internal.constants.tssrc/domains/ai-internal/ai-internal.repository.tssrc/domains/ai-internal/ai-internal.route.tssrc/domains/ai-internal/ai-internal.service.tssrc/domains/ai-internal/ai-internal.types.tssrc/domains/ai-suggestions/ai-suggestions.repository.tssrc/domains/ai-suggestions/ai-suggestions.service.tssrc/env.tssrc/lib/ai-client.tssrc/lib/queue.tssrc/middlewares/service-auth.tssrc/workers/ai-trigger.worker.tssrc/workers/standalone-worker.ts
💤 Files with no reviewable changes (1)
- src/domains/ai-suggestions/ai-suggestions.repository.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/workers/ai-trigger.worker.ts (1)
24-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid batch-wide retries for successful AI jobs
src/workers/ai-trigger.worker.ts:24-53—pg-bossretries the entire batch when this handler throws, so one failed job will replay already-succeededtriggerAiSuggestionscalls and duplicate requests to Fluent-AI. Handle per-job failures without rejecting the batch, or complete/fail jobs individually.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workers/ai-trigger.worker.ts` around lines 24 - 53, The batch handler in ai-trigger.worker.ts is rethrowing per-job errors from the Promise.all path, which causes pg-boss to retry the entire batch and duplicate successful triggerAiSuggestions calls. Update the QUEUE_NAMES.AI_SUGGESTION_TRIGGER worker to handle failures per job without rejecting the outer callback, or switch to completing/failing jobs individually inside the async job processing loop. Keep the existing logger, metricsHooks, and triggerAiSuggestions flow, but ensure one failed JobPayload does not abort the whole batch.src/domains/ai-suggestions/ai-suggestions.service.ts (1)
154-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
isAiEnabledbefore initial chapter queueing.
handleChapterAssigned()only checks the activation threshold. UnlikequeueNextVerses(), this path never consults the chapter AI toggle, so disabled assignments can still enqueue initial AI jobs on create/reassign.Suggested fix
- const isThresholdMet = await hasReachedAiActivationThreshold( - projectUnitId, - env.AI_ACTIVATION_THRESHOLD_VERSES - ); - - if (isThresholdMet) { + const [isThresholdMet, isAiEnabled] = await Promise.all([ + hasReachedAiActivationThreshold(projectUnitId, env.AI_ACTIVATION_THRESHOLD_VERSES), + getChapterAssignmentAiStatus(projectUnitId, bibleId, bookCode, chapterNumber), + ]); + + if (isAiEnabled === null) return err(ErrorCode.INVALID_REFERENCE); + + if (isThresholdMet && isAiEnabled) { await queueNextVersesForAssignment( projectUnitId, bibleId, bookCode.toUpperCase(), chapterNumber,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/ai-suggestions/ai-suggestions.service.ts` around lines 154 - 167, The initial chapter queueing path in handleChapterAssigned currently gates only on hasReachedAiActivationThreshold, so it can enqueue AI jobs even when the chapter toggle is off. Update handleChapterAssigned to also check isAiEnabled before calling queueNextVersesForAssignment, using the same AI enablement logic already used by queueNextVerses so create/reassign flows only enqueue when both conditions are satisfied.src/domains/ai-internal/ai-internal.repository.ts (1)
81-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude whitespace-only translations from the context pool.
content != ''still admits rows like' ', so this can feed blanktarget_textexamples intocontextVerses. The activation-threshold query already trims content; this filter should match it.Suggested fix
- sql`${translated_verses.content} IS NOT NULL AND ${translated_verses.content} != ''`, + sql`length(trim(${translated_verses.content})) > 0`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/ai-internal/ai-internal.repository.ts` around lines 81 - 87, The baseWhere filter in aiInternal.repository.ts still allows whitespace-only translated_verses.content values, which can leak blank target_text entries into contextVerses. Update the translated_verses.content condition in the query to match the activation-threshold logic by trimming content before checking emptiness, and keep the change localized in the baseWhere construction alongside the existing translated_verses, bible_texts, and projects predicates.src/domains/ai-internal/ai-internal.route.ts (1)
13-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the
SyntaxErrorhandling to JSON parsing only.The new branch wraps the whole handler, so an internal
SyntaxErrorthrown afterc.req.json()will now return400 Invalid JSON payloadinstead of a 500. Parse the body separately and keep the outer catch for real server faults.Suggested fix
server.post('/internal/suggestion-context', requireServiceAuth, async (c) => { try { - const body = await c.req.json(); + const body = await c.req.json().catch(() => null); + if (body === null) { + return c.json({ message: 'Invalid JSON payload' }, 400); + } const parsedBody = suggestionContextRequestSchema.parse(body); const data = await aiInternalService.getSuggestionContext(parsedBody); return c.json(data); } catch (error: any) { if (error.name === 'ZodError') { return c.json({ message: 'Validation failed', errors: error.errors }, 400); } - if (error instanceof SyntaxError) { - return c.json({ message: 'Invalid JSON payload' }, 400); - } return c.json({ message: error.message }, 500); } });Apply the same change to
/internal/ai-suggestions.Also applies to: 32-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/ai-internal/ai-internal.route.ts` around lines 13 - 28, The handler in ai-internal.route.ts is catching any SyntaxError from the whole request flow, which can misclassify internal server errors as invalid JSON. Move the JSON parsing into its own try/catch around c.req.json() in the /internal/suggestion-context route so only parsing failures return 400, and let the outer catch in the route continue returning 500 for other SyntaxError cases. Apply the same scoping fix to the /internal/ai-suggestions handler as well.src/db/migrations/meta/0012_snapshot.json (1)
160-179: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the snapshot to the new three-column usage-log key.
schema.tsand0012_add_ai_features.sqlnow scopeuq_ai_usage_user_textto(user_id, bible_text_id, project_unit_id), but this snapshot still records only the first two columns. Future Drizzle diffs will be generated from stale metadata.Suggested fix
{ "expression": "bible_text_id", "isExpression": false, "asc": true, "nulls": "last" + }, + { + "expression": "project_unit_id", + "isExpression": false, + "asc": true, + "nulls": "last" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations/meta/0012_snapshot.json` around lines 160 - 179, The snapshot metadata for uq_ai_usage_user_text is stale and still reflects only user_id and bible_text_id. Update the migration snapshot JSON so the unique index definition in the snapshot matches schema.ts and 0012_add_ai_features.sql by including project_unit_id in the column list for uq_ai_usage_user_text. This keeps the Drizzle snapshot in sync with the current three-column usage-log key and prevents incorrect future diffs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ai-suggestions-workflow.md`:
- Around line 176-178: The documentation references the wrong implementation
location for handleChapterAssigned(), which can mislead readers tracing the
workflow. Update the path reference to point to chapter-assignments.service.ts
instead of ai-suggestions.service.ts, using the handleChapterAssigned() symbol
to anchor the corrected location.
In
`@src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts`:
- Around line 236-245: The incremental sync path in buildAssignmentFilter() is
incorrectly returning an empty result when all projectIds are already in
excludeProjectIds, which prevents updated rows from being fetched for the
updatedAfter branch. Keep the already-synced project IDs as the filter input for
repo.getByProjects() when updatedAfter is present, and only use exclusion logic
to decide which project set to query rather than short-circuiting to ok([]).
Locate the fix in buildAssignmentFilter() and preserve the existing
repo.getByProjects() and toMemberChapterAssignmentResponse flow.
- Around line 211-225: The member chapter-assignment response is missing the
isAiEnabled field even though ChapterAssignmentWithProjectId already provides
it, so update toMemberChapterAssignmentResponse to pass that property through.
Also align memberChapterAssignmentResponseSchema in
users-chapter-assignments.types so the response contract includes isAiEnabled
and the mapper/schema stay in sync.
In `@src/env.ts`:
- Around line 47-48: The AI_INBOUND_SERVICE_KEY validation in env.ts is too
permissive because z.string().min(1) allows weak one-character secrets even
though requireServiceAuth uses it as the sole credential. Update the env schema
to enforce a minimum length consistent with the documented 32-character
requirement so invalid values fail fast, and keep the check attached to the
AI_INBOUND_SERVICE_KEY symbol in src/env.ts.
In `@src/workers/ai-trigger.worker.ts`:
- Around line 35-51: The batch in ai-trigger.worker.ts ends too early because
Promise.all() rejects on the first failed job while other triggerAiSuggestions
calls keep running, so onBatchEnd fires before all in-flight work is done.
Update the batching logic in the job-processing path that uses Promise.all and
the logger/metricsHooks callbacks to wait for every job to settle before calling
metricsHooks.onBatchEnd(jobs.length), and preserve per-job success/failure
tracking without aborting the rest of the batch on a single error.
---
Outside diff comments:
In `@src/db/migrations/meta/0012_snapshot.json`:
- Around line 160-179: The snapshot metadata for uq_ai_usage_user_text is stale
and still reflects only user_id and bible_text_id. Update the migration snapshot
JSON so the unique index definition in the snapshot matches schema.ts and
0012_add_ai_features.sql by including project_unit_id in the column list for
uq_ai_usage_user_text. This keeps the Drizzle snapshot in sync with the current
three-column usage-log key and prevents incorrect future diffs.
In `@src/domains/ai-internal/ai-internal.repository.ts`:
- Around line 81-87: The baseWhere filter in aiInternal.repository.ts still
allows whitespace-only translated_verses.content values, which can leak blank
target_text entries into contextVerses. Update the translated_verses.content
condition in the query to match the activation-threshold logic by trimming
content before checking emptiness, and keep the change localized in the
baseWhere construction alongside the existing translated_verses, bible_texts,
and projects predicates.
In `@src/domains/ai-internal/ai-internal.route.ts`:
- Around line 13-28: The handler in ai-internal.route.ts is catching any
SyntaxError from the whole request flow, which can misclassify internal server
errors as invalid JSON. Move the JSON parsing into its own try/catch around
c.req.json() in the /internal/suggestion-context route so only parsing failures
return 400, and let the outer catch in the route continue returning 500 for
other SyntaxError cases. Apply the same scoping fix to the
/internal/ai-suggestions handler as well.
In `@src/domains/ai-suggestions/ai-suggestions.service.ts`:
- Around line 154-167: The initial chapter queueing path in
handleChapterAssigned currently gates only on hasReachedAiActivationThreshold,
so it can enqueue AI jobs even when the chapter toggle is off. Update
handleChapterAssigned to also check isAiEnabled before calling
queueNextVersesForAssignment, using the same AI enablement logic already used by
queueNextVerses so create/reassign flows only enqueue when both conditions are
satisfied.
In `@src/workers/ai-trigger.worker.ts`:
- Around line 24-53: The batch handler in ai-trigger.worker.ts is rethrowing
per-job errors from the Promise.all path, which causes pg-boss to retry the
entire batch and duplicate successful triggerAiSuggestions calls. Update the
QUEUE_NAMES.AI_SUGGESTION_TRIGGER worker to handle failures per job without
rejecting the outer callback, or switch to completing/failing jobs individually
inside the async job processing loop. Keep the existing logger, metricsHooks,
and triggerAiSuggestions flow, but ensure one failed JobPayload does not abort
the whole batch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b147a9d-e3c4-432c-a8e5-f4a484608d8d
📒 Files selected for processing (22)
.env.example.env.testdocs/ai-suggestions-workflow.mdsrc/app.tssrc/db/migrations/0012_add_ai_features.sqlsrc/db/migrations/meta/0012_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/domains/ai-internal/ai-internal.repository.tssrc/domains/ai-internal/ai-internal.route.tssrc/domains/ai-internal/ai-internal.types.tssrc/domains/ai-suggestions/ai-suggestions.repository.tssrc/domains/ai-suggestions/ai-suggestions.service.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.repository.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.service.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.types.tssrc/domains/users/chapter-assignments/users-chapter-assignments.types.tssrc/env.tssrc/lib/ai-client.tssrc/lib/queue.tssrc/workers/ai-trigger.worker.tssrc/workers/standalone-worker.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/workers/ai-trigger.worker.ts (1)
24-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid batch-wide retries for successful AI jobs
src/workers/ai-trigger.worker.ts:24-53—pg-bossretries the entire batch when this handler throws, so one failed job will replay already-succeededtriggerAiSuggestionscalls and duplicate requests to Fluent-AI. Handle per-job failures without rejecting the batch, or complete/fail jobs individually.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workers/ai-trigger.worker.ts` around lines 24 - 53, The batch handler in ai-trigger.worker.ts is rethrowing per-job errors from the Promise.all path, which causes pg-boss to retry the entire batch and duplicate successful triggerAiSuggestions calls. Update the QUEUE_NAMES.AI_SUGGESTION_TRIGGER worker to handle failures per job without rejecting the outer callback, or switch to completing/failing jobs individually inside the async job processing loop. Keep the existing logger, metricsHooks, and triggerAiSuggestions flow, but ensure one failed JobPayload does not abort the whole batch.src/domains/ai-suggestions/ai-suggestions.service.ts (1)
154-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
isAiEnabledbefore initial chapter queueing.
handleChapterAssigned()only checks the activation threshold. UnlikequeueNextVerses(), this path never consults the chapter AI toggle, so disabled assignments can still enqueue initial AI jobs on create/reassign.Suggested fix
- const isThresholdMet = await hasReachedAiActivationThreshold( - projectUnitId, - env.AI_ACTIVATION_THRESHOLD_VERSES - ); - - if (isThresholdMet) { + const [isThresholdMet, isAiEnabled] = await Promise.all([ + hasReachedAiActivationThreshold(projectUnitId, env.AI_ACTIVATION_THRESHOLD_VERSES), + getChapterAssignmentAiStatus(projectUnitId, bibleId, bookCode, chapterNumber), + ]); + + if (isAiEnabled === null) return err(ErrorCode.INVALID_REFERENCE); + + if (isThresholdMet && isAiEnabled) { await queueNextVersesForAssignment( projectUnitId, bibleId, bookCode.toUpperCase(), chapterNumber,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/ai-suggestions/ai-suggestions.service.ts` around lines 154 - 167, The initial chapter queueing path in handleChapterAssigned currently gates only on hasReachedAiActivationThreshold, so it can enqueue AI jobs even when the chapter toggle is off. Update handleChapterAssigned to also check isAiEnabled before calling queueNextVersesForAssignment, using the same AI enablement logic already used by queueNextVerses so create/reassign flows only enqueue when both conditions are satisfied.src/domains/ai-internal/ai-internal.repository.ts (1)
81-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude whitespace-only translations from the context pool.
content != ''still admits rows like' ', so this can feed blanktarget_textexamples intocontextVerses. The activation-threshold query already trims content; this filter should match it.Suggested fix
- sql`${translated_verses.content} IS NOT NULL AND ${translated_verses.content} != ''`, + sql`length(trim(${translated_verses.content})) > 0`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/ai-internal/ai-internal.repository.ts` around lines 81 - 87, The baseWhere filter in aiInternal.repository.ts still allows whitespace-only translated_verses.content values, which can leak blank target_text entries into contextVerses. Update the translated_verses.content condition in the query to match the activation-threshold logic by trimming content before checking emptiness, and keep the change localized in the baseWhere construction alongside the existing translated_verses, bible_texts, and projects predicates.src/domains/ai-internal/ai-internal.route.ts (1)
13-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the
SyntaxErrorhandling to JSON parsing only.The new branch wraps the whole handler, so an internal
SyntaxErrorthrown afterc.req.json()will now return400 Invalid JSON payloadinstead of a 500. Parse the body separately and keep the outer catch for real server faults.Suggested fix
server.post('/internal/suggestion-context', requireServiceAuth, async (c) => { try { - const body = await c.req.json(); + const body = await c.req.json().catch(() => null); + if (body === null) { + return c.json({ message: 'Invalid JSON payload' }, 400); + } const parsedBody = suggestionContextRequestSchema.parse(body); const data = await aiInternalService.getSuggestionContext(parsedBody); return c.json(data); } catch (error: any) { if (error.name === 'ZodError') { return c.json({ message: 'Validation failed', errors: error.errors }, 400); } - if (error instanceof SyntaxError) { - return c.json({ message: 'Invalid JSON payload' }, 400); - } return c.json({ message: error.message }, 500); } });Apply the same change to
/internal/ai-suggestions.Also applies to: 32-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/ai-internal/ai-internal.route.ts` around lines 13 - 28, The handler in ai-internal.route.ts is catching any SyntaxError from the whole request flow, which can misclassify internal server errors as invalid JSON. Move the JSON parsing into its own try/catch around c.req.json() in the /internal/suggestion-context route so only parsing failures return 400, and let the outer catch in the route continue returning 500 for other SyntaxError cases. Apply the same scoping fix to the /internal/ai-suggestions handler as well.src/db/migrations/meta/0012_snapshot.json (1)
160-179: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the snapshot to the new three-column usage-log key.
schema.tsand0012_add_ai_features.sqlnow scopeuq_ai_usage_user_textto(user_id, bible_text_id, project_unit_id), but this snapshot still records only the first two columns. Future Drizzle diffs will be generated from stale metadata.Suggested fix
{ "expression": "bible_text_id", "isExpression": false, "asc": true, "nulls": "last" + }, + { + "expression": "project_unit_id", + "isExpression": false, + "asc": true, + "nulls": "last" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations/meta/0012_snapshot.json` around lines 160 - 179, The snapshot metadata for uq_ai_usage_user_text is stale and still reflects only user_id and bible_text_id. Update the migration snapshot JSON so the unique index definition in the snapshot matches schema.ts and 0012_add_ai_features.sql by including project_unit_id in the column list for uq_ai_usage_user_text. This keeps the Drizzle snapshot in sync with the current three-column usage-log key and prevents incorrect future diffs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ai-suggestions-workflow.md`:
- Around line 176-178: The documentation references the wrong implementation
location for handleChapterAssigned(), which can mislead readers tracing the
workflow. Update the path reference to point to chapter-assignments.service.ts
instead of ai-suggestions.service.ts, using the handleChapterAssigned() symbol
to anchor the corrected location.
In
`@src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts`:
- Around line 236-245: The incremental sync path in buildAssignmentFilter() is
incorrectly returning an empty result when all projectIds are already in
excludeProjectIds, which prevents updated rows from being fetched for the
updatedAfter branch. Keep the already-synced project IDs as the filter input for
repo.getByProjects() when updatedAfter is present, and only use exclusion logic
to decide which project set to query rather than short-circuiting to ok([]).
Locate the fix in buildAssignmentFilter() and preserve the existing
repo.getByProjects() and toMemberChapterAssignmentResponse flow.
- Around line 211-225: The member chapter-assignment response is missing the
isAiEnabled field even though ChapterAssignmentWithProjectId already provides
it, so update toMemberChapterAssignmentResponse to pass that property through.
Also align memberChapterAssignmentResponseSchema in
users-chapter-assignments.types so the response contract includes isAiEnabled
and the mapper/schema stay in sync.
In `@src/env.ts`:
- Around line 47-48: The AI_INBOUND_SERVICE_KEY validation in env.ts is too
permissive because z.string().min(1) allows weak one-character secrets even
though requireServiceAuth uses it as the sole credential. Update the env schema
to enforce a minimum length consistent with the documented 32-character
requirement so invalid values fail fast, and keep the check attached to the
AI_INBOUND_SERVICE_KEY symbol in src/env.ts.
In `@src/workers/ai-trigger.worker.ts`:
- Around line 35-51: The batch in ai-trigger.worker.ts ends too early because
Promise.all() rejects on the first failed job while other triggerAiSuggestions
calls keep running, so onBatchEnd fires before all in-flight work is done.
Update the batching logic in the job-processing path that uses Promise.all and
the logger/metricsHooks callbacks to wait for every job to settle before calling
metricsHooks.onBatchEnd(jobs.length), and preserve per-job success/failure
tracking without aborting the rest of the batch on a single error.
---
Outside diff comments:
In `@src/db/migrations/meta/0012_snapshot.json`:
- Around line 160-179: The snapshot metadata for uq_ai_usage_user_text is stale
and still reflects only user_id and bible_text_id. Update the migration snapshot
JSON so the unique index definition in the snapshot matches schema.ts and
0012_add_ai_features.sql by including project_unit_id in the column list for
uq_ai_usage_user_text. This keeps the Drizzle snapshot in sync with the current
three-column usage-log key and prevents incorrect future diffs.
In `@src/domains/ai-internal/ai-internal.repository.ts`:
- Around line 81-87: The baseWhere filter in aiInternal.repository.ts still
allows whitespace-only translated_verses.content values, which can leak blank
target_text entries into contextVerses. Update the translated_verses.content
condition in the query to match the activation-threshold logic by trimming
content before checking emptiness, and keep the change localized in the
baseWhere construction alongside the existing translated_verses, bible_texts,
and projects predicates.
In `@src/domains/ai-internal/ai-internal.route.ts`:
- Around line 13-28: The handler in ai-internal.route.ts is catching any
SyntaxError from the whole request flow, which can misclassify internal server
errors as invalid JSON. Move the JSON parsing into its own try/catch around
c.req.json() in the /internal/suggestion-context route so only parsing failures
return 400, and let the outer catch in the route continue returning 500 for
other SyntaxError cases. Apply the same scoping fix to the
/internal/ai-suggestions handler as well.
In `@src/domains/ai-suggestions/ai-suggestions.service.ts`:
- Around line 154-167: The initial chapter queueing path in
handleChapterAssigned currently gates only on hasReachedAiActivationThreshold,
so it can enqueue AI jobs even when the chapter toggle is off. Update
handleChapterAssigned to also check isAiEnabled before calling
queueNextVersesForAssignment, using the same AI enablement logic already used by
queueNextVerses so create/reassign flows only enqueue when both conditions are
satisfied.
In `@src/workers/ai-trigger.worker.ts`:
- Around line 24-53: The batch handler in ai-trigger.worker.ts is rethrowing
per-job errors from the Promise.all path, which causes pg-boss to retry the
entire batch and duplicate successful triggerAiSuggestions calls. Update the
QUEUE_NAMES.AI_SUGGESTION_TRIGGER worker to handle failures per job without
rejecting the outer callback, or switch to completing/failing jobs individually
inside the async job processing loop. Keep the existing logger, metricsHooks,
and triggerAiSuggestions flow, but ensure one failed JobPayload does not abort
the whole batch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5b147a9d-e3c4-432c-a8e5-f4a484608d8d
📒 Files selected for processing (22)
.env.example.env.testdocs/ai-suggestions-workflow.mdsrc/app.tssrc/db/migrations/0012_add_ai_features.sqlsrc/db/migrations/meta/0012_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/domains/ai-internal/ai-internal.repository.tssrc/domains/ai-internal/ai-internal.route.tssrc/domains/ai-internal/ai-internal.types.tssrc/domains/ai-suggestions/ai-suggestions.repository.tssrc/domains/ai-suggestions/ai-suggestions.service.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.repository.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.service.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.types.tssrc/domains/users/chapter-assignments/users-chapter-assignments.types.tssrc/env.tssrc/lib/ai-client.tssrc/lib/queue.tssrc/workers/ai-trigger.worker.tssrc/workers/standalone-worker.ts
🛑 Comments failed to post (5)
docs/ai-suggestions-workflow.md (1)
176-178: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the file path for
handleChapterAssigned().
handleChapterAssigned()is implemented inchapter-assignments.service.ts, notai-suggestions.service.ts. This misreference could mislead developers tracing the trigger flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ai-suggestions-workflow.md` around lines 176 - 178, The documentation references the wrong implementation location for handleChapterAssigned(), which can mislead readers tracing the workflow. Update the path reference to point to chapter-assignments.service.ts instead of ai-suggestions.service.ts, using the handleChapterAssigned() symbol to anchor the corrected location.src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts (2)
211-225: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate
isAiEnabledon the member chapter-assignment response.
ChapterAssignmentWithProjectIdalready carries this field from the repository, but this mapper drops it. The pairedmemberChapterAssignmentResponseSchemainsrc/domains/users/chapter-assignments/users-chapter-assignments.types.tsmirrors that omission, so this surface still does not expose the AI toggle.Suggested fix
function toMemberChapterAssignmentResponse(record: ChapterAssignmentWithProjectId) { return { chapterAssignmentId: record.id, projectId: record.projectId, projectUnitId: record.projectUnitId, bibleId: record.bibleId, bookId: record.bookId, chapterNumber: record.chapterNumber, assignedUserId: record.assignedUserId, peerCheckerId: record.peerCheckerId, status: record.status, submittedTime: record.submittedTime, createdAt: record.createdAt, updatedAt: record.updatedAt, + isAiEnabled: record.isAiEnabled, }; }export const memberChapterAssignmentResponseSchema = z.object({ chapterAssignmentId: z.number().int(), projectId: z.number().int(), projectUnitId: z.number().int(), bibleId: z.number().int(), bookId: z.number().int(), chapterNumber: z.number().int(), assignedUserId: z.number().int().nullable(), peerCheckerId: z.number().int().nullable(), status: z.string(), submittedTime: z.date().nullable(), createdAt: z.date().nullable(), updatedAt: z.date().nullable(), + isAiEnabled: z.boolean(), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts` around lines 211 - 225, The member chapter-assignment response is missing the isAiEnabled field even though ChapterAssignmentWithProjectId already provides it, so update toMemberChapterAssignmentResponse to pass that property through. Also align memberChapterAssignmentResponseSchema in users-chapter-assignments.types so the response contract includes isAiEnabled and the mapper/schema stay in sync.
236-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't short-circuit incremental sync when there are no new projects.
buildAssignmentFilter()usesexcludeProjectIdsas the already-synced set for theupdatedAfterbranch, but Lines 236-240 remove those IDs first and return[]when nothing new remains. If a client has already synced every project and only asks for deltas, it will never receive updates.Suggested fix
- const projectIds = projectsResult.data - .map((p) => p.id) - .filter((id) => !excludeProjectIds.includes(id)); + const projectIds = projectsResult.data.map((p) => p.id); if (projectIds.length === 0) return ok([]); const result = await repo.getByProjects(projectIds, excludeProjectIds, updatedAfter);📝 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.const projectIds = projectsResult.data.map((p) => p.id); if (projectIds.length === 0) return ok([]); const result = await repo.getByProjects(projectIds, excludeProjectIds, updatedAfter); if (!result.ok) return result; return ok(result.data.map(toMemberChapterAssignmentResponse));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts` around lines 236 - 245, The incremental sync path in buildAssignmentFilter() is incorrectly returning an empty result when all projectIds are already in excludeProjectIds, which prevents updated rows from being fetched for the updatedAfter branch. Keep the already-synced project IDs as the filter input for repo.getByProjects() when updatedAfter is present, and only use exclusion logic to decide which project set to query rather than short-circuiting to ok([]). Locate the fix in buildAssignmentFilter() and preserve the existing repo.getByProjects() and toMemberChapterAssignmentResponse flow.src/env.ts (1)
47-48: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require a high-entropy inbound service key.
requireServiceAuthtreats this value as the only credential for the internal callback routes, but.min(1)still accepts a one-character token. Enforce at least the same 32-character floor the example env file documents so weak secrets fail fast.Suggested fix
- AI_INBOUND_SERVICE_KEY: z.string().min(1), + AI_INBOUND_SERVICE_KEY: z.string().min(32),📝 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.// Key used to authenticate incoming webhook callbacks from fluent-ai AI_INBOUND_SERVICE_KEY: z.string().min(32),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/env.ts` around lines 47 - 48, The AI_INBOUND_SERVICE_KEY validation in env.ts is too permissive because z.string().min(1) allows weak one-character secrets even though requireServiceAuth uses it as the sole credential. Update the env schema to enforce a minimum length consistent with the documented 32-character requirement so invalid values fail fast, and keep the check attached to the AI_INBOUND_SERVICE_KEY symbol in src/env.ts.src/workers/ai-trigger.worker.ts (1)
35-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for every in-flight job before ending the batch.
Promise.all()rejects on the first failed POST, but the sibling requests keep running.onBatchEnd(jobs.length)then fires early, soworkerMetrics.activeJobscan drop to zero while AI calls are still in flight and shutdown can stop the worker mid-request.Suggested fix
try { - await Promise.all( + const results = await Promise.allSettled( jobs.map(async (job) => { const startTime = Date.now(); try { await triggerAiSuggestions([job.data]); logger.info('AI trigger job completed', { jobId: job.id }); if (metricsHooks.onJobSuccess) metricsHooks.onJobSuccess(Date.now() - startTime); } catch (error) { logger.error('AI trigger job failed', { jobId: job.id, error }); if (metricsHooks.onJobFailure) metricsHooks.onJobFailure(Date.now() - startTime); throw error; } }) ); + + const firstRejected = results.find( + (result): result is PromiseRejectedResult => result.status === 'rejected' + ); + if (firstRejected) { + throw firstRejected.reason; + } } finally { if (metricsHooks.onBatchEnd) metricsHooks.onBatchEnd(jobs.length); }📝 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.try { const results = await Promise.allSettled( jobs.map(async (job) => { const startTime = Date.now(); try { await triggerAiSuggestions([job.data]); logger.info('AI trigger job completed', { jobId: job.id }); if (metricsHooks.onJobSuccess) metricsHooks.onJobSuccess(Date.now() - startTime); } catch (error) { logger.error('AI trigger job failed', { jobId: job.id, error }); if (metricsHooks.onJobFailure) metricsHooks.onJobFailure(Date.now() - startTime); throw error; } }) ); const firstRejected = results.find( (result): result is PromiseRejectedResult => result.status === 'rejected' ); if (firstRejected) { throw firstRejected.reason; } } finally { if (metricsHooks.onBatchEnd) metricsHooks.onBatchEnd(jobs.length);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workers/ai-trigger.worker.ts` around lines 35 - 51, The batch in ai-trigger.worker.ts ends too early because Promise.all() rejects on the first failed job while other triggerAiSuggestions calls keep running, so onBatchEnd fires before all in-flight work is done. Update the batching logic in the job-processing path that uses Promise.all and the logger/metricsHooks callbacks to wait for every job to settle before calling metricsHooks.onBatchEnd(jobs.length), and preserve per-job success/failure tracking without aborting the rest of the batch on a single error.
kaseywright
requested changes
Jul 1, 2026
kaseywright
left a comment
kaseywright
left a comment
Contributor
There was a problem hiding this comment.
Here are a few initial thoughts. There will be more feedback as I think through the url and how the API <-> AI exchange should be setup.
Add design doc proposing to merge the ai-internal domain into ai-suggestions, eliminating duplicate table ownership and inconsistent error handling. The split along "who calls it" violates ARCHITECTURE.md's domain-boundary rules— both domains claim ownership of ai_suggestions. Proposes expressing the public/machine distinction via file naming (*.internal.route.ts) and middleware (requireServiceAuth) instead of a second top-level domain.
Contributor
|
I've added a new MD doc that describes how to refactor the |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domains/ai-suggestions/ai-suggestions.repository.test.ts`:
- Around line 6-20: The mock query chain in ai-suggestions.repository.test.ts
leaves mockChain.then with no default resolution, so any await on the chain can
hang if a test forgets to set a per-call implementation. Update the mockChain
setup to give then a safe fallback that resolves or rejects predictably, while
still allowing test-specific overrides, so repository methods using the chain
fail fast instead of timing out.
In `@src/domains/ai-suggestions/ai-suggestions.route.test.ts`:
- Around line 1-146: Add test coverage for POST /ai-suggestions/usage in
ai-suggestions.route.test.ts because trackUsage is mocked but never exercised.
Create a helper similar to postQueueNext to call the usage route, then add cases
that verify auth, permission, and checkProjectUnitAccess behavior plus the user
id guard. Use the existing symbols as anchors: asAuthenticatedUser,
checkProjectUnitAccess, and aiSuggestionsService.trackUsage, and assert a
successful request returns 200 with the expected response shape.
In `@src/middlewares/authenticate.ts`:
- Around line 38-41: The auth bypass in authenticate.ts is too broad because
c.req.path.startsWith('/internal') matches any path sharing that prefix, not
just internal routes. Update the condition in the authenticate middleware to
require a path boundary for the internal prefix, and keep the existing /api/auth
bypass unchanged. Use the authenticate function and its route-check logic as the
place to tighten the matching so only /internal and /internal/... are skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee5956ff-9c47-4ad4-8d28-5b815d8f5223
📒 Files selected for processing (12)
docs/ai-suggestions-internal-consolidation.mddocs/ai-suggestions-workflow.mddocs/http-decoupling-transition.mdsrc/domains/ai-suggestions/ai-suggestions.auth.middleware.tssrc/domains/ai-suggestions/ai-suggestions.repository.test.tssrc/domains/ai-suggestions/ai-suggestions.route.test.tssrc/domains/ai-suggestions/ai-suggestions.route.tssrc/domains/chapter-assignments/chapter-assignments.service.tssrc/domains/projects/chapter-assignments/project-chapter-assignments.repository.tssrc/lib/services/fluent-ai/fluent-ai.client.tssrc/middlewares/authenticate.tssrc/workers/ai-trigger.worker.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/ai-suggestions-internal-consolidation.md (2)
106-109: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSpell out the safe rollout order.
“Atomic cutover” is underspecified here: the fluent-ai worker hardcodes these URLs, so any partial deploy between repos will break the callback flow. Add an explicit deploy sequence or temporary compatibility window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ai-suggestions-internal-consolidation.md` around lines 106 - 109, The rollout guidance is too vague for the hardcoded fluent-ai callback URLs, so update the Rollout section to spell out a safe deploy order or temporary compatibility window. Refer to the fluent-api route-rename and fluent-ai URL update, and make it explicit which repo should deploy first, how long both URL shapes must be accepted, and when the old callback path can be removed.
3-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the document status.
This branch already contains the consolidated internal-route shape, so calling the doc “proposed design — not yet implemented” reads stale and will confuse readers using it as rollout guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ai-suggestions-internal-consolidation.md` around lines 3 - 6, The document status text is stale and still describes the consolidated internal-route shape as only “proposed design,” which conflicts with the branch contents. Update the status wording in this doc so it reflects the current implemented/merged state and aligns with the rollout guidance; keep the change localized to the introductory metadata near the audience/status section, and make sure the terminology matches the surrounding references like docs/http-decoupling-transition.md and docs/ai-suggestions-workflow.md.src/domains/chapter-assignments/chapter-assignments.service.ts (1)
386-411: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale/misleading comment; enqueue is outside the transaction.
The comment on Line 392 says "Await inside transaction to prevent data loss…," but the
handleChapterAssignedcall runs after thedb.transaction(...)block closes on Line 388, so it is not inside the transaction. Update or remove the comment to avoid confusion in this concurrency-sensitive area. Also, wrapping a singlerepo.updateindb.transaction(Lines 386-388) is redundant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domains/chapter-assignments/chapter-assignments.service.ts` around lines 386 - 411, The comment and transaction usage in chapter assignment AI toggling are misleading: `handleChapterAssigned` is executed after `db.transaction(...)` completes, so it is not “inside the transaction,” and the lone `repo.update` transaction in `chapterAssignmentsService` is unnecessary. Update or remove the stale comment near the `handleChapterAssigned` call, and simplify the `db.transaction`/`repo.update` flow so it matches the actual execution order while keeping the existing error handling in the `isAiEnabled` branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domains/ai-suggestions/ai-suggestions.internal.route.ts`:
- Around line 16-31: The internal AI suggestions routes are not guarding
c.req.json(), so malformed or empty JSON can throw before validation and become
a 500. Update both server.post handlers in ai-suggestions.internal.route.ts,
including the /context route and the /results route, to wrap c.req.json() in
try/catch and return a 400 response when parsing fails, then continue with the
existing suggestionContextRequestSchema validation and aiSuggestionsService
calls only after a successful parse.
In `@src/domains/ai-suggestions/ai-suggestions.service.ts`:
- Around line 193-194: Move the hardcoded context cap out of
ai-suggestions.service.ts and into the shared ai-suggestions.constants.ts module
as MAX_CONTEXT_VERSES_TOTAL, then import and use that constant where the current
limit is set in the aiSuggestions service. Remove the inline magic
number/comment pairing so the service references the centralized constant
consistently with the other domain tunables.
In `@src/domains/chapter-assignments/chapter-assignments.service.ts`:
- Around line 207-234: Defer the AI enqueue side effect until after the
assignment transaction is fully committed, because
`chapter-assignments.service.ts` currently calls
`aiSuggestionsService.handleChapterAssigned()` inside `exec`/`db.transaction`
via `result.data.shouldTriggerAi`, which can run before an `externalTx` is
durable. Update the `exec` flow to return the `shouldTriggerAi` signal (and
assignment identifiers) from `result.data.response`, then trigger
`handleChapterAssigned()` only at the outer transaction boundary or in the
caller after commit. Keep the existing `logger.error` handling around
`aiSuggestionsService.handleChapterAssigned`, but move the call so it cannot
fire for a transaction that later rolls back.
In `@src/middlewares/authenticate.ts`:
- Around line 38-41: The auth bypass in authenticate.ts is too broad because
c.req.path.includes('/internal/') will skip session auth for any future route
containing that substring. Update the route check in authenticate() to match the
known internal AI-suggestions prefix explicitly, using a dedicated
prefix/segment test for /ai-suggestions/internal/* instead of a generic
substring, while keeping the /api/auth bypass unchanged.
---
Outside diff comments:
In `@docs/ai-suggestions-internal-consolidation.md`:
- Around line 106-109: The rollout guidance is too vague for the hardcoded
fluent-ai callback URLs, so update the Rollout section to spell out a safe
deploy order or temporary compatibility window. Refer to the fluent-api
route-rename and fluent-ai URL update, and make it explicit which repo should
deploy first, how long both URL shapes must be accepted, and when the old
callback path can be removed.
- Around line 3-6: The document status text is stale and still describes the
consolidated internal-route shape as only “proposed design,” which conflicts
with the branch contents. Update the status wording in this doc so it reflects
the current implemented/merged state and aligns with the rollout guidance; keep
the change localized to the introductory metadata near the audience/status
section, and make sure the terminology matches the surrounding references like
docs/http-decoupling-transition.md and docs/ai-suggestions-workflow.md.
In `@src/domains/chapter-assignments/chapter-assignments.service.ts`:
- Around line 386-411: The comment and transaction usage in chapter assignment
AI toggling are misleading: `handleChapterAssigned` is executed after
`db.transaction(...)` completes, so it is not “inside the transaction,” and the
lone `repo.update` transaction in `chapterAssignmentsService` is unnecessary.
Update or remove the stale comment near the `handleChapterAssigned` call, and
simplify the `db.transaction`/`repo.update` flow so it matches the actual
execution order while keeping the existing error handling in the `isAiEnabled`
branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d5cf327-767f-4ec1-8087-e6e7e5751aa2
📒 Files selected for processing (14)
docs/ai-suggestions-internal-consolidation.mdsrc/app.tssrc/domains/ai-suggestions/ai-suggestions.constants.tssrc/domains/ai-suggestions/ai-suggestions.internal.route.test.tssrc/domains/ai-suggestions/ai-suggestions.internal.route.tssrc/domains/ai-suggestions/ai-suggestions.repository.test.tssrc/domains/ai-suggestions/ai-suggestions.repository.tssrc/domains/ai-suggestions/ai-suggestions.route.test.tssrc/domains/ai-suggestions/ai-suggestions.service.tssrc/domains/ai-suggestions/ai-suggestions.types.tssrc/domains/chapter-assignments/chapter-assignments.route.tssrc/domains/chapter-assignments/chapter-assignments.service.tssrc/middlewares/authenticate.tssrc/middlewares/service-auth.ts
💤 Files with no reviewable changes (1)
- src/domains/ai-suggestions/ai-suggestions.constants.ts
added 4 commits
July 6, 2026 14:25
Replace substring check (`includes('/internal/')`) with a startsWith match
against SERVICE_AUTH_PATH_PREFIXES. The substring approach would skip session
auth for any route containing '/internal/' anywhere in its path (e.g.,
'/api/foo/internal/bar'), silently bypassing authentication. Explicit prefixes
ensure only intended machine-facing routes (currently '/ai-suggestions/internal/')
bypass session auth in favor of requireServiceAuth.
Add test coverage for malformed JSON payloads on /context and /results endpoints. Both routes now catch JSON parse errors and return 400 with 'Invalid JSON payload' instead of throwing unhandled exceptions.
…ction Update comment to explain that handleChapterAssigned runs after the isAiEnabled update commits (not inside a transaction), and errors are caught to prevent a failed AI trigger from affecting the already-saved status.
Move the hardcoded 100 limit from getSuggestionContext into a named constant with a comment explaining it controls the total number of context verses (FTS + proximity combined) returned per request. Also fix trailing whitespace in workflow doc table.
kaseywright
previously approved these changes
Jul 7, 2026
added 2 commits
July 7, 2026 15:15
Resolved merge conflicts: - .env.example: Combined AI_INBOUND_SERVICE_KEY and EN_FEATURE_REPEATED_WORD_CHECK configs - src/env.ts: Combined AI_INBOUND_SERVICE_KEY and EN_FEATURE_REPEATED_WORD_CHECK env vars - src/db/migrations/meta/0012_snapshot.json: Kept pericope sets version, removed AI features tables - src/db/migrations/meta/_journal.json: Updated to include both 0012 (pericope sets) and 0014 (AI features) - src/domains/projects/chapter-assignments/project-chapter-assignments.service.ts: Kept isAiEnabled field in response mapping - Renamed 0012_add_ai_features.sql to 0014_add_ai_features.sql to avoid migration number conflict - Generated new migration 0015_add_ai_suggestions.sql via drizzle-kit
- Removed duplicate 0015_add_ai_suggestions.sql (identical to 0014_add_ai_features.sql) - Updated _journal.json to remove 0015 entry - Removed 0015_snapshot.json - Migrations now run successfully
- Add EN_FEATURE_AI_SUGGESTIONS env var to EnvSchema (same contract as repeatedWordCheck: unset derives from AI wiring, safe-off when unwired) - Register aiSuggestions in FLAGS registry with aiIsWired default resolver - Add aiSuggestions to featuresSchema OpenAPI definition - Document flag in .env.example - Tests: explicit set, wired/unwired derivation, and authenticated publication
kaseywright
approved these changes
Jul 7, 2026
kaseywright
left a comment
kaseywright
left a comment
Contributor
There was a problem hiding this comment.
This API branch now contains an env variable that will enable and disable this feature. This means that we can merge it without having the AI service available in PROD.
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
2 participants
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.
#152 , #146, #148
Summary by CodeRabbit