Skip to content

Commit 5584c00

Browse files
committed
fix: close a credential-misdirection path this branch had opened
Making the endpoint normalizer trim handled whitespace around a value but not a control character inside one, and the URL parser removes those from anywhere in its input — so a value that reads as one host could resolve to another, and the profile's key went with it. The flag and environment paths never touch the config writer, so its guard did not cover this. The normalizer now refuses the same character set the writer does, which also keeps the invariant that nothing it blesses can be refused by the write that stores it. Comparing the parsed URL back against its input was the alternative and is wrong: the parser rewrites percent-encoding, case, internationalized hosts and default ports, so legitimate endpoints would be refused. The blank-query guard tested for exactly empty, so a whitespace-only value still reached the wire — as a real zero on a numeric filter, an explicit false on a boolean one, and as an encoded space the server then rejected. It now refuses any value that is blank once trimmed, while a body string keeps its meaning, an explicit zero still sends, and a value with content around its whitespace is passed through untouched rather than trimmed. A graph-id conflict reported 409 on the v2 route and fell through the older persistence wrapper as an unclassified 500. That wrapper now classifies orchestration failures through the cause chain, which also fixes a pre-existing case where a workflow archived between authorization and the locked read reported 500 rather than 404. Persisting a chat turn claimed its row by id alone, so a conversation soft-deleted mid-turn still received the messages and was bumped back up the list. It now requires a live row. A turn whose caller hung up after the model had already answered persisted nothing, though the work was done and billed; it now persists and still reports the connection as closed. An empty workspace id from the login response was read as no workspace at all. A published description still promised a language-tag standard the schema does not enforce. The test asserting that a turn is stored before the final event drained the whole response first, so it held whichever order the code used. It now reads the stream incrementally and fails if the write moves after the event.
1 parent 29b2d8b commit 5584c00

14 files changed

Lines changed: 457 additions & 24 deletions

File tree

