fix(coda): declare nullable outputs and harden review edge cases

Author: waleedlatif1

apps/docs/content/docs/integrations/coda.mdx

@@ -667,7 +667,7 @@ Get a single row from a Coda table, including all of its cell values
 
 ### Coda Get Sharing Metadata
 
-Check whether the connected user can share or copy a Coda doc, and share it with the workspace or organization
+Check whether the connected user can share or copy a Coda doc, and whether they can share it with the workspace or organization
 
 #### Input
 

apps/sim/lib/credentials/token-service-accounts/validators/coda.test.ts

@@ -88,10 +88,10 @@ describe('validateCodaServiceAccount', () => {
   })
 
   it('rejects a success body without a login id', async () => {
-    mockFetch.mockResolvedValue(jsonResponse(200, { name: 'Jane' }))
-
-    const error = await validateCodaServiceAccount({ apiToken: 'coda-token' }).catch((e) => e)
-
-    expect(error.code).toBe('provider_unavailable')
+    for (const body of [{ name: 'Jane' }, null, { loginId: '  ' }]) {
+      mockFetch.mockResolvedValueOnce(jsonResponse(200, body))
+      const error = await validateCodaServiceAccount({ apiToken: 'coda-token' }).catch((e) => e)
+      expect(error.code).toBe('provider_unavailable')
+    }
   })
 })

apps/sim/lib/credentials/token-service-accounts/validators/coda.ts

@@ -38,8 +38,8 @@ export async function validateCodaServiceAccount(
   )
   await throwForProviderResponse(res, 'whoami')
 
