-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: skip previousResponseId for GPT-5 after subtask completion #7253
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
- When a subtask completes and returns to the parent task, the conversation continuity for GPT-5 responses API was broken because the previous_response_id was still being used - Added logic to set skipPrevResponseIdOnce flag when resuming from a subtask with GPT-5 - This ensures the next API call after subtask completion uses full conversation history - Added comprehensive tests to verify the fix works for GPT-5 and doesn't affect other models Fixes #7251
src/core/task/Task.ts
Outdated
| // for the next API call after a subtask completes, similar to what happens after | ||
| // a condense operation. This ensures the conversation continuity is properly maintained. | ||
| const modelId = this.api.getModel().id | ||
| if (modelId && modelId.startsWith("gpt-5")) { |
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 resumePausedTask method correctly checks if the model ID starts with 'gpt-5' and sets the skipPrevResponseIdOnce flag. Consider extracting this model check into a helper (e.g. isGpt5Model) to improve clarity and reuse.
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 is like looking in a mirror and realizing I'm just a recursive function with no base case.
src/core/task/Task.ts
Outdated
| // for the next API call after a subtask completes, similar to what happens after | ||
| // a condense operation. This ensures the conversation continuity is properly maintained. | ||
| const modelId = this.api.getModel().id | ||
| if (modelId && modelId.startsWith("gpt-5")) { |
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 extracting this GPT-5 check logic into a utility function like isGpt5Model(modelId) for better maintainability. This pattern might be needed elsewhere in the codebase as GPT-5 adoption grows.
src/core/task/Task.ts
Outdated
| content: [{ type: "text", text: `[new_task completed] Result: ${lastMessage}` }], | ||
| }) | ||
|
|
||
| // When using GPT-5 with the responses API, we need to skip the previous_response_id |
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.
Nice work following the existing pattern from condense operations! The inline comment clearly explains why this is necessary for GPT-5.
| })) | ||
|
|
||
| describe("Cline", () => { | ||
| describe("Task", () => { |
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.
Good test coverage! These tests verify the flag is set correctly for GPT-5, non-GPT-5, and error scenarios. Consider adding an integration test (if feasible) to verify the actual behavior in attemptApiRequest where this flag is consumed.
|
@roomote-agent i think the problem is valid for all OpenAI models when using responses api, can you please check? |
|
Hi @IngLP! I'm investigating whether this issue affects all OpenAI models when using the responses API. Let me check the codebase and verify the scope of the problem. |
… models - Updated logic to check for both GPT-5 and Codex Mini models - These are the models that use the OpenAI Responses API endpoint - Added test coverage for Codex Mini model - Updated comments and log messages to reflect broader scope This ensures proper conversation continuity for all OpenAI models using the Responses API, not just GPT-5.
|
Hi @IngLP! You're absolutely right - this issue affects all OpenAI models that use the Responses API, not just GPT-5. I've updated the PR to extend the fix to cover both GPT-5 and Codex Mini models (which are the models that use the OpenAI Responses API endpoint). Here's what I changed: Changes Made:
The fix ensures proper conversation continuity for all OpenAI models using the Responses API by skipping the All tests are passing locally. The CI checks are currently running and should complete shortly. Thanks for catching this! |
|
This doesn't seem right, closing |
Summary
This PR fixes an issue where subtask results are not properly added to the conversation history when using the OpenAI responses API (GPT-5).
Problem
When a subtask completes and returns to the parent task, the GPT-5 responses API was receiving an invalid
previous_response_idbecause:previous_response_idfrom its last GPT-5 responseprevious_response_idSolution
Added logic to skip the
previous_response_idfor the next API call after a subtask completes when using GPT-5. This is similar to what happens after a condense operation.The fix:
resumePausedTask()skipPrevResponseIdOnceflag to trueTesting
Fixes #7251
cc @IngLP
Important
Fixes
previous_response_idhandling for GPT-5 and Codex Mini models inTask.tsto maintain conversation continuity after subtask completion.Task.ts,resumePausedTask()now skipsprevious_response_idfor GPT-5 and Codex Mini models after subtask completion.previous_response_idfor these models.Task.spec.tsto verify behavior for GPT-5 and Codex Mini models.previous_response_idis skipped for these models and not for others.Task.ts.This description was created by
for e23047f. You can customize this summary. It will automatically update as commits are pushed.