apps/docs/openapi-v2-knowledge.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6843,7 +6843,7 @@
68436843
"enum": ["default", "plain", "markdown", "code"]
68446844
},
68456845
"lang": {
6846-
"description": "Optional document language, as a BCP-47 tag such as `en` or `en-US`.",
6846+
"description": "Optional document language: hyphen-separated letter and digit subtags such as `en`, `en-US`, or `zh-Hant-TW`. Only that shape is validated, not full BCP-47 conformance.",
68476847
"type": "string",
68486848
"maxLength": 35,
68496849
"pattern": "^[A-Za-z]{2,8}(?:-[A-Za-z0-9]{1,8})*$"

apps/sim/app/api/v2/chat/route.test.ts

Lines changed: 161 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
*/
44

55
import { createMockRequest } from '@sim/testing'
6+
import { sleep } from '@sim/utils/helpers'
7+
import { NextRequest } from 'next/server'
68
import { beforeEach, describe, expect, it, vi } from 'vitest'
79

810
const {
@@ -130,6 +132,9 @@ const personalAuth = {
130132
const SERVER_ISSUED_CHAT_ID = 'chat-server-1'
131133
const OWNED_CONVERSATION_ID = '11111111-1111-4111-8111-111111111111'
132134

135+
/** How long a stream read may stay pending before it counts as "nothing more yet". */
136+
const IDLE_STREAM_MS = 20
137+
133138
function chatRow(id: string) {
134139
return { id, userId: 'user-1', workspaceId: 'workspace-1', workflowId: null, type: 'mothership' }
135140
}
@@ -148,6 +153,79 @@ function callChat(body: Record<string, unknown>, headers: Record<string, string>
148153
return POST(req, { params: Promise.resolve({}) })
149154
}
150155

156+
/**
157+
* Same call, but over a request whose signal the test controls — the only way
158+
* to reproduce a caller that hangs up while the turn is still running.
159+
*/
160+
function callChatWithSignal(
161+
body: Record<string, unknown>,
162+
signal: AbortSignal,
163+
headers: Record<string, string> = {}
164+
) {
165+
const req = new NextRequest(new URL('http://localhost:3000/api/v2/chat'), {
166+
method: 'POST',
167+
headers: new Headers({
168+
'Content-Type': 'application/json',
169+
'X-API-Key': 'test-key',
170+
...headers,
171+
}),
172+
body: JSON.stringify(body),
173+
signal,
174+
})
175+
return POST(req, { params: Promise.resolve({}) })
176+
}
177+
178+
/**
179+
* Read an NDJSON response incrementally. `drain()` returns the events that have
180+
* already reached the caller and stops as soon as the producer goes quiet;
181+
* `rest()` reads to the end. A pending read is held across calls so no chunk is
182+
* dropped between the two.
183+
*/
184+
function readNdjsonStream(response: Response) {
185+
const reader = response.body!.getReader()
186+
const decoder = new TextDecoder()
187+
let buffered = ''
188+
let pending: Promise<ReadableStreamReadResult<Uint8Array>> | null = null
189+
190+
const parse = (): Array<Record<string, unknown>> => {
191+
const lines = buffered.split('\n')
192+
buffered = lines.pop() ?? ''
193+
return lines.filter((line) => line.trim().length > 0).map((line) => JSON.parse(line))
194+
}
195+
196+
const step = async (): Promise<'idle' | 'done' | 'chunk'> => {
197+
pending ??= reader.read()
198+
const settled = await Promise.race([
199+
pending.then((result) => ({ result })),
200+
sleep(IDLE_STREAM_MS).then(() => null),
201+
])
202+
if (!settled) return 'idle'
203+
pending = null
204+
if (settled.result.done) return 'done'
205+
buffered += decoder.decode(settled.result.value, { stream: true })
206+
return 'chunk'
207+
}
208+
209+
return {
210+
async drain() {
211+
const events: Array<Record<string, unknown>> = []
212+
for (;;) {
213+
const state = await step()
214+
events.push(...parse())
215+
if (state !== 'chunk') return events
216+
}
217+
},
218+
async rest() {
219+
const events: Array<Record<string, unknown>> = []
220+
for (;;) {
221+
const state = await step()
222+
events.push(...parse())
223+
if (state === 'done') return events
224+
}
225+
},
226+
}
227+
}
228+
151229
async function readNdjsonEvents(response: Response): Promise<Array<Record<string, unknown>>> {
152230
const raw = await response.text()
153231
return raw
@@ -484,16 +562,96 @@ describe('POST /api/v2/chat', () => {
484562
])
485563
})
486564

487-
it('persists the turn on the NDJSON path too, before the final event', async () => {
565+
it('persists the turn on the NDJSON path before the final event reaches the caller', async () => {
566+
// Hold the transcript write open and watch the wire: draining the whole
567+
// response first would pass just as happily with the write moved after the
568+
// final event, so the write is gated and the stream read incrementally.
569+
let persistEntered: () => void = () => {}
570+
const persistInFlight = new Promise<void>((resolve) => {
571+
persistEntered = resolve
572+
})
573+
let releasePersist: () => void = () => {}
574+
mockPersistCopilotChatTurn.mockImplementation(() => {
575+
persistEntered()
576+
return new Promise<void>((resolve) => {
577+
releasePersist = resolve
578+
})
579+
})
580+
488581
const response = await callChat(
489582
{ workspaceId: 'workspace-1', message: 'hi' },
490583
{ accept: 'application/x-ndjson' }
491584
)
492-
const events = await readNdjsonEvents(response)
585+
const stream = readNdjsonStream(response)
586+
587+
await persistInFlight
588+
const beforeRelease = await stream.drain()
589+
expect(beforeRelease.map((event) => event.type)).not.toContain('final')
590+
591+
releasePersist()
592+
const afterRelease = await stream.rest()
593+
expect(afterRelease.at(-1)?.type).toBe('final')
493594

494595
expect(mockPersistCopilotChatTurn).toHaveBeenCalledTimes(1)
495596
expect(mockPersistCopilotChatTurn.mock.calls[0][1]).toHaveLength(2)
496-
expect(events.at(-1)?.type).toBe('final')
597+
})
598+
599+
it('persists a completed turn whose caller hung up, and still reports it as client-closed', async () => {
600+
const controller = new AbortController()
601+
mockRunHeadlessCopilotLifecycle.mockImplementation(async () => {
602+
controller.abort()
603+
return successResult
604+
})
605+
606+
const response = await callChatWithSignal(
607+
{ workspaceId: 'workspace-1', message: 'hi' },
608+
controller.signal
609+
)
610+
611+
// The model already ran and was billed, so the reply is written to the
612+
// conversation it belongs to — but the caller is gone, and the status the
613+
// route reports says exactly that.
614+
expect(response.status).toBe(499)
615+
const body = await response.json()
616+
expect(body.error.code).toBe('CLIENT_CLOSED_REQUEST')
617+
expect(mockPersistCopilotChatTurn).toHaveBeenCalledTimes(1)
618+
expect(mockPersistCopilotChatTurn.mock.calls[0][0]).toBe(SERVER_ISSUED_CHAT_ID)
619+
})
620+
621+
it('persists a completed turn whose NDJSON caller hung up, and still ends in an abort event', async () => {
622+
const controller = new AbortController()
623+
mockRunHeadlessCopilotLifecycle.mockImplementation(async () => {
624+
controller.abort()
625+
return successResult
626+
})
627+
628+
const response = await callChatWithSignal(
629+
{ workspaceId: 'workspace-1', message: 'hi' },
630+
controller.signal,
631+
{ accept: 'application/x-ndjson' }
632+
)
633+
const events = await readNdjsonEvents(response)
634+
635+
const last = events.at(-1) as { type: string; error?: string }
636+
expect(last.type).toBe('error')
637+
expect(last.error).toBe('Chat request aborted')
638+
expect(mockPersistCopilotChatTurn).toHaveBeenCalledTimes(1)
639+
})
640+
641+
it('persists nothing for a failed run whose caller hung up', async () => {
642+
const controller = new AbortController()
643+
mockRunHeadlessCopilotLifecycle.mockImplementation(async () => {
644+
controller.abort()
645+
return { success: false, error: 'model exploded' }
646+
})
647+
648+
const response = await callChatWithSignal(
649+
{ workspaceId: 'workspace-1', message: 'hi' },
650+
controller.signal
651+
)
652+
653+
expect(response.status).toBe(499)
654+
expect(mockPersistCopilotChatTurn).not.toHaveBeenCalled()
497655
})
498656

499657
it('persists nothing when the run fails, so no question is stored without its answer', async () => {

apps/sim/app/api/v2/chat/route.ts

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,10 @@ export const POST = withRouteHandler(
198198
* resolved. Without it a `sim chat` turn leaves a titled conversation
199199
* that opens to an empty transcript in the web Chat list.
200200
*
201-
* By the time this runs the turn has completed and been billed, and a
202-
* streamed reply has already reached the caller, so a write failure is
203-
* logged and the successful response still stands. The write is one
204-
* transaction, so that failure leaves the transcript empty rather than
205-
* showing the question without the answer.
201+
* By the time this runs the turn has completed and been billed, so a
202+
* write failure is logged and the response the caller gets is unchanged.
203+
* The write is one transaction, so that failure leaves the transcript
204+
* empty rather than showing the question without the answer.
206205
*/
207206
const persistTurn = async (result: OrchestratorResult): Promise<void> => {
208207
try {
@@ -355,6 +354,13 @@ export const POST = withRouteHandler(
355354
})
356355
allowExplicitAbort = false
357356

357+
// Persist before the cancellation check: the turn ran and was
358+
// billed, so the reply belongs in the transcript even when the
359+
// caller stopped listening — that is the only place it survives.
360+
if (result.success) {
361+
await persistTurn(result)
362+
}
363+
358364
if (lifecycleAbortController.signal.aborted) {
359365
send({ type: 'error', error: 'Chat request aborted' })
360366
return
@@ -373,8 +379,6 @@ export const POST = withRouteHandler(
373379
return
374380
}
375381

376-
await persistTurn(result)
377-
378382
send({
379383
type: 'final',
380384
data: buildChatResultPayload(result, chatId, integrationTools),
@@ -428,6 +432,14 @@ export const POST = withRouteHandler(
428432
const result = await runLifecycle()
429433
allowExplicitAbort = false
430434

435+
// Persist before the cancellation check: the turn ran and was billed,
436+
// so the reply belongs in the transcript even when the caller stopped
437+
// listening — that is the only place it survives. The cancellation
438+
// check still decides the status the caller receives.
439+
if (result.success) {
440+
await persistTurn(result)
441+
}
442+
431443
if (lifecycleAbortController.signal.aborted || req.signal.aborted) {
432444
reqLogger.info('Chat request aborted after lifecycle completion')
433445
return v2Error('CLIENT_CLOSED_REQUEST', 'Chat request aborted')
@@ -438,8 +450,6 @@ export const POST = withRouteHandler(
438450
return v2Error('INTERNAL_ERROR', result.error || 'Chat request failed')
439451
}
440452

441-
await persistTurn(result)
442-
443453
return v2Data(buildChatResultPayload(result, chatId, integrationTools))
444454
} finally {
445455
allowExplicitAbort = false

apps/sim/lib/api/contracts/v2/knowledge.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ const v2KnowledgeDocumentProcessingOptionsSchema =
490490
lang: knowledgeDocumentUploadMetadataSchema.shape.processingOptions
491491
.unwrap()
492492
.shape.lang.describe(
493-
'Optional document language, as a BCP-47 tag such as `en` or `en-US`.'
493+
'Optional document language: hyphen-separated letter and digit subtags such as `en`, `en-US`, or `zh-Hant-TW`. Only that shape is validated, not full BCP-47 conformance.'
494494
),
495495
})
496496
.strict()

apps/sim/lib/copilot/chat/messages-store.test.ts

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { dbChainMockFns, resetDbChainMock } from '@sim/testing'
4+
import { dbChainMockFns, resetDbChainMock, schemaMock } from '@sim/testing'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
66
import {
77
appendCopilotChatMessages,
8+
persistCopilotChatTurn,
89
replaceCopilotChatMessages,
910
} from '@/lib/copilot/chat/messages-store'
1011
import type { PersistedMessage } from '@/lib/copilot/chat/persisted-message'
@@ -231,4 +232,41 @@ describe('messages-store', () => {
231232
expect(JSON.stringify(lastValuesRows())).not.toContain('huge')
232233
})
233234
})
235+
236+
describe('persistCopilotChatTurn', () => {
237+
it('claims the chat row by id AND liveness, so a soft-deleted chat matches nothing', async () => {
238+
dbChainMockFns.returning.mockResolvedValueOnce([{ model: 'claude-sonnet-4-5' }])
239+
240+
await persistCopilotChatTurn('chat-1', [userMsg, assistantMsg])
241+
242+
// The chain mock ignores predicates, so the predicate itself is the
243+
// assertion: without the liveness term the update still matches an
244+
// archived row and the turn lands in a conversation the user deleted.
245+
expect(dbChainMockFns.where).toHaveBeenCalledWith({
246+
type: 'and',
247+
conditions: [
248+
{ type: 'eq', left: schemaMock.copilotChats.id, right: 'chat-1' },
249+
{ type: 'isNull', column: schemaMock.copilotChats.deletedAt },
250+
],
251+
})
252+
})
253+
254+
it('writes the transcript with the chat model when the row is still live', async () => {
255+
dbChainMockFns.returning.mockResolvedValueOnce([{ model: 'claude-sonnet-4-5' }])
256+
257+
await persistCopilotChatTurn('chat-1', [userMsg, assistantMsg])
258+
259+
const rows = lastValuesRows()
260+
expect(rows.map((r) => r.messageId)).toEqual(['msg-user-1', 'msg-asst-1'])
261+
expect(rows[0].model).toBe('claude-sonnet-4-5')
262+
})
263+
264+
it('writes nothing when the claim matches no row', async () => {
265+
dbChainMockFns.returning.mockResolvedValueOnce([])
266+
267+
await persistCopilotChatTurn('chat-1', [userMsg, assistantMsg])
268+
269+
expect(dbChainMockFns.insert).not.toHaveBeenCalled()
270+
})
271+
})
234272
})

apps/sim/lib/copilot/chat/messages-store.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { db } from '@sim/db'
22
import { copilotChats, copilotMessages } from '@sim/db/schema'
3-
import { and, eq, notInArray, sql } from 'drizzle-orm'
3+
import { and, eq, isNull, notInArray, sql } from 'drizzle-orm'
44
import { type PersistedMessage, stripToolResultOutput } from '@/lib/copilot/chat/persisted-message'
55
import type { DbOrTx } from '@/lib/db/types'
66

@@ -87,8 +87,15 @@ export async function appendCopilotChatMessages(
8787
* turn run without that surface would leave a chat that opens to nothing.
8888
*
8989
* Both messages are written in a single transaction, so a failure leaves the
90-
* transcript untouched rather than showing a question with no answer. Does
91-
* nothing when the chat no longer exists; throws on a write failure.
90+
* transcript untouched rather than showing a question with no answer.
91+
*
92+
* The chat row is claimed under the same liveness predicate the accessible-chat
93+
* loaders use, so a chat soft-deleted while the turn was running receives
94+
* nothing: the update matches no row and the transaction returns having written
95+
* neither the transcript nor the recency bump. Dropping the turn is right here
96+
* because the user deleted the conversation after asking — resurrecting it with
97+
* a reply would undo that deletion, and the caller still has its reply in the
98+
* response. Throws on a write failure.
9299
*/
93100
export async function persistCopilotChatTurn(
94101
chatId: string,
@@ -98,7 +105,7 @@ export async function persistCopilotChatTurn(
98105
const [updated] = await tx
99106
.update(copilotChats)
100107
.set({ updatedAt: new Date() })
101-
.where(eq(copilotChats.id, chatId))
108+
.where(and(eq(copilotChats.id, chatId), isNull(copilotChats.deletedAt)))
102109
.returning({ model: copilotChats.model })
103110
if (!updated) return
104111
await appendCopilotChatMessages(chatId, messages, { chatModel: updated.model ?? null }, tx)

0 commit comments

Comments
 (0)