diff --git a/apps/sim/app/api/table/row-secret-provenance.test.ts b/apps/sim/app/api/table/row-secret-provenance.test.ts index f84a74442d2..21cbf24d0a5 100644 --- a/apps/sim/app/api/table/row-secret-provenance.test.ts +++ b/apps/sim/app/api/table/row-secret-provenance.test.ts @@ -8,12 +8,19 @@ import { PRIVATE_SECRET_PROVENANCE_BUNDLE_V1, PRIVATE_SECRET_PROVENANCE_FIELD, PRIVATE_SECRET_PROVENANCE_HEADER, + PRIVATE_TOOL_METADATA_REQUEST_HEADER, + PRIVATE_TOOL_METADATA_RESPONSE_HEADER, + RESOLVED_SECRET_PROVENANCE_METADATA_V1, } from '@/lib/execution/private-tool-metadata' +import { TableRowProvenanceError } from '@/lib/table/application/row-secret-provenance' import { rowDataNameToId } from '@/lib/table/column-keys' import { tableRowSecretProvenanceSelectionKey } from '@/lib/table/secret-provenance-selection' import type { RowData } from '@/lib/table/types' import { createTableWriteProvenanceTargets, + finalizeTableRowsProvenance, + negotiateTableRowsProvenance, + readTableRowProvenanceEnvelope, resolveTableWriteSecretProvenance, } from '@/app/api/table/row-secret-provenance' @@ -258,3 +265,77 @@ describe('resolveTableWriteSecretProvenance', () => { expect(result.success).toBe(false) }) }) + +/** + * The transport half of the envelope, used by the migrated single-row routes. + * + * These are the only cover these helpers have: the route tests assert `mapInput` + * and `present`, so each helper could be replaced by a constant without a route + * test noticing — and a constant `readTableRowProvenanceEnvelope` would silently + * downgrade every executor write from a stamped bundle to untracked. + */ +describe('readTableRowProvenanceEnvelope', () => { + it('reports no envelope when the caller sent none', () => { + const request = createMockRequest('PATCH', { data: {} }) + + expect(readTableRowProvenanceEnvelope(request, { data: {} })).toEqual({ kind: 'none' }) + }) + + it('hands the verified bundle over unresolved', () => { + const { request, payload } = bundleRequest([tableRowSecretProvenanceSelectionKey(0, 'email')]) + + const envelope = readTableRowProvenanceEnvelope(request, payload) + + expect(envelope.kind).toBe('bundle') + expect(envelope).toEqual({ kind: 'bundle', value: payload[PRIVATE_SECRET_PROVENANCE_FIELD] }) + }) + + it('rejects a declared bundle whose payload field is missing', () => { + const request = createMockRequest( + 'PATCH', + { data: {} }, + { [PRIVATE_SECRET_PROVENANCE_HEADER]: PRIVATE_SECRET_PROVENANCE_BUNDLE_V1 } + ) + + expect(() => readTableRowProvenanceEnvelope(request, { data: {} })).toThrow( + TableRowProvenanceError + ) + }) +}) + +describe('negotiateTableRowsProvenance', () => { + it('is not requested without the capability header', () => { + expect(negotiateTableRowsProvenance(createMockRequest('GET', undefined), true)).toBe(false) + }) + + it('is accepted for an internal caller that asked for it', () => { + const request = createMockRequest('GET', undefined, { + [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1, + }) + + expect(negotiateTableRowsProvenance(request, true)).toBe(true) + }) + + it('rejects a session caller that asks for the internal capability', () => { + const request = createMockRequest('GET', undefined, { + [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1, + }) + + expect(() => negotiateTableRowsProvenance(request, false)).toThrow(TableRowProvenanceError) + }) +}) + +describe('finalizeTableRowsProvenance', () => { + it('adds nothing when the use case loaded no provenance', () => { + expect(finalizeTableRowsProvenance(undefined)).toEqual({}) + }) + + it('adds the sibling body field and the capability header when it did', () => { + const finalized = finalizeTableRowsProvenance({ rows: [] }) + + expect(finalized.bodyFields).toBeDefined() + expect(new Headers(finalized.headers).get(PRIVATE_TOOL_METADATA_RESPONSE_HEADER)).toBe( + RESOLVED_SECRET_PROVENANCE_METADATA_V1 + ) + }) +}) diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.test.ts b/apps/sim/lib/copilot/tools/server/table/user-table.test.ts index 83c263815a2..c762ce5a4b8 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.test.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.test.ts @@ -14,6 +14,8 @@ const { mockDownloadWorkspaceFile, mockGetTableById, mockBatchInsertRows, + mockInsertRow, + mockUpdateRow, mockReplaceTableRows, mockAddWorkflowGroup, mockCreateTable, @@ -42,6 +44,8 @@ const { mockDownloadWorkspaceFile: vi.fn(), mockGetTableById: vi.fn(), mockBatchInsertRows: vi.fn(), + mockInsertRow: vi.fn(), + mockUpdateRow: vi.fn(), mockReplaceTableRows: vi.fn(), mockAddWorkflowGroup: vi.fn(), mockCreateTable: vi.fn(), @@ -208,10 +212,10 @@ vi.mock('@/lib/table/rows/service', () => ({ deleteRowsByFilter: mockDeleteRowsByFilter, deleteRowsByIds: vi.fn(), getRowById: vi.fn(), - insertRow: vi.fn(), + insertRow: mockInsertRow, queryRows: mockQueryRows, replaceTableRows: mockReplaceTableRows, - updateRow: vi.fn(), + updateRow: mockUpdateRow, updateRowsByFilter: mockUpdateRowsByFilter, })) @@ -1669,3 +1673,73 @@ describe('userTableServerTool.delete bounds', () => { expect(mockGetTableById).not.toHaveBeenCalled() }) }) + +/** + * Copilot is the one row-write surface whose column keys come from a model rather + * than from the schema, so `dataKeying: 'names'` is what stands between an + * LLM-authored key and the storage column it means. + * + * These pin the translated outcome rather than the literal: the shared `buildTable` + * fixture uses legacy columns with no `id`, where name-to-id mapping is the identity + * and flipping the keying is unobservable. Columns whose `id` differs from `name` are + * what make the wrong keying fail — under `'ids'` the lax write path stores the + * model's key verbatim and reports success, corrupting the row silently. + */ +describe('userTableServerTool row writes key model-supplied columns by name', () => { + const KEYED_TABLE = buildTable({ + schema: { + columns: [ + { id: 'col_name', name: 'name', type: 'string', required: true }, + { id: 'col_age', name: 'age', type: 'number' }, + ], + }, + }) + + beforeEach(() => { + vi.clearAllMocks() + mockGetTableById.mockResolvedValue(KEYED_TABLE) + }) + + it('translates an inserted row to storage column ids', async () => { + mockInsertRow.mockResolvedValue({ + id: 'row-1', + data: { col_name: 'Ada', col_age: 36 }, + position: 0, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), + }) + + await userTableServerTool.execute( + { + operation: 'insert_row', + args: { tableId: 'tbl_1', data: { name: 'Ada', age: 36 } }, + }, + buildToolContext() + ) + + expect(mockInsertRow).toHaveBeenCalledTimes(1) + expect(mockInsertRow.mock.calls[0][0].data).toEqual({ col_name: 'Ada', col_age: 36 }) + }) + + it('translates an updated row to storage column ids', async () => { + mockUpdateRow.mockResolvedValue({ + id: 'row-1', + data: { col_name: 'Grace' }, + position: 0, + executions: {}, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), + }) + + await userTableServerTool.execute( + { + operation: 'update_row', + args: { tableId: 'tbl_1', rowId: 'row-1', data: { name: 'Grace' } }, + }, + buildToolContext() + ) + + expect(mockUpdateRow).toHaveBeenCalledTimes(1) + expect(mockUpdateRow.mock.calls[0][0].data).toEqual({ col_name: 'Grace' }) + }) +}) diff --git a/apps/sim/lib/table/application/row-secret-provenance.test.ts b/apps/sim/lib/table/application/row-secret-provenance.test.ts index c43d7546dfe..100b56f2eeb 100644 --- a/apps/sim/lib/table/application/row-secret-provenance.test.ts +++ b/apps/sim/lib/table/application/row-secret-provenance.test.ts @@ -8,18 +8,13 @@ import { describe, expect, it, vi } from 'vitest' const { mocks } = vi.hoisted(() => ({ - mocks: { scopeCompatible: vi.fn(() => true), isBundle: vi.fn(() => true) }, + mocks: { scopeCompatible: vi.fn(() => true) }, })) vi.mock('@/lib/execution/durable-secret-provenance', () => ({ isPrivateSecretProvenanceScopeCompatible: mocks.scopeCompatible, })) -vi.mock('@/lib/execution/model-input-provenance', async (importOriginal) => { - const actual = await importOriginal() - return { ...actual, isPrivateSecretProvenanceBundleV1: mocks.isBundle } -}) - import { resolveRowWriteProvenance, TableRowProvenanceError, @@ -49,6 +44,31 @@ const EXECUTOR = { expiresAt: new Date('2026-01-02'), } +/** + * A provenance shape the real `isResolvedSecretTraceProvenanceV1` accepts. + * + * These fixtures run against the genuine envelope guard rather than a stub, so a + * bundle that could never survive the wire cannot pass here either. + */ +function traceProvenance(scope?: { userId: string; workspaceId: string }) { + return { version: 1 as const, complete: true, entries: [], ...(scope ? { scope } : {}) } +} + +/** A bundle shape the real `isPrivateSecretProvenanceBundleV1` accepts. */ +function bundle( + selections: Array<{ key: string; scope?: { userId: string; workspaceId: string } }>, + complete = true +) { + return { + version: 1 as const, + complete, + selections: selections.map((selection) => ({ + key: selection.key, + provenance: traceProvenance(selection.scope), + })), + } +} + function resolve(overrides: Partial[0]>) { return resolveRowWriteProvenance({ envelope: { kind: 'none' }, @@ -80,24 +100,40 @@ describe('row write provenance', () => { }) it('refuses a bundle from a session caller', () => { - expect(() => - resolve({ envelope: { kind: 'bundle', value: { complete: true, selections: [] } } }) - ).toThrow(TableRowProvenanceError) + expect(() => resolve({ envelope: { kind: 'bundle', value: bundle([]) } })).toThrow( + TableRowProvenanceError + ) }) it('refuses a bundle that is not a recognised envelope', () => { - mocks.isBundle.mockReturnValueOnce(false) - expect(() => resolve({ principal: EXECUTOR, envelope: { kind: 'bundle', value: { nope: true } } }) ).toThrow(TableRowProvenanceError) }) + it('refuses a bundle whose selections carry an unrecognised provenance shape', () => { + expect(() => + resolve({ + principal: EXECUTOR, + envelope: { + kind: 'bundle', + value: { + version: 1, + complete: true, + selections: [ + { key: JSON.stringify([0, 'col_aaa']), provenance: { scope: { kind: 'workspace' } } }, + ], + }, + }, + }) + ).toThrow(TableRowProvenanceError) + }) + it('refuses a complete bundle that does not account for every written cell', () => { expect(() => resolve({ principal: EXECUTOR, - envelope: { kind: 'bundle', value: { complete: true, selections: [] } }, + envelope: { kind: 'bundle', value: bundle([]) }, wireRows: [{ col_aaa: 'Ada', col_bbb: 36 }], storageRows: [{ col_aaa: 'Ada', col_bbb: 36 }], }) @@ -112,12 +148,7 @@ describe('row write provenance', () => { principal: EXECUTOR, envelope: { kind: 'bundle', - value: { - complete: true, - selections: [ - { key: JSON.stringify([0, 'col_aaa']), provenance: { scope: { kind: 'workspace' } } }, - ], - }, + value: bundle([{ key: JSON.stringify([0, 'col_aaa']) }]), }, }) ).toThrow(TableRowProvenanceError) @@ -126,7 +157,7 @@ describe('row write provenance', () => { it('marks an incomplete bundle unknown rather than certifying it', () => { const { stamps } = resolve({ principal: EXECUTOR, - envelope: { kind: 'bundle', value: { complete: false, selections: [] } }, + envelope: { kind: 'bundle', value: bundle([], false) }, }) expect(stamps).toEqual([{ complete: false, columns: {} }]) @@ -138,21 +169,10 @@ describe('row write provenance', () => { keying: 'names', wireRows: [{ Name: 'Ada' }], storageRows: [{ col_aaa: 'Ada' }], - envelope: { - kind: 'bundle', - value: { - complete: true, - selections: [ - { key: JSON.stringify([0, 'Name']), provenance: { scope: { kind: 'workspace' } } }, - ], - }, - }, + envelope: { kind: 'bundle', value: bundle([{ key: JSON.stringify([0, 'Name']) }]) }, }) - expect(stamps[0]).toEqual({ - complete: true, - columns: { col_aaa: { scope: { kind: 'workspace' } } }, - }) + expect(stamps[0]).toEqual({ complete: true, columns: { col_aaa: traceProvenance() } }) }) it('checks the scope against the acting principal, not a billing owner', () => { @@ -160,15 +180,17 @@ describe('row write provenance', () => { principal: EXECUTOR, envelope: { kind: 'bundle', - value: { - complete: true, - selections: [{ key: JSON.stringify([0, 'col_aaa']), provenance: { scope: {} } }], - }, + value: bundle([ + { + key: JSON.stringify([0, 'col_aaa']), + scope: { userId: 'billing-owner', workspaceId: 'workspace-1' }, + }, + ]), }, }) expect(mocks.scopeCompatible).toHaveBeenCalledWith( - {}, + { userId: 'billing-owner', workspaceId: 'workspace-1' }, { userId: 'user-1', workspaceId: 'workspace-1' } ) }) @@ -182,16 +204,10 @@ describe('row write provenance', () => { keying: 'ids', wireRows: [{ 'col-unknown': 'x' }], storageRows: [{ 'col-unknown': 'x' }], - envelope: { - kind: 'bundle', - value: { - complete: true, - selections: [{ key: JSON.stringify([0, 'col-unknown']), provenance: { scope: {} } }], - }, - }, + envelope: { kind: 'bundle', value: bundle([{ key: JSON.stringify([0, 'col-unknown']) }]) }, }) - expect(stamps[0]).toEqual({ complete: true, columns: { 'col-unknown': { scope: {} } } }) + expect(stamps[0]).toEqual({ complete: true, columns: { 'col-unknown': traceProvenance() } }) }) it('records nothing for a key that names no column, since it is never stored', () => { @@ -200,15 +216,7 @@ describe('row write provenance', () => { keying: 'names', wireRows: [{ Nope: 'x' }], storageRows: [{}], - envelope: { - kind: 'bundle', - value: { - complete: true, - selections: [ - { key: JSON.stringify([0, 'Nope']), provenance: { scope: { kind: 'workspace' } } }, - ], - }, - }, + envelope: { kind: 'bundle', value: bundle([{ key: JSON.stringify([0, 'Nope']) }]) }, }) expect(stamps[0]).toEqual({ complete: true, columns: {} }) diff --git a/apps/sim/lib/table/application/rows.test.ts b/apps/sim/lib/table/application/rows.test.ts index 2bfbeb337b9..7c017cba175 100644 --- a/apps/sim/lib/table/application/rows.test.ts +++ b/apps/sim/lib/table/application/rows.test.ts @@ -727,10 +727,13 @@ describe('row query and upsert application semantics', () => { }, }) - expect(mockLoadSecretProvenance).toHaveBeenCalledWith([row], { - userId: 'user-1', - workspaceId: TABLE.workspaceId, - }) + // Narrowed to the columns the row still holds: without `selectedValues` a + // stale sidecar entry for a dropped column rides along in the envelope, and + // the unmigrated `rows`/`query` routes have always narrowed here. + expect(mockLoadSecretProvenance).toHaveBeenCalledWith( + [{ id: row.id, updatedAt: row.updatedAt, selectedValues: row.data }], + { userId: 'user-1', workspaceId: TABLE.workspaceId } + ) expect(result.secretProvenance).toBe(provenance) }) diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index 3e0a06710fa..3b5301bb0ba 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -120,16 +120,22 @@ type TableRowsProvenance = Awaited[0], workspaceId: string, - // The loader reads only id, updatedAt and data, so a row without its - // executions sidecar is enough — see `TABLE_ROW_SIDECAR_SELECTION`. + // The loader reads only id, updatedAt and the selected values, so a row + // without its executions sidecar is enough — see `TABLE_ROW_SIDECAR_SELECTION`. rows: TableRowSummary[], include: boolean | undefined ): Promise { if (!include) return undefined - return loadTableRowSecretProvenance(rows, { - userId: requirePrincipalSubjectUserId(principal), - workspaceId, - }) + return loadTableRowSecretProvenance( + // `selectedValues` narrows the sidecar to the columns the row still holds. + // Without it a stale entry for a dropped column rides along in the envelope, + // which is how the unmigrated `rows`/`query` routes have always behaved. + rows.map((row) => ({ id: row.id, updatedAt: row.updatedAt, selectedValues: row.data })), + { + userId: requirePrincipalSubjectUserId(principal), + workspaceId, + } + ) } function requestId(input: TableScopedInput): string { diff --git a/apps/sim/lib/table/events.attribution.test.ts b/apps/sim/lib/table/events.attribution.test.ts index 351aa715f5e..e116b737fcf 100644 --- a/apps/sim/lib/table/events.attribution.test.ts +++ b/apps/sim/lib/table/events.attribution.test.ts @@ -26,11 +26,28 @@ const ATTRIBUTED_CALL_SITES = [ ] as const /** Surfaces that name the acting tab. See the note above — this is the real allowlist. */ -const ACTOR_SUPPLYING_SURFACES = ['app/api/table/[tableId]/rows/[rowId]/route.ts'] as const +const ACTOR_SUPPLYING_SURFACES = [ + 'app/api/table/[tableId]/rows/route.ts', + 'app/api/table/[tableId]/rows/[rowId]/route.ts', +] as const const APP_ROOT = join(import.meta.dirname, '../..') /** Declares the function; matching its own definition would say nothing about call sites. */ const DECLARING_MODULE = 'lib/table/events.ts' +/** Declares the input and forwards it to the signal; it names no tab of its own. */ +const FORWARDING_MODULE = 'lib/table/application/rows.ts' + +/** + * A file names a tab either by setting the input field or by passing a second + * argument to the signal directly. + * + * Deliberately keyed on the attribution rather than on `readClientId`: that reader + * is a general-purpose helper any surface may call for unrelated reasons, so + * matching it classified innocent callers as suppliers and could be sidestepped by + * aliasing or wrapping it. These two forms are what actually attribute a write, and + * they hold however the id was obtained. + */ +const SUPPLIER_PATTERNS = [/actorClientId:/, /signalTableRowsChangedByActor\([^)]*,/] as const async function* walk(dir: string): AsyncGenerator { for (const entry of await readdir(dir, { withFileTypes: true })) { @@ -41,11 +58,14 @@ async function* walk(dir: string): AsyncGenerator { } } -async function filesContaining(needle: string, skip: (relative: string) => boolean = () => false) { +async function filesMatching( + matches: (source: string) => boolean, + skip: (relative: string) => boolean = () => false +) { const found: string[] = [] for await (const file of walk(APP_ROOT)) { const source = await readFile(file, 'utf8') - if (!source.includes(needle)) continue + if (!matches(source)) continue const relative = file.slice(APP_ROOT.length + 1) if (skip(relative)) continue found.push(relative) @@ -55,8 +75,8 @@ async function filesContaining(needle: string, skip: (relative: string) => boole describe('signalTableRowsChangedByActor call sites', () => { it('is called only where the acting tab reconciles the write locally', async () => { - const callers = await filesContaining( - 'signalTableRowsChangedByActor(', + const callers = await filesMatching( + (source) => source.includes('signalTableRowsChangedByActor('), (relative) => relative === DECLARING_MODULE ) @@ -64,12 +84,9 @@ describe('signalTableRowsChangedByActor call sites', () => { }) it('is given an actor only by surfaces whose client hook reconciles locally', async () => { - const suppliers = await filesContaining( - 'actorClientId:', - // These declare or forward the field rather than naming a tab. - (relative) => - relative === 'lib/table/application/rows.ts' || - relative === 'lib/table/application/row-secret-provenance.ts' + const suppliers = await filesMatching( + (source) => SUPPLIER_PATTERNS.some((pattern) => pattern.test(source)), + (relative) => relative === DECLARING_MODULE || relative === FORWARDING_MODULE ) expect(suppliers).toEqual([...ACTOR_SUPPLYING_SURFACES].sort()) diff --git a/apps/sim/lib/table/fixture-contract.ts b/apps/sim/lib/table/fixture-contract.ts new file mode 100644 index 00000000000..a97d6dc08b3 --- /dev/null +++ b/apps/sim/lib/table/fixture-contract.ts @@ -0,0 +1,22 @@ +import type { createTableDefinition } from '@sim/testing' +import type { TableDefinition } from '@/lib/table/types' + +/** + * Compile-time tie between the shared table fixture and the domain type it stands in for. + * + * `createTableDefinition` returns a `TableDefinitionFixture` declared inside `@sim/testing`, + * which cannot import from the apps workspace, so the two shapes are only structurally + * related. The fixture then flows into untyped `vi.fn().mockResolvedValue(...)` calls, which + * erase the type entirely — and `tsconfig.json` excludes test files, so no assertion placed + * in one would ever be checked. Without this probe a new required field on + * {@link TableDefinition} is a production type error and a silent no-op across every route + * test, leaving them to exercise a table shape that no longer exists. + * + * Type-only by construction: `@sim/testing` is a devDependency, so this file must never emit + * a runtime import. + */ +type AssertAssignableToTableDefinition = T + +export type TableFixtureMatchesDomainType = AssertAssignableToTableDefinition< + ReturnType +> diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index 4857338e68a..d55a4a735f0 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -1438,9 +1438,15 @@ function toRowSummary(row: Awaited>[number]): } /** - * One row without its executions sidecar, for the surfaces that never put - * executions on the wire — the single-row read routes and the Copilot row tool. - * Loading the sidecar for them is a query whose result is discarded. + * One row without its executions sidecar, for the single-row read routes, which + * project `id`/`data`/`position`/`createdAt`/`updatedAt` and never put executions + * on the wire. Loading the sidecar for them is a query whose result is discarded. + * + * Not every `readTableRow` caller is such a surface: the Copilot `get_row` tool + * spreads the row straight onto its result, so it used to hand the model an + * executions map and no longer does. That narrowing is deliberate — the generated + * tool contract never described the field, and the bulk `query_rows` path has + * always returned an empty one — but it is a wire change, not a pure saving. * * Deliberately a separate function rather than a flag on {@link getRowById}: a * caller that forgets to pass the flag reads an empty sidecar and cannot tell