Skip to content

Commit 4f6d178

Browse files
committed
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 52c6b8b commit 4f6d178

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
@@ -785,17 +785,17 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
785785
const systemPrompt = await this.getSystemPrompt()
786786

787787
// Get condensing configuration
788-
// Using type assertion to handle the case where Phase 1 hasn't been implemented yet
789788
const state = await this.providerRef.deref()?.getState()
790-
const customCondensingPrompt = state ? (state as any).customCondensingPrompt : undefined
791-
const condensingApiConfigId = state ? (state as any).condensingApiConfigId : undefined
792-
const listApiConfigMeta = state ? (state as any).listApiConfigMeta : undefined
789+
// These properties may not exist in the state type yet, but are used for condensing configuration
790+
const customCondensingPrompt = state?.customCondensingPrompt
791+
const condensingApiConfigId = state?.condensingApiConfigId
792+
const listApiConfigMeta = state?.listApiConfigMeta
793793

794794
// Determine API handler to use
795795
let condensingApiHandler: ApiHandler | undefined
796796
if (condensingApiConfigId && listApiConfigMeta && Array.isArray(listApiConfigMeta)) {
797-
// Using type assertion for the id property to avoid implicit any
798-
const matchingConfig = listApiConfigMeta.find((config: any) => config.id === condensingApiConfigId)
797+
// Find matching config by ID
798+
const matchingConfig = listApiConfigMeta.find((config) => config.id === condensingApiConfigId)
799799
if (matchingConfig) {
800800
const profile = await this.providerRef.deref()?.providerSettingsManager.getProfile({
801801
id: condensingApiConfigId,
@@ -918,7 +918,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
918918
lastMessage.partial = false
919919
lastMessage.progressStatus = progressStatus
920920
if (options.metadata) {
921-
;(lastMessage as any).metadata = options.metadata
921+
// Add metadata to the message
922+
const messageWithMetadata = lastMessage as ClineMessage & ClineMessageWithMetadata
923+
if (!messageWithMetadata.metadata) {
924+
messageWithMetadata.metadata = {}
925+
}
926+
Object.assign(messageWithMetadata.metadata, options.metadata)
922927
}
923928

924929
// Instead of streaming partialMessage events, we do a save
@@ -1068,13 +1073,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
10681073

10691074
// Check for any stored GPT-5 response IDs in the message history
10701075
const gpt5Messages = modifiedClineMessages.filter(
1071-
(m) =>
1076+
(m): m is ClineMessage & ClineMessageWithMetadata =>
10721077
m.type === "say" &&
10731078
m.say === "text" &&
1074-
(m as ClineMessageWithMetadata).metadata?.gpt5?.previous_response_id,
1079+
!!(m as ClineMessageWithMetadata).metadata?.gpt5?.previous_response_id,
10751080
)
10761081
if (gpt5Messages.length > 0) {
1077-
const lastGpt5Message = gpt5Messages[gpt5Messages.length - 1] as ClineMessage & ClineMessageWithMetadata
1082+
const lastGpt5Message = gpt5Messages[gpt5Messages.length - 1]
1083+
// The lastGpt5Message contains the previous_response_id that can be used for continuity
10781084
}
10791085

10801086
// Remove any resume messages that may have been added before
@@ -1324,7 +1330,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
13241330
if (this.bridgeService) {
13251331
this.bridgeService
13261332
.unsubscribeFromTask(this.taskId)
1327-
.catch((error) => console.error("Error unsubscribing from task bridge:", error))
1333+
.catch((error: unknown) => console.error("Error unsubscribing from task bridge:", error))
13281334
this.bridgeService = null
13291335
}
13301336

@@ -2016,8 +2022,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
20162022
let condensingApiHandler: ApiHandler | undefined
20172023

20182024
if (condensingApiConfigId && listApiConfigMeta && Array.isArray(listApiConfigMeta)) {
2019-
// Using type assertion for the id property to avoid implicit any.
2020-
const matchingConfig = listApiConfigMeta.find((config: any) => config.id === condensingApiConfigId)
2025+
// Find matching config by ID
2026+
const matchingConfig = listApiConfigMeta.find((config) => config.id === condensingApiConfigId)
20212027

20222028
if (matchingConfig) {
20232029
const profile = await this.providerRef.deref()?.providerSettingsManager.getProfile({
@@ -2141,7 +2147,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
21412147
// Find the last assistant message that has a previous_response_id stored
21422148
const idx = findLastIndex(
21432149
this.clineMessages,
2144-
(m) =>
2150+
(m): m is ClineMessage & ClineMessageWithMetadata =>
21452151
m.type === "say" &&
21462152
m.say === "text" &&
21472153
!!(m as ClineMessageWithMetadata).metadata?.gpt5?.previous_response_id,
@@ -2325,7 +2331,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
23252331
const modelId = this.api.getModel().id
23262332
if (!modelId || !modelId.startsWith("gpt-5")) return
23272333

2328-
const lastResponseId: string | undefined = (this.api as any)?.getLastResponseId?.()
2334+
// Check if the API handler has a getLastResponseId method (OpenAiNativeHandler specific)
2335+
const handler = this.api as ApiHandler & { getLastResponseId?: () => string | undefined }
2336+
const lastResponseId = handler.getLastResponseId?.()
23292337
const idx = findLastIndex(
23302338
this.clineMessages,
23312339
(m) => m.type === "say" && m.say === "text" && m.partial !== true,

0 commit comments

Comments
 (0)