Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4026,6 +4026,74 @@ describe('useGeminiStream', () => {
expect(errorItem).toBeUndefined();
});
});

// Regression for #4169: when a pending retry error is cleared as the user
// starts a new turn, the error must be committed to the persistent
// history first — otherwise running /status (or any new turn) silently
// discards the failure the user was investigating.
it('commits pending retry error to history (without hint) when a new query starts', async () => {
mockSendMessageStream.mockReturnValueOnce(
(async function* () {
yield {
type: ServerGeminiEventType.Error,
value: { error: { message: 'First error' } },
};
})(),
);

const { result } = renderTestHook();

await act(async () => {
await result.current.submitQuery('First query');
});

await waitFor(() => {
const errorItem = result.current.pendingHistoryItems.find(
(item) => item.type === 'error',
);
expect(errorItem).toBeDefined();
});

// Sanity check: the error has NOT yet been committed to history while
// it lives as a pending retry item.
expect(mockAddItem).not.toHaveBeenCalledWith(
expect.objectContaining({ type: 'error' }),
expect.any(Number),
);

mockSendMessageStream.mockReturnValueOnce(
(async function* () {
yield {
type: ServerGeminiEventType.Content,
value: 'Second response',
};
})(),
);

await act(async () => {
await result.current.submitQuery('Second query');
});

// The pending error is now committed to history…
await waitFor(() => {
expect(mockAddItem).toHaveBeenCalledWith(
expect.objectContaining({ type: 'error' }),
expect.any(Number),
);
});

// …and the retry hint is stripped, since it is no longer actionable.
const errorCommit = mockAddItem.mock.calls.find(
([item]) => item && typeof item === 'object' && item.type === 'error',
);
expect(errorCommit?.[0]).not.toHaveProperty('hint');

// The pending region is cleared, as before.
const errorItem = result.current.pendingHistoryItems.find(
(item) => item.type === 'error',
);
expect(errorItem).toBeUndefined();
});
});

describe('Concurrent Execution Prevention', () => {
Expand Down
5 changes: 5 additions & 0 deletions packages/cli/src/ui/hooks/useGeminiStream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1688,6 +1688,11 @@ export const useGeminiStream = (
pendingRetryCountdownItemRef.current ||
pendingRetryErrorItemRef.current
) {
const pendingError = pendingRetryErrorItemRef.current;
if (pendingError && pendingError.type === 'error') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defensive narrow looks right. pendingRetryErrorItemRef is typed HistoryItemWithoutId | null, and the three set-sites (:486-490, :1091-1095, :1880-1887) all produce type: 'error' items today — but the discriminator guard keeps this robust if a future set-site lands a different variant, and HistoryItemError declares hint?: string in types.ts so the rest-spread is type-safe with no hint property in the persisted shape. No change requested; flagging because it's a thoughtful detail.

const { hint: _hint, ...errorWithoutHint } = pendingError;
addItem(errorWithoutHint, userMessageTimestamp);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — timestamp semantics worth a one-liner.

userMessageTimestamp here is Date.now() from line 1670, i.e. the START of the new turn — not when the original API failure actually happened. That's fine and probably the only viable choice (the pending ref doesn't carry the original timestamp), but at a glance a future maintainer might read this as a bug and try to "fix" the drift.

Suggest a short comment so the intent is explicit:

Suggested change
addItem(errorWithoutHint, userMessageTimestamp);
// Reuse the new turn's timestamp — the pending ref doesn't
// carry the original failure time, and this keeps the
// committed error visually adjacent to the user input that
// displaced it (e.g. `/status`).
addItem(errorWithoutHint, userMessageTimestamp);

}
clearRetryCountdown();
}
}
Expand Down
Loading