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

Ai collab feedback loop upgrades #23517

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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>;
Copy link
Contributor

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.


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;

Expand All @@ -184,8 +195,29 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 any here... feels like we should be able to use strongly typed APIs.

} catch (error) {
if (error instanceof UsageError === false) {
throw error;
}
// 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(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be pretty fragile - if the error message in tree is updated, this check will begin to silently fail. If we need to be able to match on this specific error, I recommend adding a new Error type to the tree package for this scenario specifically. Then we can against that kind of error in a way that should be detectable through API changes. We can make that error type @internal for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the current unit tests would catch that scenario, since some of them check for the correct generation of INVALID_TYPE messages. Long-term I like the idea of more specific error types, but they'd probably need to come with type guard functions as well?

/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);
}

throw error;
}
}
// If the fieldSchema is a function we can grab the constructor and make an instance of that node.
else if (typeof fieldSchema === "function") {
Expand Down Expand Up @@ -307,6 +339,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.
Copy link
Contributor

Choose a reason for hiding this comment

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

100% nitpick - take it or leave it 🙂

Suggested change
* 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.
* Produces a useful, context-rich error message to give as a response to the LLM when it has produced an {@link ModifyEdit} that either references a nonexistant field or an invalid type for the selected field.

* @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",
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT:
I know there's only two types of errorType but can we make this an enum or object?

As we expand the usage of createInvalidModifyFeedbackMsg() in other treeEdit.types we should be having more errorTypes besides NONEXISTENT_FIELD or INVALID_TYPE right?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 !== "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always be true, so maybe remove it? findClosestStringMatch will always return some field name, and I don't think we can create a field with an empty name in SharedTree. If anything this feels like it could be an assert.

const targetFieldNodeSchema = nodeFieldSchemas[closestPossibleMatchForField];
const allowedTypeIdentifiers: string[] =
targetFieldNodeSchema instanceof FieldSchema
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do something different that instanceof here? Mismatches in the version of SharedTree this library depends on and the one used by the application (which we don't really support, but could happen) will make this check not work as expected.

? [...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}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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}`;
messageSuffix = ` The node's field you selected for modification \`${modifyEdit.field}\` does not exist in this node's schema. The set of available fields for this node are: \`[${nodeFieldNames.map((field) => `'${field}'`).join(", ")}]\`.${closestPossibleMatchForFieldMessage}`;

} else if (errorType === "INVALID_TYPE") {
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, we can make this else.

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(", ")}]\`.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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(", ")}]\`.`;
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 field's schema. The set of allowed types are \`[${allowedTypeIdentifiers.map((id) => `'${id}'`).join(", ")}]\`.`;

}

return messagePrefix + messageSuffix;
}

function isNodeAllowedType(node: TreeNode, allowedTypes: TreeNodeSchema[]): boolean {
for (const allowedType of allowedTypes) {
if (Tree.is(node, allowedType)) {
Expand Down
69 changes: 69 additions & 0 deletions packages/framework/ai-collab/src/explicit-strategy/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 @ts-expect-error for all these instead. Then if we eventually replace the logic with something that TS is ok with, we'll get a complaint about the comments not finding an error anymore and we can remove them.

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) {
Copy link
Contributor

@jikim-msft jikim-msft Jan 15, 2025

Choose a reason for hiding this comment

The 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 createInvalidModifyFeedbackMsg()?

bestDistance = distance;
bestMatch = candidate;
}
}
return bestMatch;
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
Generated on: 2024-12-04T19:14:12.318Z
Generated on: 2025-01-09T16:32:20.054Z
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 populated edit log has no regression
Expand Down Expand Up @@ -76,7 +76,7 @@ interface EditWrapper {
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: "Expected field schema"
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":false}
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":false},{"__fluid_objectId":"Todo2","title":"Task 2","completed":true}]}.
Expand Down
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).
Loading
Loading