Skip to content

Commit 68c3ebb

Browse files
fix(mothership): fail outputTable writes per-row when any row matches no columns
Review feedback: the all-rows-empty guard let mixed batches slip unmatched rows through as empty objects. Reject on the first row that maps to zero columns, naming the row. Adds the missing CSV-suite parity tests (no-matching-headers, service-error surfacing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 49afd5f commit 68c3ebb

2 files changed

Lines changed: 44 additions & 3 deletions

File tree

apps/sim/lib/copilot/request/tools/tables.test.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,20 @@ describe('maybeWriteOutputToTable', () => {
123123
)
124124

125125
expect(result.success).toBe(false)
126-
expect(result.error).toContain('None of the row keys match columns')
126+
expect(result.error).toContain('Row 1 has no keys matching columns')
127+
expect(mockReplaceTableRows).not.toHaveBeenCalled()
128+
})
129+
130+
it('fails fast when only some rows match instead of writing empty rows', async () => {
131+
const result = await maybeWriteOutputToTable(
132+
FunctionExecute.id,
133+
{ outputTable: 'tbl_1' },
134+
{ success: true, output: { result: [{ name: 'Alice' }, { wrong: 'x' }] } },
135+
buildContext()
136+
)
137+
138+
expect(result.success).toBe(false)
139+
expect(result.error).toContain('Row 2 has no keys matching columns')
127140
expect(mockReplaceTableRows).not.toHaveBeenCalled()
128141
})
129142

@@ -178,4 +191,31 @@ describe('maybeWriteReadCsvToTable', () => {
178191
{ col_name: 'Bob', col_age: '40' },
179192
])
180193
})
194+
195+
it('fails fast when the file headers match no table columns', async () => {
196+
const result = await maybeWriteReadCsvToTable(
197+
ReadTool.id,
198+
{ outputTable: 'tbl_1', path: 'files/people.csv' },
199+
{ success: true, output: { content: 'wrong,headers\n1,2' } },
200+
buildContext()
201+
)
202+
203+
expect(result.success).toBe(false)
204+
expect(result.error).toContain('Row 1 has no keys matching columns')
205+
expect(mockReplaceTableRows).not.toHaveBeenCalled()
206+
})
207+
208+
it('surfaces service validation failures as tool errors', async () => {
209+
mockReplaceTableRows.mockRejectedValue(new Error('Row 1: name is required'))
210+
211+
const result = await maybeWriteReadCsvToTable(
212+
ReadTool.id,
213+
{ outputTable: 'tbl_1', path: 'files/people.csv' },
214+
{ success: true, output: { content: 'age\n30' } },
215+
buildContext()
216+
)
217+
218+
expect(result.success).toBe(false)
219+
expect(result.error).toContain('Row 1: name is required')
220+
})
181221
})

apps/sim/lib/copilot/request/tools/tables.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,10 @@ async function replaceTableRowsFromWire(
3030
): Promise<{ error?: string }> {
3131
const idByName = buildIdByName(table.schema)
3232
const idKeyedRows = rows.map((row) => rowDataNameToId(row as RowData, idByName))
33-
if (idKeyedRows.every((row) => Object.keys(row).length === 0)) {
33+
const emptyIndex = idKeyedRows.findIndex((row) => Object.keys(row).length === 0)
34+
if (emptyIndex !== -1) {
3435
return {
35-
error: `None of the row keys match columns on table "${table.name}" (columns: ${table.schema.columns.map((c) => c.name).join(', ')})`,
36+
error: `Row ${emptyIndex + 1} has no keys matching columns on table "${table.name}" (columns: ${table.schema.columns.map((c) => c.name).join(', ')})`,
3637
}
3738
}
3839
await replaceTableRows(

0 commit comments

Comments
 (0)