-
Notifications
You must be signed in to change notification settings - Fork 132
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
handle multi_tool_use.parallel #631
Conversation
The changes in the pull request mainly update the handling of tool calls in the The key changes include:
While the changes seem sound in theory, it is worth noting that the error handling does not include cases where the "multi_tool_use.parallel" call might have incorrect or missing parameters. In such cases, the function might throw errors that are not user-friendly or informative. Overall, this pull request looks good to me. LGTM 🚀. However, I would recommend enhancing the error handling for the "multi_tool_use.parallel" calls to ensure robustness.
|
const res = await workspace.readText(filename) | ||
content = res.content ?? "" | ||
} catch (e) { | ||
return "" | ||
return undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value in case of an exception has changed from an empty string "" to undefined which may affect error handling logic.
generated by pr-docs-review-commit
return_value_change
@@ -466,69 +472,6 @@ defTool( | |||
````` | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire section for 'system.fs_read_summary' has been removed. Ensure this is intentional and that no references to it remain.
generated by pr-docs-review-commit
documentation_removed
if (!tool) { | ||
logVerbose(JSON.stringify(tu, null, 2)) | ||
throw new Error(`tool ${toolName} not found`) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the case when a tool is not found in the multi_tool_use.parallel
block. This could lead to unexpected behavior or crashes. Consider adding appropriate error handling. 🛠️
generated by pr-review-commit
missing_error_handling
const tool = tools.find((f) => f.definition.name === call.name) | ||
if (!tool) { | ||
logVerbose(JSON.stringify(call, null, 2)) | ||
throw new Error(`tool ${call.name} not found`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the case when a tool is not found in the else
block. This could lead to unexpected behavior or crashes. Consider adding appropriate error handling. 🛠️
generated by pr-review-commit
missing_error_handling
if (output === undefined || output === null) | ||
throw new Error( | ||
`tool ${tool.definition.name} output is undefined` | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the case when the output of a tool is undefined or null. This could lead to unexpected behavior or crashes. Consider adding appropriate error handling. 🛠️
generated by pr-review-commit
missing_error_handling
@@ -390,17 +390,22 @@ defTool( | |||
}, | |||
pattern: { | |||
type: "string", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect indentation for the description property of the pattern object.
generated by pr-docs-review-commit
incorrect_indentation
} | ||
description: | ||
"Optional regular expression pattern to search for in the file content.", | ||
}, | ||
}, | ||
required: ["glob"], | ||
}, | ||
async (args) => { | ||
const { glob, pattern } = args | ||
console.log(pattern ? `grep ${pattern} ${glob}` : `ls ${glob}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging code with console.log should not be present in production documentation.
generated by pr-docs-review-commit
debug_code
if (!res?.length) return "No files found." | ||
return res.map((f) => f.filename).join("\n") | ||
const ress = res.map((f) => f.filename).join("\n") | ||
console.log(ress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging code with console.log should not be present in production documentation.
generated by pr-docs-review-commit
debug_code
@@ -450,10 +455,11 @@ defTool( | |||
lineend = parseInt(lineend) | |||
let content | |||
try { | |||
console.log(`cat ${filename}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging code with console.log should not be present in production documentation.
generated by pr-docs-review-commit
debug_code
@@ -450,10 +455,11 @@ defTool( | |||
lineend = parseInt(lineend) | |||
let content | |||
try { | |||
console.log(`cat ${filename}`) | |||
const res = await workspace.readText(filename) | |||
content = res.content ?? "" | |||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should return an empty string "" instead of undefined when catching an error.
generated by pr-docs-review-commit
undefined_return
@@ -466,69 +472,6 @@ defTool( | |||
````` | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The section for 'system.fs_read_summary' has been removed, but there is no indication of this change in the documentation. There should be a note or update reflecting this removal.
generated by pr-docs-review-commit
removed_feature
trace.startDetails(`📠 tool call ${call.name}`) | ||
try { | ||
const callArgs: any = call.arguments // sometimes wrapped in \`\`\`json ... | ||
? JSONLLMTryParse(call.arguments) | ||
: undefined | ||
trace.fence(call.arguments, "json") | ||
if (callArgs === undefined) trace.error("arguments failed to parse") | ||
const fd = functions.find((f) => f.definition.name === call.name) | ||
if (!fd) throw new Error(`tool ${call.name} not found`) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no error handling for the case when callArgs
is undefined. This could lead to unexpected behavior or crashes.
generated by pr-review-commit
missing_error_handling
@@ -8,7 +8,6 @@ import { LinkCard } from '@astrojs/starlight/components'; | |||
|
|||
<LinkCard title="fs_find_files" description="Finds file matching a glob pattern. Use pattern to specify a regular expression to search for in the file content." href="/genaiscript/reference/scripts/system#systemfs_find_files" /> | |||
<LinkCard title="fs_read_file" description="Reads a file as text from the file system." href="/genaiscript/reference/scripts/system#systemfs_read_file" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LinkCard for 'fs_read_summary' has been removed, but there is no indication of deprecation or replacement for users relying on this documentation.
generated by pr-docs-review-commit
missing_content
@@ -466,86 +472,6 @@ defTool( | |||
````` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire section for system.fs_read_summary
has been removed without any explanation or redirection for users who might be looking for this information.
generated by pr-docs-review-commit
removed_content
multi_tool_use.parallel is an internal hallucation in the tools support of OpenAI that sometimes leaks to our layer.
Here's a high-level summary of the changes indicated in the provided GIT_DIFF:
🛠 Tool Modifications: The core tool handling logic in
packages/core/src/chat.ts
has been substantially refactored. This includes handling multiple tool instances simultaneously for a 'parallel' multi_tool_use and refining how tool output is processed.📄 File Read Tool Change: In the system 'fs_read_file' function, an unsuccessful file read, earlier returning an empty string, now returns 'undefined'. The change affects both
packages/core/src/genaisrc/system.fs_read_file.genai.js
anddocs/src/content/docs/reference/scripts/system.mdx
.🗑️ Tool Removal: The 'fs_read_summary' system tool, which summarised the content of a file, has been completely removed from the codebase. This includes the tool definition script
packages/core/src/genaisrc/system.fs_read_summary.genai.js
, its documentation atdocs/src/content/docs/reference/scripts/system.mdx
, and its reference ondocs/src/components/BuiltinTools.mdx
.📝 Task Description Update: The task description in
docs/genaisrc/devto-blog-generator.genai.mjs
has been updated with additional requirements, such as selecting an image from assets as the hero image, and clarifications on what not to include in the blog post.🖼️ Image Alt Text Guidance: Added instruction on where to find alt text for images in the src/assets folder in
docs/genaisrc/devto-blog-generator.genai.mjs
.Remember, changes in
packages/core/src/prompt_template.d.ts
andpackages/core/src/prompt_type.ts
are user facing, any modifications in these files may affect how users interact with the software. In this instance, no changes occurred in these files.