From 3ad614bc284e1390b25925bcf9ad2d9e02804198 Mon Sep 17 00:00:00 2001 From: Sun-sunshine06 Date: Fri, 18 Sep 2026 00:30:43 +0800 Subject: [PATCH 1/2] fix: preserve completion diagnostics and recover truncated briefs Signed-off-by: Sun-sunshine06 --- .changeset/reliable-completion-feedback.md | 6 ++ packages/core/src/design-context.test.ts | 105 ++++++++++++++++++++- packages/core/src/design-context.ts | 68 ++++++++----- packages/providers/src/index.test.ts | 11 +-- packages/providers/src/index.ts | 27 +++++- 5 files changed, 181 insertions(+), 36 deletions(-) create mode 100644 .changeset/reliable-completion-feedback.md diff --git a/.changeset/reliable-completion-feedback.md b/.changeset/reliable-completion-feedback.md new file mode 100644 index 00000000..847e669c --- /dev/null +++ b/.changeset/reliable-completion-feedback.md @@ -0,0 +1,6 @@ +--- +"@open-codesign/core": patch +"@open-codesign/providers": patch +--- + +Recover a truncated design-session brief with one bounded larger-output retry, retaining the previous brief on failure and including reported usage from the truncated attempt. Keep length-stop errors typed without accepting partial completion content. diff --git a/packages/core/src/design-context.test.ts b/packages/core/src/design-context.test.ts index 8073a775..1f809187 100644 --- a/packages/core/src/design-context.test.ts +++ b/packages/core/src/design-context.test.ts @@ -1,13 +1,18 @@ import type { AgentMessage } from '@mariozechner/pi-agent-core'; +import { CompletionLengthError } from '@open-codesign/providers'; import type { ChatMessageRow, ChatToolCallPayload, ModelRef, ResourceStateV1, } from '@open-codesign/shared'; -import { describe, expect, it, vi } from 'vitest'; +import { CodesignError, ERROR_CODES } from '@open-codesign/shared'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; const completeWithRetryMock = vi.fn(); +beforeEach(() => { + completeWithRetryMock.mockReset(); +}); vi.mock('@open-codesign/providers', async () => { const actual = await vi.importActual( @@ -391,6 +396,104 @@ describe('updateDesignSessionBrief', () => { expect(completeWithRetryMock.mock.calls[0]?.[1][1].content).toContain('## Workspace MEMORY.md'); }); + it('retries a length stop once with a larger bounded budget and counts both attempts', async () => { + completeWithRetryMock + .mockRejectedValueOnce( + new CompletionLengthError({ inputTokens: 10, outputTokens: 2000, costUsd: 0.01 }), + ) + .mockResolvedValueOnce({ + content: JSON.stringify(baseBrief()), + inputTokens: 12, + outputTokens: 400, + costUsd: 0.02, + }); + const result = await updateDesignSessionBrief({ + existingBrief: baseBrief(), + conversationMessages: [agentUser('Make it warmer')], + designId: 'design-1', + designName: 'Onboarding', + model: MODEL, + apiKey: 'sk-test', + wire: 'openai-chat', + baseUrl: 'https://gateway.example/v1', + httpHeaders: { 'X-Test': 'brief' }, + }); + expect(completeWithRetryMock).toHaveBeenCalledTimes(2); + expect(completeWithRetryMock.mock.calls[0]?.[2].maxTokens).toBe(2000); + expect(completeWithRetryMock.mock.calls[1]?.[2]).toMatchObject({ + maxTokens: 4000, + wire: 'openai-chat', + baseUrl: 'https://gateway.example/v1', + apiKey: 'sk-test', + httpHeaders: { 'X-Test': 'brief' }, + }); + expect(result).toMatchObject({ inputTokens: 22, outputTokens: 2400, costUsd: 0.03 }); + }); + + it('stops after a second length error without mutating the previous brief', async () => { + const previous = baseBrief(); + const original = structuredClone(previous); + const failure = new CompletionLengthError({ + inputTokens: 10, + outputTokens: 4000, + costUsd: 0.01, + }); + completeWithRetryMock.mockRejectedValue(failure); + await expect( + updateDesignSessionBrief({ + existingBrief: previous, + conversationMessages: [], + designId: 'design-1', + designName: 'Onboarding', + model: MODEL, + apiKey: 'sk-test', + }), + ).rejects.toBe(failure); + expect(completeWithRetryMock).toHaveBeenCalledTimes(2); + expect(previous).toEqual(original); + }); + + it.each([ + new CodesignError('Authentication failed', ERROR_CODES.PROVIDER_ERROR), + new CodesignError('Aborted', ERROR_CODES.PROVIDER_ABORTED), + new Error('A gateway mentioned a token limit'), + ])('does not treat other failures as output truncation: %s', async (failure) => { + completeWithRetryMock.mockRejectedValue(failure); + await expect( + updateDesignSessionBrief({ + existingBrief: baseBrief(), + conversationMessages: [], + designId: 'design-1', + designName: 'Onboarding', + model: MODEL, + apiKey: 'sk-test', + }), + ).rejects.toBe(failure); + expect(completeWithRetryMock).toHaveBeenCalledTimes(1); + }); + + it('rejects invalid JSON after truncation recovery without replacing the previous brief', async () => { + const previous = baseBrief(); + const original = structuredClone(previous); + completeWithRetryMock + .mockRejectedValueOnce( + new CompletionLengthError({ inputTokens: 1, outputTokens: 2000, costUsd: 0 }), + ) + .mockResolvedValueOnce({ content: '{"goal":', inputTokens: 1, outputTokens: 1, costUsd: 0 }); + await expect( + updateDesignSessionBrief({ + existingBrief: previous, + conversationMessages: [], + designId: 'design-1', + designName: 'Onboarding', + model: MODEL, + apiKey: 'sk-test', + }), + ).rejects.toThrow(/valid JSON/); + expect(completeWithRetryMock).toHaveBeenCalledTimes(2); + expect(previous).toEqual(original); + }); + it('rejects invalid JSON so callers keep the previous brief', async () => { completeWithRetryMock.mockResolvedValueOnce({ content: 'not json', diff --git a/packages/core/src/design-context.ts b/packages/core/src/design-context.ts index 491a281c..ed90ec55 100644 --- a/packages/core/src/design-context.ts +++ b/packages/core/src/design-context.ts @@ -1,5 +1,9 @@ import type { AgentMessage } from '@mariozechner/pi-agent-core'; -import { completeWithRetry } from '@open-codesign/providers'; +import { + CompletionLengthError, + completeWithRetry, + type GenerateResult, +} from '@open-codesign/providers'; import type { ChatMessage, ChatMessageRow, @@ -108,6 +112,7 @@ const BRIEF_MAX_ARRAY_ITEMS = 12; const BRIEF_MAX_FIELD_CHARS = 1_200; const BRIEF_MAX_ITEM_CHARS = 240; const BRIEF_MAX_OUTPUT_TOKENS = 2_000; +const BRIEF_RETRY_OUTPUT_TOKENS = 4_000; export const DESIGN_BRIEF_SYSTEM_PROMPT = [ 'You maintain a compact structured brief for one Open CoDesign design session.', @@ -135,7 +140,8 @@ export const DESIGN_BRIEF_SYSTEM_PROMPT = [ '- Do not copy large source code, tool outputs, or full token tables.', '- Treat DESIGN.md as authoritative when mentioned; summarize decisions, not raw tokens.', "- Use the same language as the user's prompts when practical.", - '- Keep the whole JSON compact enough for a prompt brief.', + '- Keep the whole JSON below 600 words; use at most five short items per array.', + '- Use one short sentence per string field. Omit repetition and implementation details.', ].join('\n'); function isRecord(value: unknown): value is Record { @@ -493,24 +499,40 @@ export async function updateDesignSessionBrief( conversationLen: conversation.length, }); try { - const result = await completeWithRetry( - input.model, - messages, - { - apiKey: input.apiKey, - ...(input.baseUrl !== undefined ? { baseUrl: input.baseUrl } : {}), - ...(input.wire !== undefined ? { wire: input.wire } : {}), - ...(input.httpHeaders !== undefined ? { httpHeaders: input.httpHeaders } : {}), - ...(input.allowKeyless === true ? { allowKeyless: true } : {}), - ...(input.reasoningLevel !== undefined ? { reasoning: input.reasoningLevel } : {}), - maxTokens: BRIEF_MAX_OUTPUT_TOKENS, - }, - { - logger: log, - provider: input.model.provider, - ...(input.wire !== undefined ? { wire: input.wire } : {}), - }, - ); + const summarize = (maxTokens: number) => + completeWithRetry( + input.model, + messages, + { + apiKey: input.apiKey, + ...(input.baseUrl !== undefined ? { baseUrl: input.baseUrl } : {}), + ...(input.wire !== undefined ? { wire: input.wire } : {}), + ...(input.httpHeaders !== undefined ? { httpHeaders: input.httpHeaders } : {}), + ...(input.allowKeyless === true ? { allowKeyless: true } : {}), + ...(input.reasoningLevel !== undefined ? { reasoning: input.reasoningLevel } : {}), + maxTokens, + }, + { + logger: log, + provider: input.model.provider, + ...(input.wire !== undefined ? { wire: input.wire } : {}), + }, + ); + let result: GenerateResult; + let truncatedUsage: Omit | undefined; + try { + result = await summarize(BRIEF_MAX_OUTPUT_TOKENS); + } catch (err) { + if (!(err instanceof CompletionLengthError)) throw err; + truncatedUsage = err.usage; + log.info('[design-brief] step=summarize.retry-length', { + designId: input.designId, + maxTokens: BRIEF_RETRY_OUTPUT_TOKENS, + }); + // One bounded retry only. Never persist partial JSON or replace the + // previous brief when either completion/validation attempt fails. + result = await summarize(BRIEF_RETRY_OUTPUT_TOKENS); + } let parsed: unknown; try { parsed = JSON.parse(stripJsonFence(result.content)) as unknown; @@ -534,9 +556,9 @@ export async function updateDesignSessionBrief( }); return { brief, - inputTokens: result.inputTokens, - outputTokens: result.outputTokens, - costUsd: result.costUsd, + inputTokens: result.inputTokens + (truncatedUsage?.inputTokens ?? 0), + outputTokens: result.outputTokens + (truncatedUsage?.outputTokens ?? 0), + costUsd: result.costUsd + (truncatedUsage?.costUsd ?? 0), }; } catch (err) { log.warn('[design-brief] step=summarize.fail', { diff --git a/packages/providers/src/index.test.ts b/packages/providers/src/index.test.ts index ddb4bfe2..d6462f1f 100644 --- a/packages/providers/src/index.test.ts +++ b/packages/providers/src/index.test.ts @@ -1,9 +1,4 @@ -import { - type ChatMessage, - type CodesignError, - ERROR_CODES, - type ModelRef, -} from '@open-codesign/shared'; +import { type ChatMessage, ERROR_CODES, type ModelRef } from '@open-codesign/shared'; import { afterEach, describe, expect, it, vi } from 'vitest'; const getModelMock = vi.fn(); @@ -163,9 +158,11 @@ describe('complete', () => { await expect( complete(MODEL, [{ role: 'user', content: 'hi' }], { apiKey: 'sk-test' }), ).rejects.toMatchObject({ + name: 'CompletionLengthError', code: ERROR_CODES.PROVIDER_ERROR, message: expect.stringContaining('token limit'), - } satisfies Partial); + usage: { inputTokens: 1, outputTokens: 1, costUsd: 0 }, + }); }); it('synthesizes a pass-through Model when openrouter id is missing from registry', async () => { diff --git a/packages/providers/src/index.ts b/packages/providers/src/index.ts index 7ba23e15..b7826b35 100644 --- a/packages/providers/src/index.ts +++ b/packages/providers/src/index.ts @@ -65,6 +65,18 @@ export interface GenerateResult { costUsd: number; } +/** A length stop is not a successful completion. Keep usage, never partial + * content, so bounded callers can retry without under-reporting token cost. */ +export class CompletionLengthError extends CodesignError { + constructor(public readonly usage: Omit) { + super( + 'Provider stopped before completion because the response hit the token limit', + ERROR_CODES.PROVIDER_ERROR, + ); + this.name = 'CompletionLengthError'; + } +} + interface PiTextContent { type: 'text'; text: string; @@ -475,12 +487,17 @@ function assertCompleteStop(result: PiAssistantMessage): void { ERROR_CODES.PROVIDER_ABORTED, ); } + if (result.stopReason === 'length') { + throw new CompletionLengthError({ + inputTokens: result.usage?.input ?? 0, + outputTokens: result.usage?.output ?? 0, + costUsd: result.usage?.cost?.total ?? 0, + }); + } const message = - result.stopReason === 'length' - ? 'Provider stopped before completion because the response hit the token limit' - : result.stopReason === 'toolUse' - ? 'Provider returned an unresolved tool call in a non-tool completion' - : (result.errorMessage ?? 'Provider returned an error'); + result.stopReason === 'toolUse' + ? 'Provider returned an unresolved tool call in a non-tool completion' + : (result.errorMessage ?? 'Provider returned an error'); throw new CodesignError(message, ERROR_CODES.PROVIDER_ERROR); } From 20d7e3e78873957207419fe80fa03b6f609fa602 Mon Sep 17 00:00:00 2001 From: Sun-sunshine06 Date: Fri, 18 Sep 2026 00:39:10 +0800 Subject: [PATCH 2/2] test(desktop): separate OAuth module setup from status assertions Signed-off-by: Sun-sunshine06 --- apps/desktop/src/main/codex-oauth-ipc.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/main/codex-oauth-ipc.test.ts b/apps/desktop/src/main/codex-oauth-ipc.test.ts index ec270807..0c055016 100644 --- a/apps/desktop/src/main/codex-oauth-ipc.test.ts +++ b/apps/desktop/src/main/codex-oauth-ipc.test.ts @@ -8,7 +8,7 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { ConfigV3Schema, toPersistedV3 } from '@open-codesign/shared'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; const handlers = new Map unknown>(); @@ -89,6 +89,13 @@ vi.mock('@open-codesign/providers/codex', async () => { let tmpConfigDir: string; +// Cold transformation of the OAuth/provider module graph is suite setup, not +// part of the first status handler's 5s assertion budget. Under concurrent +// Windows runs that setup alone can exceed the per-test timeout. +beforeAll(async () => { + await import('./codex-oauth-ipc'); +}, 30_000); + beforeEach(() => { tmpConfigDir = mkdtempSync(join(tmpdir(), 'codex-oauth-ipc-')); fakeCachedConfig = null;