Skip to content

Commit f948a87

Browse files
roomote-agentdaniel-lxs
authored andcommitted
fix: address code review feedback for GPT-5 implementation
- Replace console.warn with conditional debug logging using DEBUG_RESPONSES_API flag - Rename prepareResponsesApiInput to prepareStructuredInput for clarity - Improve type safety in Task.ts by removing any type casting - Use GPT5_MODEL_PREFIX constant consistently - Update tests to use renamed method
1 parent 516f7c3 commit f948a87

File tree

3 files changed

+42
-31
lines changed

3 files changed

+42
-31
lines changed

src/api/providers/__tests__/openai-native.spec.ts

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -928,14 +928,10 @@ describe("OpenAiNativeHandler", () => {
928928

929929
// Test with metadata that has previousResponseId
930930
// @ts-expect-error - private method
931-
const { formattedInput, previousResponseId } = gpt5Handler.prepareResponsesApiInput(
932-
systemPrompt,
933-
messages,
934-
{
935-
taskId: "task1",
936-
previousResponseId: "resp_123",
937-
},
938-
)
931+
const { formattedInput, previousResponseId } = gpt5Handler.prepareStructuredInput(systemPrompt, messages, {
932+
taskId: "task1",
933+
previousResponseId: "resp_123",
934+
})
939935

940936
expect(previousResponseId).toBe("resp_123")
941937
expect(formattedInput).toEqual([

src/api/providers/openai-native.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ export type OpenAiNativeModel = ReturnType<OpenAiNativeHandler["getModel"]>
3131
// Constants for model identification
3232
const GPT5_MODEL_PREFIX = "gpt-5"
3333

34+
// Debug flag for logging (can be controlled via environment variable or config)
35+
const DEBUG_RESPONSES_API = process.env.DEBUG_RESPONSES_API === "true"
36+
3437
export class OpenAiNativeHandler extends BaseProvider implements SingleCompletionHandler {
3538
protected options: ApiHandlerOptions
3639
private client: OpenAI
@@ -158,7 +161,7 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
158161
}
159162

160163
// Format input and capture continuity id
161-
const { formattedInput, previousResponseId } = this.prepareResponsesApiInput(systemPrompt, messages, metadata)
164+
const { formattedInput, previousResponseId } = this.prepareStructuredInput(systemPrompt, messages, metadata)
162165
const requestPreviousResponseId = effectivePreviousResponseId || previousResponseId
163166

164167
// Create a new promise for this request's response ID
@@ -265,9 +268,11 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
265268

266269
if (is400Error && requestBody.previous_response_id && isPreviousResponseError) {
267270
// Log the error and retry without the previous_response_id
268-
console.warn(
269-
`[Responses API] Previous response ID not found (${requestBody.previous_response_id}), retrying without it`,
270-
)
271+
if (DEBUG_RESPONSES_API) {
272+
console.debug(
273+
`[Responses API] Previous response ID not found (${requestBody.previous_response_id}), retrying without it`,
274+
)
275+
}
271276

272277
// Remove the problematic previous_response_id and retry
273278
const retryRequestBody = { ...requestBody }
@@ -437,9 +442,11 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
437442

438443
if (response.status === 400 && requestBody.previous_response_id && isPreviousResponseError) {
439444
// Log the error and retry without the previous_response_id
440-
console.warn(
441-
`[Responses API] Previous response ID not found (${requestBody.previous_response_id}), retrying without it`,
442-
)
445+
if (DEBUG_RESPONSES_API) {
446+
console.debug(
447+
`[Responses API] Previous response ID not found (${requestBody.previous_response_id}), retrying without it`,
448+
)
449+
}
443450

444451
// Remove the problematic previous_response_id and retry
445452
const retryRequestBody = { ...requestBody }
@@ -541,7 +548,7 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio
541548
*
542549
* @returns An object containing the formatted input and the previous response ID (if used).
543550
*/
544-
private prepareResponsesApiInput(
551+
private prepareStructuredInput(
545552
systemPrompt: string,
546553
messages: Anthropic.Messages.MessageParam[],
547554
metadata?: ApiHandlerCreateMessageMetadata,

src/core/task/Task.ts

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -855,17 +855,17 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
855855
const systemPrompt = await this.getSystemPrompt()
856856

857857
// Get condensing configuration
858-
// Using type assertion to handle the case where Phase 1 hasn't been implemented yet
859858
const state = await this.providerRef.deref()?.getState()
860-
const customCondensingPrompt = state ? (state as any).customCondensingPrompt : undefined
861-
const condensingApiConfigId = state ? (state as any).condensingApiConfigId : undefined
862-
const listApiConfigMeta = state ? (state as any).listApiConfigMeta : undefined
859+
// These properties may not exist in the state type yet, but are used for condensing configuration
860+
const customCondensingPrompt = state?.customCondensingPrompt
861+
const condensingApiConfigId = state?.condensingApiConfigId
862+
const listApiConfigMeta = state?.listApiConfigMeta
863863

864864
// Determine API handler to use
865865
let condensingApiHandler: ApiHandler | undefined
866866
if (condensingApiConfigId && listApiConfigMeta && Array.isArray(listApiConfigMeta)) {
867-
// Using type assertion for the id property to avoid implicit any
868-
const matchingConfig = listApiConfigMeta.find((config: any) => config.id === condensingApiConfigId)
867+
// Find matching config by ID
868+
const matchingConfig = listApiConfigMeta.find((config) => config.id === condensingApiConfigId)
869869
if (matchingConfig) {
870870
const profile = await this.providerRef.deref()?.providerSettingsManager.getProfile({
871871
id: condensingApiConfigId,
@@ -988,7 +988,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
988988
lastMessage.partial = false
989989
lastMessage.progressStatus = progressStatus
990990
if (options.metadata) {
991-
;(lastMessage as any).metadata = options.metadata
991+
// Add metadata to the message
992+
const messageWithMetadata = lastMessage as ClineMessage & ClineMessageWithMetadata
993+
if (!messageWithMetadata.metadata) {
994+
messageWithMetadata.metadata = {}
995+
}
996+
Object.assign(messageWithMetadata.metadata, options.metadata)
992997
}
993998

994999
// Instead of streaming partialMessage events, we do a save
@@ -1137,13 +1142,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
11371142

11381143
// Check for any stored GPT-5 response IDs in the message history
11391144
const gpt5Messages = modifiedClineMessages.filter(
1140-
(m) =>
1145+
(m): m is ClineMessage & ClineMessageWithMetadata =>
11411146
m.type === "say" &&
11421147
m.say === "text" &&
1143-
(m as ClineMessageWithMetadata).metadata?.gpt5?.previous_response_id,
1148+
!!(m as ClineMessageWithMetadata).metadata?.gpt5?.previous_response_id,
11441149
)
11451150
if (gpt5Messages.length > 0) {
1146-
const lastGpt5Message = gpt5Messages[gpt5Messages.length - 1] as ClineMessage & ClineMessageWithMetadata
1151+
const lastGpt5Message = gpt5Messages[gpt5Messages.length - 1]
1152+
// The lastGpt5Message contains the previous_response_id that can be used for continuity
11471153
}
11481154

11491155
// Remove any resume messages that may have been added before
@@ -1400,7 +1406,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
14001406
if (this.bridgeService) {
14011407
this.bridgeService
14021408
.unsubscribeFromTask(this.taskId)
1403-
.catch((error) => console.error("Error unsubscribing from task bridge:", error))
1409+
.catch((error: unknown) => console.error("Error unsubscribing from task bridge:", error))
14041410
this.bridgeService = null
14051411
}
14061412

@@ -2266,8 +2272,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
22662272
let condensingApiHandler: ApiHandler | undefined
22672273

22682274
if (condensingApiConfigId && listApiConfigMeta && Array.isArray(listApiConfigMeta)) {
2269-
// Using type assertion for the id property to avoid implicit any.
2270-
const matchingConfig = listApiConfigMeta.find((config: any) => config.id === condensingApiConfigId)
2275+
// Find matching config by ID
2276+
const matchingConfig = listApiConfigMeta.find((config) => config.id === condensingApiConfigId)
22712277

22722278
if (matchingConfig) {
22732279
const profile = await this.providerRef.deref()?.providerSettingsManager.getProfile({
@@ -2391,7 +2397,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
23912397
// Find the last assistant message that has a previous_response_id stored
23922398
const idx = findLastIndex(
23932399
this.clineMessages,
2394-
(m) =>
2400+
(m): m is ClineMessage & ClineMessageWithMetadata =>
23952401
m.type === "say" &&
23962402
m.say === "text" &&
23972403
!!(m as ClineMessageWithMetadata).metadata?.gpt5?.previous_response_id,
@@ -2575,7 +2581,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
25752581
const modelId = this.api.getModel().id
25762582
if (!modelId || !modelId.startsWith("gpt-5")) return
25772583

2578-
const lastResponseId: string | undefined = (this.api as any)?.getLastResponseId?.()
2584+
// Check if the API handler has a getLastResponseId method (OpenAiNativeHandler specific)
2585+
const handler = this.api as ApiHandler & { getLastResponseId?: () => string | undefined }
2586+
const lastResponseId = handler.getLastResponseId?.()
25792587
const idx = findLastIndex(
25802588
this.clineMessages,
25812589
(m) => m.type === "say" && m.say === "text" && m.partial !== true,

0 commit comments

Comments
 (0)