Ai collab feedback loop upgrades #23517

seanimam · 2025-01-09T17:03:24Z

Description

The error messages produced from attempting an invalid modify edit are being used as part of the feedback loop for an LLM so that when it makes an invalid error, it gets a failure message and description as to why it failed. Until now, these error messages were simply what the shared tree library produced, which may be helpful for developers but loses meaningful context for an LLM looking at what it has done in the last via a prompt.

agentEditReducer.ts - Catches errors during the modify switch case and replaces the error message with a context rich one.
agentEditingReducer.spec.ts - Refactors tests to have a describe block just for various 'modify' tree edit test cases & adds new cases for when the new error messages would be thrown.
utils.ts - Adds new helper methods for determining what the closest match for an invalid field would be. This helps with providing a 'did you mean to use the field xxx' message
utils.spec.ts - Adds tests for new helper methods for finding the closest match to a given string.

Reviewer Guidance

Analyze the updated messaging in the prompt snapshots for different types of failed modifications. LMK if you see a missed invalid modify case.
Analyze my use of schema extraction for field/leafnodes in building the message

…onexistant field or invalid type for field

… 'modify' edit tests in agentEditReducer.spec.ts

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)

packages/framework/ai-collab/src/test/explicit-strategy/snapshots/Prompt_Regression_Snapshot_Tests/Editing_System_Prompt_With_Plan_With_Log.snap: Language not supported
packages/framework/ai-collab/src/test/explicit-strategy/snapshots/Prompt_Regression_Snapshot_Tests/Editing_System_Prompt_With_Plan_With_Log_With_Failed_Edits.snap: Language not supported

… expected type

jikim-msft · 2025-01-15T01:26:28Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

+function createInvalidModifyFeedbackMsg(
+	modifyEdit: Modify,
+	treeNode: TreeNode,
+	errorType: "NONEXISTENT_FIELD" | "INVALID_TYPE",


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?

Alternatively, should this be 2 separate functions? The shared logic could be extracted into a helper function to avoid code duplication.

jikim-msft · 2025-01-15T01:26:55Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

+		}
+
+		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}`;
+	} else if (errorType === "INVALID_TYPE") {


For now, we can make this else.

jikim-msft · 2025-01-15T01:39:41Z

packages/framework/ai-collab/src/test/explicit-strategy/agentEditingReducer.spec.ts

+				"str": "testStr",
+				"vectors": [
+					{
+						"id": (view.root.vectors[0] as Vector).id,


I see id is expected for vectors but not for primitive values such as string or bool. Is this expected? Does idGenerator() generate and embed id to non-primitive nodes?

Our schema explicitly adds an id field to Vector (lines 40-52), that's why it only shows up there. It's not that non-primitives get ids by default, it's only those whose schema says they have one.

jikim-msft · 2025-01-15T01:48:01Z

packages/framework/ai-collab/src/test/explicit-strategy/agentEditingReducer.spec.ts

+			);
+		});
+
+		it("modify edit with non existent field fails", () => {


Correct me if I'm wrong, but this test is passing in my local environment.

Also, I think it might be useful to add a test case of a root node with multiple fields of identical Levenshtein distance (so a maybe have fields x1 and x2 and try to modify x3). And verify that the error log has good readability with multiple values.

The test needs to pass, otherwise we wouldn't be able to merge this :). The title just means that the test is checking a given scenario fails in expected way (if we're expecting it to fail but it doesn't, then the test itself would fail).

jikim-msft · 2025-01-15T01:56:42Z

packages/framework/ai-collab/src/explicit-strategy/utils.ts

+
+	for (const candidate of possibleMatches) {
+		const distance = levenshteinDistance(input, candidate);
+		if (distance < bestDistance) {


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

Josmithr · 2025-01-22T21:59:11Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

+					}
+					// 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(


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.

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?

Josmithr · 2025-01-22T22:00:59Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

@@ -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.


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.

Josmithr · 2025-01-22T22:03:04Z

packages/framework/ai-collab/src/explicit-strategy/utils.ts

+ * 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 {


Do we need to implement this ourselves? Is there an external library we could use for this?

alexvy86 · 2025-01-23T15:59:33Z

packages/framework/ai-collab/src/explicit-strategy/utils.ts

+
+	// 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.


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.

alexvy86 · 2025-01-23T16:02:56Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

+			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}`;


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}`;

alexvy86 · 2025-01-23T16:05:10Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

+				: [(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(", ")}]\`.`;


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

alexvy86 · 2025-01-23T16:07:03Z

