Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Refactor message types to use unified schema #456

Merged
merged 3 commits into from
Jan 2, 2025

Conversation

nadeesha
Copy link
Contributor

@nadeesha nadeesha commented Jan 2, 2025

PR Type

Enhancement, Bug fix, Tests


Description

  • Unified message schema introduced for consistent message handling.

  • Refactored message validation and parsing logic for robustness.

  • Updated tests to align with the new schema and validation.

  • Removed redundant logging and improved code formatting.


Changes walkthrough 📝

Relevant files
Enhancement
8 files
contract.ts
Introduced unified message schema for message handling.   
+37/-32 
data.ts
Updated message data type to use unified schema.                 
+1/-2     
index.ts
Integrated unified schema in email message handling.         
+42/-39 
index.ts
Applied unified schema in Slack integration workflows.     
+100/-89
router.ts
Adjusted message assertion to use unified schema.               
+2/-2     
tool-call.ts
Refactored tool-call logic to use unified schema.               
+25/-40 
workflow-messages.ts
Centralized message validation and parsing with unified schema.
+71/-107
workflows.ts
Updated workflow detail logic for unified schema.               
+1/-1     
Tests
3 files
agent.ai.test.ts
Updated agent tests to validate against unified schema.   
+44/-78 
model-call.test.ts
Refactored model-call tests for schema validation.             
+39/-46 
tool-call.test.ts
Enhanced tool-call tests with unified schema validation. 
+25/-34 
Bug fix
2 files
router.ts
Adjusted workflow router to handle null results.                 
+4/-2     
cache.ts
Removed redundant logging in cache utility.                           
+0/-2     

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

@nadeesha nadeesha marked this pull request as ready for review January 2, 2025 21:33
Copy link
Contributor

qodo-merge-pro bot commented Jan 2, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type Safety

The refactoring of message types introduces new type assertions and validations. Need to verify that all message type checks and conversions are handled correctly to prevent runtime type errors.

Error Handling

Email handling logic has been modified with new message type validations. Need to verify error cases are properly handled and that email responses are correctly formatted.

Integration Changes

Slack integration changes affect message handling and approvals. Need to verify that all Slack interactions continue to work correctly with the new message schema.

Copy link
Contributor

qodo-merge-pro bot commented Jan 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Remove optional chaining operator to prevent silent failures when calling critical API methods

The client?.chat.postMessage() call uses optional chaining which could silently fail
if client is undefined. Since client is initialized right before this call, use
non-optional chaining to catch potential errors.

control-plane/src/modules/integrations/slack/index.ts [129-134]

-client?.chat.postMessage({
-  thread_ts: runMetadata[THREAD_META_KEY],
+client.chat.postMessage({
+  thread_ts: runMetadata[THREAD_META_KEY], 
   channel: runMetadata[CHANNEL_META_KEY],
   mrkdwn: true,
   text: messageData.message,
 });
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Removing optional chaining is important as it could mask critical API failures. Since client is initialized right before this call, using non-optional chaining would help catch potential errors early.

8
Handle conflicting tool results by throwing an error instead of silently using the last result

Add error handling for the case where multiple tools return different results.
Currently, it only logs an error but continues execution, which could lead to
inconsistent states. Consider throwing an error or implementing a resolution
strategy.

control-plane/src/modules/workflows/agent/nodes/tool-call.ts [66-73]

 if (!!acc.result && !!result.result && result.result !== acc.result) {
-  logger.error("Multiple tools returned different results. Last one will be used.", {
+  logger.error("Multiple tools returned different results", {
     result,
     accResult: acc.result,
   });
+  throw new AgentError("Multiple tools returned conflicting results. Cannot proceed.");
 }
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion addresses a critical issue where conflicting tool results could lead to inconsistent states. Throwing an error instead of silently continuing with the last result is a more robust approach to handle such conflicts.

8
Add array bounds check to prevent undefined access in loop condition

The while loop condition in stripQuoteTail() should check if lines array is not
empty before accessing index 0 to avoid potential undefined access.

control-plane/src/modules/email/index.ts [196-198]

-while ((lines[0] && lines[0].startsWith(">")) || lines[0].trim() === "") {
+while (lines.length > 0 && (lines[0].startsWith(">") || lines[0].trim() === "")) {
   lines.shift();
 }
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: Adding an array bounds check prevents potential runtime errors from accessing undefined array elements, making the code more robust and safer.

7
Add proper validation for tool call IDs to prevent undefined behavior

Add validation for the toolCall.id before using it in the filter condition. The
current code assumes the id exists when filtering resolved tool calls, which could
lead to undefined behavior.

control-plane/src/modules/workflows/agent/nodes/tool-call.ts [59]

-.filter(toolCall => !resolvedToolsCalls.has(toolCall.id ?? ""))
+.filter(toolCall => {
+  if (!toolCall.id) {
+    logger.warn("Tool call missing ID", { toolCall });
+    return false;
+  }
+  return !resolvedToolsCalls.has(toolCall.id);
+})
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion improves error handling by properly validating tool call IDs and logging warnings when they're missing, preventing potential undefined behavior in the application.

7

@nadeesha nadeesha enabled auto-merge (rebase) January 2, 2025 22:02
@nadeesha nadeesha merged commit f99db79 into main Jan 2, 2025
27 checks passed
@nadeesha nadeesha deleted the nadeesha/refactor-message-schema-or0p branch January 2, 2025 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants