-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: make initial prompt preservation conditional on power-steering feature #8094
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
Conversation
…eature - Add powerSteeringEnabled parameter to summarizeConversation function - Check power steering experiment flag before preserving first message - Update all callers to pass power steering state - Update tests to handle new parameter This prevents tasks from being restarted unnecessarily after context condensing when power steering is disabled. Fixes #8093
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.
Reviewing my own code because apparently I trust no one, not even myself.
| isAutomaticTrigger?: boolean, | ||
| customCondensingPrompt?: string, | ||
| condensingApiHandler?: ApiHandler, | ||
| powerSteeringEnabled?: boolean, |
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.
Is this intentional? The parameter defaults to undefined but is used as a boolean. Consider explicitly defaulting to false for clarity:
| powerSteeringEnabled?: boolean, | |
| export async function summarizeConversation( | |
| messages: ApiMessage[], | |
| apiHandler: ApiHandler, | |
| systemPrompt: string, | |
| taskId: string, | |
| prevContextTokens: number, | |
| isAutomaticTrigger?: boolean, | |
| customCondensingPrompt?: string, | |
| condensingApiHandler?: ApiHandler, | |
| powerSteeringEnabled: boolean = false, | |
| ): Promise<SummarizeResponse> { |
| // Conditionally preserve the first message based on power steering setting | ||
| // When power steering is enabled, preserve the initial prompt to maintain task context | ||
| // When disabled, don't preserve it to prevent task restarts after condensing | ||
| const firstMessage = powerSteeringEnabled ? messages[0] : undefined |
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.
Could we simplify this logic for better readability?
| const firstMessage = powerSteeringEnabled ? messages[0] : undefined | |
| // Conditionally preserve the first message based on power steering setting | |
| // When power steering is enabled, preserve the initial prompt to maintain task context | |
| // When disabled, don't preserve it to prevent task restarts after condensing | |
| const preserveFirstMessage = powerSteeringEnabled ?? false | |
| const firstMessage = preserveFirstMessage ? messages[0] : undefined | |
| const messagesToKeepFrom = preserveFirstMessage ? 1 : 0 |
This makes the boolean logic more explicit and easier to follow.
| * @param {boolean} isAutomaticTrigger - Whether the summarization is triggered automatically | ||
| * @param {string} customCondensingPrompt - Optional custom prompt to use for condensing | ||
| * @param {ApiHandler} condensingApiHandler - Optional specific API handler to use for condensing | ||
| * @param {boolean} powerSteeringEnabled - Whether power steering is enabled (controls initial prompt preservation) |
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 JSDoc could be more explicit about the behavioral difference. Consider adding:
| * @param {boolean} powerSteeringEnabled - Whether power steering is enabled (controls initial prompt preservation) | |
| * @param {boolean} powerSteeringEnabled - Whether power steering is enabled (controls initial prompt preservation). | |
| * When true: preserves the first message to maintain task context. | |
| * When false: excludes first message to prevent task restarts after condensing. |
| defaultSystemPrompt, | ||
| taskId, | ||
| DEFAULT_PREV_CONTEXT_TOKENS, | ||
| false, |
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.
Consider adding edge case tests:
- Test behavior when
powerSteeringEnabledis explicitlyundefined - Test with empty message arrays to ensure no index errors
- Test with single message to verify boundary conditions
These would help ensure robustness of the first message preservation logic.
Summary
This PR fixes issue #8093 by making the initial prompt preservation in context condensing conditional on the experimental power-steering feature being enabled.
Problem
Previously, context condensing always preserved the initial prompt/first message, which caused tasks to be restarted unnecessarily when the preserved initial prompt was reintroduced after condensing.
Solution
powerSteeringEnabledparameter to thesummarizeConversationfunctionChanges
powerSteeringEnabledparameterTesting
Fixes #8093
Important
Make initial prompt preservation in context condensing conditional on the power-steering feature being enabled.
summarizeConversationfunction now includes apowerSteeringEnabledparameter to conditionally preserve the first message.Task.tsandsliding-window/index.tsto check the power steering flag before condensing.condense.spec.ts,index.spec.ts, andsliding-window.spec.tsupdated to handle thepowerSteeringEnabledparameter.sliding-window/index.ts.Task.tsusingexperiments.isEnabled.This description was created by
for 334d310. You can customize this summary. It will automatically update as commits are pushed.