fix(knowledge): check the slot values actually being written, however they arrived

Four ways past the same guard, all one mistake: it was attached per call
site on a belief about how the values were produced, rather than to the
values about to be written.

A tags payload that is truthy but not a JSON array, or not JSON at all, left
the caller's raw slot values in place while the guard was skipped, because
the flag keyed on the payload being present rather than on the name-keyed
path having run. It is unconditional now, inside the transaction, against
the values being inserted. That also closes a read taken before the lock on
the successful path, which a deletion could invalidate before the insert —
the same check-then-act the previous commit closed for the other branch.

The predicate deciding whether an update writes a slot trimmed before
testing, while every writer clears only on the exact empty value. So a
whitespace-only value skipped both the lock and the check and was then
stored: verbatim for a text tag, as zero for a number, as false for a
boolean. It now tests exactly what the writers test, so the two cannot
disagree again. A value with meaningful surrounding space was a write
before and still is.

The bulk upload path accepted the same slots and had no guard at all. Its
transaction already takes the row lock first, so the check is one read for
the batch rather than one per document.

The shape underneath is still worth changing: three write sites each
hand-wire both the lock and the check, and nothing makes a fourth do
either. A seam owning "write the tag columns" would remove the chance to
forget, and is left as follow-up rather than carried here.

Author: waleedlatif1

apps/sim/lib/knowledge/documents/service.ts

