-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: remove assistantMessageParser experiment flag and enable for all users #7300
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
feat: remove assistantMessageParser experiment flag and enable for all users #7300
Conversation
…l users - Remove assistantMessageParser from experiment definitions in packages/types - Remove ASSISTANT_MESSAGE_PARSER from shared experiments configuration - Update Task.ts to always initialize and use AssistantMessageParser - Remove conditional logic that checked for experiment flag - Remove unused parseAssistantMessage import - Update test files to remove assistantMessageParser references - All tests passing successfully
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.
Reviewed my own code. Found it suspiciously clean. Must have missed something.
| import { RooIgnoreController } from "../ignore/RooIgnoreController" | ||
| import { RooProtectedController } from "../protect/RooProtectedController" | ||
| import { type AssistantMessageContent, presentAssistantMessage, parseAssistantMessage } from "../assistant-message" | ||
| import { type AssistantMessageContent, presentAssistantMessage } from "../assistant-message" |
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 import statement still includes parseAssistantMessage which is no longer used after this refactor. Could we clean this up?
| "preventFocusDisruption", | ||
| "assistantMessageParser", | ||
| ] as const | ||
| export const experimentIds = ["powerSteering", "multiFileApplyDiff", "preventFocusDisruption"] as const |
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.
Since this removes an experimental flag and makes the feature default for all users, should we consider adding a note about any potential impacts on existing users who may have had the experiment disabled? Perhaps in the PR description or as a comment here?
| } | ||
|
|
||
| // Initialize the assistant message parser | ||
| this.assistantMessageParser = new AssistantMessageParser() |
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.
Now that AssistantMessageParser is enabled for all users, could we verify that it has adequate test coverage? The existing tests have been updated to remove the experiment references, but it would be good to ensure the parser functionality itself is well-tested.
daniel-lxs
left a comment
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.
LGTM
This PR removes the
assistantMessageParserexperimental flag and enables the AssistantMessageParser functionality for all users.Changes Made
assistantMessageParserfrom experiment definitions inpackages/types/src/experiment.tsASSISTANT_MESSAGE_PARSERfrom shared experiments configuration insrc/shared/experiments.tsTask.tsto always initialize and useAssistantMessageParserparseAssistantMessageimportassistantMessageParserreferencesTesting
All tests have been run and are passing:
src/shared/__tests__/experiments.spec.ts✅webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx✅src/core/task/__tests__/Task.spec.ts✅Type Safety
Type checking has been completed successfully across all packages.
Context
This change was requested via Slack to promote the experimental
assistantMessageParserfeature to production, making it available for all users by default.Important
Remove
assistantMessageParserexperiment flag and enableAssistantMessageParserfor all users.assistantMessageParserexperiment flag fromexperiment.tsandexperiments.ts.Task.tsto always initialize and useAssistantMessageParser.assistantMessageParserinTask.ts.experiments.spec.tsandExtensionStateContext.spec.tsxto removeassistantMessageParserreferences.parseAssistantMessageimport inTask.ts.This description was created by
for 73fd577. You can customize this summary. It will automatically update as commits are pushed.