Skip to content

Commit 1655ff5

Browse files
committed
fix(knowledge): content-address the conversation hash, and keep capped
listings out of deletion reconciliation Two round-5 findings, one of which reverts my own round-4 change. Greptile P1: metadata proxies still collide. updatedAt is millisecond- resolution, and message count plus stored byte size both survive a same-millisecond replacement that happens to preserve them, so a real transcript change could still hash identically. Each proxy narrowed the window without closing it. The hash is now keyed on md5 of the stored JSON, computed in Postgres so the listing still transfers 32 characters rather than the payload — the reason data is not selected there. The hash now moves if and only if the transcript moved. Cursor Bugbot: my round-4 relaxation was wrong, and Bugbot's own round-4 finding does not hold here. A cap implies descending order (round 2), and a descending keyset can miss a row updated between pages — it moves ABOVE the cursor rather than below it, so unlike ascending it is not re-seen. That makes "we consumed exactly the budget" no proof of "we saw everything", and treating it as proof let reconciliation hard-delete a source item that still exists. A capped listing now always blocks reconciliation, with the ordering interaction written down so the relaxation is not reintroduced. The shared isListingTruncated helper is removed rather than left unused; the Asana rule it copied is sound for Asana because Asana lists ascending.
1 parent a9daa27 commit 1655ff5

5 files changed

Lines changed: 69 additions & 90 deletions

File tree

apps/sim/connectors/sim-conversations/sim-conversations.test.ts

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const BASE_ROW: ConversationRow = {
2727
updatedAt: new Date('2026-01-02T00:00:00.000Z'),
2828
messageCount: 6,
2929
approxBytes: 1024,
30+
contentDigest: 'd41d8cd98f00b204e9800998ecf8427e',
3031
}
3132

