fix: incorrect intent checking for namespace messages #822
Conversation
| @@ -14,7 +14,7 @@ | |||
| "--extensionDevelopmentPath=${workspaceFolder}" | |||
| ], | |||
| "outFiles": [ | |||
| "${workspaceFolder}/out/**/*.js" | |||
| "${workspaceFolder}/dist/**/*.js" | |||
There was a problem hiding this comment.
This change allows the debugger to bind breakpoints we put in the typescript files.
| @@ -39,7 +39,7 @@ export function getHistoryMessages({ | |||
| ]; | |||
| if ( | |||
| responseTypesToSkip.indexOf( | |||
| (historyItem.result as ChatResult)?.metadata.intent | |||
| (historyItem.result as ChatResult)?.metadata?.intent | |||
There was a problem hiding this comment.
This is another thing I noticed - not sure if there were cases where it would have manifested, but technically metadata is optional on vscode.ChatResult. Even though we're casting to our ChatResult type, this cast would still succeed if metadata is undefined, making the unconditional access here incorrect.
| if ( | ||
| connectionNames.includes(request.prompt) && | ||
| (context.history[context.history.length - 1] as vscode.ChatResponseTurn) | ||
| ?.result?.metadata?.askToConnect |
There was a problem hiding this comment.
This is where the bug was - askToConnect was replaced with intent, but because this was operating on the vscode.ChatResult.metadata, which is of type any, the compiler didn't complain.
|
Going through the Jira tickets, perhaps VSCODE-609 would have actually caught this. |
Description
As I was looking through the namespace buildMessages implementation for something different, I noticed we're using a non-existent field from the chat result metadata to slice the message history. It doesn't appear we have tests for it, which is why it slipped. I restructured the check a little to more explicitly leverage the type system, but perhaps we should have tests to catch future cases.
Checklist
Motivation and Context
Types of changes