-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: preserve context when Orchestrator calls same mode multiple times #7132
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
- Add subtaskContextByMode Map to track completion results by mode - Enhance new task messages with context from previous subtasks of same mode - Store subtask results in resumePausedTask for future reference - Add comprehensive tests for context preservation functionality Fixes #7131
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.
| contexts.shift() | ||
| } | ||
| // Extract a concise summary from the result message | ||
| const summary = lastMessage.length > 200 ? lastMessage.substring(0, 200) + "..." : lastMessage |
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 200 characters intentional for the context truncation? This seems quite aggressive for complex subtask results. Consider increasing to 500 characters or making it configurable to preserve more meaningful context.
| // Store the subtask result as context for future calls to the same mode | ||
| if (this.currentSubtaskMode && lastMessage) { | ||
| if (!this.subtaskContextByMode) { | ||
| this.subtaskContextByMode = new Map() |
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 error handling for Map operations. While unlikely, memory constraints with very large context histories could cause issues. A try-catch block here would make the code more robust.
| isPaused: boolean = false | ||
| pausedModeSlug: string = defaultModeSlug | ||
| private pauseInterval: NodeJS.Timeout | undefined | ||
| subtaskContextByMode?: Map<string, string[]> |
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.
These new properties would benefit from JSDoc comments explaining their purpose and lifecycle. Consider adding:
| ) | ||
| }) | ||
|
|
||
| describe("context 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.
Good test coverage! Consider adding edge case tests for:
- Verifying FIFO behavior when context exceeds 3 items
- Handling empty or null result messages
- Behavior with very long context messages that get truncated
These would make the test suite even more robust.
Summary
This PR fixes issue #7131 where the Orchestrator mode incorrectly assumes that subsequent calls to the same mode (particularly Architect mode) retain context from previous calls. Each new task was starting with a clean slate, causing confusion when the Orchestrator tried to reference previous work.
Problem
When the Orchestrator mode calls the Architect mode multiple times:
The second call would fail because the Architect had no memory of the previous task.
Solution
Implemented context preservation by:
subtaskContextByModeMap to the Task class to track completion results by modenewTaskToolto include context from previous subtasks of the same moderesumePausedTaskto store subtask results for future referenceChanges
src/core/tools/newTaskTool.tsto prepend context from previous same-mode subtaskssrc/core/task/Task.tsto track and store subtask contextsrc/core/tools/__tests__/newTaskTool.spec.tsTesting
Example
When Orchestrator calls Architect mode multiple times, the second call now receives:
Fixes #7131
Important
Fixes context loss in Orchestrator by preserving subtask results across multiple calls to the same mode.
subtaskContextByModeandcurrentSubtaskModetoTaskclass inTask.ts.resumePausedTask()inTask.tsto store subtask results.newTaskTool()innewTaskTool.tsto prepend context to messages.newTaskTool.spec.tsfor context preservation across multiple mode calls.This description was created by
for 34e0746. You can customize this summary. It will automatically update as commits are pushed.