From 1875dcd6b93bf73e5a3bb22622c207967dfaccb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Thu, 22 Oct 2026 02:09:37 +0000 Subject: [PATCH 1/2] fix(cloud-agent-sdk): validate question entries and reset retry and cleared-transcript state --- .../cloud-agent-sdk/src/normalizer.test.ts | 76 +++++++++++++++++-- packages/cloud-agent-sdk/src/normalizer.ts | 2 +- packages/cloud-agent-sdk/src/schemas.ts | 35 ++++++++- .../src/session-manager.test.ts | 71 +++++++++++++++++ .../cloud-agent-sdk/src/session-manager.ts | 14 +++- 5 files changed, 189 insertions(+), 9 deletions(-) diff --git a/packages/cloud-agent-sdk/src/normalizer.test.ts b/packages/cloud-agent-sdk/src/normalizer.test.ts index 76953e0e90e..353643f9dc3 100644 --- a/packages/cloud-agent-sdk/src/normalizer.test.ts +++ b/packages/cloud-agent-sdk/src/normalizer.test.ts @@ -967,27 +967,79 @@ describe('normalize', () => { }); describe('question.asked', () => { + const question = { + question: 'Continue?', + header: 'Approval', + options: [{ label: 'Yes', description: 'Run the command' }], + }; + it('extracts requestId and questions from tool.callID', () => { const data = { id: 'q-1', tool: { callID: 'call-1' }, - questions: [{ text: 'Continue?' }], + questions: [question], }; const result = normalize(createRaw('question.asked', data)); expect(result).toEqual({ type: 'question.asked', requestId: 'q-1', - questions: [{ text: 'Continue?' }], + questions: [question], }); }); it('works when tool is absent', () => { - const data = { id: 'q-1', questions: [{ text: 'Yes?' }] }; + const data = { id: 'q-1', questions: [question] }; + const result = normalize(createRaw('question.asked', data)); + expect(result).toEqual({ + type: 'question.asked', + requestId: 'q-1', + questions: [question], + }); + }); + + it('keeps the valid prefix and stops at the first malformed entry', () => { + const data = { + id: 'q-1', + questions: [question, { text: 'Continue?' }, question], + }; + const result = normalize(createRaw('question.asked', data)); + expect(result).toEqual({ + type: 'question.asked', + requestId: 'q-1', + questions: [question], + }); + }); + + it('keeps only the shared QuestionInfo fields', () => { + const data = { + id: 'q-1', + questions: [{ ...question, extra: 'ignored' }], + }; const result = normalize(createRaw('question.asked', data)); expect(result).toEqual({ type: 'question.asked', requestId: 'q-1', - questions: [{ text: 'Yes?' }], + questions: [question], + }); + }); + + it('treats a list whose first entry is malformed as carrying no questions', () => { + const data = { id: 'q-1', questions: [{ header: 'Approval' }, question] }; + const result = normalize(createRaw('question.asked', data)); + expect(result).toEqual({ + type: 'question.asked', + requestId: 'q-1', + questions: undefined, + }); + }); + + it('keeps an explicitly empty questions array', () => { + const data = { id: 'q-1', questions: [] }; + const result = normalize(createRaw('question.asked', data)); + expect(result).toEqual({ + type: 'question.asked', + requestId: 'q-1', + questions: [], }); }); @@ -2332,12 +2384,24 @@ describe('normalizeCliEvent', () => { const data = { id: 'q-1', tool: { callID: 'call-1' }, - questions: [{ text: 'Continue?' }], + questions: [ + { + question: 'Continue?', + header: 'Approval', + options: [{ label: 'Yes', description: '' }], + }, + ], }; expect(normalizeCliEvent('question.asked', data)).toEqual({ type: 'question.asked', requestId: 'q-1', - questions: [{ text: 'Continue?' }], + questions: [ + { + question: 'Continue?', + header: 'Approval', + options: [{ label: 'Yes', description: '' }], + }, + ], }); }); }); diff --git a/packages/cloud-agent-sdk/src/normalizer.ts b/packages/cloud-agent-sdk/src/normalizer.ts index 82721b91cb4..9b5b27b3b55 100644 --- a/packages/cloud-agent-sdk/src/normalizer.ts +++ b/packages/cloud-agent-sdk/src/normalizer.ts @@ -450,7 +450,7 @@ function normalizeInnerEvent(eventType: string, data: unknown): NormalizedEvent return { type: 'question.asked', requestId: r.data.id, - questions: r.data.questions as QuestionInfo[] | undefined, + questions: r.data.questions, }; } diff --git a/packages/cloud-agent-sdk/src/schemas.ts b/packages/cloud-agent-sdk/src/schemas.ts index 43494852dbd..ea86239b553 100644 --- a/packages/cloud-agent-sdk/src/schemas.ts +++ b/packages/cloud-agent-sdk/src/schemas.ts @@ -764,10 +764,43 @@ export const sessionQueueChangedDataSchema = z .passthrough(); export type SessionQueueChangedData = z.infer; +/** Runtime mirror of `QuestionInfo` in `@kilocode/app-shared/opencode`. */ +const questionInfoSchema = z.object({ + question: z.string(), + header: z.string(), + options: z.array( + z.object({ + label: z.string(), + description: z.string(), + mode: z.string().optional(), + }) + ), + multiple: z.boolean().optional(), + custom: z.boolean().optional(), +}); + +/** + * A question list is positional: the answer payload mirrors the CLI's question + * indices (`answerQuestion(requestId, answers: string[][])`). Keep the longest + * run of entries that match `QuestionInfo` and stop at the first entry that + * does not, so a rendered question keeps its CLI index instead of every later + * answer sliding onto the wrong question. No usable entry means the payload + * carries no questions the client can render. + */ +const questionListSchema = z.array(z.unknown()).transform(entries => { + const questions: z.infer[] = []; + for (const entry of entries) { + const parsed = questionInfoSchema.safeParse(entry); + if (!parsed.success) break; + questions.push(parsed.data); + } + return questions.length > 0 ? questions : entries.length > 0 ? undefined : []; +}); + export const questionAskedDataSchema = z.object({ id: z.string(), tool: z.object({ callID: z.string() }).optional(), - questions: z.array(z.unknown()).optional().catch(undefined), + questions: questionListSchema.optional().catch(undefined), }); export type QuestionAskedData = z.infer; diff --git a/packages/cloud-agent-sdk/src/session-manager.test.ts b/packages/cloud-agent-sdk/src/session-manager.test.ts index 0249fe53588..3ebfb20120b 100644 --- a/packages/cloud-agent-sdk/src/session-manager.test.ts +++ b/packages/cloud-agent-sdk/src/session-manager.test.ts @@ -6981,6 +6981,36 @@ describe('createSessionManager', () => { expect(mockSession.state.clearFailedMessage).toHaveBeenCalledWith('m1'); }); + it('clears the terminal error when the retried failure held it', async () => { + const config = createMockConfig(); + const mgr = createSessionManager(config); + + await switchAndCaptureSubscriber(config, mgr); + + config.store.set(mgr.atoms.error, 'The message could not be delivered'); + mockSession.state.clearFailedMessage.mockReturnValueOnce(true); + + mgr.clearFailedMessage('m1'); + + // The service undid the failure that reached `errorAtom` through + // `config.onError`, so the banner must not outlive the accepted retry. + expect(atomValue(config.store, mgr.atoms.error)).toBeNull(); + }); + + it('keeps the banner when the retried failure did not hold the error', async () => { + const config = createMockConfig(); + const mgr = createSessionManager(config); + + await switchAndCaptureSubscriber(config, mgr); + + config.store.set(mgr.atoms.error, 'Session terminated'); + mockSession.state.clearFailedMessage.mockReturnValueOnce(false); + + mgr.clearFailedMessage('m1'); + + expect(atomValue(config.store, mgr.atoms.error)).toBe('Session terminated'); + }); + it('deletes a client-materialised failed row so a retry cannot duplicate it', async () => { const config = createMockConfig(); const mgr = createSessionManager(config); @@ -7746,6 +7776,47 @@ describe('createSessionManager', () => { expect(atomValue(config.store, mgr.atoms.hasOlderMessages)).toBe(false); }); + it('resets the omitted-item accounting so a cleared view reports no omitted history', async () => { + const fetchSnapshotPage = createPageFetchMock(async (id, options) => { + if (id !== 'ses-1') return makePage({ kiloSessionId: id }); + if (!options.cursor) { + return makePage({ + kiloSessionId: id, + messages: [makePageMessage('msg-initial', 'ses-1', 'initial')], + nextCursor: 'cursor-A', + omittedItemCount: 2, + }); + } + return makePage({ + kiloSessionId: id, + messages: [makePageMessage('msg-older', 'ses-1', 'older')], + nextCursor: null, + omittedItemCount: 3, + }); + }); + const cached = new Map(); + const config = createMockConfig({ + fetchSnapshotPage, + writeCachedSnapshotPage: page => cached.set(page.info.id, page), + readCachedSnapshotPage: async id => cached.get(id) ?? null, + fetchSession: async id => ({ ...defaultFetchedSession, kiloSessionId: id }), + }); + const mgr = createSessionManager(config); + + await mgr.switchSession(kiloId('ses-1')); + await mgr.loadOlderMessages(); + expect(atomValue(config.store, mgr.atoms.olderMessagesOmittedItemCount)).toBe(5); + + mgr.clearTranscript(); + expect(atomValue(config.store, mgr.atoms.olderMessagesOmittedItemCount)).toBe(0); + + // Leaving caches the cleared view; a stale count here would repaint the + // omitted banner on the next open of this session. + await mgr.switchSession(kiloId('ses-2')); + expect(cached.get('ses-1')?.omittedItemCount).toBe(0); + mgr.destroy(); + }); + it('purges all replayed history on reconnect with no post-clear send', async () => { // E2E case 4: /clear → kill/reconnect, no send → view stays cleared. const fetchSnapshotPage = jest.fn().mockResolvedValue({ diff --git a/packages/cloud-agent-sdk/src/session-manager.ts b/packages/cloud-agent-sdk/src/session-manager.ts index 8aabcc3934c..c3861cc6269 100644 --- a/packages/cloud-agent-sdk/src/session-manager.ts +++ b/packages/cloud-agent-sdk/src/session-manager.ts @@ -3272,6 +3272,12 @@ function createSessionManager(config: SessionManagerConfig): SessionManager { currentSession.state.clearCommits(); olderMessagesCursor = null; store.set(hasOlderMessagesAtom, false); + // The cleared view holds nothing, so the omitted-history accounting starts + // from zero exactly as a non-preserved `clearAllAtoms` does: a stale count + // would keep the omitted banner up, and a stale initial-page baseline would + // subtract the wrong contribution from the next first page. + store.set(olderMessagesOmittedItemCountAtom, 0); + initialPageOmittedItemCount = 0; // Reset the retained-history stack so a later `trimRetainedHistory` // cannot restore a pre-clear cursor. retainedHistoryStack = []; @@ -3325,7 +3331,13 @@ function createSessionManager(config: SessionManagerConfig): SessionManager { if (owner === null) return; const ownerIsActive = owner === activeSessionId; if (ownerIsActive) { - currentSession?.state.clearFailedMessage(messageId); + // The service reports when the pruned entry held the terminal error and + // has undone it. That error reached `errorAtom` through `config.onError`, + // which the service state cannot reach, so a retry that resolves the + // failure must clear it here — the durable-seed path does the same. + if (currentSession?.state.clearFailedMessage(messageId)) { + store.set(errorAtom, null); + } // A client-materialised row is a local ghost: the accepted re-send // supersedes its content and materialises its own row, so leaving the // original would render the prompt twice — once for the failed From 06366b02123171584f479daa625bac20f002482f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Thu, 22 Oct 2026 02:19:00 +0000 Subject: [PATCH 2/2] fix(cloud-agent-sdk): type validated question entries as the shared question shape --- packages/cloud-agent-sdk/src/schemas.ts | 40 ++++++++++++++++++------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/packages/cloud-agent-sdk/src/schemas.ts b/packages/cloud-agent-sdk/src/schemas.ts index ea86239b553..f0b8f4d19fb 100644 --- a/packages/cloud-agent-sdk/src/schemas.ts +++ b/packages/cloud-agent-sdk/src/schemas.ts @@ -1,5 +1,6 @@ import * as z from 'zod'; import { sessionIdSchema } from '@kilocode/session-ingest-contracts'; +import type { QuestionInfo } from '@kilocode/app-shared/opencode'; import { sortRemoteModelCatalogProviders } from './remote-model-order'; import type { KiloSessionId, SlashCommandCatalogStatus } from './types'; @@ -765,20 +766,39 @@ export const sessionQueueChangedDataSchema = z export type SessionQueueChangedData = z.infer; /** Runtime mirror of `QuestionInfo` in `@kilocode/app-shared/opencode`. */ +const questionOptionSchema = z.object({ + label: z.string(), + description: z.string(), + mode: z.string().optional(), +}); + const questionInfoSchema = z.object({ question: z.string(), header: z.string(), - options: z.array( - z.object({ - label: z.string(), - description: z.string(), - mode: z.string().optional(), - }) - ), + options: z.array(questionOptionSchema), multiple: z.boolean().optional(), custom: z.boolean().optional(), }); +/** + * Convert a parsed entry to the shared type. Zod types an absent optional + * property as `T | undefined`, which `exactOptionalPropertyTypes` rejects + * against `QuestionInfo`, so the optionals are only added when present. + */ +function toQuestionInfo(parsed: z.infer): QuestionInfo { + return { + question: parsed.question, + header: parsed.header, + options: parsed.options.map(option => ({ + label: option.label, + description: option.description, + ...(option.mode === undefined ? {} : { mode: option.mode }), + })), + ...(parsed.multiple === undefined ? {} : { multiple: parsed.multiple }), + ...(parsed.custom === undefined ? {} : { custom: parsed.custom }), + }; +} + /** * A question list is positional: the answer payload mirrors the CLI's question * indices (`answerQuestion(requestId, answers: string[][])`). Keep the longest @@ -787,12 +807,12 @@ const questionInfoSchema = z.object({ * answer sliding onto the wrong question. No usable entry means the payload * carries no questions the client can render. */ -const questionListSchema = z.array(z.unknown()).transform(entries => { - const questions: z.infer[] = []; +const questionListSchema = z.array(z.unknown()).transform((entries): QuestionInfo[] | undefined => { + const questions: QuestionInfo[] = []; for (const entry of entries) { const parsed = questionInfoSchema.safeParse(entry); if (!parsed.success) break; - questions.push(parsed.data); + questions.push(toQuestionInfo(parsed.data)); } return questions.length > 0 ? questions : entries.length > 0 ? undefined : []; });