From 0555030748b3a5d30597c41ea30f62c01ae8582a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 23:22:23 -0700 Subject: [PATCH 1/2] 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. --- .../api/table/row-secret-provenance.test.ts | 81 ++++++++++++ .../tools/server/table/user-table.test.ts | 78 +++++++++++- .../application/row-secret-provenance.test.ts | 116 ++++++++++-------- apps/sim/lib/table/application/rows.test.ts | 11 +- apps/sim/lib/table/application/rows.ts | 18 ++- apps/sim/lib/table/events.attribution.test.ts | 17 ++- apps/sim/lib/table/fixture-contract.ts | 22 ++++ apps/sim/lib/table/rows/service.ts | 12 +- 8 files changed, 280 insertions(+), 75 deletions(-) create mode 100644 apps/sim/lib/table/fixture-contract.ts 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..f99217678f4 100644 --- a/apps/sim/lib/table/events.attribution.test.ts +++ b/apps/sim/lib/table/events.attribution.test.ts @@ -26,11 +26,16 @@ 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 reader; every other file that calls it is naming a tab. */ +const CLIENT_ID_MODULE = 'lib/api/client-id.ts' async function* walk(dir: string): AsyncGenerator { for (const entry of await readdir(dir, { withFileTypes: true })) { @@ -64,12 +69,12 @@ describe('signalTableRowsChangedByActor call sites', () => { }) it('is given an actor only by surfaces whose client hook reconciles locally', async () => { + // Keyed on the reader rather than the `actorClientId:` field name: a surface can + // name a tab positionally — `signalTableRowsChangedByActor(id, readClientId(req))` + // — and matching the field name alone silently missed one of the two real suppliers. 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' + 'readClientId(', + (relative) => relative === CLIENT_ID_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 From a03e47112a048e4abd65a1de1bf61741d9d925cd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 18 Aug 2026 07:41:21 -0700 Subject: [PATCH 2/2] 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. --- apps/sim/lib/table/events.attribution.test.ts | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/apps/sim/lib/table/events.attribution.test.ts b/apps/sim/lib/table/events.attribution.test.ts index f99217678f4..e116b737fcf 100644 --- a/apps/sim/lib/table/events.attribution.test.ts +++ b/apps/sim/lib/table/events.attribution.test.ts @@ -34,8 +34,20 @@ const ACTOR_SUPPLYING_SURFACES = [ 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 reader; every other file that calls it is naming a tab. */ -const CLIENT_ID_MODULE = 'lib/api/client-id.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 })) { @@ -46,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) @@ -60,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 ) @@ -69,12 +84,9 @@ describe('signalTableRowsChangedByActor call sites', () => { }) it('is given an actor only by surfaces whose client hook reconciles locally', async () => { - // Keyed on the reader rather than the `actorClientId:` field name: a surface can - // name a tab positionally — `signalTableRowsChangedByActor(id, readClientId(req))` - // — and matching the field name alone silently missed one of the two real suppliers. - const suppliers = await filesContaining( - 'readClientId(', - (relative) => relative === CLIENT_ID_MODULE + 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())