@@ -2184,6 +2184,14 @@ export async function createDocumentRecords(
     const documentRecords = []
     const documentProvenances: (DurableSecretProvenance | undefined)[] = []
     const returnData: DocumentData[] = []
+    /**
+     * One representative value per slot the batch writes into, so the check
+     * below costs one definition read for the whole batch rather than one per
+     * document. Which document contributed a slot does not matter: the check
+     * asks only whether a definition covers the slot, and any document landing a
+     * value in an uncovered slot fails the batch.
+     */
+    const batchTagSlotValues: Record<string, unknown> = {}
 
     for (const [documentIndex, docData] of resolvedDocuments.entries()) {
       const documentId = generateId()
@@ -2240,6 +2248,11 @@ export async function createDocumentRecords(
         boolean2: processedTags.boolean2 ?? null,
         boolean3: processedTags.boolean3 ?? null,
       }
+      for (const [key, value] of Object.entries(baseDocument)) {
+        if (value !== null && value !== undefined && batchTagSlotValues[key] === undefined) {
+          batchTagSlotValues[key] = value
+        }
+      }
       const source = createKnowledgeDocumentSourceValue(baseDocument)
       const binding = storageKey
         ? (bindingByKey.get(storageKey) ?? sourceBindingByKey.get(storageKey))
@@ -2276,6 +2289,14 @@ export async function createDocumentRecords(
     }
 
     if (documentRecords.length > 0) {
+      /**
+       * The same check the single-document insert makes, against the same
+       * locked snapshot: this batch reaches the identical columns from the
+       * identical caller-supplied slot values, so leaving it out here left the
+       * whole bulk upload path as a way around the guard.
+       */
+      await assertTagSlotsAreDefined(knowledgeBaseId, batchTagSlotValues, tx)
+
       await tx.insert(document).values(documentRecords)
       for (const [documentIndex, record] of documentRecords.entries()) {
         const provenance = documentProvenances[documentIndex]
@@ -2676,14 +2697,6 @@ export async function createSingleDocument(
       }
     }
   }
-  /**
-   * Only the slot-keyed branch needs checking: `resolveDocumentTags` above
-   * already refuses a name it cannot resolve to a definition, so the name-keyed
-   * branch cannot reach an undefined slot. The check itself runs inside the
-   * transaction below, under the knowledge base's row lock, because tag
-   * deletion clears the slot and drops its definition under that same lock.
-   */
-  const validateTagSlots = !documentData.documentTagsData
 
   const newDocument = {
     id: documentId,
@@ -2708,9 +2721,18 @@ export async function createSingleDocument(
 
     await tx.execute(sql`SELECT 1 FROM knowledge_base WHERE id = ${knowledgeBaseId} FOR UPDATE`)
 
-    if (validateTagSlots) {
-      await assertTagSlotsAreDefined(knowledgeBaseId, processedTags, tx)
-    }
+    /**
+     * Checked against exactly the slot values about to be inserted, whatever
+     * produced them. Conditioning this on how the tags were supplied is what let
+     * two bypasses through: a `documentTagsData` payload that is truthy but not
+     * a JSON array leaves the caller's direct slot values in place while
+     * skipping the name-keyed resolution, and even a payload that does resolve
+     * was matched against definitions read before this transaction opened, so a
+     * tag deletion committing in between stranded the value the resolution had
+     * just approved. Both disappear once the check reads the same locked
+     * snapshot the insert writes into.
+     */
+    await assertTagSlotsAreDefined(knowledgeBaseId, processedTags, tx)
 
     const kb = await tx
       .select({

apps/sim/lib/knowledge/documents/tag-slot-lock.test.ts

@@ -60,7 +60,11 @@ vi.mock('@/lib/knowledge/documents/processing-outbox-event', () => ({
   enqueueKnowledgeDocumentProcessing: mockEnqueueKnowledgeDocumentProcessing,
 }))
 
-import { createSingleDocument, updateDocument } from '@/lib/knowledge/documents/service'
+import {
+  createDocumentRecords,
+  createSingleDocument,
+  updateDocument,
+} from '@/lib/knowledge/documents/service'
 
 const KNOWLEDGE_BASE_ID = 'kb-1'
 const NOW = new Date('2026-01-01T00:00:00.000Z')
@@ -146,6 +150,17 @@ describe('updateDocument tag-slot validation', () => {
     expect(dbChainMockFns.execute).not.toHaveBeenCalled()
     expect(orderForTable(dbChainMockFns.update, document)).toBeGreaterThan(0)
   })
+
+  it('treats a whitespace-only value as a write, because the writer stores it rather than clearing', async () => {
+    queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
+
+    await expect(
+      updateDocument('doc-1', { tag2: ' ' }, 'req-1', { knowledgeBaseId: KNOWLEDGE_BASE_ID })
+    ).rejects.toMatchObject({ code: 'validation' })
+
+    expect(dbChainMockFns.execute).toHaveBeenCalled()
+    expect(dbChainMockFns.update).not.toHaveBeenCalled()
+  })
 })
 
 describe('createSingleDocument tag-slot validation', () => {
@@ -202,4 +217,77 @@ describe('createSingleDocument tag-slot validation', () => {
 
     expect(orderForTable(dbChainMockFns.insert, document)).toBe(-1)
   })
+
+  it('refuses a slot carried alongside a tags payload that is not a JSON array', async () => {
+    queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
+
+    await expect(
+      createSingleDocument(
+        { ...documentData, tag2: 'purple', documentTagsData: '{"tagName":"colour"}' },
+        KNOWLEDGE_BASE_ID,
+        'req-1'
+      )
+    ).rejects.toMatchObject({ code: 'validation' })
+
+    expect(orderForTable(dbChainMockFns.insert, document)).toBe(-1)
+  })
+
+  it('refuses a slot carried alongside a tags payload that is not valid JSON', async () => {
+    queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
+
+    await expect(
+      createSingleDocument(
+        { ...documentData, tag2: 'purple', documentTagsData: 'not json at all' },
+        KNOWLEDGE_BASE_ID,
+        'req-1'
+      )
+    ).rejects.toMatchObject({ code: 'validation' })
+
+    expect(orderForTable(dbChainMockFns.insert, document)).toBe(-1)
+  })
+
+  it('refuses a whitespace-only slot value, which the insert would store rather than clear', async () => {
+    queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
+
+    await expect(
+      createSingleDocument({ ...documentData, tag2: ' ' }, KNOWLEDGE_BASE_ID, 'req-1')
+    ).rejects.toMatchObject({ code: 'validation' })
+
+    expect(orderForTable(dbChainMockFns.insert, document)).toBe(-1)
+  })
+
+  describe('createDocumentRecords', () => {
+    it('checks the batch inside the insert transaction, under the knowledge-base row lock', async () => {
+      queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
+
+      await createDocumentRecords(
+        [{ ...documentData, tag1: 'priority' }],
+        KNOWLEDGE_BASE_ID,
+        'req-1'
+      )
+
+      const transactionOrder = dbChainMockFns.transaction.mock.invocationCallOrder[0]
+      const lockOrder = dbChainMockFns.execute.mock.invocationCallOrder[0] ?? -1
+      const definitionReadOrder = orderForTable(dbChainMockFns.from, knowledgeBaseTagDefinitions)
+      const documentInsertOrder = orderForTable(dbChainMockFns.insert, document)
+
+      expect(lockOrder).toBeGreaterThan(transactionOrder)
+      expect(definitionReadOrder).toBeGreaterThan(lockOrder)
+      expect(documentInsertOrder).toBeGreaterThan(definitionReadOrder)
+    })
+
+    it('refuses a batch whose slot no definition covers, and inserts nothing', async () => {
+      queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
+
+      await expect(
+        createDocumentRecords(
+          [{ ...documentData }, { ...documentData, tag2: 'purple' }],
+          KNOWLEDGE_BASE_ID,
+          'req-1'
+        )
+      ).rejects.toMatchObject({ code: 'validation' })
+
+      expect(orderForTable(dbChainMockFns.insert, document)).toBe(-1)
+    })
+  })
 })

apps/sim/lib/knowledge/tags/service.ts

@@ -205,11 +205,17 @@ export async function getNextAvailableSlot(
  * Get all tag definitions for a knowledge base
  */
 /**
- * The slots a write would put a nonempty value into.
+ * The slots a write would leave a value in.
  *
- * Clearing is not a write for this purpose: an empty value removes whatever a
- * slot holds, so it can never leave a value stranded behind a missing
- * definition.
+ * Only the exact empty value counts as clearing, because that is exactly what
+ * the writers treat as clearing: they map `''`, `null` and `undefined` to a
+ * null column and persist everything else as given. A value that merely looks
+ * empty is still stored — a whitespace-only text value is preserved verbatim, a
+ * whitespace-only number parses to `0`, and a whitespace-only boolean falls
+ * back to `false` — so classifying it as a clear would skip both the knowledge
+ * base's row lock and the definition check while still landing a value in a
+ * slot no definition covers. Matching the writers' own condition rather than a
+ * looser one is what keeps the two from drifting apart again.
  */
 function collectWrittenTagSlots(slotValues: Record<string, unknown>): string[] {
   return Object.entries(slotValues)
@@ -218,7 +224,7 @@ function collectWrittenTagSlots(slotValues: Record<string, unknown>): string[] {
         (VALID_TAG_SLOTS as readonly string[]).includes(slot) &&
         value !== undefined &&
         value !== null &&
-        String(value).trim().length > 0
+        String(value) !== ''
     )
     .map(([slot]) => slot)
 }
@@ -227,7 +233,7 @@ function collectWrittenTagSlots(slotValues: Record<string, unknown>): string[] {
  * Whether a write touches a tag slot at all.
  *
  * A writer uses this to decide whether it needs the knowledge base's row lock:
- * only a write that lands a nonempty slot value can race tag deletion, so a
+ * only a write that lands a slot value can race tag deletion, so a
  * write that touches no slot must not take — or wait on — that lock.
  */
 export function writesTagSlots(slotValues: Record<string, unknown>): boolean {

apps/sim/lib/knowledge/tags/undefined-slot-write.test.ts

@@ -6,7 +6,7 @@ import { knowledgeBaseTagDefinitions } from '@sim/db/schema'
 import { queueTableRows, resetDbChainMock } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 import { OrchestrationError } from '@/lib/core/orchestration/types'
-import { assertTagSlotsAreDefined } from '@/lib/knowledge/tags/service'
+import { assertTagSlotsAreDefined, writesTagSlots } from '@/lib/knowledge/tags/service'
 
 const KNOWLEDGE_BASE_ID = 'kb-1'
 const NOW = new Date('2026-01-01T00:00:00.000Z')
@@ -67,4 +67,26 @@ describe('assertTagSlotsAreDefined', () => {
       assertTagSlotsAreDefined(KNOWLEDGE_BASE_ID, { filename: 'renamed.txt', enabled: true })
     ).resolves.toBeUndefined()
   })
+
+  it('refuses a whitespace-only value, which every writer stores rather than clears', async () => {
+    queueTableRows(knowledgeBaseTagDefinitions, [definition('tag1', 'category')])
+
+    await expect(
+      assertTagSlotsAreDefined(KNOWLEDGE_BASE_ID, { tag2: ' ', number1: '  ', boolean1: '\t' })
+    ).rejects.toMatchObject({ code: 'validation' })
+  })
+})
+
+describe('writesTagSlots', () => {
+  it('counts only the exact empty value as clearing', () => {
+    expect(writesTagSlots({ tag1: '' })).toBe(false)
+    expect(writesTagSlots({ tag1: null, tag2: undefined })).toBe(false)
+    expect(writesTagSlots({ filename: 'renamed.txt' })).toBe(false)
+  })
+
+  it('counts a value that only looks empty as a write, since the writers preserve it', () => {
+    expect(writesTagSlots({ tag1: ' ' })).toBe(true)
+    expect(writesTagSlots({ number1: '  ' })).toBe(true)
+    expect(writesTagSlots({ boolean1: '\n' })).toBe(true)
+  })
 })