-  const body = await parseProviderJson<CodaWhoamiResponse>(res, 'whoami')
-  if (!body.loginId) {
+  const body = await parseProviderJson<CodaWhoamiResponse | null>(res, 'whoami')
+  if (typeof body?.loginId !== 'string' || !body.loginId.trim()) {
     throw new TokenServiceAccountValidationError('provider_unavailable', 502, {
       step: 'whoami',
       reason: 'missing loginId in response',

apps/sim/scripts/check-block-registry.ts

@@ -37,20 +37,21 @@ const gitOpts = { encoding: 'utf-8' as const, cwd: gitRoot }
 type IdMap = Record<string, Set<string>>
 
 /**
- * Extracts subblock IDs from the `subBlocks: [ ... ]` section of a block
- * definition. Only grabs the top-level `id:` of each subblock object —
- * ignores nested IDs inside `options`, `columns`, etc. Callers pass the source
- * starting at the block's own definition, because a file can declare an untyped
- * legacy block before the typed block derived from it. A derived `subBlocks`
- * expression (not an array literal) yields no IDs rather than borrowing a later
- * bracket.
+ * Returns the index of the `[` opening the first `subBlocks:` array literal in
+ * `source`, or null when that `subBlocks` value is an expression instead.
  */
-function extractSubBlockIds(source: string): string[] {
-  const literal = /subBlocks:(\s*)(.)/.exec(source)
-  if (!literal || literal[2] !== '[') return []
-
-  const bracketStart = literal.index + literal[0].length - 1
+function findSubBlocksLiteral(source: string): number | null {
+  const match = /subBlocks:\s*(\S)/.exec(source)
+  if (!match || match[1] !== '[') return null
+  return match.index + match[0].length - 1
+}
 
+/**
+ * Extracts subblock IDs from the `subBlocks: [ ... ]` array literal whose
+ * opening bracket is at `bracketStart`. Only grabs the top-level `id:` of each
+ * subblock object — ignores nested IDs inside `options`, `columns`, etc.
+ */
+function extractSubBlockIds(source: string, bracketStart: number): string[] {
   const ids: string[] = []
   let braceDepth = 0
   let bracketDepth = 0
@@ -96,22 +97,40 @@ type PreviousIdsResult =
   | { kind: 'noop' }
   | { kind: 'ok'; map: IdMap }
 
+/**
+ * Reads a block's subblock IDs from its source at the base ref. A file can
+ * declare an untyped legacy block before the typed block, so a typed block
+ * with a `subBlocks` array literal is read from its own definition. A typed
+ * block that derives `subBlocks` (for example by filtering the legacy block's)
+ * cannot be evaluated here, so it is read from the legacy literal: IDs the
+ * derivation already dropped then look removed. That fails closed while the
+ * file is being edited, and the block is skipped while the file is unchanged,
+ * since this diff cannot have removed anything from it.
+ */
+function extractPreviousIds(content: string, definitionStart: number, fileChanged: boolean) {
+  const ownLiteral = findSubBlocksLiteral(content.slice(definitionStart))
+  if (ownLiteral !== null) return extractSubBlockIds(content, definitionStart + ownLiteral)
+  if (!fileChanged) return []
+  const legacyLiteral = findSubBlocksLiteral(content)
+  return legacyLiteral === null ? [] : extractSubBlockIds(content, legacyLiteral)
+}
+
 function getPreviousIds(): PreviousIdsResult {
   const registryPath = 'apps/sim/blocks/registry.ts'
   const blocksDir = 'apps/sim/blocks/blocks'
 
-  let hasChanges = false
+  let changedPaths: Set<string>
   try {
     const diff = execSync(
       `git diff --name-only ${baseRef} -- ${registryPath} ${blocksDir}`,
       gitOpts
     ).trim()
-    hasChanges = diff.length > 0
+    changedPaths = new Set(diff ? diff.split('\n') : [])
   } catch {
     return { kind: 'skip', reason: 'Could not diff against base ref' }
   }
 
-  if (!hasChanges) {
+  if (changedPaths.size === 0) {
     return { kind: 'noop' }
   }
 
@@ -137,7 +156,7 @@ function getPreviousIds(): PreviousIdsResult {
       if (!typeMatch) continue
       const blockType = typeMatch[1]
 
-      const ids = extractSubBlockIds(content.slice(typeMatch.index ?? 0))
+      const ids = extractPreviousIds(content, typeMatch.index ?? 0, changedPaths.has(filePath))
       if (ids.length === 0) continue
 
       map[blockType] = new Set(ids)

apps/sim/tools/coda/coda.live.test.ts

@@ -53,8 +53,12 @@ function schemaViolations(
   path: string,
   violations: string[]
 ): void {
-  if (value === null || value === undefined) {
-    if (!schema.optional) violations.push(`${path}: required but ${value}`)
+  if (value === null) {
+    if (!schema.nullable) violations.push(`${path}: null but not declared nullable`)
+    return
+  }
+  if (value === undefined) {
+    if (!schema.optional) violations.push(`${path}: missing but not declared optional`)
     return
   }
   switch (schema.type) {

apps/sim/tools/coda/coda.test.ts

@@ -12,16 +12,36 @@ import { codaDeleteRowsTool } from '@/tools/coda/delete_rows'
 import { codaListDocsTool } from '@/tools/coda/list_docs'
 import { codaListRowsTool } from '@/tools/coda/list_rows'
 import { codaPublishDocTool } from '@/tools/coda/publish_doc'
+import { codaResolveBrowserLinkTool } from '@/tools/coda/resolve_browser_link'
 import { codaUpdateAclSettingsTool } from '@/tools/coda/update_acl_settings'
 import { codaUpdatePageTool } from '@/tools/coda/update_page'
 import { codaUpdateRowTool } from '@/tools/coda/update_row'
 import { codaUpsertRowsTool } from '@/tools/coda/upsert_rows'
 import { buildCodaUrl, CODA_FIELD_UPDATE_RETRY, CODA_RETRY } from '@/tools/coda/utils'
 import { codaWhoamiTool } from '@/tools/coda/whoami'
 import { ErrorExtractorId, extractErrorMessageWithId } from '@/tools/error-extractors'
+import type { OutputProperty } from '@/tools/types'
 
 const table = { accessToken: 'token', docId: 'AbCDeFGH', tableId: 'grid-pqRst-U' }
 
+/** Lists output paths a tool returned as null whose schema does not declare `nullable`. */
+function findUndeclaredNulls(
+  value: unknown,
+  properties: Record<string, OutputProperty> | undefined,
+  path: string
+): string[] {
+  if (!properties || value === null || typeof value !== 'object') return []
+  return Object.entries(properties).flatMap(([key, schema]) => {
+    const child = (value as Record<string, unknown>)[key]
+    const childPath = `${path}.${key}`
+    if (child === null) return schema.nullable ? [] : [childPath]
+    if (Array.isArray(child)) {
+      return child.flatMap((item) => findUndeclaredNulls(item, schema.items?.properties, childPath))
+    }
+    return findUndeclaredNulls(child, schema.properties, childPath)
+  })
+}
+
 function resolveUrl<P>(url: string | ((params: P) => string), params: P): string {
   return typeof url === 'function' ? url(params) : url
 }
@@ -46,6 +66,12 @@ describe('Coda request URLs', () => {
     )
   })
 
+  it('rejects a blank browser link instead of sending no url', () => {
+    expect(() =>
+      resolveUrl(codaResolveBrowserLinkTool.request.url, { accessToken: 'token', url: '   ' })
+    ).toThrow('url is required')
+  })
+
   it('builds list docs URLs without a doc path', () => {
     expect(
       resolveUrl(codaListDocsTool.request.url, {
@@ -84,6 +110,26 @@ describe('Coda row bodies', () => {
     })
   })
 
+  it('maps a column named cells instead of reading it as the cells wrapper', () => {
+    expect(
+      codaUpdateRowTool.request.body!({
+        ...table,
+        rowId: 'i-1',
+        cells: { cells: 'x', Status: 'Done' },
+      })
+    ).toEqual({
+      row: {
+        cells: [
+          { column: 'cells', value: 'x' },
+          { column: 'Status', value: 'Done' },
+        ],
+      },
+    })
+    expect(
+      codaUpdateRowTool.request.body!({ ...table, rowId: 'i-1', cells: { cells: 'x' } })
+    ).toEqual({ row: { cells: [{ column: 'cells', value: 'x' }] } })
+  })
+
   it('rejects an empty upsert', () => {
     expect(() => codaUpsertRowsTool.request.body!({ ...table, rows: '[]' })).toThrow(
       'at least one row'
@@ -339,6 +385,27 @@ describe('Coda tool registration', () => {
     expect(allTools).toHaveLength(60)
   })
 
+  it.each(allTools)(
+    '%s declares every output it can return as null as nullable',
+    async (_, tool) => {
+      const config = tool as {
+        outputs: Record<string, OutputProperty>
+        transformResponse: (response: Response, params: object) => Promise<{ output: unknown }>
+      }
+      const sparseBody = {
+        items: [{ doc: {}, page: {}, metrics: [{}] }],
+        customDocDomains: [{}],
+        resource: {},
+        id: 'x',
+      }
+      const { output } = await config.transformResponse(
+        new Response(JSON.stringify(sparseBody)),
+        table
+      )
+      expect(findUndeclaredNulls(output, config.outputs, '')).toEqual([])
+    }
+  )
+
   it.each(allTools)(
     '%s retries safely repeatable calls and authenticates with the Coda credential',
     (_, tool) => {

apps/sim/tools/coda/create_doc.ts

@@ -165,7 +165,7 @@ export const codaCreateDocTool: ToolConfig<CodaCreateDocParams, CodaCreateDocRes
     requestId: {
       type: 'string',
       description: 'Coda request ID for the doc creation',
-      optional: true,
+      nullable: true,
     },
   },
 }

apps/sim/tools/coda/get_control.ts

@@ -59,11 +59,12 @@ export const codaGetControlTool: ToolConfig<CodaGetControlParams, CodaControlRes
           type: 'string',
           description:
             'Control type (aiBlock, button, checkbox, datePicker, dateRangePicker, dateTimePicker, lookup, multiselect, select, scale, slider, reaction, textbox, timePicker)',
-          optional: true,
+          nullable: true,
         },
         value: {
           type: 'json',
           description: 'Current value (string, number, boolean, or array of these)',
+          nullable: true,
         },
       },
     },

apps/sim/tools/coda/get_formula.ts

@@ -55,6 +55,7 @@ export const codaGetFormulaTool: ToolConfig<CodaGetFormulaParams, CodaFormulaRes
         value: {
           type: 'json',
           description: 'Computed value (string, number, boolean, or array of these)',
+          nullable: true,
         },
       },
     },

apps/sim/tools/coda/get_mutation_status.ts

@@ -49,7 +49,7 @@ export const codaGetMutationStatusTool: ToolConfig<
     warning: {
       type: 'string',
       description: 'Warning if the change completed with caveats',
-      optional: true,
+      nullable: true,
     },
   },
 }