3233
const META = {
@@ -229,7 +230,7 @@ describe('conversationToStub', () => {
229230

230231
it('hashes on the update watermark, which moves whenever a message is appended', () => {
231232
const base = conversationToStub(BASE_ROW).contentHash
232-
expect(base).toBe('memory:mem-1:2026-01-02T00:00:00.000Z:6:1024')
233+
expect(base).toBe('memory:mem-1:2026-01-02T00:00:00.000Z:d41d8cd98f00b204e9800998ecf8427e')
233234

234235
const appended = conversationToStub({
235236
...BASE_ROW,
@@ -256,18 +257,31 @@ describe('conversationToStub', () => {
256257
const appendedSameMs = conversationToStub({
257258
...BASE_ROW,
258259
messageCount: BASE_ROW.messageCount + 2,
259-
approxBytes: BASE_ROW.approxBytes + 180,
260+
contentDigest: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
260261
}).contentHash
261262

262263
expect(appendedSameMs).not.toBe(indexed)
263264
})
264265

265-
/** A same-count replacement still moves the byte size. */
266-
it('distinguishes a same-millisecond, same-count content replacement', () => {
266+
/**
267+
* The case metadata proxies could not close: a same-millisecond replacement that
268+
* preserves both message count and stored byte size. Only the content digest moves.
269+
*/
270+
it('distinguishes a replacement that preserves count and byte size', () => {
267271
expect(
268-
conversationToStub({ ...BASE_ROW, approxBytes: BASE_ROW.approxBytes + 40 }).contentHash
272+
conversationToStub({
273+
...BASE_ROW,
274+
contentDigest: 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb',
275+
}).contentHash
269276
).not.toBe(conversationToStub(BASE_ROW).contentHash)
270277
})
278+
279+
/** Metadata churn without a content change must NOT force a re-index. */
280+
it('is unchanged when only the byte-size hint moves', () => {
281+
expect(
282+
conversationToStub({ ...BASE_ROW, approxBytes: BASE_ROW.approxBytes + 40 }).contentHash
283+
).toBe(conversationToStub(BASE_ROW).contentHash)
284+
})
271285
})
272286

273287
describe('cursor', () => {

apps/sim/connectors/sim-conversations/sim-conversations.ts

Lines changed: 33 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import type {
1111
} from '@/connectors/types'
1212
import {
1313
CONNECTOR_MAX_FILE_BYTES,
14-
isListingTruncated,
1514
isSkippedDocument,
1615
markSkipped,
1716
parseTagDate,
@@ -39,6 +38,7 @@ export interface ConversationRow {
3938
updatedAt: Date
4039
messageCount: number
4140
approxBytes: number
41+
contentDigest: string
4242
}
4343

4444
interface Cursor {
@@ -169,15 +169,15 @@ export function renderTranscript(
169169
*
170170
* Single source of truth for `contentHash`, used by both listing and hydration.
171171
*
172-
* `updatedAt` is the primary watermark — `Memory.appendMessage` bumps it on every
173-
* appended message — but it only has millisecond resolution, so two appends landing
174-
* in the same millisecond as the previously indexed value would hash identically and
175-
* `classifyExternalDoc` would call the transcript unchanged, leaving the new messages
176-
* out of the knowledge base until some later write moved the clock.
172+
* Keyed on a digest of the stored JSON rather than on metadata proxies. `updatedAt`
173+
* is only millisecond-resolution, and message count plus byte size still collide for
174+
* a same-millisecond replacement that happens to preserve both — each proxy narrows
175+
* the window without closing it. A content digest closes it outright: the hash moves
176+
* if and only if the transcript moved.
177177
*
178-
* `messageCount` closes that: an append always increments it. `approxBytes` covers
179-
* the rarer case of a same-count replacement. Both are already selected for the
180-
* listing, so neither costs an extra query.
178+
* Postgres computes the digest, so the listing transfers 32 characters instead of the
179+
* payload — the reason `data` is deliberately not selected here. `updatedAt` stays in
180+
* the hash purely so a stored value is legible when debugging.
181181
*/
182182
export function conversationToStub(row: ConversationRow): ExternalDocument {
183183
return {
@@ -187,7 +187,7 @@ export function conversationToStub(row: ConversationRow): ExternalDocument {
187187
contentDeferred: true,
188188
mimeType: 'text/plain',
189189
// No sourceUrl: conversations have no page of their own in the app.
190-
contentHash: `memory:${row.id}:${row.updatedAt.toISOString()}:${row.messageCount}:${row.approxBytes}`,
190+
contentHash: `memory:${row.id}:${row.updatedAt.toISOString()}:${row.contentDigest}`,
191191
metadata: {
192192
conversationId: row.key,
193193
messageCount: row.messageCount,
@@ -218,6 +218,12 @@ const CONVERSATION_ROW_COLUMNS = {
218218
CASE WHEN jsonb_typeof(${memory.data}) = 'array'
219219
THEN jsonb_array_length(${memory.data}) ELSE 0 END`,
220220
approxBytes: sql<number>`pg_column_size(${memory.data})`,
221+
/**
222+
* Content-addressed change detection, computed in Postgres so the transcript
223+
* itself never crosses the wire during listing. `jsonb::text` is canonical
224+
* (keys sorted, whitespace normalized), so the digest is stable for a value.
225+
*/
226+
contentDigest: sql<string>`md5(${memory.data}::text)`,
221227
} as const
222228

223229
function readPrefix(sourceConfig: Record<string, unknown>): string {
@@ -291,15 +297,23 @@ export const simConversationsConnector: ConnectorConfig = {
291297
* matching note in the files connector. Exhausting the source at exactly
292298
* `maxConversations` is a complete listing.
293299
*/
294-
if (
295-
isListingTruncated({
296-
capReached,
297-
droppedFromPage: documents.length < items.length,
298-
morePagesAvailable: pageFilled,
299-
})
300-
) {
301-
syncContext.listingCapped = true
302-
}
300+
/**
301+
* A capped listing can NEVER certify completeness, so it always blocks deletion
302+
* reconciliation — even when the source happened to run out at exactly
303+
* `maxConversations` with nothing dropped.
304+
*
305+
* The reason is the descending order a cap implies: a row updated between pages
306+
* moves ABOVE the keyset cursor and is skipped for this run. (Ascending has the
307+
* opposite skew — the row moves below the cursor and is re-seen, which the
308+
* engine dedupes.) So "we consumed exactly the budget" does not prove "we saw
309+
* everything", and treating it as proof lets reconciliation hard-delete a source
310+
* item that still exists.
311+
*
312+
* Do not relax this into a `droppedFromPage || morePagesAvailable` check. That
313+
* reads correct in isolation and is how the Asana connector decides truncation,
314+
* but Asana lists ascending — the inference does not transfer.
315+
*/
316+
syncContext.listingCapped = true
303317
return { documents, hasMore: false }
304318
}
305319

apps/sim/connectors/sim-files/sim-files.ts

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import type {
2323
} from '@/connectors/types'
2424
import {
2525
CONNECTOR_MAX_FILE_BYTES,
26-
isListingTruncated,
2726
isSkippedDocument,
2827
markSkipped,
2928
parseMultiValue,
@@ -390,15 +389,23 @@ export const simFilesConnector: ConnectorConfig = {
390389
* source would never leave the knowledge base. Mirrors `decideTaskCap`'s
391390
* `droppedFromPage || (hitLimit && morePagesAvailable)` in the Asana connector.
392391
*/
393-
if (
394-
isListingTruncated({
395-
capReached,
396-
droppedFromPage: documents.length < items.length,
397-
morePagesAvailable: pageFilled,
398-
})
399-
) {
400-
syncContext.listingCapped = true
401-
}
392+
/**
393+
* A capped listing can NEVER certify completeness, so it always blocks deletion
394+
* reconciliation — even when the source happened to run out at exactly
395+
* `maxFiles` with nothing dropped.
396+
*
397+
* The reason is the descending order a cap implies: a row updated between pages
398+
* moves ABOVE the keyset cursor and is skipped for this run. (Ascending has the
399+
* opposite skew — the row moves below the cursor and is re-seen, which the
400+
* engine dedupes.) So "we consumed exactly the budget" does not prove "we saw
401+
* everything", and treating it as proof lets reconciliation hard-delete a source
402+
* item that still exists.
403+
*
404+
* Do not relax this into a `droppedFromPage || morePagesAvailable` check. That
405+
* reads correct in isolation and is how the Asana connector decides truncation,
406+
* but Asana lists ascending — the inference does not transfer.
407+
*/
408+
syncContext.listingCapped = true
402409
return { documents, hasMore: false }
403410
}
404411

apps/sim/connectors/utils-listing-cap.test.ts

Lines changed: 0 additions & 37 deletions
This file was deleted.

apps/sim/connectors/utils.ts

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -229,25 +229,6 @@ export function takeIndexableWithinCap<T>(
229229
return { documents, indexableCount, capReached: alreadyIndexed + indexableCount >= max }
230230
}
231231

232-
/**
233-
* Whether a capped listing actually stopped short of the source.
234-
*
235-
* `takeIndexableWithinCap` reports `capReached` — the budget is spent — which is NOT
236-
* the same as truncation. A source that runs out at exactly the cap yields a complete
237-
* listing, and marking it truncated would suppress deletion reconciliation forever,
238-
* so an item deleted at the source could never leave the knowledge base.
239-
*
240-
* Same rule as `decideTaskCap` in the Asana connector: truncated only when this page
241-
* dropped items, or the budget ran out with more pages still available.
242-
*/
243-
export function isListingTruncated(args: {
244-
capReached: boolean
245-
droppedFromPage: boolean
246-
morePagesAvailable: boolean
247-
}): boolean {
248-
return args.droppedFromPage || (args.capReached && args.morePagesAvailable)
249-
}
250-
251232
/**
252233
* Raised by a connector when a file exceeds its size cap mid-download — i.e. the
253234
* listing did not report a size, so the limit is only discovered while streaming.

0 commit comments

Comments
 (0)