Prevent Codex duplicate completions #8125
Conversation
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for your contribution! I've reviewed the changes and have some suggestions for improvement. The implementation looks solid overall - the duplicate detection mechanism using sharedPrefixLength is elegant and the tests provide good coverage of the main scenarios.
| session && | ||
| priorAssistantEntry && | ||
| priorAssistantEntry[0] >= previousProcessed - 1 && | ||
| /task completed/i.test(this.extractMessageText(priorAssistantEntry[1])) |
There was a problem hiding this comment.
Is this intentional that the session reset uses a case-insensitive regex /task completed/i? This could match unintended phrases like "subtask completed" or "task completed partially". Could we consider using a more specific pattern or exact match to avoid false positives?
| if (holdingChunks) { | ||
| holdingChunks = false | ||
| pendingChunks.push(sanitizedChunk) | ||
| for (const pending of pendingChunks.splice(0)) { |
There was a problem hiding this comment.
When holdingChunks is true and chunks are accumulated, the array is cleared with pendingChunks.splice(0). Could we consider using pendingChunks.length = 0 for better performance? It's a minor optimization but cleaner.
| if (chunk.type === "text") { | ||
| let text = chunk.text | ||
| if (hasPreviousAssistant && previousCursor < previousAssistantText.length && text) { | ||
| const expected = previousAssistantText.slice(previousCursor, previousCursor + text.length) |
There was a problem hiding this comment.
The duplicate detection logic is complex and crucial for this fix. Would it be helpful to add debug logging when duplicates are detected and trimmed? This could aid troubleshooting in production. For example, adding console.debug('Trimmed duplicate prefix of length:', overlap) when overlap > 0.
| const usageChunk = chunks.find((chunk) => chunk.type === "usage") | ||
| expect(usageChunk).toBeTruthy() | ||
| expect(fallbackSpy).not.toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
Great test coverage! Could we add an additional test case for partial duplicates? For example, when the Codex CLI returns only part of the previous message repeated. This would ensure the sharedPrefixLength logic handles all edge cases.
| taskId: "task-123", | ||
| } | ||
|
|
||
| const makeSession = (chunks: ApiStreamChunk[]) => ({ |
There was a problem hiding this comment.
The makeSession helper is useful. If other Codex-related tests need similar mocking in the future, would it be beneficial to extract this to a shared test utility file like __tests__/helpers/codex-mocks.ts?
1 participant
Summary
Testing