-
Notifications
You must be signed in to change notification settings - Fork 536
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
Ai collab feedback loop upgrades #23517
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -32,11 +32,12 @@ import { | |||||
type TreeEditObject, | ||||||
type TreeEditValue, | ||||||
typeField, | ||||||
type Modify, | ||||||
} from "./agentEditTypes.js"; | ||||||
import type { IdGenerator } from "./idGenerator.js"; | ||||||
import type { JsonValue } from "./jsonTypes.js"; | ||||||
import { toDecoratedJson } from "./promptGeneration.js"; | ||||||
import { fail } from "./utils.js"; | ||||||
import { fail, findClosestStringMatch } from "./utils.js"; | ||||||
|
||||||
function populateDefaults( | ||||||
json: JsonValue, | ||||||
|
@@ -172,9 +173,19 @@ export function applyAgentEdit( | |||||
const node = getNodeFromTarget(treeEdit.target, idGenerator); | ||||||
const { treeNodeSchema } = getSimpleNodeSchema(node); | ||||||
|
||||||
const fieldSchema = | ||||||
(treeNodeSchema.info as Record<string, ImplicitFieldSchema>)[treeEdit.field] ?? | ||||||
fail("Expected field schema"); | ||||||
const nodeFieldSchemas = treeNodeSchema.info as Record<string, ImplicitFieldSchema>; | ||||||
|
||||||
const fieldSchema = nodeFieldSchemas[treeEdit.field]; | ||||||
|
||||||
// If the LLM attempts to modify a field that does not exist in the target schema we generate a useful error message that can be used as part of the feedback loop. | ||||||
if (fieldSchema === undefined) { | ||||||
const errorMessage = createInvalidModifyFeedbackMsg( | ||||||
treeEdit, | ||||||
node, | ||||||
"NONEXISTENT_FIELD", | ||||||
); | ||||||
throw new UsageError(errorMessage); | ||||||
} | ||||||
|
||||||
const modification = treeEdit.modification; | ||||||
|
||||||
|
@@ -184,8 +195,26 @@ export function applyAgentEdit( | |||||
let insertedObject: TreeNode | undefined; | ||||||
// if fieldSchema is a LeafnodeSchema, we can check that it's a valid type and set the field. | ||||||
if (isPrimitive(modification)) { | ||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access | ||||||
(node as any)[treeEdit.field] = modification; | ||||||
try { | ||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access | ||||||
(node as any)[treeEdit.field] = modification; | ||||||
Comment on lines
+199
to
+200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this was already here, just checking why we need to cast as |
||||||
} catch (error) { | ||||||
if (error instanceof UsageError) { | ||||||
// If the LLM attempts to use the wrong type for a field, we generate a useful error message that can be used as part of the feedback loop. | ||||||
const isInvalidTypeError = | ||||||
error.message.match( | ||||||
/The provided data is incompatible with all of the types allowed by the schema./, | ||||||
) !== null; | ||||||
if (isInvalidTypeError === true) { | ||||||
const errorMessage = createInvalidModifyFeedbackMsg( | ||||||
treeEdit, | ||||||
node, | ||||||
"INVALID_TYPE", | ||||||
); | ||||||
throw new UsageError(errorMessage); | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
// If the fieldSchema is a function we can grab the constructor and make an instance of that node. | ||||||
else if (typeof fieldSchema === "function") { | ||||||
|
@@ -307,6 +336,60 @@ export function applyAgentEdit( | |||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* Produces a useful, context-rich error message for the LLM when the LLM produces an {@link ModifyEdit} that either references a nonexistant field or an invalid type for the selected field. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 100% nitpick - take it or leave it 🙂
Suggested change
|
||||||
* @param errorType - The type of error message to produce. You must determine the error type before calling this function. | ||||||
* - `'NONEXISTENT_FIELD'` is used when the field does not exist in the node's schema. | ||||||
* - `'INVALID_TYPE'` is used when the field exists but the type of the modification is invalid. | ||||||
*/ | ||||||
function createInvalidModifyFeedbackMsg( | ||||||
modifyEdit: Modify, | ||||||
treeNode: TreeNode, | ||||||
errorType: "NONEXISTENT_FIELD" | "INVALID_TYPE", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: As we expand the usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, should this be 2 separate functions? The shared logic could be extracted into a helper function to avoid code duplication. |
||||||
): string { | ||||||
const { treeNodeSchema } = getSimpleNodeSchema(treeNode); | ||||||
const nodeFieldSchemas = treeNodeSchema.info as Record<string, ImplicitFieldSchema>; | ||||||
|
||||||
const messagePrefix = `You attempted an invalid modify edit on the node with id '${modifyEdit.target.target}' and schema '${treeNodeSchema.identifier}'.`; | ||||||
let messageSuffix = ""; | ||||||
if (errorType === "NONEXISTENT_FIELD") { | ||||||
const nodeFieldNames = Object.keys(nodeFieldSchemas); | ||||||
const closestPossibleMatchForField = findClosestStringMatch( | ||||||
modifyEdit.field, | ||||||
nodeFieldNames, | ||||||
); | ||||||
|
||||||
let closestPossibleMatchForFieldMessage = ""; | ||||||
if (closestPossibleMatchForField !== "") { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will always be true, so maybe remove it? |
||||||
const targetFieldNodeSchema = nodeFieldSchemas[closestPossibleMatchForField]; | ||||||
const allowedTypeIdentifiers: string[] = | ||||||
targetFieldNodeSchema instanceof FieldSchema | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do something different that |
||||||
? [...targetFieldNodeSchema.allowedTypeSet.values()].map( | ||||||
(schema) => schema.identifier, | ||||||
) | ||||||
: [(targetFieldNodeSchema as TreeNodeSchema).identifier]; | ||||||
closestPossibleMatchForFieldMessage = ` If you are sure you are trying to modify this node, did you mean to use the field \`${closestPossibleMatchForField}\` which has the following set of allowed types: \`[${allowedTypeIdentifiers.map((id) => `'${id}'`).join(", ")}]\`?`; | ||||||
} | ||||||
|
||||||
messageSuffix = ` The node's field you selected for modification \`${modifyEdit.field}\` does not exist in this nodes schema. The set of available fields for this node are: \`[${nodeFieldNames.map((field) => `'${field}'`).join(", ")}]\`.${closestPossibleMatchForFieldMessage}`; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} else if (errorType === "INVALID_TYPE") { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, we can make this |
||||||
const targetFieldNodeSchema = nodeFieldSchemas[modifyEdit.field]; | ||||||
const allowedTypeIdentifiers = | ||||||
targetFieldNodeSchema instanceof FieldSchema | ||||||
? [ | ||||||
[...targetFieldNodeSchema.allowedTypeSet.values()].map( | ||||||
(schema) => schema.identifier, | ||||||
), | ||||||
] | ||||||
: [(targetFieldNodeSchema as TreeNodeSchema).identifier]; | ||||||
|
||||||
// TODO: If the invalid modification is a new object, it won't be clear what part of the object is invalid for the given type. If we could give some more detailed guidance on what was wrong with the object it would be ideal. | ||||||
messageSuffix = ` You cannot set the node's field \`${modifyEdit.field}\` to the value \`${modifyEdit.modification}\` with type \`${typeof modifyEdit.modification}\` because this type is incompatible with all of the types allowed by the node's schema. The set of allowed types are \`[${allowedTypeIdentifiers.map((id) => `'${id}'`).join(", ")}]\`.`; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
return messagePrefix + messageSuffix; | ||||||
} | ||||||
|
||||||
function isNodeAllowedType(node: TreeNode, allowedTypes: TreeNodeSchema[]): boolean { | ||||||
for (const allowedType of allowedTypes) { | ||||||
if (Tree.is(node, allowedType)) { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,3 +58,72 @@ export function getOrCreate<K, V>( | |
} | ||
return value; | ||
} | ||
|
||
/** | ||
* Computes the Levenshtein distance between two strings. | ||
* The Levenshtein distance between two strings is the minimum number of single-character edits (insertions, deletions, or substitutions) required to change one string into the other. | ||
*/ | ||
export function levenshteinDistance(a: string, b: string): number { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to implement this ourselves? Is there an external library we could use for this? |
||
/* eslint-disable @typescript-eslint/ban-ts-comment */ | ||
|
||
// Initialize a 2D array to store the minimum edit distance at each step | ||
const dp: number[][] = Array.from({ length: a.length + 1 }, (_, i) => [i]); | ||
|
||
// Initialize the first row of the DP table (representing the cost of transforming an empty string to the first j characters of b) | ||
for (let j = 1; j <= b.length; j++) { | ||
// @ts-ignore - we know the accessed indexes of dp are valid. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Josh to maybe look for a library that implements this if we can find a small one. If we keep the code, let's use |
||
dp[0][j] = j; // Cost of transforming an empty string to the first j characters of b is j insertions | ||
} | ||
|
||
// Fill in the DP table by considering each character in both strings | ||
for (let i = 1; i <= a.length; i++) { | ||
for (let j = 1; j <= b.length; j++) { | ||
// If characters match, no substitution is needed, otherwise, substitution has a cost of 1 | ||
const cost = a[i - 1] === b[j - 1] ? 0 : 1; | ||
|
||
// For each pair of characters from a and b, compute the minimum edit distance | ||
// The DP state dp[i][j] is the minimum of: | ||
// 1. Deleting a character from a (dp[i-1][j] + 1) | ||
// 2. Inserting a character into a (dp[i][j-1] + 1) | ||
// 3. Substituting a character (dp[i-1][j-1] + cost) | ||
// The cost will be the minimum of these three options | ||
// @ts-ignore - we know the accessed indexes of dp are valid. | ||
dp[i][j] = Math.min( | ||
// Deletion: Remove a character from string a | ||
// @ts-ignore | ||
dp[i - 1][j] + 1, | ||
// Insertion: Add a character to string a | ||
// @ts-ignore | ||
dp[i][j - 1] + 1, | ||
// Substitution: Change one character to another | ||
// @ts-ignore | ||
dp[i - 1][j - 1] + cost, | ||
); | ||
} | ||
} | ||
|
||
// The value in dp[a.length][b.length] holds the final minimum edit distance | ||
// @ts-ignore | ||
return dp[a.length][b.length]; | ||
} | ||
/* eslint-enable @typescript-eslint/ban-ts-comment */ | ||
|
||
/** | ||
* Returns the closest match from a list of possibleMatches to the input string, | ||
* based on the smallest Levenshtein distance. | ||
* | ||
* @remarks this is intended to be used to help steer the LLM towards the correct field name if it attempts to use a field that does not exist on a given tree node. | ||
*/ | ||
export function findClosestStringMatch(input: string, possibleMatches: string[]): string { | ||
let bestMatch = ""; | ||
let bestDistance = Number.POSITIVE_INFINITY; | ||
|
||
for (const candidate of possibleMatches) { | ||
const distance = levenshteinDistance(input, candidate); | ||
if (distance < bestDistance) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this condition only honor the first-seen candidate and disregard other candidates with same Levenshtein distance? Can we return an array containing all the candidates and use string concatenation in the |
||
bestDistance = distance; | ||
bestMatch = candidate; | ||
} | ||
} | ||
return bestMatch; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
--- | ||
Generated on: 2025-01-09T16:28:33.565Z | ||
description: This is a snapshot file utilized for testing purposes. | ||
Test Suite Title: Prompt Generation Regression Tests | ||
Test Title: Editing System Prompt with plan and edit log with invalid `modify` has no regression | ||
--- | ||
|
||
You are a collaborative agent who interacts with a JSON tree by performing edits to achieve a user-specified goal. | ||
The application that owns the JSON tree has the following guidance about your role: "You're a helpful AI assistant". | ||
Edits are JSON objects that conform to the schema described below. The top-level object you produce for a given edit is an "EditWrapper" object which contains one of the following interfaces: "Modify", null or an array node only edit: "Insert", "Remove", "Move". | ||
|
||
Here are the schema definitions for an edit: | ||
// A pointer to a specific object node in the tree, identified by the target object's Id. | ||
interface ObjectTarget { | ||
target: string; // The id of the object (as specified by the object's __fluid_objectId property) that is being referenced | ||
} | ||
|
||
// Sets a field on a specific ObjectTarget. | ||
interface Modify { | ||
type: "modify"; | ||
explanation: string; // A description of what this edit is meant to accomplish in human readable English | ||
target: ObjectTarget; // A pointer to a specific object node in the tree, identified by the target object's Id. | ||
field: "title" | "description" | "completed"; | ||
modification: any; // Domain-specific content here | ||
} | ||
|
||
// A pointer to a location either just before or just after an object that is in an array | ||
interface ObjectPlace { | ||
type: "objectPlace"; | ||
target: string; // The id (__fluid_objectId) of the object that the new/moved object should be placed relative to. This must be the id of an object that already existed in the tree content that was originally supplied. | ||
place: "before" | "after"; // Where the new/moved object will be relative to the target object - either just before or just after | ||
} | ||
|
||
// either the "start" or "end" of an array, as specified by a "parent" ObjectTarget and a "field" name under which the array is stored (useful for prepending or appending) | ||
interface ArrayPlace { | ||
type: "arrayPlace"; | ||
parentId: string; // The id (__fluid_objectId) of the parent object of the array. This must be the id of an object that already existed in the tree content that was originally supplied. | ||
field: string; // The key of the array to insert into | ||
location: "start" | "end"; // Where to insert into the array - either the start or the end | ||
} | ||
|
||
// A range of objects in the same array specified by a "from" and "to" Place. The "to" and "from" objects MUST be in the same array. | ||
interface Range { | ||
from: ObjectPlace; // A pointer to a location either just before or just after an object that is in an array | ||
to: ObjectPlace; // A pointer to a location either just before or just after an object that is in an array | ||
} | ||
|
||
// Inserts a new object at a specific Place or ArrayPlace. | ||
interface Insert { | ||
type: "insert"; | ||
explanation: string; // A description of what this edit is meant to accomplish in human readable English | ||
content: any; // Domain-specific content here | ||
destination: ArrayPlace | ObjectPlace; | ||
} | ||
|
||
// Deletes an object or Range of objects from the tree. | ||
interface Remove { | ||
type: "remove"; | ||
explanation: string; // A description of what this edit is meant to accomplish in human readable English | ||
source: ObjectTarget | Range; | ||
} | ||
|
||
// Moves an object or Range of objects to a new Place or ArrayPlace. | ||
interface Move { | ||
type: "move"; | ||
explanation: string; // A description of what this edit is meant to accomplish in human readable English | ||
source: ObjectTarget | Range; | ||
destination: ArrayPlace | ObjectPlace; | ||
} | ||
|
||
interface EditWrapper { | ||
edit: Insert | Remove | Move | Modify | null; // The next edit to apply to the tree, or null if the task is complete. | ||
} | ||
|
||
|
||
The tree is a JSON object with the following schema: interface TestTodoAppSchema { title: string; description: string; todos: Todo[]; } interface Todo { title: string; completed: boolean; } | ||
You have made a plan to accomplish the user's goal. The plan is: "Change the completed field to false for the todo at index 0 in the list of todos". You will perform one or more edits that correspond to that plan to accomplish the goal. | ||
You have already performed the following edits: | ||
1. {"type":"modify","explanation":"Change the completed field to false for the todo at index 0 in the list of todos","target":{"target":"Todo1"},"field":"complete","modification":false} This edit produced an error, and was discarded. The error message was: "You attempted an invalid modify edit on the node with id 'Todo1' and schema 'test.Todo'. The node's field you selected for modification `complete` does not exist in this nodes schema. The set of available fields for this node are: `['title', 'completed']`. If you are sure you are trying to modify this node, did you mean to use the field `completed` which has the following set of allowed types: `['com.fluidframework.leaf.boolean']`?" | ||
2. {"type":"modify","explanation":"Change the completed field to false for the todo at index 0 in the list of todos","target":{"target":"Todo1"},"field":"completed","modification":"yes"} This edit produced an error, and was discarded. The error message was: "You attempted an invalid modify edit on the node with id 'Todo1' and schema 'test.Todo'. You cannot set the node's field `completed` to the value `yes` with type `string` because this type is incompatible with all of the types allowed by the node's schema. The set of allowed types are `['com.fluidframework.leaf.boolean']`." | ||
This means that the current state of the tree reflects these changes. | ||
The current state of the tree is: {"__fluid_objectId":"TestTodoAppSchema1","title":"My First Todo List","description":"This is a list of todos","todos":[{"__fluid_objectId":"Todo1","title":"Task 1","completed":true},{"__fluid_objectId":"Todo2","title":"Task 2","completed":true}]}. | ||
Before you made the above edits the user requested you accomplish the following goal: | ||
"Change the completed to false for the first task and create a new edit" | ||
If the goal is now completed or is impossible, you should return null. | ||
Otherwise, you should create an edit that makes progress towards the goal. It should have an English description ("explanation") of which edit to perform (specifying one of the allowed edit types). |
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.
How certain are we that this cast is always correct? Wondering if we need some runtime check here.