Skip to content

Commit 34aac38

Browse files
authored
test(table): pin the row-write behavior the suite could not fail on (#6811)
* test(table): pin the row-write behavior the suite could not fail on Mutation testing across the branch found four changes that could be reverted with the whole suite green, plus two comments asserting something untrue. - Copilot row writes now assert the translated storage keys, not the literal keying flag. The shared table fixture uses legacy columns with no id, where name-to-id mapping is the identity, so the wrong keying was unobservable; columns whose id differs from name make it fail. Copilot is the one row-write surface whose keys come from a model, and under id keying the lax write path stores those keys verbatim and reports success. - The three provenance transport helpers get direct cover. The route tests only reach mapInput and present, so each could be replaced by a constant, and a constant envelope reader silently downgrades every executor write to untracked. - The domain provenance fixtures run against the real envelope guard instead of a stub that always accepted them; none of the old fixtures was a shape the guard admits. - The attribution allowlist keys on the client-id reader rather than the field name, so it no longer misses a surface that names the acting tab positionally -- one of the two real suppliers was already invisible to it. Also restores the selectedValues narrowing on the migrated row routes, so a stale sidecar entry for a dropped column no longer rides along in the envelope, matching what the unmigrated rows and query routes have always done. Adds a compile-time tie between the shared table fixture and TableDefinition. tsconfig excludes test files, so an assertion placed in one is never checked; a new required field was a production type error and a silent no-op across every route test. Corrects the getRowSummaryById TSDoc, which claimed the Copilot row tool never put executions on the wire. It did, and that narrowing is a deliberate wire change rather than a pure saving. * test(table): key the attribution audit on the write, not on the id reader The allowlist matched the `readClientId` call site. That reader is a general-purpose helper any surface may call for unrelated reasons, so an innocent caller elsewhere in the app would be classified as naming a tab, and aliasing or wrapping the reader would slip past it. Match the two forms that actually attribute a write instead -- setting `actorClientId`, or passing a second argument to the signal -- which hold however the id was obtained and say nothing about unrelated readers.
1 parent 51f1d45 commit 34aac38

8 files changed

Lines changed: 297 additions & 80 deletions

File tree

apps/sim/app/api/table/row-secret-provenance.test.ts

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,19 @@ import {
88
PRIVATE_SECRET_PROVENANCE_BUNDLE_V1,
99
PRIVATE_SECRET_PROVENANCE_FIELD,
1010
PRIVATE_SECRET_PROVENANCE_HEADER,
11+
PRIVATE_TOOL_METADATA_REQUEST_HEADER,
12+
PRIVATE_TOOL_METADATA_RESPONSE_HEADER,
13+
RESOLVED_SECRET_PROVENANCE_METADATA_V1,
1114
} from '@/lib/execution/private-tool-metadata'
15+
import { TableRowProvenanceError } from '@/lib/table/application/row-secret-provenance'
1216
import { rowDataNameToId } from '@/lib/table/column-keys'
1317
import { tableRowSecretProvenanceSelectionKey } from '@/lib/table/secret-provenance-selection'
1418
import type { RowData } from '@/lib/table/types'
1519
import {
1620
createTableWriteProvenanceTargets,
21+
finalizeTableRowsProvenance,
22+
negotiateTableRowsProvenance,
23+
readTableRowProvenanceEnvelope,
1724
resolveTableWriteSecretProvenance,
1825
} from '@/app/api/table/row-secret-provenance'
1926

@@ -258,3 +265,77 @@ describe('resolveTableWriteSecretProvenance', () => {
258265
expect(result.success).toBe(false)
259266
})
260267
})
268+
269+
/**
270+
* The transport half of the envelope, used by the migrated single-row routes.
271+
*
272+
* These are the only cover these helpers have: the route tests assert `mapInput`
273+
* and `present`, so each helper could be replaced by a constant without a route
274+
* test noticing — and a constant `readTableRowProvenanceEnvelope` would silently
275+
* downgrade every executor write from a stamped bundle to untracked.
276+
*/
277+
describe('readTableRowProvenanceEnvelope', () => {
278+
it('reports no envelope when the caller sent none', () => {
279+
const request = createMockRequest('PATCH', { data: {} })
280+
281+
expect(readTableRowProvenanceEnvelope(request, { data: {} })).toEqual({ kind: 'none' })
282+
})
283+
284+
it('hands the verified bundle over unresolved', () => {
285+
const { request, payload } = bundleRequest([tableRowSecretProvenanceSelectionKey(0, 'email')])
286+
287+
const envelope = readTableRowProvenanceEnvelope(request, payload)
288+
289+
expect(envelope.kind).toBe('bundle')
290+
expect(envelope).toEqual({ kind: 'bundle', value: payload[PRIVATE_SECRET_PROVENANCE_FIELD] })
291+
})
292+
293+
it('rejects a declared bundle whose payload field is missing', () => {
294+
const request = createMockRequest(
295+
'PATCH',
296+
{ data: {} },
297+
{ [PRIVATE_SECRET_PROVENANCE_HEADER]: PRIVATE_SECRET_PROVENANCE_BUNDLE_V1 }
298+
)
299+
300+
expect(() => readTableRowProvenanceEnvelope(request, { data: {} })).toThrow(
301+
TableRowProvenanceError
302+
)
303+
})
304+
})
305+
306+
describe('negotiateTableRowsProvenance', () => {
307+
it('is not requested without the capability header', () => {
308+
expect(negotiateTableRowsProvenance(createMockRequest('GET', undefined), true)).toBe(false)
309+
})
310+
311+
it('is accepted for an internal caller that asked for it', () => {
312+
const request = createMockRequest('GET', undefined, {
313+
[PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1,
314+
})
315+
316+
expect(negotiateTableRowsProvenance(request, true)).toBe(true)
317+
})
318+
319+
it('rejects a session caller that asks for the internal capability', () => {
320+
const request = createMockRequest('GET', undefined, {
321+
[PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1,
322+
})
323+
324+
expect(() => negotiateTableRowsProvenance(request, false)).toThrow(TableRowProvenanceError)
325+
})
326+
})
327+
328+
describe('finalizeTableRowsProvenance', () => {
329+
it('adds nothing when the use case loaded no provenance', () => {
330+
expect(finalizeTableRowsProvenance(undefined)).toEqual({})
331+
})
332+
333+
it('adds the sibling body field and the capability header when it did', () => {
334+
const finalized = finalizeTableRowsProvenance({ rows: [] })
335+
336+
expect(finalized.bodyFields).toBeDefined()
337+
expect(new Headers(finalized.headers).get(PRIVATE_TOOL_METADATA_RESPONSE_HEADER)).toBe(
338+
RESOLVED_SECRET_PROVENANCE_METADATA_V1
339+
)
340+
})
341+
})

apps/sim/lib/copilot/tools/server/table/user-table.test.ts

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ const {
1414
mockDownloadWorkspaceFile,
1515
mockGetTableById,
1616
mockBatchInsertRows,
17+
mockInsertRow,
18+
mockUpdateRow,
1719
mockReplaceTableRows,
1820
mockAddWorkflowGroup,
1921
mockCreateTable,
@@ -42,6 +44,8 @@ const {
4244
mockDownloadWorkspaceFile: vi.fn(),
4345
mockGetTableById: vi.fn(),
4446
mockBatchInsertRows: vi.fn(),
47+
mockInsertRow: vi.fn(),
48+
mockUpdateRow: vi.fn(),
4549
mockReplaceTableRows: vi.fn(),
4650
mockAddWorkflowGroup: vi.fn(),
4751
mockCreateTable: vi.fn(),
@@ -208,10 +212,10 @@ vi.mock('@/lib/table/rows/service', () => ({
208212
deleteRowsByFilter: mockDeleteRowsByFilter,
209213
deleteRowsByIds: vi.fn(),
210214
getRowById: vi.fn(),
211-
insertRow: vi.fn(),
215+
insertRow: mockInsertRow,
212216
queryRows: mockQueryRows,
213217
replaceTableRows: mockReplaceTableRows,
214-
updateRow: vi.fn(),
218+
updateRow: mockUpdateRow,
215219
updateRowsByFilter: mockUpdateRowsByFilter,
216220
}))
217221

@@ -1669,3 +1673,73 @@ describe('userTableServerTool.delete bounds', () => {
16691673
expect(mockGetTableById).not.toHaveBeenCalled()
16701674
})
16711675
})
1676+
1677+
/**
1678+
* Copilot is the one row-write surface whose column keys come from a model rather
1679+
* than from the schema, so `dataKeying: 'names'` is what stands between an
1680+
* LLM-authored key and the storage column it means.
1681+
*
1682+
* These pin the translated outcome rather than the literal: the shared `buildTable`
1683+
* fixture uses legacy columns with no `id`, where name-to-id mapping is the identity
1684+
* and flipping the keying is unobservable. Columns whose `id` differs from `name` are
1685+
* what make the wrong keying fail — under `'ids'` the lax write path stores the
1686+
* model's key verbatim and reports success, corrupting the row silently.
1687+
*/
1688+
describe('userTableServerTool row writes key model-supplied columns by name', () => {
1689+
const KEYED_TABLE = buildTable({
1690+
schema: {
1691+
columns: [
1692+
{ id: 'col_name', name: 'name', type: 'string', required: true },
1693+
{ id: 'col_age', name: 'age', type: 'number' },
1694+
],
1695+
},
1696+
})
1697+
1698+
beforeEach(() => {
1699+
vi.clearAllMocks()
1700+
mockGetTableById.mockResolvedValue(KEYED_TABLE)
1701+
})
1702+
1703+
it('translates an inserted row to storage column ids', async () => {
1704+
mockInsertRow.mockResolvedValue({
1705+
id: 'row-1',
1706+
data: { col_name: 'Ada', col_age: 36 },
1707+
position: 0,
1708+
createdAt: new Date('2024-01-01'),
1709+
updatedAt: new Date('2024-01-01'),
1710+
})
1711+
1712+
await userTableServerTool.execute(
1713+
{
1714+
operation: 'insert_row',
1715+
args: { tableId: 'tbl_1', data: { name: 'Ada', age: 36 } },
1716+
},
1717+
buildToolContext()
1718+
)
1719+
1720+
expect(mockInsertRow).toHaveBeenCalledTimes(1)
1721+
expect(mockInsertRow.mock.calls[0][0].data).toEqual({ col_name: 'Ada', col_age: 36 })
1722+
})
1723+
1724+
it('translates an updated row to storage column ids', async () => {
1725+
mockUpdateRow.mockResolvedValue({
1726+
id: 'row-1',
1727+
data: { col_name: 'Grace' },
1728+
position: 0,
1729+
executions: {},
1730+
createdAt: new Date('2024-01-01'),
1731+
updatedAt: new Date('2024-01-01'),
1732+
})
1733+
1734+
await userTableServerTool.execute(
1735+
{
1736+
operation: 'update_row',
1737+
args: { tableId: 'tbl_1', rowId: 'row-1', data: { name: 'Grace' } },
1738+
},
1739+
buildToolContext()
1740+
)
1741+
1742+
expect(mockUpdateRow).toHaveBeenCalledTimes(1)
1743+
expect(mockUpdateRow.mock.calls[0][0].data).toEqual({ col_name: 'Grace' })
1744+
})
1745+
})

0 commit comments

Comments
 (0)