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.

Sign up for GitHub

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

Fix/isValidMessage #5579

Closed
wants to merge 7 commits into from
Closed

Fix/isValidMessage #5579

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7fa8234
add more criteria for detecting invalid messages
code-october Oct 3, 2024
a039690
add response judgment conditions for summarizing messages request
code-october Oct 3, 2024
4bb52b2
调整判断规则、优化代码规范
code-october Oct 11, 2024
7d7ca64
Merge branch 'ChatGPTNextWeb:main' into fix/isValidMessage
code-october Oct 12, 2024
dd793e0
完善判断逻辑，添加jest测试样例
code-october Oct 12, 2024
fe9c31c
fix isValidMessage.test.ts
code-october Oct 12, 2024
b0236f7
fix jest error
code-october Oct 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/store/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,7 @@ export const useChatStore = createPersistStore(
},
onFinish(message) {
console.log("[Memory] ", message);
if (!isValidMessage(message)) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Potential unvalidated message processing detected in app/store/chat.ts at line 696.

Please ensure that isValidMessage is invoked consistently wherever messages are processed to maintain validation integrity.

🔗 Analysis chain

Verify that message validation is consistently applied

In the onFinish callback, the check if (!isValidMessage(message)) return; ensures that invalid messages are not processed. To maintain consistency and robustness, ensure that all instances where messages are processed include validation using isValidMessage.

Run the following script to check for other message handling without validation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find instances where messages are handled without validation.
# Expected: All message processing should involve validation via isValidMessage.

rg --type ts --no-heading --line-number --color never 'function\s+\w+\(.*message.*\)' app/store/ | while read -r line; do
  file=$(echo "$line" | cut -d: -f1)
  lineno=$(echo "$line" | cut -d: -f2)
  function_def=$(echo "$line" | cut -d: -f3-)
  function_body=$(awk "NR>$lineno && NR<($lineno+20)" "$file")
  if ! echo "$function_body" | grep -q "isValidMessage"; then
    echo "Potential unvalidated message in $file:$lineno - $function_def"
  fi
done

Length of output: 2295

get().updateCurrentSession((session) => {
session.lastSummarizeIndex = lastSummarizeIndex;
session.memoryPrompt = message; // Update the memory prompt for stored it in local storage
Expand All @@ -693,6 +694,17 @@ export const useChatStore = createPersistStore(
}

function isValidMessage(message: any): boolean {
if (message.startsWith("```") && message.endsWith("```")) {
const jsonString = message.slice(3, -3).trim();
try {
const jsonObject = JSON.parse(jsonString);
if (jsonObject.error) {
return false;
}
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify JSON extraction and improve accuracy

The current implementation checks for messages enclosed in triple backticks and attempts to parse the content as JSON. However, it might be more precise to specifically check for messages starting with ```json to accurately identify JSON-formatted messages.

Consider adjusting the function to enhance accuracy:

-              if (message.startsWith("```") && message.endsWith("```")) {
-                const jsonString = message.slice(3, -3).trim();
+              if (typeof message === "string" && message.startsWith("```json") && message.endsWith("```")) {
+                const jsonString = message.slice(7, -3).trim();
                 try {
                   const jsonObject = JSON.parse(jsonString);
                   if (jsonObject.error) {
                     return false;
                   }
                 } catch (e) {
                   console.log("Invalid JSON format.");
                   return false;
                 }
               }
-              return typeof message === "string" && !message.startsWith("```json");
+              return true;

This refactor:

  • Ensures the message is a string before processing.
  • Checks specifically for JSON code blocks starting with ```json.
  • Slices the content correctly to extract the JSON string.
  • Returns false immediately if JSON parsing fails.
  • Returns true at the end, assuming all invalid conditions have been checked.

console.log("Invalid JSON format.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Return false when JSON parsing fails

In the catch block of isValidMessage, when JSON parsing fails, the function logs "Invalid JSON format." but does not return false. This means that messages with invalid JSON will not be correctly marked as invalid.

Consider returning false when JSON parsing fails to ensure invalid messages are properly handled.

Apply this diff to return false when JSON parsing fails:

                 } catch (e) {
                   console.log("Invalid JSON format.");
+                  return false;
                 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
console.log("Invalid JSON format.");
}
console.log("Invalid JSON format.");
return false;
}

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure message is a string before invoking string methods

In the isValidMessage function, string methods startsWith and endsWith are called on message without verifying that message is indeed a string. If message is not a string, this can lead to runtime errors.

To prevent potential errors, check if message is a string before invoking string methods.

Apply this diff to add a type check:

+              if (typeof message !== "string") {
+                return false;
+              }
               if (message.startsWith("```") && message.endsWith("```")) {
                 const jsonString = message.slice(3, -3).trim();
                 try {
                   const jsonObject = JSON.parse(jsonString);
                   if (jsonObject.error) {
                     return false;
                   }
                 } catch (e) {
                   console.log("Invalid JSON format.");
+                  return false;
                 }
               }
-              return typeof message === "string" && !message.startsWith("```json");
+              return !message.startsWith("```json");

return typeof message === "string" && !message.startsWith("```json");
}
},
Expand Down
Loading