fix(tools): Revamp agentic tools #1067
Conversation
junaid-shirur
requested review from
devesh-juspay,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
Summary of ChangesHello @junaid-shirur, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request, marked as a Work In Progress (WIP), primarily focuses on two areas: refactoring the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring by renaming Intent to MailParticipant for better clarity, along with adding a suite of new Google tools. While the changes are extensive and seem to be a work in progress, I've identified several issues that should be addressed to improve code quality, correctness, and maintainability. Key areas for improvement include removing debug code, addressing hardcoded values, fixing inconsistencies in naming and logic, and correcting a bug in query construction. Please see my detailed comments below.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRenames Intent → MailParticipant across types, prompts, provider and chat surfaces; updates function signatures and variable names to use mailParticipants/participants; adds Google/global search tools, Vespa binding Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ChatAPI as Chat API
participant Resolver as resolveNamesToEmails
participant Provider as AI Provider
participant Vespa as Vespa Search
User->>ChatAPI: Query (names / participants)
ChatAPI->>Resolver: resolveNamesToEmails(mailParticipants, ...)
Resolver->>Provider: extractEmailsFromContext(participants, userCtx, retrievedCtx, params)
Provider-->>Resolver: { emails: MailParticipant }
Resolver-->>ChatAPI: resolvedMailParticipants
ChatAPI->>Vespa: executeVespaSearch(..., mailParticipants=resolvedMailParticipants, ...)
Vespa-->>ChatAPI: results
ChatAPI-->>User: Answer + references
note right of Provider #ddeeff: provider/model selection & logging
sequenceDiagram
autonumber
actor Agent
participant Tools as Agent Tools
participant Vespa as Vespa Search
rect rgb(240,248,255)
note over Tools: New Google tool surface (Gmail/Drive/Calendar/Contacts)
Agent->>Tools: searchGmail/searchDrive/... (params + mailParticipants)
Tools->>Vespa: executeVespaSearch(app, entity, mailParticipants, timeRange, ...)
Vespa-->>Tools: search results
Tools-->>Agent: normalized results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
server/api/chat/chat.ts (2)
220-327: Fix resolveNamesToEmails: wrong return shape, duplicate span.end(), and weak typing
- Returning
resolutionResult.emails || []likely breaks downstream expecting a MailParticipant-shaped object.searchSpan?.end()is called twice (Line 283 and Line 321).- Type should be
Promise<MailParticipant>; merge resolved emails onto the original participant object.Apply:
-export async function resolveNamesToEmails( - intent: MailParticipant, +export async function resolveNamesToEmails( + mailParticipant: MailParticipant, email: string, userCtx: string, userMetadata: UserMetadataType, span?: Span, -): Promise<any> { +): Promise<MailParticipant> { @@ - const extractedNames = extractNamesFromIntent(intent) + const extractedNames = extractNamesFromIntent(mailParticipant) @@ - if (allNames.length === 0) { + if (allNames.length === 0) { resolveSpan?.setAttribute("no_names_found", true) resolveSpan?.end() - return intent + return mailParticipant } @@ - if (allNamesAreEmails) { + if (allNamesAreEmails) { resolveSpan?.setAttribute("all_names_are_emails", true) resolveSpan?.setAttribute("skip_resolution", true) resolveSpan?.end() - return intent + return mailParticipant } @@ - let resolvedData: MailParticipant = {} + let resolvedData: MailParticipant = mailParticipant const resolutionResult = await extractEmailsFromContext( extractedNames, userCtx, searchContext, { modelId: config.defaultFastModel, json: false, stream: false }, ) - resolvedData = resolutionResult.emails || [] + const resolvedEmails = (resolutionResult?.emails ?? {}) as Partial<MailParticipant> + resolvedData = { ...mailParticipant, ...resolvedEmails } - searchSpan?.end() return resolvedData } catch (error) { resolveSpan?.end() - return intent + return mailParticipant } }
3519-3526: Double span end and premature end before downstream usage
span?.end()is called twice and is also ended before passingspantoprocessResultsForMetadata, which still uses it.
- Remove both early ends here to avoid ending before downstream processing. End it at the outer scope if needed.
- span?.setAttribute("context", buildContext(items, 20, userMetadata)) - span?.end() + span?.setAttribute("context", buildContext(items, 20, userMetadata)) @@ - if (!items.length) { - span?.end() + if (!items.length) { loggerWithChild({ email: email }).info( "No documents found for unspecific metadata retrieval", ) yield { text: METADATA_NO_DOCUMENTS_FOUND } return }server/api/chat/tools.ts (1)
110-136: Fix the shadowedselectedItemvariableThe inner
let selectedIteminside thetryblock shadows the outerselectedItem. After the block, the outer variable is still{}, soselectedItemsin the returned object is always empty, breaking downstream logic that relies on the parsed selections. Reuse the outer variable instead of redeclaring it.- let selectedItem: any = {} + selectedItem = {}
♻️ Duplicate comments (6)
server/ai/provider/index.ts (2)
1605-1616: Respect user timezone when building prompts.Hard-coding
Asia/Kolkatahere will misstate the date for anyone outside IST. Thread the realuserTimeZone(already available on the request/metadata path) intogetDateForAIinstead of forcing a fixed zone.
434-435: Remove debugconsole.logbefore shipping.Please drop this debug log (or route it through the structured logger) so we don’t leak internal model/provider selections into stdout in production.
server/api/chat/chat.ts (2)
221-221: Rename param for clarity and consistency with typeRename
intenttomailParticipantto match theMailParticipanttype. Please update usages inside the function as well.
3474-3475: Update log label: "Intent" → "Mail Participants"Keep terminology consistent with
classification.filters.mailParticipants.- `[GetItems] Calling getItems - Schema: ${schema}, App: ${apps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, Intent: ${JSON.stringify(classification.filters.mailParticipants)}`, + `[GetItems] Calling getItems - Schema: ${schema}, App: ${apps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, Mail Participants: ${JSON.stringify(classification.filters.mailParticipants)}`,server/api/chat/tools.ts (2)
267-270: Restore dynamic user timezone handlingHard-coding
userTimezoneto"Asia/Kolkata"(and passing the same togetDateForAI) ignores the actual user context and repeats the earlier bug that was already flagged. Please derive the timezone from the user metadata/config so all users get correct local dates.
2142-2148: Correct the Gmail attachment query construction
enhancedQuerybecomes syntactically invalid (...pdf,docx)) and lacks the required field qualifiers, so Vespa receives a broken filter. Build it like the Drive tool does—prepend the proper filter key and join withOR.- if (params.fileType && params.fileType.length > 0) { - enhancedQuery += `${params.fileType.join(",")})` - } + if (params.fileType && params.fileType.length > 0) { + enhancedQuery += ` filetype:(${params.fileType.join(" OR ")})` + }
🧹 Nitpick comments (2)
server/api/chat/chat.ts (2)
3427-3430: Update log label: "Resolved intent" → "Resolved mailParticipants"Align log text with the refactor.
- loggerWithChild({ email: email }).info( - `[${QueryType.SearchWithoutFilters}] Resolved intent: ${JSON.stringify(resolvedMailParticipants)}`, - ) + loggerWithChild({ email: email }).info( + `[${QueryType.SearchWithoutFilters}] Resolved mailParticipants: ${JSON.stringify(resolvedMailParticipants)}`, + )
3589-3592: Update log label: "Resolved intent" → "Resolved mailParticipants"Consistency with filters and prior logs.
- loggerWithChild({ email: email }).info( - `[SearchWithFilters] Resolved intent: ${JSON.stringify(resolvedMailParticipants)}`, - ) + loggerWithChild({ email: email }).info( + `[SearchWithFilters] Resolved mailParticipants: ${JSON.stringify(resolvedMailParticipants)}`, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/ai/provider/index.ts(10 hunks)server/ai/types.ts(2 hunks)server/api/chat/agents.ts(3 hunks)server/api/chat/chat.ts(18 hunks)server/api/chat/mapper.ts(8 hunks)server/api/chat/tools.ts(20 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T16:18:07.128Z
Learnt from: Aayushjshah
PR: xynehq/xyne#787
File: server/api/chat/mapper.ts:631-636
Timestamp: 2025-09-08T16:18:07.128Z
Learning: In server/api/chat/mapper.ts, the filter_query parameter for getSlackThreads should remain required: false (optional) in the mapper definition, even if it differs from the AgentTool definition in server/api/chat/tools.ts. This is an intentional design decision where parameter requirements can differ between the mapper and tool layers.
Applied to files:
server/api/chat/tools.ts
🧬 Code graph analysis (4)
server/ai/provider/index.ts (3)
server/ai/prompts.ts (1)
temporalDirectionJsonPrompt(1637-1966)server/utils/index.ts (1)
getDateForAI(3-29)server/ai/types.ts (2)
MailParticipant(393-393)ModelParams(284-309)
server/api/chat/mapper.ts (1)
server/shared/types.ts (1)
DriveEntity(33-33)
server/api/chat/chat.ts (3)
server/ai/types.ts (1)
MailParticipant(393-393)server/api/chat/chunk-selection.ts (1)
getChunkCountPerDoc(13-148)server/api/chat/utils.ts (1)
collectFollowupContext(79-144)
server/api/chat/tools.ts (7)
server/ai/types.ts (1)
MailParticipant(393-393)server/utils/index.ts (1)
getDateForAI(3-29)server/search/vespa.ts (1)
SearchVespaThreads(136-136)server/utils.ts (1)
getErrorMessage(103-106)server/api/chat/types.ts (2)
MinimalAgentFragment(88-94)AgentTool(110-134)server/api/chat/mapper.ts (2)
googleTools(1021-1261)convertToAgentToolParameters(848-866)server/tracer.ts (1)
Span(24-38)
🔇 Additional comments (5)
server/api/chat/chat.ts (5)
2294-2304: Good fallback for selected-context pathClear UX when no answer found from selected context. LGTM.
3046-3053: Filters destructuring update looks correctSwitch to
mailParticipantsin filters is consistent with the refactor.
3240-3276: Validate shape compatibility of resolvedMailParticipantsEnsure
resolveNamesToEmailsreturns a MailParticipant-shaped object; otherwise passing it asmailParticipantsto search/getItems will break filtering.If you adopt the fix suggested in resolveNamesToEmails, this will be consistent.
4519-4544: Follow-up context carry-forward: good guardrails
- Excluding assistant messages with empty content avoids noise.
- Carry-forward dedup via Set is correct.
7325-7328: Safer access with optional chainingPrevents index errors when assistant response is missing. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/api/chat/utils.ts (1)
1038-1053: Fix undefinedIntenttype usage.
Intentno longer exists, so this function now fails to compile and yields the wrong shape. Swap the argument and return type to the newMailParticipant.-export function extractNamesFromIntent(intent: any): Intent { - if (!intent || typeof intent !== "object") return {} - - const result: Intent = {} +export function extractNamesFromIntent( + intent: MailParticipant | null | undefined, +): MailParticipant { + if (!intent || typeof intent !== "object") return {} + + const result: MailParticipant = {}server/api/chat/tools.ts (1)
185-226: Stop shadowingselectedItem.The inner
let selectedItemcreates a new binding, so the outer variable that you return stays{}. Every caller (e.g. Drive search) loses the agent’s selected items and can’t scope searches correctly. Reuse the outer variable instead of redeclaring it.- let selectedItem: any = {} + selectedItem = {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/api/chat/mapper.ts(10 hunks)server/api/chat/tools.ts(23 hunks)server/api/chat/types.ts(1 hunks)server/api/chat/utils.ts(1 hunks)server/search/vespa.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T16:18:07.128Z
Learnt from: Aayushjshah
PR: xynehq/xyne#787
File: server/api/chat/mapper.ts:631-636
Timestamp: 2025-09-08T16:18:07.128Z
Learning: In server/api/chat/mapper.ts, the filter_query parameter for getSlackThreads should remain required: false (optional) in the mapper definition, even if it differs from the AgentTool definition in server/api/chat/tools.ts. This is an intentional design decision where parameter requirements can differ between the mapper and tool layers.
Applied to files:
server/api/chat/tools.ts
🧬 Code graph analysis (1)
server/api/chat/tools.ts (9)
server/api/chat/types.ts (2)
MinimalAgentFragment(88-94)AgentTool(110-134)server/api/chat/utils.ts (1)
searchToCitation(441-539)server/ai/context.ts (1)
answerContextMap(741-809)server/types.ts (1)
UserMetadataType(605-605)server/utils/index.ts (1)
getDateForAI(3-29)server/api/chat/mapper.ts (3)
searchGlobalTool(979-1020)convertToAgentToolParameters(850-868)googleTools(1022-1215)server/search/vespa.ts (2)
SearchVespaThreads(136-136)searchGoogleApps(131-131)server/utils.ts (1)
getErrorMessage(103-106)server/search/utils.ts (1)
extractDriveIds(74-117)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
server/api/chat/mapper.ts (4)
988-989: Fix typo: "retireve" → "retrieve".The word "retireve" is misspelled. This typo appears in multiple tool descriptions throughout the file.
Apply this diff:
- - query should be keywords focus to retireve the most relevant content from corpus.`, + - query should be keywords focused to retrieve the most relevant content from corpus.`,
1032-1035: Improve description clarity and fix typo.The description has formatting issues and a typo:
- Run-on sentence with awkward flow
- "retireve" should be "retrieve"
- "keywords focus" should be "keywords focused"
Apply this diff:
- description: `Extracted search terms from the user's query to find emails if query doesn't contain important semantic terms, will fetch based on different params e.g. timeRange, participants, labels etc. + description: `Extracted search terms from the user's query to find emails. If the query doesn't contain important semantic terms, the tool will fetch emails based on other parameters like timeRange, participants, and labels. - don't apply filters in the query like "from:sahebjot" or "subject:meeting". - - query should be keywords focus to retireve the most relevant content from corpus. + - query should be keywords focused to retrieve the most relevant content from corpus. `,
1072-1073: Fix grammar: "search" → "searches".Subject-verb agreement issue: "it also search" should be "it also searches".
Apply this diff:
description: - "If true, it also search for the attachments in the emails.", + "If true, it also searches for the attachments in the emails.",
1129-1129: Add.join()to properly format the file types list.Using
.map()without.join()will render the array representation in the description string instead of a formatted list.Apply this diff:
- description: `Filter files by type (e.g., ${Object.values(DriveEntity).map((e) => `- ${e}\n`)}).`, + description: `Filter files by type (e.g., ${Object.values(DriveEntity).map((e) => `${e}`).join(', ')}).`,server/api/chat/tools.ts (2)
254-254: Fix property name:mailParticipant→mailParticipants.There's an inconsistency in the naming of this property. Here it's
mailParticipant(singular), but inserver/ai/types.tsand other parts of the codebase, the corresponding property is namedmailParticipants(plural). This will lead to runtime errors.Apply this diff:
- mailParticipant?: MailParticipant | null + mailParticipants?: MailParticipant | nullNote: You'll also need to update all references to this property throughout this file (lines 289, 334, 344, 444).
266-269: Do not freeze user metadata at module load.
userMetadatais now a module-level constant with a hard-codedAsia/Kolkatatimezone and adateForAIsnapshot taken at process start. That makes every response use the wrong timezone for most users and the date quickly drifts out of sync. Please derive this per request (e.g., parseusrCtx) and stop hard-coding the timezone.You should:
- Remove the module-level
userMetadataconstant- Extract user timezone from request context in each tool execution
- Compute
dateForAIper-request using the extracted timezone- Provide a sensible fallback (e.g., UTC) if the user timezone is missing
🧹 Nitpick comments (1)
server/api/chat/tools.ts (1)
1012-1060: Consider making thread-root detection more explicit.The automatic thread message fetching is a good enhancement. However, the thread-root detection at lines 1020-1022 relies on comparing
String(createdAtNum) === threadIdStr, which assumes the threadId format matches the timestamp format. This could be fragile if Slack's API or the data model changes.Consider adding a comment explaining this assumption or using a more explicit field/flag if available in the schema to identify thread roots, rather than relying on string equality of timestamps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/chat/mapper.ts(10 hunks)server/api/chat/tools.ts(23 hunks)server/api/chat/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api/chat/utils.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T16:18:07.128Z
Learnt from: Aayushjshah
PR: xynehq/xyne#787
File: server/api/chat/mapper.ts:631-636
Timestamp: 2025-09-08T16:18:07.128Z
Learning: In server/api/chat/mapper.ts, the filter_query parameter for getSlackThreads should remain required: false (optional) in the mapper definition, even if it differs from the AgentTool definition in server/api/chat/tools.ts. This is an intentional design decision where parameter requirements can differ between the mapper and tool layers.
Applied to files:
server/api/chat/tools.ts
🧬 Code graph analysis (1)
server/api/chat/tools.ts (6)
server/api/chat/types.ts (2)
MinimalAgentFragment(88-94)AgentTool(110-134)server/api/chat/utils.ts (3)
searchToCitation(441-539)getChannelIdsFromAgentPrompt(152-170)expandEmailThreadsInResults(249-273)server/ai/context.ts (1)
answerContextMap(741-809)server/api/chat/mapper.ts (2)
searchGlobalTool(979-1020)convertToAgentToolParameters(850-868)server/utils.ts (1)
getErrorMessage(103-106)server/search/utils.ts (1)
extractDriveIds(74-117)
🔇 Additional comments (1)
server/api/chat/tools.ts (1)
1761-2087: LGTM: Google tools implementations are well-structured.The four Google tools (searchGmail, searchDriveFiles, searchCalendarEvents, searchGoogleContacts) follow a consistent and robust pattern:
- Proper input validation and type checking
- Agent permission verification
- Appropriate error handling and logging
- Consistent response formatting via
formatSearchToolResponse- Gmail tool includes thread expansion for better context
- Drive tool handles folder expansion from selectedItems
Note: The tools currently use the module-level
userMetadataconstant which has the hardcoded timezone issue flagged separately (lines 266-269). Once that's fixed, these tools will inherit the per-request timezone handling automatically.
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 (4)
server/search/vespa.ts (1)
134-155: Fix getItems wrapper: param typing and payload shape mismatches
- selectedItem is read but not declared in the param type.
- You send processedCollectionSelections, but GetItemsParams expects collectionSelections. This triggers excess property errors.
Apply this diff:
-export const getItems = async ( - params: Omit<GetItemsParams, "processedCollectionSelections"> & { - collectionSelections?: Array<{ - collectionIds?: string[] - collectionFolderIds?: string[] - collectionFileIds?: string[] - }> - }, -) => { +export const getItems = async ( + params: Omit<GetItemsParams, "collectionSelections"> & { + collectionSelections?: Array<{ + collectionIds?: string[] + collectionFolderIds?: string[] + collectionFileIds?: string[] + }> + // used to derive driveIds + selectedItem?: Partial<Record<Apps, string[]>> + }, +) => { const driveIds = await extractDriveIds( { selectedItem: params.selectedItem }, params.email, ) const processedCollectionSelections = await extractCollectionVespaIds({ collectionSelections: params.collectionSelections, }) - return await vespa.getItems.bind(vespa)({ - processedCollectionSelections, - driveIds, - ...params, - }) + return await vespa.getItems.bind(vespa)({ + ...params, + driveIds, + // map processed -> expected input + collectionSelections: processedCollectionSelections, + }) }server/api/chat/chat.ts (2)
3403-3419: getItems param 'mailParticipants' is not part of the wrapper typeThe wrapper currently accepts intent (from GetItemsParams), not mailParticipants. Passing mailParticipants breaks excess-property checks.
Apply this diff to align with current types (follow-up PR can add mailParticipants to the contract):
- mailParticipants: resolvedMailParticipants || {}, + intent: resolvedMailParticipants || {},
224-231: Tighten types in resolveNamesToEmails and fix default value
- Return type should be MailParticipant, not any.
- resolvedData should default to {} (object), not [] (array), to match MailParticipant.
Apply this diff:
-export async function resolveNamesToEmails( +export async function resolveNamesToEmails( intent: MailParticipant, email: string, userCtx: string, userMetadata: UserMetadataType, span?: Span, -): Promise<any> { +): Promise<MailParticipant> { @@ - let resolvedData: MailParticipant = {} + let resolvedData: MailParticipant = {} @@ - resolvedData = resolutionResult.emails || [] + resolvedData = resolutionResult.emails || {}Also applies to: 315-324
server/ai/types.ts (1)
384-405: Fix leftoverintentreferences
Replace the obsoleteintent: parsed?.filters?.intent || {}at server/api/chat/chat.ts line 6491 withmailParticipants: parsed?.filters?.mailParticipants || {}.
♻️ Duplicate comments (4)
server/api/chat/chat.ts (2)
1981-1991: Undefined SearchModes.AttachmentRank and limit mismatch
- SearchModes.AttachmentRank is not in the enum → compile error.
- Limit should use nonCollectionFileIds.length per earlier note to match the searched set.
Apply this diff:
- results = await searchVespaInFiles( + results = await searchVespaInFiles( builtUserQuery, email, nonCollectionFileIds, { - limit: fileIds?.length, - alpha: userAlpha, - rankProfile: SearchModes.AttachmentRank, + limit: nonCollectionFileIds.length, + alpha: userAlpha, + // Use a valid rank profile; fall back to NativeRank + rankProfile: SearchModes.NativeRank, }, )
3427-3427: Log label: use 'Mail Participants' instead of 'Intent'The message logs mailParticipants but labels it as Intent. Update for clarity/consistency.
- `[GetItems] Calling getItems - Schema: ${schema}, App: ${apps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, Intent: ${JSON.stringify(classification.filters.mailParticipants)}`, + `[GetItems] Calling getItems - Schema: ${schema}, App: ${apps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, Mail Participants: ${JSON.stringify(classification.filters.mailParticipants)}`,server/api/chat/tools.ts (2)
268-271: Critical: Remove module-level userMetadata with hardcoded timezone.This module-level constant causes every request to use the hardcoded
Asia/Kolkatatimezone instead of the actual user's timezone, and thedateForAIbecomes stale as it's computed only once at module load.Per previous reviews, this needs to be computed per-request. Extract the user's timezone from the request context (e.g.,
usrCtxparameter) and computeuserMetadatawithin each tool's execute function, with a fallback to UTC if unavailable.
256-256: Fix naming inconsistency: mailParticipant vs mailParticipants.The property is named
mailParticipant(singular) here, but throughout the rest of the file it's referenced asmailParticipants(plural) - see lines 346, 446. This inconsistency combined with the pipeline failure indicatingmailParticipantsdoesn't exist inVespaQueryConfigsuggests a type mismatch.Please verify the correct property name from the
VespaQueryConfigtype definition and use it consistently throughout this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/ai/types.ts(2 hunks)server/api/chat/agents.ts(3 hunks)server/api/chat/chat.ts(14 hunks)server/api/chat/tools.ts(25 hunks)server/api/chat/utils.ts(2 hunks)server/search/vespa.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api/chat/agents.ts
- server/api/chat/utils.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T16:18:07.128Z
Learnt from: Aayushjshah
PR: xynehq/xyne#787
File: server/api/chat/mapper.ts:631-636
Timestamp: 2025-09-08T16:18:07.128Z
Learning: In server/api/chat/mapper.ts, the filter_query parameter for getSlackThreads should remain required: false (optional) in the mapper definition, even if it differs from the AgentTool definition in server/api/chat/tools.ts. This is an intentional design decision where parameter requirements can differ between the mapper and tool layers.
Applied to files:
server/api/chat/tools.ts
🧬 Code graph analysis (2)
server/api/chat/chat.ts (1)
server/ai/types.ts (1)
MailParticipant(393-393)
server/api/chat/tools.ts (7)
server/api/chat/types.ts (2)
MinimalAgentFragment(88-94)AgentTool(110-134)server/api/chat/utils.ts (2)
searchToCitation(442-540)expandEmailThreadsInResults(250-274)server/ai/context.ts (1)
answerContextMap(887-985)server/api/chat/mapper.ts (3)
searchGlobalTool(979-1020)convertToAgentToolParameters(850-868)googleTools(1022-1216)server/utils.ts (1)
getErrorMessage(103-106)server/search/vespa.ts (2)
SearchVespaThreads(159-159)searchGoogleApps(132-132)server/search/utils.ts (1)
extractDriveIds(95-138)
🪛 GitHub Actions: TypeScript Build Check
server/search/vespa.ts
[error] 132-132: Property 'searchGoogleApps' does not exist on type 'VespaService'.
[error] 144-144: Property 'selectedItem' does not exist on type 'Omit<GetItemsParams, "processedCollectionSelections"> & { collectionSelections?: { collectionIds?: string[] | undefined; collectionFolderIds?: string[] | undefined; collectionFileIds?: string[] | undefined; }[] | undefined; }'.
[error] 151-151: Object literal may only specify known properties, and 'processedCollectionSelections' does not exist in type 'GetItemsParams'.
server/api/chat/chat.ts
[error] 1989-1989: Property 'AttachmentRank' does not exist on type 'typeof SearchModes'.
[error] 3415-3415: Object literal may only specify known properties, and 'mailParticipants' does not exist in type 'Omit<GetItemsParams, "processedCollectionSelections"> & { collectionSelections?: { collectionIds?: string[] | undefined; collectionFolderIds?: string[] | undefined; collectionFileIds?: string[] | undefined; }[] | undefined; }'.
server/api/chat/tools.ts
[error] 19-19: Module '@xyne/vespa-ts/types' has no exported member 'GoogleApps'.
[error] 77-77: Module '@xyne/vespa-ts' has no exported member 'EventStatusType'.
[error] 79-79: Module '@xyne/vespa-ts' has no exported member 'MailParticipant'.
[error] 346-346: Object literal may only specify known properties, and 'mailParticipants' does not exist in type 'Partial'.
[error] 446-446: Object literal may only specify known properties, and 'mailParticipants' does not exist in type 'Omit<GetItemsParams, "processedCollectionSelections"> & { collectionSelections?: { collectionIds?: string[] | undefined; collectionFolderIds?: string[] | undefined; collectionFileIds?: string[] | undefined; }[] | undefined; }'.
🪛 GitHub Check: test
server/search/vespa.ts
[failure] 132-132: TypeError: undefined is not an object (evaluating 'vespa.searchGoogleApps.bind')
at /home/runner/work/xyne/xyne/server/search/vespa.ts:132:39
at loadAndEvaluateModule (2:1)
🔇 Additional comments (5)
server/api/chat/tools.ts (5)
88-153: LGTM with caveat on userMetadata.The
formatSearchToolResponsefunction provides good abstraction for formatting search results into a consistent response structure. The logic for building summary text and fragments is clear and handles various search contexts well.Note: This function uses the module-level
userMetadataconstant at line 117, which has timezone issues (addressed in a separate comment).
363-369: Good validation logic for multi-app support.The validation logic properly handles both single apps and arrays of apps, with clear error messages that indicate which apps are not allowed. The string formatting for singular/plural cases is well done.
Also applies to: 423-430
527-586: LGTM: searchGlobal refactored to use mapper definitions.The tool has been properly refactored to use
searchGlobalTooldefinition andconvertToAgentToolParametersfrom the mapper, providing better consistency. The fallback chainparams.query || userMessage || ""is reasonable.
1021-1097: Excellent: Automatic thread message fetching improves UX.The automatic fetching of thread messages when thread roots are found is a great user experience improvement. The implementation correctly identifies thread roots by comparing
createdAtwiththreadId, fetches the thread messages, and merges them with the original results. Error handling for failed thread fetches is also well done.
1777-2101: LGTM: Consistent Google tools implementation.The four new Google tools (
searchGmail,searchDriveFiles,searchCalendarEvents,searchGoogleContacts) follow a consistent pattern with:
- Proper agent permission checks
- Correct parameter handling and time range conversions
- Use of the
formatSearchToolResponseutility- Good error handling
The implementation is clean and maintainable. Note that these tools depend on the
GoogleAppsimport which is currently failing (addressed in a separate comment).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
server/ai/agentPrompts.ts (1)
203-221: Fix inconsistent JSON keys and typo (“goo”) in instructions.
- Use a single convention: rewrittenQueries (not rewritten_queries) and dateRange (not date_range).
- Typo: “goo for better query rewrites” → “go for better query rewrites”.
These inconsistencies can cause parse failures and non‑compliant outputs.
Apply this minimal edit inside the template:
- - Do not answer if you do not have valid context and goo for better query rewrites + - Do not answer if you do not have valid context and go for better query rewrites -3. **Mutual Exclusivity:** Only one of "answer" or "rewritten_queries" should be present. +3. **Mutual Exclusivity:** Only one of "answer" or "rewrittenQueries" should be present. - - If an answer is not provided, set "answer" to null and provide "rewritten_queries" along with the "date_range". + - If an answer is not provided, set "answer" to null and provide "rewrittenQueries" along with the "dateRange".server/ai/prompts.ts (1)
1950-1966: Remove stray merge separator (‘=======’) from prompt.A leftover merge divider is present inside the temporalDirectionJsonPrompt template. This will be rendered verbatim to the model and can confuse output formatting.
Apply this edit:
-======= If NO relevant items are found in Retrieved Context or context doesn't match query: { "answer": null }server/api/chat/chat.ts (2)
1987-1989: Fix missingSearchModes.AttachmentRankenum member.The TypeScript build is failing because
SearchModeshas noAttachmentRankmember. Either add this enum entry (and corresponding server support) or use one of the existing rank profiles. Until this is resolved the file won’t compile. Refer to the CI error at Line 1989.
3404-3441:getItemsdoes not acceptmailParticipants.
GetItemsParamscurrently rejects amailParticipantsfield, so these new arguments make TypeScript fail (see CI error around Line 3415). Please extend the type definition (and handler) to accept this filter or drop the property here; otherwise the build will keep breaking.
♻️ Duplicate comments (5)
server/ai/provider/index.ts (2)
857-859: Use nullish coalescing assignment for brevity.Replace the explicit undefined check with
??=for clarity.- if (assistantResponse === undefined) { - assistantResponse = "" - } + assistantResponse ??= ""
1603-1603: Avoid hardcoded timezone in getDateForAI.Use the user’s timezone (from user metadata or a parameter) instead of "Asia/Kolkata".
Apply this change and add an optional function parameter for timezone:
- const dateForAI = getDateForAI({ userTimeZone: "Asia/Kolkata" }) + const dateForAI = getDateForAI({ + userTimeZone: + (params as any)?.userTimezone || + Intl.DateTimeFormat().resolvedOptions().timeZone, + })And (outside this line) consider adding an optional
userTimezone?: stringtoModelParamsor thread it explicitly into this function to avoidany.server/api/chat/tools.ts (3)
740-748: Replace console.log statements with structured logging.Multiple
console.logstatements remain in production code. These should use theLoggerinstance for proper structured logging.Apply this pattern throughout:
- console.log( - "[Retrieve Slack Thread] Common Vespa searchOptions:", - JSON.stringify(searchOptionsVespa, null, 2), - ) - - const searchQuery = params.filter_query - console.log( - `[get_slack_messages_from_user] Using searchVespa with filter_query: '${searchQuery}'`, - ) + Logger.debug({ searchOptionsVespa }, "[Retrieve Slack Thread] Common Vespa searchOptions") + Logger.debug({ filterQuery: params.filter_query }, "[get_slack_messages_from_user] Using searchVespa with filter_query")Also applies to:
- Line 1379-1381:
[get_slack_messages_from_channel] Using filter_query- Line 1588-1590:
[getSlackMessagesFromTimeRange] Using filter_queryAlso applies to: 1379-1381, 1588-1590
266-269: Critical: Do not freeze user metadata at module load.This is a duplicate of a previous critical issue.
userMetadatais a module-level constant with a hard-codedAsia/Kolkatatimezone and adateForAIsnapshot taken at process start. This causes every response to use the wrong timezone for most users and the date quickly becomes stale.Remove the module-level constant and derive user metadata per request:
-const userMetadata: UserMetadataType = { - userTimezone: "Asia/Kolkata", - dateForAI: getDateForAI({ userTimeZone: "Asia/Kolkata" }), -}Then update all usages to compute userMetadata dynamically. For example, in
formatSearchToolResponse:async function formatSearchToolResponse( searchResults: VespaSearchResponse | null, searchContext: { query?: string app?: string labels?: string[] timeRange?: { startTime: number; endTime: number } offset?: number limit?: number searchType?: string }, userTimezone: string = "UTC", // Add parameter with fallback ): Promise<{ result: string; contexts: MinimalAgentFragment[] }> { const userMetadata: UserMetadataType = { userTimezone, dateForAI: getDateForAI({ userTimeZone: userTimezone }), } // ... rest of function }Repeat this pattern for all functions that use
userMetadata(lines 117, 481, 495, 785, 1077, 1420, 1629).
254-254: Critical: Fix property name causing pipeline failures.Pipeline failures indicate that
mailParticipants(plural) does not exist inVespaQueryConfig(line 344) orGetItemsParams(line 444). However, line 254 defines the parameter asmailParticipant(singular). This inconsistency is causing TypeScript build failures.Based on the pipeline errors and the parameter definition, use the singular form consistently:
interface UnifiedSearchOptions { // ... - mailParticipant?: MailParticipant | null + mailParticipants?: MailParticipant | null // ... }Then update the usages:
const commonSearchOptions: Partial<VespaQueryConfig> = { // ... - mailParticipants: mailParticipant || null, + mailParticipant: options.mailParticipant || null, // ... }searchResults = await getItems({ // ... - mailParticipants: options.mailParticipant || null, + mailParticipant: options.mailParticipant || null, })Note: Verify the correct property name in the type definitions. If the interface uses
mailParticipants(plural), then update line 254 to match. If it usesmailParticipant(singular), then update lines 344 and 444 to match.Also applies to: 344-344, 444-444
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/ai/agentPrompts.ts(3 hunks)server/ai/prompts.ts(5 hunks)server/ai/provider/index.ts(9 hunks)server/api/chat/chat.ts(25 hunks)server/api/chat/tools.ts(25 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T16:18:07.128Z
Learnt from: Aayushjshah
PR: xynehq/xyne#787
File: server/api/chat/mapper.ts:631-636
Timestamp: 2025-09-08T16:18:07.128Z
Learning: In server/api/chat/mapper.ts, the filter_query parameter for getSlackThreads should remain required: false (optional) in the mapper definition, even if it differs from the AgentTool definition in server/api/chat/tools.ts. This is an intentional design decision where parameter requirements can differ between the mapper and tool layers.
Applied to files:
server/api/chat/tools.ts
🧬 Code graph analysis (3)
server/ai/provider/index.ts (3)
server/ai/prompts.ts (1)
temporalDirectionJsonPrompt(1637-1966)server/utils/index.ts (1)
getDateForAI(3-29)server/ai/types.ts (2)
MailParticipant(393-393)ModelParams(284-309)
server/api/chat/tools.ts (8)
server/api/chat/types.ts (2)
MinimalAgentFragment(88-94)AgentTool(110-134)server/api/chat/utils.ts (1)
searchToCitation(442-540)server/ai/context.ts (1)
answerContextMap(887-985)server/utils/index.ts (1)
getDateForAI(3-29)server/api/chat/mapper.ts (3)
searchGlobalTool(979-1020)convertToAgentToolParameters(850-868)googleTools(1022-1216)server/utils.ts (1)
getErrorMessage(103-106)server/search/vespa.ts (2)
SearchVespaThreads(159-159)searchGoogleApps(132-132)server/search/utils.ts (1)
extractDriveIds(95-138)
server/api/chat/chat.ts (2)
server/ai/types.ts (1)
MailParticipant(393-393)server/api/chat/utils.ts (1)
extractNamesFromIntent(1039-1055)
🪛 GitHub Actions: TypeScript Build Check
server/api/chat/tools.ts
[error] 19-19: Module '"@xyne/vespa-ts/types"' has no exported member 'GoogleApps'.
[error] 77-77: Module '"@xyne/vespa-ts"' has no exported member 'EventStatusType'.
[error] 79-79: Module '"@xyne/vespa-ts"' has no exported member 'MailParticipant'.
[error] 344-344: Object literal may only specify known properties, and 'mailParticipants' does not exist in type 'Partial'.
[error] 444-444: Object literal may only specify known properties, and 'mailParticipants' does not exist in type 'Omit<GetItemsParams, "processedCollectionSelections"> & { collectionSelections?: { collectionIds?: string[] | undefined; collectionFolderIds?: string[] | undefined; collectionFileIds?: string[] | undefined; }[] | undefined; }'.
server/api/chat/chat.ts
[error] 1989-1989: Property 'AttachmentRank' does not exist on type 'typeof SearchModes'.
[error] 3415-3415: Object literal may only specify known properties, and 'mailParticipants' does not exist in type 'Omit<GetItemsParams, "processedCollectionSelections"> & { collectionSelections?: { collectionIds?: string[] | undefined; collectionFolderIds?: string[] | undefined; collectionFileIds?: string[] | undefined; }[] | undefined; }'.
🔇 Additional comments (11)
server/ai/agentPrompts.ts (2)
1211-1248: mailParticipants extraction rules look good; confirm downstream alignment.The switch from intent → mailParticipants is clear and covers names/emails/orgs with sensible defaults.
Please confirm:
- Query parser/types accept mailParticipants in the output.
- Name-to-email resolution pipeline consumes these fields correctly. Based on learnings.
1421-1430: New filters.mailParticipants field in output schema.Schema addition is consistent with the extraction rules; ensure the router and validators (zod/types) include this field to avoid drops at runtime.
server/ai/prompts.ts (2)
1364-1414: mailParticipants extraction block is clear; keep naming consistent with other prompts.Looks consistent with agentPrompts.ts. Ensure all references uniformly use mailParticipants across prompts and routers.
1432-1442: filters.mailParticipants included in schema.Good addition. Verify that any schema validators (zod) and consumers accept this field to prevent silent drops.
server/ai/provider/index.ts (1)
1879-1901: Name-to-email resolution wiring looks correct.The participants string and passing
namesintonameToEmailResolutionPrompt(as intent) match the updated prompts signature; extraction result parsing retains field defaults.Ensure the prompt returns the expected JSON shape:
{ "emails": { from:[], to:[], cc:[], bcc:[] } }.server/api/chat/tools.ts (6)
1871-1960: LGTM! Drive files search implementation is well-structured.The
searchDriveFilestool correctly:
- Validates email and agent permissions
- Extracts Drive IDs from selected items for scoped searches
- Handles file type filtering
- Converts time ranges properly
- Uses the standardized
formatSearchToolResponsehelper
1962-2046: LGTM! Calendar events search implementation is correct.The
searchCalendarEventstool properly:
- Validates email and agent permissions
- Handles attendees filtering
- Converts event status parameters
- Processes time ranges correctly
- Uses consistent formatting
1019-1067: LGTM! Automatic thread fetching is a good UX improvement.The enhancement to automatically fetch thread messages when parent messages are found is well-implemented:
- Correctly identifies thread roots by comparing
createdAtwiththreadId- Handles errors gracefully without failing the entire request
- Logs useful debugging information
- Clearly indicates to users that thread messages are included
One minor suggestion: Consider adding a maximum limit on thread fetching to prevent performance issues if many thread roots are found.
// Fetch thread messages if any thread roots were found (limit to reasonable number) if (threadIdsToFetch.length > 0) { const maxThreadsToFetch = 10 // Prevent fetching too many threads const threadsToFetch = threadIdsToFetch.slice(0, maxThreadsToFetch) if (threadIdsToFetch.length > maxThreadsToFetch) { Logger.debug( `[get_slack_messages] Limiting thread fetch to ${maxThreadsToFetch} of ${threadIdsToFetch.length} thread roots`, ) } Logger.debug( `[get_slack_messages] Fetching threads for ${threadsToFetch.length} thread roots`, ) // ... rest of fetching logic }
193-209: LGTM! Improved collection ID parsing logic.The updated logic for parsing collection IDs, folder IDs, and file IDs with proper prefix handling is clearer and more maintainable than before. The regex pattern replacement approach is robust.
525-584: LGTM! searchGlobal tool properly refactored.The renamed
searchGlobaltool correctly:
- Uses
searchGlobalTooldefinition from mapper- Converts parameters using
convertToAgentToolParameters- Parses agent app integrations
- Delegates to
executeVespaSearchhelper- Handles errors appropriately
1774-1869: No thread expansion gap or duplication detected The unified helper (tools.ts 456–463) and thesearchGmailtool (tools.ts 1841–1850) both expand email threads appropriately, withsearchGmailbypassing the helper so no duplicate expansion occurs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/api/chat/agents.ts (1)
4335-4373: Refactor to eliminate variable shadowing and ensure consistent classification structure.The code declares
classificationtwice with the same name in nested scopes:
- Line 4335: Outer
let classificationwithisFollowUpfield- Line 4362: Inner
const classificationwithoutisFollowUpfieldThis shadowing creates several issues:
- Confusing control flow: It's unclear which
classificationis used in different code paths- Inconsistent structure: The inner declaration omits
isFollowUp, which could cause issues if downstream code expects this field- Maintenance risk: Future developers may not realize the inner declaration shadows the outer one
Recommended approach:
-let classification: TemporalClassifier & QueryRouterResponse = { - direction: parsed.temporalDirection, - type: parsed.type as QueryType, - filterQuery: parsed.filter_query, - isFollowUp: parsed.isFollowUp, - filters: { - ...(parsed?.filters ?? {}), - apps: parsed.filters?.apps || [], - entities: parsed.filters?.entities as any, - mailParticipants: parsed.mailParticipants || {}, - }, -} if (parsed.answer === null || parsed.answer === "") { const ragSpan = streamSpan.startSpan("rag_processing") // ... query rewrite logic ... - const classification: TemporalClassifier & QueryRouterResponse = { + // Reuse outer classification with consistent structure + classification = { direction: parsed.temporalDirection, type: parsed.type as QueryType, filterQuery: parsed.filter_query, + isFollowUp: parsed.isFollowUp, // Add missing field for consistency filters: { ...(parsed?.filters ?? {}), apps: parsed.filters?.apps || [], entities: parsed.filters?.entities as any, mailParticipants: parsed.mailParticipants || {}, }, }Alternatively, use distinct variable names like
conversationClassificationandragClassificationif they serve different purposes.server/api/chat/chat.ts (1)
315-327: Fix double span end and incorrect fallback type in resolveNamesToEmails
- searchSpan is ended earlier (Line 287). Ending it again is incorrect.
- Fallback should be an empty object, not an array, to match MailParticipant.
- let resolvedData: MailParticipant = {} + let resolvedData: MailParticipant = {} const resolutionResult = await extractEmailsFromContext( extractedNames, userCtx, searchContext, { modelId: config.defaultFastModel, json: false, stream: false }, ) - resolvedData = resolutionResult.emails || [] + resolvedData = resolutionResult.emails || {} - searchSpan?.end() return resolvedData
♻️ Duplicate comments (2)
server/api/chat/chat.ts (2)
1975-2003: Correct fileId grouping and fix limit sizing for file searches
- Attachments with prefix att_ are incorrectly grouped under collectionFileIds; they should be searched via searchVespaInFiles with AttachmentRank.
- Limit should match the actual array being searched to avoid under/over-fetching. This was also flagged previously.
- const collectionFileIds = fileIds.filter((fid) => fid.startsWith("clf-") || fid.startsWith("att_")) - const nonCollectionFileIds = fileIds.filter((fid) => !fid.startsWith("clf-") && !fid.startsWith("att")) - const attachmentFileIds = fileIds.filter((fid) => fid.startsWith("attf_")) + const collectionFileIds = fileIds.filter((fid) => fid.startsWith("clf-")) + const attachmentFileIds = fileIds.filter( + (fid) => fid.startsWith("att_") || fid.startsWith("attf_"), + ) + const nonCollectionFileIds = fileIds.filter( + (fid) => + !fid.startsWith("clf-") && + !fid.startsWith("att_") && + !fid.startsWith("attf_"), + ) if(nonCollectionFileIds && nonCollectionFileIds.length > 0) { results = await searchVespaInFiles(builtUserQuery, email, nonCollectionFileIds, { - limit: fileIds?.length, + limit: nonCollectionFileIds.length, alpha: userAlpha, }) if (results.root.children) { combinedSearchResponse.push(...results.root.children) } } @@ - if(attachmentFileIds && attachmentFileIds.length > 0) { + if (attachmentFileIds && attachmentFileIds.length > 0) { results = await searchVespaInFiles(builtUserQuery, email, attachmentFileIds, { - limit: fileIds?.length, + limit: attachmentFileIds.length, alpha: userAlpha, rankProfile: SearchModes.AttachmentRank, }) if (results.root.children) { combinedSearchResponse.push(...results.root.children) } }
3404-3441: Unify log key and log the resolved mailParticipantsTo match the refactor and improve clarity, prefer “Mail Participants” label and log the resolved value. This was requested earlier.
- `[GetItems] Calling getItems with agent prompt - Schema: ${schema}, App: ${agentApps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, mailParticipants: ${JSON.stringify(classification.filters.mailParticipants)}`, + `[GetItems] Calling getItems with agent prompt - Schema: ${schema}, App: ${agentApps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, Mail Participants: ${JSON.stringify(resolvedMailParticipants)}`, @@ - mailParticipants: resolvedMailParticipants || {}, + mailParticipants: resolvedMailParticipants || {},And for the non-agent path:
- `[GetItems] Calling getItems - Schema: ${schema}, App: ${apps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, mailParticipants: ${JSON.stringify(classification.filters.mailParticipants)}`, + `[GetItems] Calling getItems - Schema: ${schema}, App: ${apps?.map((a) => a).join(", ")}, Entity: ${entities?.map((e) => e).join(", ")}, Mail Participants: ${JSON.stringify(resolvedMailParticipants)}`, @@ - mailParticipants: resolvedMailParticipants || {}, + mailParticipants: resolvedMailParticipants || {},
🧹 Nitpick comments (1)
server/api/chat/chat.ts (1)
224-231: Return a precise type for resolveNamesToEmailsThe function always returns a MailParticipant-shaped object. Tighten the return type for safety and clarity.
-export async function resolveNamesToEmails( +export async function resolveNamesToEmails( mailParticipants: MailParticipant, email: string, userCtx: string, userMetadata: UserMetadataType, span?: Span, -): Promise<any> { +): Promise<MailParticipant> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/api/chat/agents.ts(11 hunks)server/api/chat/chat.ts(25 hunks)server/api/chat/utils.ts(2 hunks)server/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api/chat/utils.ts
- server/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
server/api/chat/agents.ts (4)
server/shared/types.ts (1)
AttachmentEntity(39-39)server/ai/types.ts (2)
TemporalClassifier(279-282)QueryRouterResponse(461-461)server/api/chat/utils.ts (1)
collectFollowupContext(81-153)server/api/chat/chat.ts (1)
UnderstandMessageAndAnswer(3788-3923)
server/api/chat/chat.ts (2)
server/ai/types.ts (1)
MailParticipant(393-393)server/api/chat/utils.ts (1)
extractNamesFromIntent(1046-1062)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
server/api/chat/agents.ts (3)
395-401: LGTM: Attachment entity filtering.The logic correctly filters out attachment entities from citations by checking against the
AttachmentEntityenum values.
1629-1665: LGTM: Async transformations of Vespa results.The code correctly transforms
VespaSearchResultarrays intoMinimalAgentFragmentarrays usingPromise.allwith async map operations. The same pattern is properly applied forchatContexts,threadContexts, and other result sets throughout the file.
4176-4186: LGTM: Consistent mailParticipants initialization.The rename from
intenttomailParticipantsis consistently applied in bothqueryFiltersandparsedobject initializations, with appropriate empty object defaults.server/api/chat/chat.ts (1)
2976-2983: mailParticipants destructure looks goodNew filter field is plumbed correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/chat/chat.ts (1)
224-231: Fix return type and assignment in resolveNamesToEmailsReturn type should be MailParticipant, and fallback must not be an array. Current
resolvedData = resolutionResult.emails || []risks type/shape breakage.Apply this diff:
-export async function resolveNamesToEmails( +export async function resolveNamesToEmails( mailParticipants: MailParticipant, email: string, userCtx: string, userMetadata: UserMetadataType, span?: Span, -): Promise<any> { +): Promise<MailParticipant> { @@ - let resolvedData: MailParticipant = {} + let resolvedData: MailParticipant = {} const resolutionResult = await extractEmailsFromContext( extractedNames, userCtx, searchContext, { modelId: config.defaultFastModel, json: false, stream: false }, ) - - resolvedData = resolutionResult.emails || [] + resolvedData = + (resolutionResult?.emails as MailParticipant) || extractedNamesAlso applies to: 314-325
♻️ Duplicate comments (1)
server/api/chat/chat.ts (1)
1974-1991: Use correct limits for per‑set file searches (duplicate of prior feedback)The limit should match the array being searched, not the total fileIds. Use nonCollectionFileIds.length and attachmentFileIds.length respectively.
Apply this diff:
- if (nonCollectionFileIds && nonCollectionFileIds.length > 0) { + if (nonCollectionFileIds && nonCollectionFileIds.length > 0) { results = await searchVespaInFiles( builtUserQuery, email, nonCollectionFileIds, { - limit: fileIds?.length, + limit: nonCollectionFileIds.length, alpha: userAlpha, }, ) } @@ - if (attachmentFileIds && attachmentFileIds.length > 0) { + if (attachmentFileIds && attachmentFileIds.length > 0) { results = await searchVespaInFiles( builtUserQuery, email, attachmentFileIds, { - limit: fileIds?.length, + limit: attachmentFileIds.length, alpha: userAlpha, rankProfile: SearchModes.AttachmentRank, }, ) }Also applies to: 2006-2015
🧹 Nitpick comments (3)
server/api/chat/utils.ts (2)
40-41: Unify MailParticipant type source across filesThis file imports MailParticipant from @xyne/vespa-ts/types while chat.ts imports it from "@/ai/types". Pick one canonical source (prefer "@/ai/types" if that’s the project’s schema owner) to avoid subtle structural drift.
1050-1066: Minor: rename parameter to reflect new semantics; keep type-safe initializationRename intent to mailParticipants (or obj) for clarity post‑refactor. If MailParticipant has all-optional keys this is fine; otherwise initialize as Partial and cast on return.
Example:
-export function extractNamesFromIntent(intent: any): MailParticipant { - if (!intent || typeof intent !== "object") return {} +export function extractNamesFromIntent(mailParticipants: any): MailParticipant { + if (!mailParticipants || typeof mailParticipants !== "object") return {} - const result: MailParticipant = {} + const result: Partial<MailParticipant> = {} const fieldsToCheck = ["from", "to", "cc", "bcc"] as const for (const field of fieldsToCheck) { - if (Array.isArray(intent[field]) && intent[field].length > 0) { - const uniqueValues = [...new Set(intent[field])].filter((v) => v) + if (Array.isArray(mailParticipants[field]) && mailParticipants[field].length > 0) { + const uniqueValues = [...new Set(mailParticipants[field])].filter((v) => v) if (uniqueValues.length > 0) { result[field] = uniqueValues } } } - return result + return result as MailParticipantserver/api/chat/chat.ts (1)
17-27: Type source consistency for MailParticipantThis file imports MailParticipant from "@/ai/types" while utils.ts uses "@xyne/vespa-ts/types". Use a single canonical source to prevent drift and confusing structural typing.
Would you standardize on "@/ai/types" (MailParticipantSchema) across server/api?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/api/chat/chat.ts(28 hunks)server/api/chat/utils.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/api/chat/utils.ts (1)
server/ai/types.ts (1)
MailParticipant(393-393)
server/api/chat/chat.ts (3)
server/ai/types.ts (1)
MailParticipant(393-393)server/api/chat/utils.ts (2)
extractNamesFromIntent(1050-1066)collectFollowupContext(81-157)server/search/vespa.ts (1)
searchVespaInFiles(118-118)
## [3.14.2](v3.14.1...v3.14.2) (2025-10-14) ### Bug Fixes * **tools:** Revamp agentic tools ([#1067](#1067)) ([3ac0b9e](3ac0b9e))
2 participants
Description
Testing
Additional Notes
Summary by CodeRabbit
New Features
Refactor
Chores