packages/framework/ai-collab/src/test/explicit-strategy/agentEditingReducer.spec.ts

@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/no-unsafe-member-access */


Not a fan of disabling lint rules globally like this without a good explanation. Were there so many instances of it? Also, I think this needs to go under the header so policy checks pass.

alexvy86 · 2025-01-23T16:09:32Z

packages/framework/ai-collab/src/test/explicit-strategy/agentEditingReducer.spec.ts

+			);
+		});
+
+		it("modify edit with non existent field fails", () => {


The test needs to pass, otherwise we wouldn't be able to merge this :). The title just means that the test is checking a given scenario fails in expected way (if we're expecting it to fail but it doesn't, then the test itself would fail).

alexvy86 · 2025-01-23T16:37:20Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

+		);
+
+		let closestPossibleMatchForFieldMessage = "";
+		if (closestPossibleMatchForField !== "") {


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.

alexvy86 · 2025-01-23T16:39:01Z

packages/framework/ai-collab/src/explicit-strategy/agentEditReducer.ts

+		if (closestPossibleMatchForField !== "") {
+			const targetFieldNodeSchema = nodeFieldSchemas[closestPossibleMatchForField];
+			const allowedTypeIdentifiers: string[] =
+				targetFieldNodeSchema instanceof FieldSchema


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.

alexvy86 · 2025-01-23T16:42:35Z

packages/framework/ai-collab/src/test/explicit-strategy/agentEditingReducer.spec.ts

-	it("modify edits", () => {
-		const tree = factory.create(
+	describe("modify edits", () => {
+		let tree = factory.create(


Nit: slightly better if we just declare these here but don't initialize them, since the beforeEach will run anyway and overwrite them before the first test runs.

alexvy86 · 2025-01-23T16:45:49Z

packages/framework/ai-collab/src/test/explicit-strategy/agentEditingReducer.spec.ts

+				"str": "testStr",
+				"vectors": [
+					{
+						"id": (view.root.vectors[0] as Vector).id,


Our schema explicitly adds an id field to Vector (lines 40-52), that's why it only shows up there. It's not that non-primitives get ids by default, it's only those whose schema says they have one.

alexvy86 · 2025-01-23T16:49:57Z

packages/framework/ai-collab/src/test/explicit-strategy/agentEditingReducer.spec.ts

+				"str": "testStr",
+				"vectors": [
+					{
+						"id": (view.root.vectors[0] as Vector).id,


I think this works?

Suggested change

"id": (view.root.vectors[0] as Vector).id,

"id": vectorId,

seanimam added 2 commits January 8, 2025 20:27

Completed improved feedback loop messages for invalid modify's with n…

1f78ced

…onexistant field or invalid type for field

Completed modify message feedback loop updates & updates/refactors to…

c4e692b

… 'modify' edit tests in agentEditReducer.spec.ts

seanimam requested review from a team and Copilot January 9, 2025 17:03

github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch labels Jan 9, 2025

Copilot AI reviewed Jan 9, 2025

View reviewed changes

update agentEditReducer to still throw errors when they are not of an…

37771b9

… expected type

jikim-msft reviewed Jan 15, 2025

View reviewed changes

Josmithr reviewed Jan 22, 2025

View reviewed changes

alexvy86 reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ai collab feedback loop upgrades #23517

Ai collab feedback loop upgrades #23517

seanimam commented Jan 9, 2025 •

edited

Loading

jikim-msft Jan 15, 2025

Josmithr Jan 22, 2025

jikim-msft Jan 15, 2025

jikim-msft Jan 15, 2025

alexvy86 Jan 23, 2025

jikim-msft Jan 15, 2025 •

edited

Loading

alexvy86 Jan 23, 2025

jikim-msft Jan 15, 2025 •

edited

Loading

Josmithr Jan 22, 2025

alexvy86 Jan 23, 2025

Josmithr Jan 22, 2025

Josmithr Jan 22, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

alexvy86 Jan 23, 2025

	* 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.

	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}`;

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

		@@ -1,3 +1,4 @@
		/* eslint-disable @typescript-eslint/no-unsafe-member-access */

Ai collab feedback loop upgrades #23517

Are you sure you want to change the base?

Ai collab feedback loop upgrades #23517

Conversation

seanimam commented Jan 9, 2025 • edited Loading

Description

Reviewer Guidance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jikim-msft Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jikim-msft Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanimam commented Jan 9, 2025 •

edited

Loading

jikim-msft Jan 15, 2025 •

edited

Loading

jikim-msft Jan 15, 2025 •

edited

Loading