Uh oh!
There was an error while loading. Please reload this page.
feat(knowledge): sim-native KB connectors for workspace files and agent conversations - #6315
feat(knowledge): sim-native KB connectors for workspace files and agent conversations#6315mzxchandra wants to merge 13 commits into
Conversation
Safety-net commit of in-progress work recovered after a tmux crash. Implementation per the "Sim Native KB Connectors" plan is complete; end-to-end manual testing has not started yet. - new `sim` auth mode + ConnectorSyncContext in connectors/types.ts - sim.fileFolders selector, extracted query options, selector field unblocked for credential-less connectors - connectors/sim-files: recursive folder scoping, folder picker - connectors/sim-conversations: conversationId-prefix scoping, one KB document per conversation rendered as a markdown transcript - lib/knowledge/connectors/source-config.ts - unit tests for both connectors and the registry
Adds SimLogoIcon to the shared icons module, reusing the v1.0 brand-guide logotype paths already used by the landing navbar's SimWordmark, and points both sim_files and sim_conversations at it. SimWordmark itself is not reusable here: it takes no props (so a caller's className is ignored) and carries a navbar-specific size and offset. The icon fills with var(--text-body) rather than currentColor so the brand mark stays solid ink instead of taking the tile's muted icon color.
…e-kb-connectors # Conflicts: # packages/testing/src/mocks/schema.mock.ts
Pre-landing review surfaced three real defects, two of them found independently by more than one reviewer. Stop PATCH wiping tagSlotMapping. Update replaces sourceConfig wholesale, and sanitizing the caller payload stripped the server-derived slot mapping that is written once at creation and never re-sent. Every connector that declares tagDefinitions would have silently stopped writing tags after any edit, not just the new ones. Server-owned keys are now carried forward from the stored row, with the never-persisted keys kept distinct from the server-owned-and-persisted one. Gate both connectors on secret provenance. The manual knowledge-base upload path refuses a workspace file whose provenance is unavailable, but the sync engine re-uploads extracted text under a fresh kb/ key, so the processor's check resolved zero rows and passed vacuously. Agent memory had no check at all, while every other reader of memory.data pairs it with its provenance sidecar. Both now skip unsafe items visibly instead of indexing them. Also: cover resolveConnectorAuth, which rewrote credential resolution for every connector and had no tests; cover sanitizeConnectorSourceConfig, a stated tenancy control with none; assert the LIKE-escaping wiring rather than only the helper, verified by mutation; drop two vacuous assertions that could not fail; make collectsCredential exhaustive like the other four branch points; remove a provably dead branch in normalizeExt.
PR SummaryHigh Risk Overview New connectors: UI: Add-connector modal and selector fields use Adds Reviewed by Cursor Bugbot for commit 1655ff5. Bugbot is set up for automated code reviews on this repo. Configure here. |
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryAdds credential-less Sim-native connectors for workspace files and agent conversations, together with the authentication, selector, source-configuration, and sync-engine infrastructure they require.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/connectors/sim-conversations/sim-conversations.ts | Implements workspace-scoped conversation listing and hydration with canonical JSON content digests, provenance gating, bounded transcripts, pagination, and tag mapping; the prior hash-collision issue is fixed. |
| apps/sim/connectors/sim-files/sim-files.ts | Implements workspace-file traversal and hydration with workspace scoping, provenance checks, caps, pagination, and stable content hashing. |
| apps/sim/lib/knowledge/connectors/sync-engine.ts | Extends connector synchronization with typed credential-less authentication and workspace-bound sync context while preserving hydrated-hash reconciliation. |
| apps/sim/app/api/knowledge/[id]/connectors/route.ts | Adds exhaustive connector authentication handling and sanitizes client-provided source configuration before validation and persistence. |
| apps/sim/app/api/knowledge/[id]/connectors/[connectorId]/route.ts | Supports credential-less connector updates while preserving server-owned source configuration and validating against the knowledge base workspace. |
| apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/add-connector-modal/add-connector-modal.tsx | Renders authentication controls exhaustively by auth mode and correctly omits credentials for Sim-native connectors; the prior documentation-style issue is fixed. |
| apps/sim/lib/knowledge/connectors/source-config.ts | Centralizes stripping of client-controlled reserved keys and preservation of server-owned connector configuration. |
| apps/sim/connectors/types.ts | Adds the credential-less sim auth arm and typed workspace-bound synchronization context. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
UI[Connector UI] --> API[Knowledge connector API]
API --> Config[Sanitized source config]
API --> Sync[Connector sync engine]
Sync --> Auth{Auth mode}
Auth -->|sim| Native[Workspace-scoped Sim data]
Auth -->|oauth| OAuth[OAuth credential]
Auth -->|apiKey| Key[Encrypted API key]
Native --> Files[Workspace files]
Native --> Conversations[Agent conversations]
Files --> Guard[Secret provenance checks]
Conversations --> Guard
Guard --> Diff[Content-hash classification]
Diff --> KB[Knowledge documents and embeddings]
Reviews (6): Last reviewed commit: "fix(knowledge): content-address the conv..." | Re-trigger Greptile
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Greptile P1. The hash was `memory:{id}:{updatedAt}`, and updatedAt only has
millisecond resolution. Appends landing in the same millisecond as the value
already indexed hashed identically, so classifyExternalDoc called the
transcript unchanged and the new messages stayed out of the knowledge base
until some later write moved the clock.
messageCount closes the reported path — an append always increments it — and
approxBytes covers a same-count replacement. Both were already selected for
the listing, so neither costs an extra query. Mutation-checked: reverting to
the timestamp-only hash fails three tests.
Also switches the sim auth arm's line comment to TSDoc per the repo rule
(Greptile P2).mzxchandra
commented
Aug 6, 2026
mzxchandra
commented
Aug 6, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Cursor Bugbot, high severity. Both connectors listed ascending by updatedAt and then took the first N indexable rows, so a cap meant "the oldest N". The sharper consequence was the second-order one: an already-indexed item that got edited moved past the cap window, stopped being listed, and — since listingCapped suppresses deletion reconciliation — left a permanently stale document in the knowledge base. Ordering now follows from whether the listing is complete or bounded. Uncapped stays ascending, which is the safe walk: a row updated mid-sync moves toward the end and may be emitted twice, and the engine dedupes on externalId. Capped switches to descending so the cap means "the N most recently active", which is what maxFiles/maxConversations are for. Its own risk — a row updated mid-sync slipping behind the cursor — is already covered, because a capped listing is declared incomplete and the next sync picks the row up. The keyset comparison flips with the order (gt/lt) or page two would repeat page one; both filter builders take the direction explicitly. Verified against the live database: capping to 3 now returns the three most recently active conversations rather than the three oldest.
mzxchandra
commented
Aug 6, 2026
mzxchandra
commented
Aug 6, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Cursor Bugbot, medium severity. The hash already carried folderId so a move re-indexes, but the resolved folderPath is what gets stored as the tag, and renaming an ancestor folder rewrites that path while writing only the folder table. contentUpdatedAt, originalName and folderId all stay put, so classifyExternalDoc saw no change and the document kept the old folder tag indefinitely. folderPath now participates in the hash. Both listDocuments and getDocument resolve it from the same pathById map, so the listing and hydration phases still produce byte-identical hashes. Mutation-checked: dropping it from the hash fails the new test.
mzxchandra
commented
Aug 6, 2026
mzxchandra
commented
Aug 6, 2026
@cursor review |
mzxchandra
commented
Aug 6, 2026
mzxchandra
commented
Aug 6, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
…ting Cursor Bugbot, medium severity. Both connectors set listingCapped whenever takeIndexableWithinCap reported capReached — but capReached means the budget is spent, not that anything was left behind. A source that runs out at exactly maxFiles/maxConversations produces a COMPLETE listing, and marking it partial suppressed shouldReconcileDeletions permanently, so an item deleted at the source could never leave the knowledge base. Extracts the decision as isListingTruncated in connectors/utils.ts, matching decideTaskCap in the Asana connector: truncated only when this page dropped items, or the budget ran out with more pages still available. Both connectors now share it rather than repeating the cap bookkeeping. Mutation-checked: collapsing it back to `return args.capReached` reproduces the reported bug and fails the exhausted-at-exactly-the-cap test.
mzxchandra
commented
Aug 6, 2026
mzxchandra
commented
Aug 6, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
mzxchandra
commented
Aug 6, 2026
mzxchandra
commented
Aug 6, 2026
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1655ff5. Configure here.
Summary
Adds the first two Sim-native knowledge-base connectors — connectors that read Sim's own data rather than an external SaaS — plus the infrastructure that makes credential-less connectors possible.
New auth mode (
sim).ConnectorAuthConfigwas a two-arm union where OAuth was the implicitelse, so a credential-less connector died with"OAuth connector is missing credential ID". Rather than turning five two-way branches into five three-way ones, the branch points are now exhaustiveswitches closed bynever, andresolveAccessTokenbecameresolveConnectorAuth()returning a discriminated union whosesimarm has noaccessTokenfield at all — so an empty-string bearer is unrepresentable rather than merely discouraged.ConnectorSyncContextis typed andreadonly, putting the tenancy boundary in the type system.sim_filessyncs a Files-module folder into a KB.sim_conversationssyncs agent-block conversation memory, scoped by conversation-ID prefix (not by workflow — oneconversationIdis routinely shared across blocks and workflows).sim.fileFoldersselector. Two independent blockers had to be cleared:ConnectorSelectorFieldnever setworkspaceIdon its selector context (so any workspace-scoped selector was permanentlyenabled: falsethere), and it gated readiness oncredentialId, which asimconnector never has.Why no incremental sync
shouldReconcileDeletionsopens withif (isIncremental) return false, so incremental sync disables deletion reconciliation outright. Separately,contentUpdatedAtadvances only on content writes — rename, move, soft-delete and restore bumpupdatedAtonly. The usual reason to sync incrementally (an external API's rate limit) doesn't apply to one indexed local query, so both connectors do full listings. Content is still only re-fetched whencontentHashactually changes.Why not a workspace API key
Sim has workspace-scoped API keys, but a key's entire informational content is "which workspace" — which the connector's own knowledge base already answers. So it is either redundant (matches the KB's workspace) or dangerous (doesn't, and becomes a cron-driven cross-workspace read running as no user). Enforcing one would also mean routing the sync through HTTP; holding one without enforcing it would be security theater. The revocable artifact here is the connector row itself.
Test Coverage
711 tests pass across
connectors,lib/knowledge, andhooks/selectors.sim-filespure helperssim-conversationspure helpersresolveConnectorAuthcredentialUserIdchoicesanitizeConnectorSourceConfigconnectors/registry.test.tsKnown gaps, deliberately not closed here:
listDocuments/getDocumentare covered end-to-end by a live-DB harness rather than unit tests, and the three modal components have no component tests.Pre-Landing Review
Four specialists (security, api-contract, testing, maintainability). Three real defects found and fixed:
tagSlotMappingwiped on every PATCH(found independently by security + api-contract). Update replacessourceConfigwholesale, and sanitizing stripped the server-derived mapping the client never re-sends. Every connector declaringtagDefinitions(~40) would silently stop writing tags after any edit.kb/key, so the processor's check resolved zero rows androws.every()returnedtruevacuously. Agent memory had no check at all. Both now skip unsafe items visibly.escapeLikePrefixshipped green while a%prefix exported the whole workspace); twonot.toContain('victim-ws')assertions could not fail because the builders take nosourceConfig.Also fixed:
collectsCredentialwas the one branch point that skipped exhaustiveness; a provably dead branch innormalizeExt.Automated review rounds
memory:{id}:{updatedAt}, andupdatedAtis millisecond-resolution. An append landing in the same millisecond as the indexed value hashed identically, soclassifyExternalDocreturnedunchangedand the new messages never reached the KB until a later write moved the clock.messageCount(an append always increments it) andapproxBytes(covers same-count replacement). Both were already selected, so no extra query.listingCappedsuppresses deletion reconciliation, the KB kept a permanently stale copy.Each fix is mutation-checked: reverting the hash change fails three tests, hardcoding the keyset
gtfails one, and removingescapeLikePrefixfrom the filter builder fails one. A test that cannot fail was not worth committing.The two reviewers caught disjoint classes of problem — Greptile the hash collision, Bugbot the cap-ordering freeze, neither the other's.
Verification
Manual E2E: 24/24 connector-level assertions against the real sync engine and Postgres, plus 4 UI checks by hand.
docsUpdated: 1, docsUnchanged: 2, title follows. The exact case acontentUpdatedAtwatermark would miss.workspaceIdis inert — a connector built withsourceConfig.workspaceIdpointing at an empty workspace returned the KB workspace's own 3 files, byte-identical to a clean connector. Honoring the injection would have returned 0.context='mothership'upload not indexed — the clause protecting private copilot uploads.support_matched exactly 1 conversation, not the 5 it matches unescaped.simconnectors; folder picker populates without a credential; canonical-pair toggle round-trips.Known issues not fixed here
getDocumentlogs and returnsnull, correct for transient failures but a permanently missing blob never surfaces as a failed row.use-connector-config-fields.ts, not in this diff): in the add modal, picking a folder then toggling to advanced loses the value, persistingfolderId: ''— silent scope widening. Affects every connector with a canonical pair, Google Drive included.azure_devops(8 text vs 7) andgoogle_calendar(3 date vs 2) always drop the overflow. Ratcheted into an allowlist inregistry.test.tsso the invariant holds for everything else.Test plan
connectors,lib/knowledge,hooks/selectors)tsc --noEmitclean repo-widecheck:api-validation,check:client-boundary,check:react-querypass🤖 Generated with Claude Code