From 916d9af373c13a8dc7da8e8e4c87d044487bac90 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 3 Sep 2026 14:08:16 -0700 Subject: [PATCH] fix(knowledge): let Slack enter per-member access, and resolve its provider in one place --- .agents/skills/ship/SKILL.md | 4 +- .../docs/content/docs/cli/troubleshooting.mdx | 7 +++- ...use-connector-member-group-options.test.ts | 41 +++++++++++++++++++ .../use-connector-member-group-options.ts | 14 +++---- apps/sim/lib/credential-groups/providers.ts | 25 +++++++++-- .../connectors/member-provisioning.ts | 9 ++-- 6 files changed, 79 insertions(+), 21 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.test.ts diff --git a/.agents/skills/ship/SKILL.md b/.agents/skills/ship/SKILL.md index d625e72c2ec..1e34b1cbe81 100644 --- a/.agents/skills/ship/SKILL.md +++ b/.agents/skills/ship/SKILL.md @@ -37,10 +37,10 @@ When the user runs `/ship`: - `bun run check:migrations origin/staging` must pass (staging is the PR base). Do not silence a flagged statement with a `-- migration-safe:` annotation unless `/db-migrate` confirmed the old code no longer depends on it; otherwise split the destructive change into a later deploy. 6. **Run pre-ship checks** from the repo root before staging. This has two phases: first **regenerate** every committed artifact so generated files never drift into a CI failure (this is what catches things like `agent-stream-docs` going stale after a `models.ts` edit), then run the **full audit suite** CI's `Lint and Test` job enforces. Both phases parallelize — but only across commands that write **disjoint** outputs — and a bare `wait` swallows child exit codes, so both phases below explicitly collect each job's status and abort ship if any failed. - **Phase A — regenerate the always-in-repo committed artifacts (parallel), then let step 7 stage whatever changed.** Regenerate only the generators whose inputs live entirely in this repo and that any ordinary code change can drift — `agent-stream-docs:generate` (derives from the provider model registry) and `skills:sync` (derives from `.agents/skills/**`). They write disjoint trees (`apps/docs/…/agent.mdx` vs the `.claude`/`.cursor` command projections), so they parallelize safely, and each is idempotent (a no-op when already in sync): + **Phase A — regenerate the always-in-repo committed artifacts (parallel), then let step 7 stage whatever changed.** Regenerate only the generators whose inputs live entirely in this repo and that any ordinary code change can drift — `agent-stream-docs:generate` (derives from the provider model registry), `skills:sync` (derives from `.agents/skills/**`), and `docs-manifest:generate` (derives from `apps/docs/content/docs/**`, so any added, removed, or renamed docs page drifts it). They write disjoint trees (`apps/docs/…/agent.mdx`, the `.claude`/`.cursor` command projections, and `apps/sim/lib/copilot/generated/docs-manifest.ts`), so they parallelize safely, and each is idempotent (a no-op when already in sync). `docs-manifest:generate` in particular is what keeps Phase B's `docs-manifest:check` from aborting a ship it gives the user no way to fix: ```bash rm -f /tmp/ship-gen-results - for g in agent-stream-docs:generate skills:sync; do + for g in agent-stream-docs:generate skills:sync docs-manifest:generate; do ( bun run "$g" >"/tmp/ship-gen-${g//:/-}.log" 2>&1; echo "$? $g" >>/tmp/ship-gen-results ) & done wait diff --git a/apps/docs/content/docs/cli/troubleshooting.mdx b/apps/docs/content/docs/cli/troubleshooting.mdx index b429a455b4a..503005a8cd0 100644 --- a/apps/docs/content/docs/cli/troubleshooting.mdx +++ b/apps/docs/content/docs/cli/troubleshooting.mdx @@ -100,7 +100,7 @@ sim --version Then upgrade with the package manager you installed it with — using a different one installs a second copy instead of replacing the executable on your `PATH`: - + ```bash npm install -g sim@latest @@ -116,6 +116,11 @@ one installs a second copy instead of replacing the executable on your `PATH`: bun add -g sim@latest ``` + + ```bash + yarn global add sim@latest + ``` + The CLI can also tell you this through a cached daily check on eligible diff --git a/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.test.ts b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.test.ts new file mode 100644 index 00000000000..6a7c8cd4701 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.test.ts @@ -0,0 +1,41 @@ +/** + * `supported` is what decides whether the Access field renders at all, and it is + * exactly `connectorMemberGroupProvider(...) !== null`. A connector that declares + * `permissionScopedListing` crawls once per member, so resolving it to `null` + * hides per-member access from the one kind of connector that has it. + * + * @vitest-environment node + */ +import { assert, describe, expect, it, vi } from 'vitest' + +vi.mock('@/hooks/queries/credential-groups', () => ({ useCredentialGroups: vi.fn() })) + +import { canConnectPersonally } from '@/lib/sim-search/connectors' +import { connectorMemberGroupProvider } from '@/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options' +import { getAllConnectorMeta } from '@/connectors/registry' + +const permissionScopedOAuthConnectors = Object.entries(getAllConnectorMeta()).filter(([, meta]) => + canConnectPersonally(meta) +) + +describe('connectorMemberGroupProvider', () => { + /** A registry-driven `it.each([])` runs zero cases, so the suite must not be empty. */ + it('has permission-scoped OAuth connectors to check', () => { + expect(permissionScopedOAuthConnectors.length).toBeGreaterThan(0) + }) + + it.each(permissionScopedOAuthConnectors)( + 'resolves a credential-group provider for %s', + (_id, meta) => { + expect(connectorMemberGroupProvider(meta)).not.toBeNull() + } + ) + + it('returns null for a connector that does not crawl per member', () => { + const plain = Object.values(getAllConnectorMeta()).find( + (meta) => meta.auth.mode === 'oauth' && !canConnectPersonally(meta) + ) + assert(plain) + expect(connectorMemberGroupProvider(plain)).toBeNull() + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.ts b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.ts index 6806f4c8af4..b3a37ee395e 100644 --- a/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.ts +++ b/apps/sim/app/workspace/[workspaceId]/knowledge/[id]/hooks/use-connector-member-group-options.ts @@ -3,9 +3,9 @@ import { useMemo } from 'react' import type { ComboboxOption } from '@sim/emcn' import { - type CredentialGroupStandardOAuthProvider, + type CredentialGroupProvider, + findCredentialGroupProviderFromProviderId, getCredentialGroupProviderId, - getCredentialGroupStandardOAuthProviderFromProviderId, isCredentialGroupProvider, } from '@/lib/credential-groups/providers' import type { ConnectorMeta } from '@/connectors/types' @@ -31,15 +31,11 @@ export function decodeConnectorMemberGroupOption( } /** The credential-group provider that collects accounts for this connector, if any. */ -function connectorMemberGroupProvider( +export function connectorMemberGroupProvider( connectorConfig: ConnectorMeta -): CredentialGroupStandardOAuthProvider | null { +): CredentialGroupProvider | null { if (connectorConfig.auth.mode !== 'oauth' || !connectorConfig.permissionScopedListing) return null - try { - return getCredentialGroupStandardOAuthProviderFromProviderId(connectorConfig.auth.provider) - } catch { - return null - } + return findCredentialGroupProviderFromProviderId(connectorConfig.auth.provider) } /** The config fields a per-member connector hides: its listing caps, which the server clears. */ diff --git a/apps/sim/lib/credential-groups/providers.ts b/apps/sim/lib/credential-groups/providers.ts index 09638d94ef2..5464fb4af15 100644 --- a/apps/sim/lib/credential-groups/providers.ts +++ b/apps/sim/lib/credential-groups/providers.ts @@ -262,12 +262,31 @@ export function getCredentialGroupProviderId(provider: CredentialGroupProvider): return getCredentialGroupProviderService(provider).providerId } +/** + * The credential group provider collecting accounts for an OAuth provider id, + * or `null` when none does. + * + * Every provider counts here, not only the standard OAuth ones: Slack is + * collected through a custom bot app, so resolving against + * {@link CREDENTIAL_GROUP_STANDARD_OAUTH_PROVIDER_IDS} misses it. Callers that + * treat a miss as an ordinary answer take this rather than catching the throw + * from {@link getCredentialGroupProviderFromProviderId}, so the choice of which + * provider set counts is made in one place instead of at each call site. + */ +export function findCredentialGroupProviderFromProviderId( + providerId: string +): CredentialGroupProvider | null { + return ( + CREDENTIAL_GROUP_PROVIDER_IDS.find( + (candidate) => getCredentialGroupProviderId(candidate) === providerId + ) ?? null + ) +} + export function getCredentialGroupProviderFromProviderId( providerId: string ): CredentialGroupProvider { - const provider = CREDENTIAL_GROUP_PROVIDER_IDS.find( - (candidate) => getCredentialGroupProviderId(candidate) === providerId - ) + const provider = findCredentialGroupProviderFromProviderId(providerId) if (!provider) throw new Error(`Unsupported managed credential provider: ${providerId}`) return provider } diff --git a/apps/sim/lib/knowledge/connectors/member-provisioning.ts b/apps/sim/lib/knowledge/connectors/member-provisioning.ts index 3b9fa1ec37e..9f12819e76f 100644 --- a/apps/sim/lib/knowledge/connectors/member-provisioning.ts +++ b/apps/sim/lib/knowledge/connectors/member-provisioning.ts @@ -17,8 +17,7 @@ import { inviteCredentialGroupEnrollment, } from '@/lib/credential-groups/enrollments' import { - type CredentialGroupProvider, - getCredentialGroupProviderFromProviderId, + findCredentialGroupProviderFromProviderId, getCredentialGroupProviderId, isCredentialGroupProvider, isCredentialGroupStandardOAuthProvider, @@ -115,10 +114,8 @@ export async function provisionKnowledgeConnectorMembersBinding(input: { throw new OrchestrationError('validation', 'Only an OAuth connector can sync per member') } const providerId = connectorMeta.auth.provider - let provider: CredentialGroupProvider - try { - provider = getCredentialGroupProviderFromProviderId(providerId) - } catch { + const provider = findCredentialGroupProviderFromProviderId(providerId) + if (!provider) { throw new OrchestrationError( 'validation', `${connectorMeta.name} accounts cannot be collected through a Credential Group yet`