Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: make initial prompt preservation conditional on power-steering feature #8094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
72 changes: 66 additions & 6 deletions src/core/condense/__tests__/condense.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,17 @@ describe("Condense", () => {
{ role: "user", content: "Ninth message" },
]

const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
const result = await summarizeConversation(
messages,
mockApiHandler,
"System prompt",
taskId,
5000,
false,
undefined,
undefined,
true,
) // powerSteeringEnabled = true to preserve first message

// Verify the first message is preserved
expect(result.messages[0]).toEqual(messages[0])
Expand Down Expand Up @@ -106,7 +116,17 @@ describe("Condense", () => {
{ role: "user", content: "Thanks!" },
]

const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
const result = await summarizeConversation(
messages,
mockApiHandler,
"System prompt",
taskId,
5000,
false,
undefined,
undefined,
true,
) // powerSteeringEnabled = true to preserve first message

// The first message with slash command should be intact
expect(result.messages[0].content).toBe(slashCommandContent)
Expand All @@ -131,7 +151,17 @@ describe("Condense", () => {
{ role: "user", content: "Perfect!" },
]

const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
const result = await summarizeConversation(
messages,
mockApiHandler,
"System prompt",
taskId,
5000,
false,
undefined,
undefined,
true,
) // powerSteeringEnabled = true to preserve first message

// The first message with complex content should be preserved
expect(result.messages[0].content).toEqual(complexContent)
Expand All @@ -146,7 +176,17 @@ describe("Condense", () => {
{ role: "assistant", content: "Fourth message" },
]

const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
const result = await summarizeConversation(
messages,
mockApiHandler,
"System prompt",
taskId,
5000,
false,
undefined,
undefined,
false,
)

// Should return an error since we have only 4 messages (first + 3 to keep)
expect(result.error).toBeDefined()
Expand All @@ -165,7 +205,17 @@ describe("Condense", () => {
{ role: "user", content: "Final message" },
]

const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
const result = await summarizeConversation(
messages,
mockApiHandler,
"System prompt",
taskId,
5000,
false,
undefined,
undefined,
false,
)

// Should return an error due to recent summary in last N messages
expect(result.error).toBeDefined()
Expand Down Expand Up @@ -198,7 +248,17 @@ describe("Condense", () => {
{ role: "user", content: "Seventh" },
]

const result = await summarizeConversation(messages, emptyHandler, "System prompt", taskId, 5000, false)
const result = await summarizeConversation(
messages,
emptyHandler,
"System prompt",
taskId,
5000,
false,
undefined,
undefined,
false,
)

expect(result.error).toBeDefined()
expect(result.messages).toEqual(messages)
Expand Down
75 changes: 66 additions & 9 deletions src/core/condense/__tests__/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

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:

  1. Test behavior when powerSteeringEnabled is explicitly undefined
  2. Test with empty message arrays to ensure no index errors
  3. Test with single message to verify boundary conditions

These would help ensure robustness of the first message preservation logic.

undefined,
undefined,
false, // powerSteeringEnabled
)
expect(result.messages).toEqual(messages)
expect(result.cost).toBe(0)
Expand All @@ -155,6 +159,10 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)
expect(result.messages).toEqual(messages)
expect(result.cost).toBe(0)
Expand All @@ -181,21 +189,22 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Check that the API was called correctly
expect(mockApiHandler.createMessage).toHaveBeenCalled()
expect(maybeRemoveImageBlocks).toHaveBeenCalled()

// Verify the structure of the result
// The result should be: first message + summary + last N messages
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP) // First + summary + last N

// Check that the first message is preserved
expect(result.messages[0]).toEqual(messages[0])
// When powerSteeringEnabled is false, the result should be: summary + last N messages
expect(result.messages.length).toBe(1 + N_MESSAGES_TO_KEEP) // summary + last N

// Check that the summary message was inserted correctly
const summaryMessage = result.messages[1]
const summaryMessage = result.messages[0]
expect(summaryMessage.role).toBe("assistant")
expect(summaryMessage.content).toBe("This is a summary")
expect(summaryMessage.isSummary).toBe(true)
Expand Down Expand Up @@ -244,6 +253,10 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Should return original messages when summary is empty
Expand All @@ -265,7 +278,17 @@ describe("summarizeConversation", () => {
{ role: "user", content: "Tell me more", ts: 7 },
]

await summarizeConversation(messages, mockApiHandler, defaultSystemPrompt, taskId, DEFAULT_PREV_CONTEXT_TOKENS)
await summarizeConversation(
messages,
mockApiHandler,
defaultSystemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined,
undefined,
false,
)

// Verify the final request message
const expectedFinalMessage = {
Expand Down Expand Up @@ -312,6 +335,10 @@ describe("summarizeConversation", () => {
systemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Verify that countTokens was called with the correct messages including system prompt
Expand Down Expand Up @@ -355,6 +382,10 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
prevContextTokens,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Should return original messages when context would grow
Expand Down Expand Up @@ -395,11 +426,15 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
prevContextTokens,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Should successfully summarize
// Result should be: first message + summary + last N messages
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP) // First + summary + last N
// When powerSteeringEnabled is false, the result should be: summary + last N messages
expect(result.messages.length).toBe(1 + N_MESSAGES_TO_KEEP) // summary + last N
expect(result.cost).toBe(0.03)
expect(result.summary).toBe("Concise summary")
expect(result.error).toBeUndefined()
Expand All @@ -416,6 +451,10 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Should return original messages when not enough to summarize
Expand Down Expand Up @@ -444,6 +483,10 @@ describe("summarizeConversation", () => {
defaultSystemPrompt,
taskId,
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Should return original messages when recent summary exists
Expand Down Expand Up @@ -493,6 +536,7 @@ describe("summarizeConversation", () => {
false,
undefined,
invalidCondensingHandler,
false, // powerSteeringEnabled
)

// Should return original messages when both handlers are invalid
Expand Down Expand Up @@ -601,6 +645,8 @@ describe("summarizeConversation with custom settings", () => {
DEFAULT_PREV_CONTEXT_TOKENS,
false,
customPrompt,
undefined,
false, // powerSteeringEnabled
)

// Verify the custom prompt was used
Expand All @@ -622,6 +668,8 @@ describe("summarizeConversation with custom settings", () => {
DEFAULT_PREV_CONTEXT_TOKENS,
false,
" ", // Empty custom prompt
undefined,
false, // powerSteeringEnabled
)

// Verify the default prompt was used
Expand All @@ -639,6 +687,8 @@ describe("summarizeConversation with custom settings", () => {
DEFAULT_PREV_CONTEXT_TOKENS,
false,
undefined, // No custom prompt
undefined,
false, // powerSteeringEnabled
)

// Verify the default prompt was used again
Expand All @@ -660,6 +710,7 @@ describe("summarizeConversation with custom settings", () => {
false,
undefined,
mockCondensingApiHandler,
false, // powerSteeringEnabled
)

// Verify the condensing handler was used
Expand All @@ -680,6 +731,7 @@ describe("summarizeConversation with custom settings", () => {
false,
undefined,
undefined,
false, // powerSteeringEnabled
)

// Verify the main handler was used
Expand Down Expand Up @@ -711,6 +763,7 @@ describe("summarizeConversation with custom settings", () => {
false,
undefined,
invalidHandler,
false, // powerSteeringEnabled
)

// Verify the main handler was used as fallback
Expand All @@ -737,6 +790,8 @@ describe("summarizeConversation with custom settings", () => {
DEFAULT_PREV_CONTEXT_TOKENS,
false,
"Custom prompt",
undefined,
false, // powerSteeringEnabled
)

// Verify telemetry was called with custom prompt flag
Expand All @@ -761,6 +816,7 @@ describe("summarizeConversation with custom settings", () => {
false,
undefined,
mockCondensingApiHandler,
false, // powerSteeringEnabled
)

// Verify telemetry was called with custom API handler flag
Expand All @@ -785,6 +841,7 @@ describe("summarizeConversation with custom settings", () => {
true, // isAutomaticTrigger
"Custom prompt",
mockCondensingApiHandler,
false, // powerSteeringEnabled
)

// Verify telemetry was called with both flags
Expand Down
20 changes: 14 additions & 6 deletions src/core/condense/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export type SummarizeResponse = {
* @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)
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

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:

Suggested change
* @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.

* @returns {SummarizeResponse} - The result of the summarization operation (see above)
*/
export async function summarizeConversation(
Expand All @@ -91,6 +92,7 @@ export async function summarizeConversation(
isAutomaticTrigger?: boolean,
customCondensingPrompt?: string,
condensingApiHandler?: ApiHandler,
powerSteeringEnabled?: boolean,
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

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:

Suggested change
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> {

): Promise<SummarizeResponse> {
TelemetryService.instance.captureContextCondensed(
taskId,
Expand All @@ -101,10 +103,14 @@ export async function summarizeConversation(

const response: SummarizeResponse = { messages, cost: 0, summary: "" }

// Always preserve the first message (which may contain slash command content)
const firstMessage = messages[0]
// Get messages to summarize, excluding the first message and last N messages
const messagesToSummarize = getMessagesSinceLastSummary(messages.slice(1, -N_MESSAGES_TO_KEEP))
// 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
Copy link
Contributor Author

@roomote roomote bot Sep 17, 2025

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?

Suggested change
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.

const messagesToKeepFrom = powerSteeringEnabled ? 1 : 0

// Get messages to summarize, excluding the conditionally preserved first message and last N messages
const messagesToSummarize = getMessagesSinceLastSummary(messages.slice(messagesToKeepFrom, -N_MESSAGES_TO_KEEP))

if (messagesToSummarize.length <= 1) {
const error =
Expand Down Expand Up @@ -188,8 +194,10 @@ export async function summarizeConversation(
isSummary: true,
}

// Reconstruct messages: [first message, summary, last N messages]
const newMessages = [firstMessage, summaryMessage, ...keepMessages]
// Reconstruct messages: [first message (if power steering enabled), summary, last N messages]
const newMessages = firstMessage
? [firstMessage, summaryMessage, ...keepMessages]
: [summaryMessage, ...keepMessages]

// Count the tokens in the context for the next API request
// We only estimate the tokens in summaryMesage if outputTokens is 0, otherwise we use outputTokens
Expand Down
Loading
Loading